BResult improvements, allow returning values on failure #25608

ryanofsky · 2022-07-13T18:40:13Z

This is a draft PR 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 refactor: Reduce number of LoadChainstate parameters and return values #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.

ryanofsky · 2022-07-13T18:48:07Z

I would like to clean this up more and implement it on top of #25308 so the LoadChainstate function can use this class. But this class contains all the essential things I think are missing from BResult: support for error values, support for void values, support for multiple errors, chained errors, and warnings.

Was motivated to work on this by suggestion to use BResult in #25308 #25308 (comment) and by #25601, which adds the same error type functionality this PR does, but doesn't support returning custom errors for error handling and returning standardized errors for error reporting at the same time.

jamesob · 2022-07-13T18:48:16Z

src/util/result.cpp

+#include <util/string.h>
+
+namespace util {
+bilingual_str ErrorDescription(const Result<void>& result)


Any reason not to just include this as a method called e.g. result.ErrorsStr()?

This is an error formatting function and I think Result class should only be responsible for returning errors, not formatting them.

Also I generally think classes with private state should have fewer methods accessing that state to keep code readable and maintainable. If a function can use a class's public interface, it should use that instead of poking around at internals.

jamesob

Nice! Concept ACK

jamesob · 2022-07-13T18:50:06Z

src/util/result.h

+    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


I think you meant Result<T>

I think you meant Result<T>

I should probably rewrite this comment but it is trying to say if you have an existing function returning std::optional<T> you can most likely change it to return util::Result<T> and provide error information to new callers without affecting existing callers.

Oh yeah, my mistake, misread 😅. Existing comment seems fine.

jamesob · 2022-07-13T19:05:41Z

src/test/result_tests.cpp

+
+util::Result<int> ChainedFailFn(FnStatus arg, int ret)
+{
+    return {util::ErrorChain{}, Untranslated("chained fail"), StatusFailFn(arg), ret};


Might be nice to have an interface that uses static constructor shorthands for concision, like

return Result::Err(ret, StatusFailFn(arg), Untranslated("chained fail"));

or for the function above

return Result::Err(ret, Untranslated("status fail"));

Edit: ah, I guess not so easy because in practice Result would need to be templated.

It would be possible and not very difficult to change syntax from

return {util::Error{}, "error string", error_value};

to

return util::Error("error string", error_value);

for both Error and ErrorChain with some template magic. It just made the implementation a little uglier so I decided to drop it.

w0xlt

Concept ACK. I don't have a strong opinion between this approach or #25601.

But I don't think we should maintain BResult. As done at #25601, the use of BResult should be replaced by the new interface.

It's going to be confusing to have a class that shouldn't be used anymore, as said in the comment, when that can easily be replaced.

w0xlt · 2022-07-13T20:01:00Z

src/util/result.h

+
+    //! 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}; }


Maybe a GetWarnings() method ?
Or m_errors and m_warnings can be public.
I think there are some success cases that also return warnings.

Maybe a GetWarnings() method ? Or m_errors and m_warnings can be public. I think there are some success cases that also return warnings.

Yes sorry interface is just incomplete while in draft.

DrahtBot · 2022-07-14T03:51:24Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25601 (util: Make BResult error a generic type instead of only bilingual_str by w0xlt)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

martinus · 2022-07-14T11:27:15Z

src/util/result.h

+class Result<void>
+{
+protected:
+    std::vector<bilingual_str> m_errors;


how about moving the members into a single std::unique_ptr, something like this:

protected: struct ErrorsAndWarnings { std::vector<bilingual_str> m_errors; std::vector<bilingual_str> m_warnings; }; std::unique_ptr<ErrorsAndWarnings> m_errors_and_warnings{};

This has the advantage that the default case when no error/warning happens is really fast: no temporary std::vector need to be constructed. Also, sizeof() is much smaller, only a single pointer. Then Result is also noncopyable. I think this would be an advantage, because usually these are supposed to be moved as the return value, and not copied.

how about moving the members into a single std::unique_ptr, something like this:

This is a great idea, and implemented in #25665

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 bitcoin#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.

ryanofsky · 2022-07-19T16:45:48Z

Rebased 39769cc -> dd91f29 (pr/bresult.2 -> pr/bresult.3, compare) due to conflict with #25594

(@martinus suggestion to optimize happy path when there are no warnings or errors a makes a lot of sense so I started implementing that, but I figured I'd rebase this in the meantime due to the conflict)

maflcko · 2022-07-20T17:34:42Z

src/util/result.h

+    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


I don't think this is true? std::optional with nullopt must not be dereferenced, whereas Result can be, and would return a default constructed object?

I don't think this is true? std::optional with nullopt must not be dereferenced, whereas Result can be, and would return a default constructed object?

I can clarify comment, but this interface is superset of std::optional interface and allows dereferencing in cases when original object can't be derefenced. The object does not have to be default constructable since util::Error{} can forward any constructor arguments

maflcko · 2022-07-20T17:37:35Z

src/util/result.h

+template<class T>
+class BResult {
+private:
+    util::Result<std::optional<T>> m_result;


I don't think this is too helpful to force most call sites (that can use BResult or use it today) into a double wrapping util::Result<std::optional<T>>. I liked a single wrapping better.

I don't think this is too helpful to force most call sites (that can use BResult or use it today) into a double wrapping util::Result<std::optional<T>>. I liked a single wrapping better.

Right this was supposed to be only temporary within the PR. A followup commit basically like 6a06a7c (from #25665) would replace BResult<T> with util::Result<T>

std::optional was used here just because it's the simplest type definition that preserves BResult semantics.

ryanofsky · 2022-07-21T12:55:48Z

Closing for now in favor of #25665 which implements Martinus suggestion. Will post a comparison of different approaches there.

jamesob reviewed Jul 13, 2022

View reviewed changes

ryanofsky mentioned this pull request Jul 13, 2022

util: Make BResult error a generic type instead of only bilingual_str #25601

Closed

w0xlt reviewed Jul 13, 2022

View reviewed changes

DrahtBot mentioned this pull request Jul 14, 2022

refactor: Return BResult from restoreWallet #25594

Merged

DrahtBot added the Needs rebase label Jul 14, 2022

martinus reviewed Jul 14, 2022

View reviewed changes

ryanofsky force-pushed the pr/bresult branch from 39769cc to dd91f29 Compare July 19, 2022 16:43

DrahtBot removed the Needs rebase label Jul 19, 2022

maflcko reviewed Jul 20, 2022

View reviewed changes

ryanofsky changed the title ~~BResult improvements~~ BResult improvements, allow returning values on failure Jul 21, 2022

ryanofsky closed this Jul 21, 2022

ryanofsky mentioned this pull request Jul 27, 2022

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

Open

bitcoin locked and limited conversation to collaborators Jul 21, 2023

BResult improvements, allow returning values on failure #25608

BResult improvements, allow returning values on failure #25608

Uh oh!

Conversation

ryanofsky commented Jul 13, 2022

Uh oh!

ryanofsky commented Jul 13, 2022

Uh oh!

jamesob Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jul 19, 2022

Uh oh!

maflcko Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jul 21, 2022

Uh oh!

Uh oh!

jamesob Jul 13, 2022 •

edited

Loading

ryanofsky Jul 13, 2022 •

edited

Loading

jamesob Jul 13, 2022 •

edited

Loading

jamesob Jul 13, 2022 •

edited

Loading

w0xlt left a comment •

edited

Loading

w0xlt Jul 13, 2022 •

edited

Loading

DrahtBot commented Jul 14, 2022 •

edited

Loading

maflcko Jul 20, 2022 •

edited

Loading

ryanofsky Jul 21, 2022 •

edited

Loading