BResult improvements

ryanofsky · ryanofsky · commit 39769cc0599a · 2022-07-13T14:32:18.000-04:00
This is a draft commit trying to make some improvements to the `BResult` class to make it more usable. Adds a `util::Result` class with the following improvements (and implements `BResult` in terms of it so existing code doesn't have to change): - Supports returning error strings and error information at the same time. This functionality is needed by the `LoadChainstate` function in #25308 or any function that can fail in multiple ways which need to be handled differently. And it means Result class is compatible with rust-style error handling and pattern matching, unlike BResult. - Makes Result class provide same operators and methods as std::optional, so it doesn't require calling less familiar `HasRes`/`GetObj`/`ReleaseObj` methods, and so error reporting functionality can be easily added or dropped from existing code by switching between util::Result and std::optional. - Supports `Result<void>` return values, so it is possible to return error reporting information from functions in a uniform way even if they don't have other return values. - Supports `Result<bilingual_str>` return values. Drops ambiguous and potentially misleading `BResult` constructor that treats any bilingual string as an error, and any other type as a non-error. Adds `util::Error` constructor so errors have to be explicitly constructed and not any `bilingual_str` will be turned into one. - Puts `src/util/` code in the `util::` namespace so naming reflects code organization and it is obvious where the class is coming from. Drops "B" from name because it is undocumented what it stands for (bilingual?) - Adds unit tests.
diff --git a/src/Makefile.am b/src/Makefile.am
@@ -676,6 +676,7 @@ libbitcoin_util_a_SOURCES = \
   util/moneystr.cpp \
   util/rbf.cpp \
   util/readwritefile.cpp \
+  util/result.cpp \
   util/settings.cpp \
   util/thread.cpp \
   util/threadnames.cpp \
diff --git a/src/Makefile.test.include b/src/Makefile.test.include
@@ -118,6 +118,7 @@ BITCOIN_TESTS =\
   test/raii_event_tests.cpp \
   test/random_tests.cpp \
   test/rest_tests.cpp \
+  test/result_tests.cpp \
   test/reverselock_tests.cpp \
   test/rpc_tests.cpp \
   test/sanity_tests.cpp \
diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp
@@ -0,0 +1,106 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <util/result.h>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_AUTO_TEST_SUITE(result_tests)
+
+util::Result<void> VoidSuccessFn()
+{
+    return {};
+}
+
+util::Result<void> VoidFailFn()
+{
+    return {util::Error{}, Untranslated("void fail")};
+}
+
+util::Result<int> IntSuccessFn(int ret)
+{
+    return {ret};
+}
+
+util::Result<int> IntFailFn()
+{
+    return {util::Error{}, Untranslated("int fail")};
+}
+
+enum FnStatus { SUCCESS, ERR1, ERR2 };
+
+util::Result<FnStatus> StatusSuccessFn(FnStatus ret)
+{
+    return {ret};
+}
+
+util::Result<FnStatus> StatusFailFn(FnStatus ret)
+{
+    return {util::Error{}, Untranslated("status fail"), ret};
+}
+
+util::Result<int> ChainedFailFn(FnStatus arg, int ret)
+{
+    return {util::ErrorChain{}, Untranslated("chained fail"), StatusFailFn(arg), ret};
+}
+
+template<typename T>
+void ExpectSuccess(const util::Result<T>& result)
+{
+    BOOST_CHECK(result);
+}
+
+template<typename T>
+void ExpectFail(const util::Result<T>& result, bilingual_str str)
+{
+    BOOST_CHECK(!result);
+    BOOST_CHECK_EQUAL(ErrorDescription(result).original, str.original);
+    BOOST_CHECK_EQUAL(ErrorDescription(result).translated, str.translated);
+}
+
+template<typename T, typename... Args>
+void ExpectValue(const util::Result<T>& result, bool has_value, Args&&... args)
+{
+    BOOST_CHECK_EQUAL(result.value(), T{std::forward<Args>(args)...});
+    BOOST_CHECK_EQUAL(result.has_value(), has_value);
+    BOOST_CHECK_EQUAL(&result.value(), &*result);
+}
+
+template<typename T, typename... Args>
+void ExpectSuccessValue(const util::Result<T>& result, Args&&... args)
+{
+    ExpectSuccess(result);
+    ExpectValue(result, true, std::forward<Args>(args)...);
+}
+
+template<typename T, typename... Args>
+void ExpectFailValue(const util::Result<T>& result, bilingual_str str, Args&&... args)
+{
+    ExpectFail(result, str);
+    ExpectValue(result, false, std::forward<Args>(args)...);
+}
+
+BOOST_AUTO_TEST_CASE(check_returned)
+{
+    ExpectSuccess(VoidSuccessFn());
+    ExpectFail(VoidFailFn(), Untranslated("void fail"));
+    ExpectSuccessValue(IntSuccessFn(5), 5);
+    ExpectFail(IntFailFn(), Untranslated("int fail"));
+    ExpectSuccessValue(StatusSuccessFn(SUCCESS), SUCCESS);
+    ExpectFailValue(StatusFailFn(ERR2), Untranslated("status fail"), ERR2);
+    ExpectFailValue(ChainedFailFn(ERR1, 5), Untranslated("status fail, chained fail"), 5);
+}
+
+BOOST_AUTO_TEST_CASE(check_dereference_operators)
+{
+    util::Result<std::pair<int, std::string>> mutable_result;
+    const auto& const_result{mutable_result};
+    mutable_result.value() = {1, "23"};
+    BOOST_CHECK_EQUAL(mutable_result->first, 1);
+    BOOST_CHECK_EQUAL(const_result->second, "23");
+    (*mutable_result).first = 5;
+    BOOST_CHECK_EQUAL((*const_result).first, 5);
+}
+
+BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/util/result.cpp b/src/util/result.cpp
@@ -0,0 +1,14 @@
+// Copyright (c) 2022 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 <util/string.h>
+
+namespace util {
+bilingual_str ErrorDescription(const Result<void>& result)
+{
+    return Join(result.GetErrors(), Untranslated(", "));
+}
+} // namespace util
+
diff --git a/src/util/result.h b/src/util/result.h
@@ -6,37 +6,117 @@
 #define BITCOIN_UTIL_RESULT_H
 
 #include <util/translation.h>
-#include <variant>
+#include <optional>
 
-/*
- * 'BResult' is a generic class useful for wrapping a return object
- * (in case of success) or propagating the error cause.
-*/
-template<class T>
-class BResult {
-private:
-    std::variant<bilingual_str, T> m_variant;
+namespace util {
+
+//! Function result type intended for high-level functions that return error and
+//! warning strings in addition to normal result types.
+//!
+//! The Result<T> class is meant to be a drop-in replacement for
+//! std::optional<T> except it has additional methods to return error and
+//! warning strings for error reporting. Also, unlike std::optional, in order to
+//! support error handling in cases where callees need to pass additional
+//! information about failures back to callers, Result<T> objects can always be
+//! dereferenced regardless of the function's success or failure.
+//!
+//! This class is not intended to be used by low-level functions that do not
+//! return error or warning strings. These functions should use plain
+//! std::optional or std::variant types instead.
+//!
+//! See unit tests in result_tests.cpp for example usages.
+template<typename T>
+class Result;
+
+//! Tag types for result constructors.
+struct Error{};
+struct ErrorChain{};
+
+//! Result<void> specialization. Only returns errors and warnings, no values.
+template<>
+class Result<void>
+{
+protected:
+    std::vector<bilingual_str> m_errors;
+    std::vector<bilingual_str> m_warnings;
 
 public:
-    BResult() : m_variant(Untranslated("")) {}
-    BResult(const T& _obj) : m_variant(_obj) {}
-    BResult(const bilingual_str& error) : m_variant(error) {}
+    //! Success case constructor.
+    Result() = default;
 
-    /* Whether the function succeeded or not */
-    bool HasRes() const { return std::holds_alternative<T>(m_variant); }
+    //! Error case constructor for a single error.
+    Result(Error, bilingual_str error)
+    {
+        m_errors.emplace_back(std::move(error));
+    }
 
-    /* In case of success, the result object */
-    const T& GetObj() const {
-        assert(HasRes());
-        return std::get<T>(m_variant);
+    //! Error case constructor for a chained error.
+    Result(ErrorChain, bilingual_str error, Result<void>&& previous) : m_errors{std::move(previous.m_errors)}, m_warnings{std::move(previous.m_warnings)}
+    {
+        m_errors.emplace_back(std::move(error));
     }
 
-    /* In case of failure, the error cause */
-    const bilingual_str& GetError() const {
-        assert(!HasRes());
-        return std::get<bilingual_str>(m_variant);
+    //! Success check.
+    operator bool() const { return m_errors.empty(); }
+
+    //! Error retrieval.
+    const std::vector<bilingual_str>& GetErrors() const { return m_errors; }
+    std::tuple<const std::vector<bilingual_str>&, const std::vector<bilingual_str>&> GetErrorsAndWarnings() const { return {m_errors, m_warnings}; }
+};
+
+template<typename T>
+class Result : public Result<void>
+{
+protected:
+    T m_result;
+
+public:
+    //! Constructors that forward to the base class and pass additional arguments to m_result.
+    template<typename... Args>
+    Result(Args&&... args) : m_result{std::forward<Args>(args)...} {}
+    template<typename Str, typename...Args>
+    Result(Error, Str&& str, Args&&... args) : Result<void>{Error{}, std::forward<Str>(str)}, m_result{std::forward<Args>(args)...} {};
+    template<typename Str, typename Prev, typename...Args>
+    Result(ErrorChain, Str&& str, Prev&& prev, Args&&... args) : Result<void>{ErrorChain{}, std::forward<Str>(str), std::forward<Prev>(prev)}, m_result{std::forward<Args>(args)...} {};
+
+    //! std::optional methods, so Result<T> can be easily swapped for
+    //! std::optional<T> to add error reporting to existing code or remove it if
+    //! it is no longer needed.
+    bool has_value() const { return m_errors.empty(); }
+    const T& value() const { return m_result; }
+    T& value() { return m_result; }
+    template<typename U> T value_or(const U& default_value) const
+    {
+        return has_value() ? value() : default_value;
     }
+    const T* operator->() const { return &value(); }
+    const T& operator*() const { return value(); }
+    T* operator->() { return &value(); }
+    T& operator*() { return value(); }
+};
+
+//! Helper method to retrieve a simple error string from Result<T> or
+//! Result<void>.
+bilingual_str ErrorDescription(const Result<void>& result);
+} // namespace util
 
+/**
+ * Backwards-compatible interface for util::Result class. New code should prefer
+ * util::Result class which supports returning error information along with
+ * result information and supports returing `void` and `bilingual_str` results.
+*/
+template<class T>
+class BResult {
+private:
+    util::Result<std::optional<T>> m_result;
+
+public:
+    BResult() : m_result{util::Error{}, Untranslated("")} {};
+    BResult(const T& value) : m_result{value} {}
+BResult(const bilingual_str& error) : m_result{util::Error{}, error} {}
+    bool HasRes() const { return m_result.has_value(); }
+    const T& GetObj() const { assert(HasRes()); return *m_result.value(); }
+    const bilingual_str& GetError() const { assert(!HasRes()); return m_result.GetErrors().back(); }
     explicit operator bool() const { return HasRes(); }
 };
 
