refactor: Add util::Result failure values, multiple error and warning messages #25665

This pull changes both the interface, as well as the internal representation. I wonder if changing the interface and renames can be done separate from the restructuring.

Hopefully those would be uncontroversial and could be reviewed easier/faster, so that less silent merge conflicts arise as the class gets used more and more. Also, less code will need to be changes if the interface changes are made earlier rather than later.

This pull changes both the interface, as well as the internal representation. I wonder if changing the interface and renames can be done separate from the restructuring.

Only the third commit drops BResult usages, so the answer should be yes. IMO std::optional has a nice interface and it is a shame to reinvent it with stranger HasRes GetObj ReleaseObj methods.

With separate I meant separate pulls sweat_smile

Oh sorry, I thought you were asking literally if it is possible

I think the stuff that can be split out:

• Makes Result class provide same operators and methods as std::optional
• 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?)
• 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.

re: #25665 (review)

Yeah, I think that makes sense, so that we can first figure out the interface, and then start using it more widely, not the other way round.

Thanks, moved to #25721 now and rewrote PR description

nit:

0    bilingual_str result{ErrorString(errors)};

re: #25665 (review)

nit:

Thanks, used suggestion.

re: #25665 (review)

I’m not sure why iwyu doesn’t catch it, but shouldn’t forward, types and utility also be included here?

Thanks, added utility. (I think the others are nonstandard.) IWYU might not catch references to code in templates that aren’t instantiated, so I’m not too surprised it didn’t add this.

re: #25665 (review)

Could probably make sense to split this up and use it there: #25616 (comment)

Is the suggestion to add the GetErrors() method in this PR, but delay adding the GetWarnings() method until followup PR #25722? I could do that, but I don’t think it would simplify this PR very much (it would remove a few lines) and it would add more churn to result implementation/documentation/unit tests review

re: #25665 (review)

This would allow reviewers to just look at the GetWarnings implementation, API, and proposed usage, without having to think about union/void/... etc.

Oh. I guess there could be a third PR independent of this PR and #25722 that adds a std::vector<bilingual_str> m_warnings; field to util::Result and AddWarning()/GetWarnings() methods. I don’t think it would be as useful as you might be thinking without template constructors that can move errors & warnings from one result object to another, though.

Maybe if you are interested you could open a PR that does this? I’d be happy to review it. I’m just not sure exactly what you want to do here because the goal of #25722 is a lot broader. Not just dropping std::vector<bilingual_str>& warnings output arguments but also dropping DatabaseStatus& status output arguments and returning errors and warnings directly from nested calls.

The thing is that you are basically adding a ton of dead code in this pull. bitcoind compiles fine without it, so I might be picky, but my preference would be to only add the code when it is needed. This also makes it easier for reviewers because they can think about actual use cases and don’t have to imagine them from the unit tests.

  0diff --git a/src/util/result.cpp b/src/util/result.cpp
  1index 9526b5b785..199a6579d7 100644
  2--- a/src/util/result.cpp
  3+++ b/src/util/result.cpp
  4@@ -3,26 +3,8 @@
  5 // file COPYING or https://www.opensource.org/licenses/mit-license.php.
  6 
  7 #include <util/result.h>
  8-#include <util/string.h>
  9 
 10 namespace util {
 11 namespace detail {
 12-bilingual_str JoinMessages(const std::vector<bilingual_str>& errors, const std::vector<bilingual_str> warnings)
 13-{
 14-    bilingual_str result;
 15-    for (const auto& messages : {errors, warnings}) {
 16-        for (const auto& message : messages) {
 17-            if (!result.empty()) result += Untranslated(" ");
 18-            result += message;
 19-        }
 20-    }
 21-    return result;
 22-}
 23-
 24-void MoveMessages(std::vector<bilingual_str>& src, std::vector<bilingual_str>& dest)
 25-{
 26-    dest.insert(dest.end(), std::make_move_iterator(src.begin()), std::make_move_iterator(src.end()));
 27-    src.clear();
 28-}
 29 } // namespace detail
 30 } // namespace util
 31diff --git a/src/util/result.h b/src/util/result.h
 32index 32fe40763f..8eb5b552d3 100644
 33--- a/src/util/result.h
 34+++ b/src/util/result.h
 35@@ -17,21 +17,11 @@
 36 
 37 namespace util {
 38 namespace detail {
 39-//! Empty string list
 40-const std::vector<bilingual_str> EMPTY_LIST{};
 41-
 42-//! Helper function to join messages in space separated string.
 43-bilingual_str JoinMessages(const std::vector<bilingual_str>& errors, const std::vector<bilingual_str> warnings);
 44-
 45-//! Helper function to move messages from one vector to another.
 46-void MoveMessages(std::vector<bilingual_str>& src, std::vector<bilingual_str>& dest);
 47-
 48-//! Error information only allocated if there are errors or warnings.
 49+//! Error information only allocated if there is an error.
 50 template <typename F>
 51 struct ErrorInfo {
 52     std::optional<std::conditional_t<std::is_same<F, void>::value, std::monostate, F>> failure{};
 53-    std::vector<bilingual_str> errors;
 54-    std::vector<bilingual_str> warnings;
 55+    bilingual_str error;
 56 };
 57 
 58 //! Result base class which is inherited by Result<T, F>.
 59@@ -66,25 +56,7 @@ protected:
 60 public:
 61     void AddError(bilingual_str error)
 62     {
 63-        if (!error.empty()) Info().errors.emplace_back(std::move(error));
 64-    }
 65-
 66-    void AddWarning(bilingual_str warning)
 67-    {
 68-        if (!warning.empty()) Info().warnings.emplace_back(std::move(warning));
 69-    }
 70-
 71-    //! Operator moving warning and error messages from other result to this.
 72-    //! Only moves message strings, does not change success or failure values of
 73-    //! either Result object.
 74-    template<typename O>
 75-    O&& operator<<(O&& other)
 76-    {
 77-        if (other.m_info) {
 78-            if (!other.m_info->errors.empty()) MoveMessages(other.m_info->errors, Info().errors);
 79-            if (!other.m_info->warnings.empty()) MoveMessages(other.m_info->warnings, Info().warnings);
 80-        }
 81-        return std::forward<O>(other);
 82+        Info().error = std::move(error);
 83     }
 84 
 85     //! Success check.
 86@@ -93,8 +65,7 @@ public:
 87     //! Error retrieval.
 88     template <typename _F = F>
 89     std::enable_if_t<!std::is_same<_F, void>::value, const _F&> GetFailure() const { assert(!*this); return *m_info->failure; }
 90-    const std::vector<bilingual_str>& GetErrors() const { return m_info ? m_info->errors : EMPTY_LIST; }
 91-    const std::vector<bilingual_str>& GetWarnings() const { return m_info ? m_info->warnings : EMPTY_LIST; }
 92+    bilingual_str GetError() const { return m_info ? m_info->error : bilingual_str{}; }
 93 };
 94 
 95 //! Result base class for T value type. Holds value and provides accessor methods.
 96@@ -145,9 +116,6 @@ public:
 97 struct Error {
 98     bilingual_str message;
 99 };
100-struct Warning {
101-    bilingual_str message;
102-};
103 
104 //! The util::Result class provides a standard way for functions to return error
105 //! and warning strings in addition to optional result values.
106@@ -190,23 +158,11 @@ protected:
107         }, std::forward<Args>(args)...);
108     }
109 
110-    template <typename Fn, typename... Args>
111-    void Construct(const Fn& fn, Warning warning, Args&&... args)
112-    {
113-        this->AddWarning(std::move(warning.message));
114-        Construct(fn, std::forward<Args>(args)...);
115-    }
116-
117-    template <typename Fn, typename OT, typename OF, typename... Args>
118-    void Construct(const Fn& fn, Result<OT, OF>&& other, Args&&... args)
119-    {
120-        *this << other;
121-        Construct(fn, std::forward<Args>(args)...);
122-    }
123-
124     void MoveConstruct(Result& other)
125     {
126-        *this << other;
127+        if (other.m_info) {
128+            this->AddError(std::move(other.m_info->error));
129+        }
130         if (other) this->MoveValue(other); else this->Info().failure = std::move(other.m_info->failure);
131     }
132 
133@@ -233,7 +189,7 @@ public:
134 //! are present. More complicated applications should use GetErrors() and
135 //! GetWarning() methods directly.
136 template <typename T, typename F>
137-bilingual_str ErrorString(const Result<T, F>& result) { return detail::JoinMessages(result.GetErrors(), result.GetWarnings()); }
138+bilingual_str ErrorString(const Result<T, F>& result) { return result.GetError(); }
139 } // namespace util
140 
141 #endif // BITCOIN_UTIL_RESULT_H

re: #25665 (review)

The thing is that you are basically adding a ton of dead code in this pull

Interesting and thanks for the diff! Now I understand what you are suggesting, and I think I’d be ok with making that change. I wasn’t really looking at that code as dead because I’m immediately using it in the next pull #25722 and also using it in unit tests this PR. I also didn’t consider it to be a ton of code, since the code in your diff is just getter/setter functions that provide access to errors/warnings variables. But no objection to splitting it off into another pull.

re: #25665 (review)

Yeah, I don’t feel too strong as well. Though, if you keep it, it would be good to fixup the typo: #25665 (comment)

Ok, I’m hedging right now by moving multiple error & warning support into a separate commit but not a separate PR. I maybe have a slight preference for just squashing the commits again to reduce churn, but I’m also fine with keeping separate commits, or moving the commit to another PR if one of those alternatives seems better, so let me know!

Also fixed the typo

What is the difference between when the this->m_info is true and when it is false?

The implementation is optimized for the “happy path” where a success value is set and there is no failure value and no error or warning message strings. In the happy path case, m_info is null and no allocation is needed. Otherwise, if there has been any kind of error or warning m_info will be non-null.

While as far as I understand in both cases the this->m_info is destroyed and replaced by other.m_info. Am I correct?

No that’s not really correct. As the comment “Operator moving warning and error messages from one result to another” says, only warning and error strings are moved from other to *this. The success and value statuses of other and *this objects are unchanged, and the success and failure values of both objects (if any) are also unchanged. Existing warning and error strings in *this are also not affected, the new errors and warnings just get appended and don’t replace existing one.

So why bother to check other.m_info && !this->m_info and other.m_info && this->m_info?

I’m not sure what other code you might expect to see here, but in both cases this is just moving other.m_info->errors strings to this->m_info->errors and moving other.m_info->warnings strings to this->m_info->warnings while not deleting any existing strings and not changing other.m_info->failure and this->m_info->failure values.

I don’t understand why the m_value is in a union while there is only one member in the union? Would you explain why?

I’ll add a code comment, but using a union avoids m_value getting constructor and destructor being called automatically, so in the failure case m_value is never constructed.

Perhaps could take the opportunity to make this something like:

0enum class ChainstateLoadError { FAILURE_TRY_REINDEX, FAILURE_FATAL, INTERRUPTED };

So that it’s more inline with indicating “what to do about the failure”.

Could also move the “4 types of outcomes” comment up here where it’s clearer.

re: #25665 (review)

So that it’s more inline with indicating “what to do about the failure”.

I agree it’s good to suggest how failures can be handled, so I tried to do that in the comment. But I think if failure has a clear cause it’s best for the error name to describe the cause, especially since not every application will want to handle errors the same way, and bitcoin-qt, bitcoind, and bitcoin-chainstate all handle errors differently.

I’m also don’t think FAILURE_TRY_REINDEX is an appropriate synonym for FAILURE, since generic failures can happen on any exception, even if reindexing was already requested. Trying to reindex when reindexing fails does not make a lot of sense. I’d be happy to rename FAILURE to FAILURE_GENERIC. But would probably do it in a separate PR to not complicate this one.

Could also move the “4 types of outcomes” comment up here where it’s clearer.

Thanks for pointing it out. I think that comment actually does not add much information anymore, and is a little confusing because it is mentioning shutdowns when the check_interrupt callback and INTERRUPTED error could indicate any custom interruption, not just a shutdown.

I can add more documentation if you think anything is missing, but I think a simple message of “It is fine to treat any error code as a failure and ignore the specific cause” is the most helpful takeaway. Setting interrupt callbacks and trying to reindex are optional enhancements mostly helpful for interactive applications.

re: #25665 (review)

This comment is no longer true.

Thanks, removed

re: #25665 (review)

Would it be sufficient to use explicit operator bool() const instead here? That would avoid e.g. “(result + 3)” from compiling (and evaluating to 3 or 4).

Good catch, added explicit. This was an unintended regression (operator bool was explicit before this PR)

re: #25665 (review)

Do we want to expose these 2 helper methods in the header?

Nope, good point. Moved these to the details namespace.

What about renaming this function to ErrorWarningString() since at the moment it can contain both? I can see some scenario where people would want to access just the error but not the warning string (and of course it’s just a convenience fn), so that would allow them to create the more narrow ErrorString() later on?

Perhaps a brief docstring above this fn would be beneficial as well since that’s the one people will mostly use.

re: #25665 (review)

What about renaming this function to ErrorWarningString() since at the moment it can contain both? I can see some scenario where people would want to access just the error but not the warning string (and of course it’s just a convenience fn), so that would allow them to create the more narrow ErrorString() later on?

Added some more documentation about how this should be used. I think a function that just puts errors not warnings in a result message could be a potential footgun, because warnings could be unintentionally dropped if errors and warnings are returned together. For example if code initially doesn’t generate any warnings, then someone adds one not realizing it won’t show up anywhere.

I didn’t think about the footgun, that’s a good point why we probably indeed won’t want to add the other fn later. I still think it’s a bit awkward that the function name doesn’t entirely capture what the function is doing, but I think it’s within reason and may be worth the trade-off for brevity (vs ErrorWarningString(). Not sure what I prefer, so feel free to ignore.

The new docstring is great, thanks!

re: #25665 (review)

I still think it’s a bit awkward that the function name doesn’t entirely capture what the function is doing, but I think it’s within reason and may be worth the trade-off for brevity (vs ErrorWarningString(). Not sure what I prefer, so feel free to ignore.

Could call it util::MessageString(result) instead of util::ErrorString(result). I think the longer name would be ok too. I was pushing back more against changing the functionality than changing the name.

I do think any renaming should happen in a separate PR, before or after this one. The function already exists and is called in current code. This PR is backwards compatible just extends the Result API without requiring changes to existing code.

The function already exists and is called in current code.

Right, I did not consider that. Don’t think it’s worth the extra PR without more demand for it so I’m happy to just leave it at ErrorString until then.

re: #25665 (review)

nit: I think this docstring is more relevant for m_info, perhaps move it to above its declaration in ResultBase?

I think it’s relevant here because the struct is pretty big thing to have in a return type, so good to say it won’t typically be needed.

Also, this may be a stylistic thing, but I tend to prefer class-level documentation that gets at why/how questions, over member level documentation only answers more basic what questions, and can also clutter the class definition.

re: #25665 (review)

Maybe I’m confused, but I don’t think I agree. Since the optional allocation of an ErrorInfo is a result of how Info() is implemented, not of how the ErrorInfo struct is defined - isn’t that where it should be documented?

I’m only pushing back against removing this comment here. I’m happy to add more documentation anywhere would would like.

I think the answer to your question is no, because this isn’t a general purpose API. This is a custom, private struct that is never exposed externally and exists for one purpose. The comment is describing what the purpose is. If I were reading this code and saw this struct, I would be wondering why there is such a heavyweight struct being used in a lightweight result type, why error information is segregated from normal result information, and why a separate struct definition is needed at all instead using normal class members. This comment explains what the purpose of the struct is, why it exists and how it is used, and I think is appropriate documentation for an single-purpose piece of a larger implementation.

nit: You could add a bit more info on T, F for quick understanding?

0//! Result base class which is inherited by Result<T, F>.
1//! T is the type of the success return value, or void if there is none.
2//! F is the type of the failure return value, or void if there is none.

re: #25665 (review)

nit: You could add a bit more info on T, F for quick understanding?

Added. Nice suggestion!

This is repeated a few times, what do you think about this:

 0diff --git a/src/util/result.h b/src/util/result.h
 1index 60e0b3db6..c6aa65891 100644
 2--- a/src/util/result.h
 3+++ b/src/util/result.h
 4@@ -56,17 +56,19 @@ public:
 5     void AddError(bilingual_str error)
 6     {
 7         if (error.empty()) return;
 8-        if (!m_info) m_info = std::make_unique<ErrorInfo<F>>();
 9+        CheckInitInfo();
10         m_info->errors.emplace_back(std::move(error));
11     }
12 
13     void AddWarning(bilingual_str warning)
14     {
15         if (warning.empty()) return;
16-        if (!m_info) m_info = std::make_unique<ErrorInfo<F>>();
17+        CheckInitInfo();
18         m_info->warnings.emplace_back(std::move(warning));
19     }
20 
21+    void CheckInitInfo() { if (!m_info) m_info = std::make_unique<ErrorInfo<F>>(); }
22+
23     //! Success check.
24     explicit operator bool() const { return !m_info || !m_info->failure; }
25 
26@@ -159,7 +161,7 @@ protected:
27     {
28         this->AddError(std::move(error.message));
29         Construct([&](auto&&... x) {
30-            if (!this->m_info) this->m_info = std::make_unique<detail::ErrorInfo<F>>();
31+            this->CheckInitInfo();
32             this->m_info->failure.emplace(std::forward<decltype(x)>(x)...);
33         }, std::forward<Args>(args)...);
34     }

re: #25665 (review)

This is repeated a few times, what do you think about this:

Nice catch! I implemented something similar to what you suggested to dedup

re: #25665 (review)

As part of the public interface, I think these could benefit from a very brief per-method docstring (that probably just mirrors the std::optional docstring)?

I’m a little reluctant to change this code here because it is just moving (not new), but would encourage you to submit a separate PR to add more documentation to existing code if it would be helpful. I’m happy to rebase this as other PR are merged.

I do personally tend not to like documenting individual class members if it’s obvious what they do, and save comments for when there is something non-obvious or higher level to point out, but I wouldn’t have any objection to more detailed documentation.

nit: slightly more concise and clear on intent imo

0    bool has_value() const { return bool(*this); }

re: #25665 (review)

nit: slightly more concise and clear on intent imo

Ok! I would have defaulted to that but had seen some feedback bool() was not good #25616 (review), #25721 (review) so was trying to avoid it. Switched to bool{} for now.

re: #25665 (review)

I think it would be helpful to also test success with a falsy non-void value and failure with a truthy non-void value

Makes sense, added tests

re: #25665 (review)

I think this line is redundant since we already call this->AddError() right above?

I think it was needed to avoid a segfault if the error message was empty, but in any case this line is gone now from implementing your earlier suggestion to dedup this.

re: #25665 (review)

nit: I think this should be on a new line

I dropped the else if here since it was redundant, but I think I do prefer the compact style with one line

nit: ternary looks more appropriate?

0        *this ? this->DestroyValue() : this->m_info->failure.reset();

re: #25665 (review)

nit: ternary looks more appropriate?

I think I prefer if statement to ternary here, and I’m not used to seeing ternary statements rather than expressions in the codebase, but would be happy to change if there are other opinions.

Hmm you’re right, we’re not using ternaries as statements it seems. I couldn’t find a single instance of a one-line if ...; else either though, and unfortunately the developer notes are rather ambiguous on the topic.

In that case I’d prefer if and else on separate lines (personal preference and to not be the first in the repo) but no strong view so won’t comment on it further.

nit:

0    //! Operator moving warning and error messages from other Result to this.

re: #25665 (review)

nit:

Thanks, that’s clearer

re: #25665 (review)

Since ErrorInfo doesn’t have a move constructor, do the move semantics make sense here? (I’m still easily confused by move semantics, so I’m probably missing something - sorry)

ErrorInfo should have an implicit ErrorInfo(ErrorInfo&&) move constructor, but it actually isn’t relevant here, because the new ErrorInfo object that’s being allocated isn’t being constructed with that constructor. Instead the newly allocated ErrorInfo is aggregate initialized, and the two errors and warnings variables are individually move-constructed, rather than the whole ErrorInfo object being move constructed.

If the question here is whether the std::move calls here have any effect, the answer is yes they do because errors and warnings variables are vectors, and vectors have move constructors.

Thank you for the thoughtful response, that was very helpful. Your usage of move semantics here now makes sense to me.

Now I’m just confused that the designated initialization compiles fine even though the spec says it’s since C++20 and I haven’t configured with --enable-c++20 (as does the CI I think)?

Now I’m just confused that the designated initialization compiles fine even though the spec says it’s since C++20 and I haven’t configured with --enable-c++20 (as does the CI I think)?

Oh, we are just outside of the c++17 spec there. The build enables designated initializers as an extension since #24531

@ryanofsky the following change fixes the warnings for me, but i don’t know if this is the best way to deal with this

0    std::optional<std::conditional_t<std::is_same<F, void>::value, std::monostate, F>> failure = std::nullopt;

re: #25665 (review)

@ryanofsky the following change fixes the warnings for me, but i don’t know if this is the best way to deal with this

Thanks! I think it might be sufficient to replace = std::nullopt with {}, so I will try that first.

re: #25665 (review)

Since we’re expecting T to be a container type, would this benefit from living in span.h for reusability? The generic implementation/naming isn’t really necessary in result.h, and people won’t come looking here when they need something to move elements between containers.

It would be ok to move this somewhere, especially if something else was calling it. But I don’t think span.h would be the right place since this shouldn’t work on a span due to spans having fixed size. It also seems ok to me to keep this as a private helper function in a detail namespace as long as it is only called one place.

Sorry for the ghost comment. I had realized after posting that this comment about moving it to span.h wasn’t really applicable so I removed it, but apparently not timely enough. I agree with you.

The only further thought I had was that if there’s no need to be generic (yet - and maybe never) it might be worth not doing that to simplify things a bit where possible? E.g. the below is easier to quickly understand imo (and maybe compiles a bit faster?):

0//! Helper to move warnings and errors from one ErrorInfo to another.
1void MoveMessages(std::vector<bilingual_str>& src, std::vector<bilingual_str>& dest)
2{
3    dest.insert(dest.end(), std::make_move_iterator(src.begin()), std::make_move_iterator(src.end()));
4    src.clear();
5}

No strong views either way though, so feel free to ignore. Just trying to minimize on what’s already a lot of genericness.

re: #25665 (review)

Interesting! I do think the generic version is shorter and easier to understand. But I like moving more code from the .h file to the .cpp file, and I like the consistency between JoinMessages and MoveMessages, so I took this suggestion. Thanks!

re: #25665 (review)

In case you don’t remove this, this is wrong anyway, as it is missing a & in the second argument

Thanks, removed from the main commit and added&

nit: Can use is_same_v instead of ...::value? Also, missing LIFETIMEBOUND?

(Same feedback on other lines where this is applicable)

re: #25665 (review)

nit: Can use is_same_v instead of ...::value? Also, missing LIFETIMEBOUND?

Thanks, added these improvements

re: #25665 (review)

nit: EMPTY_LIST ( and #include <vector> ) seems unnecessary in this commit, think it belongs in “refactor: Add util::Result multiple error and warning messages”

Good catch! Removed

I think it’s a little confusing. If I understand correctly, template <typename F> class ResultBase<void, F> needs to know about template <typename T, typename F> class ResultBase; but the latter can only be declared after the former because it is a derived class. Therefore, a forward declaration is required.

If the ResultBase classes shouldn’t be used anywhere outside of result.h and their only purpose is to be the base class of Result, I would suggest merging them all into a single class.

Hmm, I wonder if you can say more about what is confusing or misleading. This is just a forward declaration for a template class.

It is true that the reason for the forward declaring ResultBase<T, F> is to allow it to inherit from ResultBase<void, F>. So ResultBase<T, F> must be defined after ResultBase<void, F>, but declared before it.

But this is a pretty standard thing for C and C++ code. Definition of one thing 1 depends on declaration of thing 2, so thing 2 needs to be forward declared. It can happen for normal classes and functions as well as templates.

If the ResultBase classes shouldn’t be used anywhere outside of result.h and their only purpose is to be the base class of Result, I would suggest merging them all into a single class.

This isn’t easily possible because Result<void, F> inherits directly from detail::ResultBase<void, F> and does not inherit from detail::ResultBase<T, F>. It does not have an m_value member or value() functions or a dereferencing operator. If the result type T is void the Result class doesn’t hold a value and can’t be dereferenced.

Since the friend declaration is gone, I think this should be inline?

0inline bilingual_str ErrorString(const Result<T, F>& result) { return detail::JoinMessages(result.GetErrors(), result.GetWarnings()); }

re: #25665 (review)

Since the friend declaration is gone, I think this should be inline?

Yes, makes sense to be inline.

nit: typo

0    //! Value accessors that do nothing because this class has value type T=void.

re: #25665 (review)

nit: typo

Thanks, fixed

nit in https://github.com/bitcoin/bitcoin/commit/3507da864a1dd7be1bc72ada26d830a4da0c37ae:

I think in C++17 you can remove all of the template and enable_if_t stuff and just write const auto& GetFailure() const for the return type.

re: #25665 (review)

I think in C++17 you can remove all of the template and enable_if_t stuff and just write const auto& GetFailure() const for the return type.

Thanks, that is better. Simplified now.

re: #25665 (review)

Hmm, I meant const auto&, not auto, or is auto in this context magically the same as const auto&?

No, I just messed up and unintentionally did a copy. Fixed this and added tests to make sure GetFailure() does not copy.

nit in c14e904f66505b3e89ca1138c8d2fa4e3d0916d0:

Can remove inline, as all template are inline by definition.

re: #25665 (review)

Can remove inline, as all template are inline by definition.

Thanks, removed inline

nit in 3507da864a1dd7be1bc72ada26d830a4da0c37ae:

Maybe add a comment to explain those a bit more? While the ResultBase() constructor leaves the object uninitialized, the Result constructor guarantees that the object is either filled with a value or an error.

re: #25665 (review)

Maybe add a comment to explain those a bit more? While the ResultBase() constructor leaves the object uninitialized, the Result constructor guarantees that the object is either filled with a value or an error.

Sure, I moved the DestroyValue call to this ~ResultBase destructor instead of the other Result constructor to make this more self contained and clear. Also added a comment explaining why the empty constructor is required. Hopefully these are improvements. Also happy to make other changes to clarify.

nit in 3507da864a1dd7be1bc72ada26d830a4da0c37ae:

doxygen will attach the comment to the constructor. In any case, I think this can be removed since it is not adding any info that isn’t already there.

re: #25665 (review)

nit in 3507da8:

doxygen will attach the comment to the constructor. In any case, I think this can be removed since it is not adding any info that isn’t already there.

Thanks, dropped. This was used to group methods in an earlier version of PR that had more methods below.

nit in 3507da864a1dd7be1bc72ada26d830a4da0c37ae:

Are those really “accessors”, not “setters”?

re: #25665 (review)

nit in 3507da8:

Are those really “accessors”, not “setters”?

Changed to setters (I thought accessors was a general term for getters and setters)

nit in 3507da864a1dd7be1bc72ada26d830a4da0c37ae:

Seems overkill to include variant, when it could be replaced by copy-pasting the one line:

0struct MonoState{}; // Similar to std::monostate

re: #25665 (review)

Seems overkill to include variant, when it could be replaced by copy-pasting the one line:

Thanks, dropped the dependency on variant

52a4e50fb4b1171ee0f6814b0a50bc70cdd77134: I don’t like that this takes over error strings, but leaves the value and failure untouched. It seems fine to have a result and warnings, but having a result and also an error at the same time seems odd.

Same with operator=. I think this is the first time I’ve seen that after calling =, state is preserved from before = was called.

re: #25665 (review)

52a4e50: I don’t like that this takes over error strings, but leaves the value and failure untouched. It seems fine to have a result and warnings, but having a result and also an error at the same time seems odd.

Agree having both a result value and an error message should be avoided. Also having neither a result value nor an error message should be avoided. But there are tradeoffs around where and how strictly you want to enforce these things. The main thing currently enforcing value/error consistency is having a constructor that sets error messages and failure values at the same time and does not allow setting a result value, and a constructor that only sets a success value and does not allow setting an error message.

But this leaves open the question of what helpers functions like operator>> should do when they combine multiple result values that have already been constructed.

The use-case for operator>> is when you have an outer function returning Result<T, F> calling inner functions returning Result<T1, F1>, and Result<T2, F2>, etc. Examples would be LoadWalletInternal, CreateWallet, and DoMigration from #25722. The outer function can handle failures from inner functions it calls any way it wants: passing failure values up to its caller, translating failure values, ignoring failures, retrying after failures, falling back to an alternate approaches, etc. It sets success and failure values directly, and it can use operator>> to collect error and warning messages separately and pass them along. I don’t think operator>> should be involved in success and failure value handling. I think it would be bad if operator>> discarded error messages, or it it threw runtime exceptions, instead of just passing messages on to ultimately get displayed or logged.

In C++ generally operator>> is used for things as varied as bit shifting and stream I/O and can be interpreted as “move data from this place to another place” so I think it reasonable that this operator>> just moves error and warning messages from one Result object to another, as long as behavior is clearly documented.

Same with operator=. I think this is the first time I’ve seen that after calling =, state is preserved from before = was called.

Yes it is true that assigning to an existing result does not erase warning and errors messages already accumulated in the result. It only sets the success or failure value and appends new warning and error messages to existing ones.

I think this this behavior is safe and useful. Setting a value should not automatically erase warning and error messages that are meant to be displayed or logged. But if this behavior is too surprising for an operator= method, we don’t actually need to make Result assignable, and could rename operator=() to Set() or SetValue(). It looks like even after #25722 there are only 3 operator= calls in the codebase outside of tests, so this would be an easy change.

Also having neither a result value nor an error message should be avoided.

Well, this is already impossible for the reasons you gave. (Edit: When calling only the constructors)

Agree having both a result value and an error message should be avoided.

Then, why not make it impossible? If there is an outer function returning Result<T, F> that wants to combine Result<T1, F1> and Result<T2, F2>, then it seems better if it explicitly takes (moves) the result T1/F1 out and translates it into T/F. For example, if it passes up a failure value, it seems best to just create a fresh Result (of the outer type) with the failure and return that. (Same if it translates failure values). If it ignores failures, it would be good to translate them to warnings first and not blindly take them over as errors with the >> operator. (Same if it retries or falls back).

I think this this behavior is safe and useful.

Why couldn’t the same be achieved by explicitly constructing a new Result with either an error or a value?

Also having neither a result value nor an error message should be avoided.

Well, this is already impossible for the reasons you gave.

“Should be avoided” means that users should avoid it, and the implementation makes it easy to avoid by default. The reasons I gave were reasons why a function combining multiple result values needs to either (1) allow result values and error messages to exist at the same time or (2) discard error messages or result values or (3) throw exceptions. And I believe the best choice for operator>> is (1), just to be a plain message mover that moves message strings and leaves result values alone. I linked to use-cases showing how this works in practice.

Agree having both a result value and an error message should be avoided.

Then, why not make it impossible? If there is an outer function returning Result<T, F> that wants to combine Result<T1, F1> and Result<T2, F2>, then it seems better if it explicitly takes (moves) the result T1/F1 out and translates it into T/F. For example, if it passes up a failure value, it seems best to just create a fresh Result (of the outer type) with the failure and return that.

This is actually what the implementation does. But if the outer function returns success value, and there are error messages, I don’t think it is good default behavior to throw away the error messages or raise an exception. I think the best default behavior is to keep error messages so they can be displayed or logged.

(Same if it translates failure values). If it ignores failures, it would be good to translate them to warnings first and not blindly take them over as errors with the >> operator. (Same if it retries or falls back).

So change you are asking for there is basically:

 0--- a/src/util/result.h
 1+++ b/src/util/result.h
 2@@ -222,7 +222,7 @@ public:
 3     Result&& operator>>(O&& other LIFETIMEBOUND) &&
 4     {
 5         if (this->m_info) {
 6-            if (!this->m_info->errors.empty()) detail::MoveMessages(this->m_info->errors, other.Info().errors);
 7+            if (!this->m_info->errors.empty()) detail::MoveMessages(this->m_info->errors, other ? other.Info().warnings : other.Info().errors);
 8             if (!this->m_info->warnings.empty()) detail::MoveMessages(this->m_info->warnings, other.Info().warnings);
 9         }
10         return std::move(*this);

I wouldn’t object to it, but it just seems more invasive and doesn’t offer practical benefits.

I think this this behavior is safe and useful.

Why couldn’t the same be achieved by explicitly constructing a new Result with either an error or a value?

I think

0WarnFn1() >> result;
1WarnFn2() >> result;
2result = FailFn(...);
3return result;

or

0WarnFn1() >> result;
1WarnFn2() >> result;
2result.Set(FailFn(...));
3return result;

is cleaner than

0WarnFn1() >> result;
1WarnFn2() >> result;
2auto result2 = FailFn(...);
3std::move(result) >> result2;
4return result2;

because it doesn’t require introducing multiple result variables. If you are trying to get rid of both operator= and operator>>, I believe operator= or Set is also better than:

0auto result2 = FailFn(...);
1return result2 ? Result<int, FnError>(std::move(result), result2.value()) : Result<int, FnError>(std::move(result), Error{}, resul2t.GetFailure());

I’m happy to rename operator= to Set if you think operator= is misleading. But if you look at the places where these functions are used, it is easier to see why they are useful. Conversely, if you think there is a footgun here, it would be helpful to see an example of the footgun.

But if you look at the places where these functions are used, it is easier to see why they are useful.

I linked to some places where operator>> is used already: LoadWalletInternal, CreateWallet, and DoMigration from #25722

For operator= (again happy to rename this to Set) examples are: AddressTableModel::addRow, SQLiteDatabase::Verify, AvailableCoinsTestingSetup, FuzzedWallet::GetScriptPubKey

Allowing for a void failure type seems to make this incompatible to be switched out to std::expected https://en.cppreference.com/w/cpp/utility/expected ?

With multiple warning and error messages this may already be incompatible, though?

re: #25665 (review)

Allowing for a void failure type seems to make this incompatible to be switched out to std::expected https://en.cppreference.com/w/cpp/utility/expected ?

With multiple warning and error messages this may already be incompatible, though?

Yes, if you are thinking that util::Result could wrap std::expected, that would be probably be awkward and not worth it.

But I don’t think there is a conflict because the two classes are mostly doing different things. The util::Result class is mostly providing error-reporting functionality (passing error and warning strings up to the user). The std::expected class is only providing error-handling functionality (passing failure and success values between functions). Here is how I would choose the between the two classes:

• If your function never fails, it should just return success value directly.
• If your function can fail, but doesn’t provide any error strings or specific failure information, it should return std::optional.
• If your function can fail, and provides failure information but not error strings, it should return std::expected.
• If your function can fail, and generates error or warning strings it should return util::Result.

We have a lot of functions that generate error strings as you can see by all the code using util::Error and util::Result presently, and in more code that is converted to use util::Result in this PR and #25722. After std::expected is available, most of these functions still be better off using util::Result instead of std::expected so they are able to pass back error strings in a uniform way.

But when std::expected is available, we may want to tweak the util::Result class to make it easier to switch between std::expected and util::Result with minimal code changes. For example, the util::Result already has a value_or method to be compatible with std::optional. It could also have and_then and or_else methods to be compatible with std::expected.

Can you add an example for when the Result error type in a chain of function calls remains the same, but the value type changes? Is there a better way to do this conversion than manually constructing the result again (like below)?

0util::Result<std::string, FnError> CastFailFn() {
1    auto res = IntFailFn(1, false);
2    return {util::Error{ErrorString(res)}, res.GetFailure()};
3}

re: #25665 (review)

Can you add an example for when the Result error type in a chain of function calls remains the same, but the value type changes? Is there a better way to do this conversion than manually constructing the result again (like below)?

Yes, there is definitely a better way to pass along errors and warnings from one result to another without calling ErrorString and flattening them. The util::Result constructor will move errors and warnings directly from an existing result object if you just pass the other result object as an argument with std::move. If you look at the MultiWarnFn there was an example of this feature in the success case. But I added a new StrFailFn function now based on your code that demonstrates it in both the success and failure cases.

I am also thinking of adding a util::Messages{Result&&} helper similar to existing util::Error{std::string} and util::Warning{std::string} helpers to make it more obvious how you can construct a Result value with errors and warning from different sources.

Revisiting this makes me realize I should probably add a tutorial-style document that describes how to use the Result class with standalone functions that return util::Result, chained functions that return util::Result values of the same type, and chained functions that return util::Result values of different types.

I should probably add a tutorial-style document that describes how to use the Result class with standalone functions that return util::Result, chained functions that return util::Result values of the same type, and chained functions that return util::Result values of different types.

I think this would be valuable, either in util/result.h directly or in doc/developer-notes.md or another file in /doc.

I am also thinking of adding a util::Messages{Result&&} helper.

That sounds like a worthwhile improvement of the ergonomics here.

re: #25665 (review)

I am also thinking of adding a util::Messages{Result&&} helper.

That sounds like a worthwhile improvement of the ergonomics here.

Thanks, added this

f1b46f4017a

0init.cpp:948:12: error: 'operator=' is a private member of 'util::Result<void>'
1    result = init::SetLoggingLevel(args);
2    ~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
3./util/result.h:47:13: note: declared private here
4    Result& operator=(Result&&) = default;
5            ^

re: #25665 (review)

Thanks for testing the early commit, should be fixed now

40f09de73e6 lint-locale-dependence.py

0The locale dependent function std::to_string(...) appears to be used:
1src/test/result_tests.cpp:101:    return {std::move(result), std::to_string(*result)};

re: #25665 (review)

Thanks, this should be fixed now

332e847c9ec tuple not needed per iwyu in tidy ci https://cirrus-ci.com/task/6540065057275904?logs=ci#L20325, while touching could add the others

0+#include <assert.h>
1 #include <memory>
2 #include <optional>
3-#include <tuple>
4 #include <utility>
5 #include <vector>
6+#include <new>
7+#include <type_traits>

re: #25665 (review)

332e847 tuple not needed per iwyu in tidy ci https://cirrus-ci.com/task/6540065057275904?logs=ci#L20325, while touching could add the others

Thanks, updated includes