From 643b881011c4b698ad6c86f34f92cdc8342f63e6 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 26 Mar 2024 14:17:01 -0400 Subject: [PATCH] refactor: Add util::Result multiple error and warning messages Add util::Result support for returning warning messages and multiple errors, not just a single error string. This provides a way for functions to report errors and warnings in a standard way, and simplifies interfaces. The functionality is unit tested here, and put to use in followup PR https://github.com/bitcoin/bitcoin/pull/25722 --- src/kernel/CMakeLists.txt | 1 + src/test/result_tests.cpp | 74 +++++++++++++++++- src/util/CMakeLists.txt | 1 + src/util/result.cpp | 33 ++++++++ src/util/result.h | 157 +++++++++++++++++++++++++++++++++----- 5 files changed, 244 insertions(+), 22 deletions(-) create mode 100644 src/util/result.cpp diff --git a/src/kernel/CMakeLists.txt b/src/kernel/CMakeLists.txt index 2e07ba042a40a..88e4e2b150035 100644 --- a/src/kernel/CMakeLists.txt +++ b/src/kernel/CMakeLists.txt @@ -66,6 +66,7 @@ add_library(bitcoinkernel ../util/hasher.cpp ../util/moneystr.cpp ../util/rbf.cpp + ../util/result.cpp ../util/serfloat.cpp ../util/signalinterrupt.cpp ../util/strencodings.cpp diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp index eef978ca6ac91..430c40046a65d 100644 --- a/src/test/result_tests.cpp +++ b/src/test/result_tests.cpp @@ -2,10 +2,17 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include +#include +#include #include +#include +#include +#include +#include inline bool operator==(const bilingual_str& a, const bilingual_str& b) { @@ -90,15 +97,60 @@ enum FnError { ERR1, ERR2 }; util::Result IntFailFn(int i, bool success) { - if (success) return i; + if (success) return {util::Warning{Untranslated(strprintf("int %i warn.", i))}, i}; return {util::Error{Untranslated(strprintf("int %i error.", i))}, i % 2 ? ERR1 : ERR2}; } +util::Result StrFailFn(int i, bool success) +{ + util::Result result; + if (auto int_result{IntFailFn(i, success) >> result}) { + result.Update(strprintf("%i", *int_result)); + } else { + result.Update({util::Error{Untranslated("str error")}, int_result.GetFailure()}); + } + return result; +} + util::Result EnumFailFn(FnError ret) { return {util::Error{Untranslated("enum fail.")}, ret}; } +util::Result WarnFn() +{ + return {util::Warning{Untranslated("warn.")}}; +} + +util::Result MultiWarnFn(int ret) +{ + util::Result result; + for (int i = 0; i < ret; ++i) { + result.AddWarning(Untranslated(strprintf("warn %i.", i))); + } + result.Update(ret); + return result; +} + +util::Result ChainedFailFn(FnError arg, int ret) +{ + util::Result result{util::Error{Untranslated("chained fail.")}, ret}; + EnumFailFn(arg) >> result; + WarnFn() >> result; + return result; +} + +util::Result AccumulateFn(bool success) +{ + util::Result result; + util::Result x = MultiWarnFn(1) >> result; + BOOST_REQUIRE(x); + util::Result y = MultiWarnFn(2) >> result; + BOOST_REQUIRE(y); + result.Update(IntFailFn(*x + *y, success)); + return result; +} + util::Result TruthyFalsyFn(int i, bool success) { if (success) return i; @@ -140,7 +192,13 @@ BOOST_AUTO_TEST_CASE(check_returned) ExpectFail(NoCopyNoMoveFn(5, false), Untranslated("nocopynomove 5 error."), 5); ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S")); ExpectResult(StrFn(Untranslated("S"), false), false, Untranslated("str S error.")); + ExpectSuccess(StrFailFn(1, true), Untranslated("int 1 warn."), "1"); + ExpectFail(StrFailFn(2, false), Untranslated("int 2 error. str error"), ERR2); ExpectFail(EnumFailFn(ERR2), Untranslated("enum fail."), ERR2); + ExpectFail(ChainedFailFn(ERR1, 5), Untranslated("chained fail. enum fail. warn."), 5); + ExpectSuccess(MultiWarnFn(3), Untranslated("warn 0. warn 1. warn 2."), 3); + ExpectSuccess(AccumulateFn(true), Untranslated("warn 0. warn 0. warn 1. int 3 warn."), 3); + ExpectFail(AccumulateFn(false), Untranslated("int 3 error. warn 0. warn 0. warn 1."), ERR1); ExpectSuccess(TruthyFalsyFn(0, true), {}, 0); ExpectFail(TruthyFalsyFn(0, false), Untranslated("failure value 0."), 0); ExpectSuccess(TruthyFalsyFn(1, true), {}, 1); @@ -156,7 +214,7 @@ BOOST_AUTO_TEST_CASE(check_update) result.Update({util::Error{Untranslated("error")}, ERR1}); ExpectFail(result, Untranslated("error"), ERR1); result.Update(2); - ExpectSuccess(result, Untranslated(""), 2); + ExpectSuccess(result, Untranslated("error"), 2); // Test the same thing but with non-copyable success and failure types. util::Result result2{0}; @@ -164,7 +222,7 @@ BOOST_AUTO_TEST_CASE(check_update) result2.Update({util::Error{Untranslated("error")}, 3}); ExpectFail(result2, Untranslated("error"), 3); result2.Update(4); - ExpectSuccess(result2, Untranslated(""), 4); + ExpectSuccess(result2, Untranslated("error"), 4); } BOOST_AUTO_TEST_CASE(check_dereference_operators) @@ -188,6 +246,16 @@ BOOST_AUTO_TEST_CASE(check_value_or) BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), Untranslated("B")); } +BOOST_AUTO_TEST_CASE(check_message_accessors) +{ + util::Result result{util::Error{Untranslated("Error.")}, util::Warning{Untranslated("Warning.")}}; + BOOST_CHECK_EQUAL(Assert(result.GetMessages())->errors.size(), 1); + BOOST_CHECK_EQUAL(Assert(result.GetMessages())->errors[0], Untranslated("Error.")); + BOOST_CHECK_EQUAL(Assert(result.GetMessages())->warnings.size(), 1); + BOOST_CHECK_EQUAL(Assert(result.GetMessages())->warnings[0], Untranslated("Warning.")); + BOOST_CHECK_EQUAL(util::ErrorString(result), Untranslated("Error. Warning.")); +} + struct Derived : NoCopyNoMove { using NoCopyNoMove::NoCopyNoMove; }; diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index 4999dbf13f040..dc5121f77cfc4 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -17,6 +17,7 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL moneystr.cpp rbf.cpp readwritefile.cpp + result.cpp serfloat.cpp signalinterrupt.cpp sock.cpp diff --git a/src/util/result.cpp b/src/util/result.cpp new file mode 100644 index 0000000000000..ea79375aa3ac6 --- /dev/null +++ b/src/util/result.cpp @@ -0,0 +1,33 @@ +// Copyright (c) 2024 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include +#include +#include + +namespace util { +namespace detail { +bilingual_str JoinMessages(const Messages& messages) +{ + bilingual_str result; + for (const auto& messages : {messages.errors, messages.warnings}) { + for (const auto& message : messages) { + if (!result.empty()) result += Untranslated(" "); + result += message; + } + } + return result; +} +} // namespace detail + +void ResultTraits::Update(Messages& dst, Messages& src) { + dst.errors.insert(dst.errors.end(), std::make_move_iterator(src.errors.begin()), std::make_move_iterator(src.errors.end())); + dst.warnings.insert(dst.warnings.end(), std::make_move_iterator(src.warnings.begin()), std::make_move_iterator(src.warnings.end())); + src.errors.clear(); + src.warnings.clear(); +} +} // namespace util diff --git a/src/util/result.h b/src/util/result.h index 8706220aa90b5..f4ec3be623ec9 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -17,9 +17,15 @@ #include namespace util { +//! Default MessagesType, simple list of errors and warnings. +struct Messages { + std::vector errors{}; + std::vector warnings{}; +}; + //! The Result class provides //! an efficient way for functions to return structured result information, as -//! well as descriptive error messages. +//! well as descriptive error and warning messages. //! //! Logically, a result object is equivalent to: //! @@ -58,15 +64,67 @@ namespace util { //! return function results. //! //! Usage examples can be found in \example ../test/result_tests.cpp. -template +template class Result; //! Wrapper object to pass an error string to the Result constructor. struct Error { bilingual_str message; }; +//! Wrapper object to pass a warning string to the Result constructor. +struct Warning { + bilingual_str message; +}; + +//! Type trait that can be specialized to control the way SuccessType / +//! FailureType / MessagesType values are combined. Default behavior +//! is for new values to overwrite existing ones, but this can be specialized +//! for custom behavior when Result::Update() method is called or << operator is +//! used. For example, this is specialized for Messages struct below to append +//! error and warning messages instead of overwriting them. It can also be used, +//! for example, to merge FailureType values when a function can return multiple +//! failures. +template +struct ResultTraits { + template + static void Update(O& dst, T& src) + { + dst = std::move(src); + } +}; + +//! Type trait that can be specialized to let the Result class work with custom +//! MessagesType types holding error and warning messages. +template +struct MessagesTraits; + +//! ResultTraits specialization for Messages struct. +template<> +struct ResultTraits { + static void Update(Messages& dst, Messages& src); +}; + +//! MessagesTraits specialization for Messages struct. +template<> +struct MessagesTraits { + static void AddError(Messages& messages, bilingual_str error) + { + messages.errors.emplace_back(std::move(error)); + } + static void AddWarning(Messages& messages, bilingual_str warning) + { + messages.warnings.emplace_back(std::move(warning)); + } + static bool HasMessages(const Messages& messages) + { + return messages.errors.size() || messages.warnings.size(); + } +}; namespace detail { +//! Helper function to join messages in space separated string. +bilingual_str JoinMessages(const Messages& messages); + //! Substitute for std::monostate that doesn't depend on std::variant. struct Monostate{}; @@ -157,9 +215,9 @@ class Result : public detail::SuccessHolder requires (std::decay_t::is_result) Result(O&& other) @@ -179,7 +237,10 @@ class Result : public detail::SuccessHolder(*this, other); } - //! Update this result by moving from another result object. + //! Update this result by moving from another result object. Existing + //! success, failure, and messages values are updated (using + //! ResultTraits::Update specializations), so errors and warning messages + //! get appended instead of overwriting existing ones. Result& Update(Result&& other) LIFETIMEBOUND { Move(*this, other); @@ -187,11 +248,18 @@ class Result : public detail::SuccessHolder::AddError(this->EnsureFailData().messages, std::move(error)); + } + void AddWarning(bilingual_str warning) + { + if (!warning.empty()) MessagesTraits::AddWarning(this->EnsureFailData().messages, std::move(warning)); + } + protected: template friend class Result; @@ -216,12 +284,22 @@ class Result : public detail::SuccessHolder static void Construct(Result& result, util::Error error, Args&&... args) { - result.EnsureFailData().messages = std::move(error.message); + result.AddError(std::move(error.message)); Construct(result, std::forward(args)...); } + //! Construct() overload peeling off a util::Warning constructor argument. + template + static void Construct(Result& result, util::Warning warning, Args&&... args) + { + result.AddWarning(std::move(warning.message)); + Construct(result, std::forward(args)...); + } + //! Move success, failure, and messages from source Result object to - //! destination object. The source and + //! destination object. Existing values are updated (using + //! ResultTraits::Update specializations), so destination errors and warning + //! messages get appended to instead of overwritten. The source and //! destination results are not required to have the same types, and //! assigning void source types to non-void destinations type is allowed, //! since no source information is lost. But assigning non-void source types @@ -230,16 +308,27 @@ class Result : public detail::SuccessHolder static void Move(DstResult& dst, SrcResult& src) { - // Move messages values first, then move success or failure value below. - if (src.GetMessages() && !src.GetMessages()->empty()) { - dst.EnsureMessages() = std::move(*src.GetMessages()); - } + // Use operator>> to move messages value first, then move + // success or failure value below. + src >> dst; // If DstConstructed is true, it means dst has either a success value or - // a failure value set, which needs to be destroyed and replaced. If + // a failure value set, which needs to be updated or replaced. If // DstConstructed is false, then dst is being constructed now and has no // values set. if constexpr (DstConstructed) { - if (dst) { + if (dst && src) { + // dst and src both hold success values, so update dst from src and return + if constexpr (!std::is_same_v) { + ResultTraits::Update(*dst, *src); + } + return; + } else if (!dst && !src) { + // dst and src both hold failure values, so update dst from src and return + if constexpr (!std::is_same_v) { + ResultTraits::Update(dst.GetFailure(), src.GetFailure()); + } + return; + } else if (dst) { // dst has a success value, so destroy it before replacing it with src failure value if constexpr (!std::is_same_v) { using DstSuccess = typename DstResult::SuccessType; @@ -277,10 +366,40 @@ class Result : public detail::SuccessHolder result; +//! auto r1 = DoSomething() >> result; +//! auto r2 = DoSomethingElse() >> result; +//! ... +//! return result; +//! +template +requires (std::decay_t::is_result) +decltype(auto) operator>>(SrcResult&& src LIFETIMEBOUND, DstResult&& dst) +{ + using SrcType = std::decay_t; + if (src.GetMessages() && MessagesTraits::HasMessages(*src.GetMessages())) { + ResultTraits::Update(dst.EnsureMessages(), *src.GetMessages()); + } + return static_cast(src); +} + +//! Join error and warning messages in a space separated string. This is +//! intended for simple applications where there's probably only one error or +//! warning message to report, but multiple messages should not be lost if they +//! are present. More complicated applications should use Result::GetMessages() +//! method directly. template bilingual_str ErrorString(const Result& result) { - return !result && result.GetMessages() ? *result.GetMessages() : bilingual_str{}; + const auto* messages{result.GetMessages()}; + return messages ? detail::JoinMessages(*messages) : bilingual_str{}; } } // namespace util