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

@@ -68,6 +68,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

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 <tinyformat.h>
 #include <util/check.h>
 #include <util/result.h>
+#include <util/translation.h>
 
+#include <algorithm>
 #include <boost/test/unit_test.hpp>
+#include <memory>
+#include <ostream>
+#include <string>
+#include <utility>
 
 inline bool operator==(const bilingual_str& a, const bilingual_str& b)
 {
@@ -90,15 +97,60 @@ enum FnError { ERR1, ERR2 };
 
 util::Result<int, FnError> 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<std::string, FnError> StrFailFn(int i, bool success)
+{
+    util::Result<std::string, FnError> 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<NoCopyNoMove, FnError> EnumFailFn(FnError ret)
 {
     return {util::Error{Untranslated("enum fail.")}, ret};
 }
 
+util::Result<void> WarnFn()
+{
+    return {util::Warning{Untranslated("warn.")}};
+}
+
+util::Result<int> MultiWarnFn(int ret)
+{
+    util::Result<int> result;
+    for (int i = 0; i < ret; ++i) {
+        result.AddWarning(Untranslated(strprintf("warn %i.", i)));
+    }
+    result.Update(ret);
+    return result;
+}
+
+util::Result<void, int> ChainedFailFn(FnError arg, int ret)
+{
+    util::Result<void, int> result{util::Error{Untranslated("chained fail.")}, ret};
+    EnumFailFn(arg) >> result;
+    WarnFn() >> result;
+    return result;
+}
+
+util::Result<int, FnError> AccumulateFn(bool success)
+{
+    util::Result<int, FnError> result;
+    util::Result<int> x = MultiWarnFn(1) >> result;
+    BOOST_REQUIRE(x);
+    util::Result<int> y = MultiWarnFn(2) >> result;
+    BOOST_REQUIRE(y);
+    result.Update(IntFailFn(*x + *y, success));
+    return result;
+}
+
 util::Result<int, int> TruthyFalsyFn(int i, bool success)
 {
     if (success) return i;
@@ -142,7 +194,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);
@@ -158,7 +216,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<NoCopy, NoCopy> result2{0};
@@ -166,7 +224,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)
@@ -190,6 +248,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<void> 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;
 };

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

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 <util/result.h>
+
+#include <algorithm>
+#include <initializer_list>
+#include <iterator>
+#include <util/translation.h>
+
+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<Messages>::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

src/util/result.h

@@ -17,9 +17,15 @@
 #include <vector>
 
 namespace util {
+//! Default MessagesType, simple list of errors and warnings.
+struct Messages {
+    std::vector<bilingual_str> errors{};
+    std::vector<bilingual_str> warnings{};
+};
+
 //! The Result<SuccessType, FailureType, MessagesType> 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 <typename SuccessType = void, typename FailureType = void, typename MessagesType = bilingual_str>
+template <typename SuccessType = void, typename FailureType = void, typename MessagesType = Messages>
 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<typename T>
+struct ResultTraits {
+    template<typename O>
+    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<typename MessagesType>
+struct MessagesTraits;
+
+//! ResultTraits specialization for Messages struct.
+template<>
+struct ResultTraits<Messages> {
+    static void Update(Messages& dst, Messages& src);
+};
+
+//! MessagesTraits specialization for Messages struct.
+template<>
+struct MessagesTraits<Messages> {
+    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 @@ public:
     static constexpr bool is_result{true};
 
     //! Construct a Result object setting a success or failure value and
-    //! optional error messages. Initial util::Error arguments are processed
+    //! optional warning and error messages. Initial util::Error and
-    //! first to add error messages.
+    //! util::Warning arguments are processed first to add warning and error
-    //! Then, any remaining arguments are passed to the SuccessType
+    //! messages. Then, any remaining arguments are passed to the SuccessType
     //! constructor and used to construct a success value in the success case.
     //! In the failure case, if any util::Error arguments were passed, any
     //! remaining arguments are passed to the FailureType constructor and used
@@ -171,7 +229,7 @@ public:
     }
 
     //! Move-construct a Result object from another Result object, moving the
-    //! success or failure value and any error message.
+    //! success or failure value and any error or warning messages.
     template <typename O>
     requires (std::decay_t<O>::is_result)
     Result(O&& other)
@@ -179,7 +237,10 @@ public:
         Move</*Constructed=*/false>(*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</*Constructed=*/true>(*this, other);
@@ -187,11 +248,18 @@ public:
     }
 
     //! Disallow operator= and require explicit Result::Update() calls to avoid
-    //! confusion in the future when the Result class gains support for richer
+    //! accidentally clearing error and warning messages when combining results.
-    //! error reporting, and callers should have ability to set a new result
-    //! value without clearing existing error messages.
     Result& operator=(Result&&) = delete;
 
+    void AddError(bilingual_str error)
+    {
+        if (!error.empty()) MessagesTraits<MessagesType>::AddError(this->EnsureFailData().messages, std::move(error));
+    }
+    void AddWarning(bilingual_str warning)
+    {
+        if (!warning.empty()) MessagesTraits<MessagesType>::AddWarning(this->EnsureFailData().messages, std::move(warning));
+    }
+
 protected:
     template <typename, typename, typename>
     friend class Result;
@@ -216,12 +284,22 @@ protected:
     template <bool Failure, typename Result, typename... Args>
     static void Construct(Result& result, util::Error error, Args&&... args)
     {
-        result.EnsureFailData().messages = std::move(error.message);
+        result.AddError(std::move(error.message));
         Construct</*Failure=*/true>(result, std::forward<Args>(args)...);
     }
 
+    //! Construct() overload peeling off a util::Warning constructor argument.
+    template <bool Failure, typename Result, typename... Args>
+    static void Construct(Result& result, util::Warning warning, Args&&... args)
+    {
+        result.AddWarning(std::move(warning.message));
+        Construct<Failure>(result, std::forward<Args>(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 @@ protected:
     template <bool DstConstructed, typename DstResult, typename SrcResult>
     static void Move(DstResult& dst, SrcResult& src)
     {
-        // Move messages values first, then move success or failure value below.
+        // Use operator>> to move messages value first, then move
-        if (src.GetMessages() && !src.GetMessages()->empty()) {
+        // success or failure value below.
-            dst.EnsureMessages() = std::move(*src.GetMessages());
+        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<typename SrcResult::SuccessType, void>) {
+                    ResultTraits<typename SrcResult::SuccessType>::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<typename SrcResult::FailureType, void>) {
+                    ResultTraits<typename SrcResult::FailureType>::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<typename DstResult::SuccessType, void>) {
                     using DstSuccess = typename DstResult::SuccessType;
@@ -277,10 +366,40 @@ protected:
     }
 };
 
+//! Move information from a source Result object to a destination object. It
+//! only moves MessagesType values without affecting SuccessType or
+//! FailureType values of either Result object.
+//!
+//! This is useful for combining error and warning messages from multiple result
+//! objects into a single object, e.g.:
+//!
+//!    util::Result<void> result;
+//!    auto r1 = DoSomething() >> result;
+//!    auto r2 = DoSomethingElse() >> result;
+//!    ...
+//!    return result;
+//!
+template <typename SrcResult, typename DstResult>
+requires (std::decay_t<SrcResult>::is_result)
+decltype(auto) operator>>(SrcResult&& src LIFETIMEBOUND, DstResult&& dst)
+{
+    using SrcType = std::decay_t<SrcResult>;
+    if (src.GetMessages() && MessagesTraits<typename SrcType::MessagesType>::HasMessages(*src.GetMessages())) {
+        ResultTraits<typename SrcType::MessagesType>::Update(dst.EnsureMessages(), *src.GetMessages());
+    }
+    return static_cast<SrcType&&>(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 <typename Result>
 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