From 86607f813ddb7318fa58e8ce2d277f8421e7ff9f Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Tue, 6 Sep 2022 11:10:25 -0400
Subject: [PATCH] Add util::ResultPtr class

The util::ResultPtr class is a wrapper for util::Result providing syntax sugar
to make it less awkward to use with returned pointers. Instead of needing to be
deferenced twice with **result or (*result)->member, it only needs to be
dereferenced once with *result or result->member. Also when it is used in a
boolean context, instead of evaluating to true when there is no error and the
pointer is null, it evaluates to false so it is straightforward to determine
whether the result points to something.

This PR is only partial a solution to #26004 because while it makes it easier
to check for null pointer values, it does not make it impossible to return a
null pointer value inside a successful result. It would be possible to enforce
that successful results always contain non-null pointers by using a not_null
type (https://github.com/bitcoin/bitcoin/issues/24423) in combination with
ResultPtr, though.
---
 src/test/result_tests.cpp | 35 +++++++++++++++++++++++++++++++++++
 src/util/result.h         | 27 +++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp
index 595de9028bc..327625f90d3 100644
--- a/src/test/result_tests.cpp
+++ b/src/test/result_tests.cpp
@@ -275,7 +275,42 @@ util::Result<std::unique_ptr<NoCopyNoMove>> DerivedToBaseFn(util::Result<std::un
 
 BOOST_AUTO_TEST_CASE(derived_to_base)
 {
+    // Check derived to base conversions work for util::Result
     BOOST_CHECK_EQUAL(*Assert(*Assert(DerivedToBaseFn(std::make_unique<Derived>(5)))), 5);
+
+    // Check conversions work between util::Result and util::ResultPtr
+    util::ResultPtr<std::unique_ptr<Derived>> derived{std::make_unique<Derived>(5)};
+    util::ResultPtr<std::unique_ptr<NoCopyNoMove>> base{DerivedToBaseFn(std::move(derived))};
+    BOOST_CHECK_EQUAL(*Assert(base), 5);
+}
+
+//! For testing ResultPtr, return pointer to a pair of ints, or return pointer to null, or return an error message.
+util::ResultPtr<std::unique_ptr<std::pair<int, int>>> PtrFn(std::optional<std::pair<int, int>> i, bool success)
+{
+    if (success) return i ? std::make_unique<std::pair<int, int>>(*i) : nullptr;
+    return util::Error{Untranslated(strprintf("PtrFn(%s) error.", i ? strprintf("%i, %i", i->first, i->second) : "nullopt"))};
+}
+
+BOOST_AUTO_TEST_CASE(check_ptr)
+{
+    auto result_pair = PtrFn(std::pair{1, 2}, true);
+    ExpectResult(result_pair, true, {});
+    BOOST_CHECK(result_pair);
+    BOOST_CHECK_EQUAL(result_pair->first, 1);
+    BOOST_CHECK_EQUAL(result_pair->second, 2);
+    BOOST_CHECK(*result_pair == std::pair(1,2));
+
+    auto result_null = PtrFn(std::nullopt, true);
+    ExpectResult(result_null, true, {});
+    BOOST_CHECK(!result_null);
+
+    auto result_error_pair = PtrFn(std::pair{1, 2}, false);
+    ExpectResult(result_error_pair, false, Untranslated("PtrFn(1, 2) error."));
+    BOOST_CHECK(!result_error_pair);
+
+    auto result_error_null = PtrFn(std::nullopt, false);
+    ExpectResult(result_error_null, false, Untranslated("PtrFn(nullopt) error."));
+    BOOST_CHECK(!result_error_null);
 }
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/util/result.h b/src/util/result.h
index f4ec3be623e..dc7ee05d686 100644
--- a/src/util/result.h
+++ b/src/util/result.h
@@ -390,6 +390,33 @@ decltype(auto) operator>>(SrcResult&& src LIFETIMEBOUND, DstResult&& dst)
     return static_cast<SrcType&&>(src);
 }
 
+//! Wrapper around util::Result that is less awkward to use with pointer types.
+//!
+//! It overloads pointer and bool operators so it isn't necessary to dereference
+//! the result object twice to access the result value, so it possible to call
+//! methods with `result->Method()` rather than `(*result)->Method()` and check
+//! whether the pointer is null with `if (result)` rather than `if (result &&
+//! *result)`.
+//!
+//! The `ResultPtr` class just adds syntax sugar to `Result` class. It is still
+//! possible to access the result pointer directly using `ResultPtr` `value()`
+//! and `has_value()` methods.
+template <typename T, typename F = void>
+class ResultPtr : public Result<T, F>
+{
+public:
+    // Inherit Result constructors (excluding copy and move constructors).
+    using Result<T, F>::Result;
+
+    // Inherit Result copy and move constructors.
+    template<typename O>
+    ResultPtr(O&& other) : Result<T, F>{std::forward<O>(other)} {}
+
+    explicit operator bool() const noexcept { return this->has_value() && this->value(); }
+    auto* operator->() const { assert(this->value()); return &*this->value(); }
+    auto& operator*() const { assert(this->value()); return *this->value(); }
+};
+
 //! 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