BResult improvements, allow returning values on failure #25608

ryanofsky commented at 6:40 PM on July 13, 2022: contributor

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 #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 commented at 6:48 PM on July 13, 2022: contributor

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.

in src/util/result.cpp:9 in 39769cc059 outdated

   0 | @@ -0,0 +1,14 @@
   1 | +// Copyright (c) 2022 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or https://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include <util/result.h>
   6 | +#include <util/string.h>
   7 | +
   8 | +namespace util {
   9 | +bilingual_str ErrorDescription(const Result<void>& result)

jamesob commented at 6:48 PM on July 13, 2022:

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

ryanofsky commented at 6:53 PM on July 13, 2022:

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.

in src/util/result.h:84 in 39769cc059 outdated

 100 | +    Result(Error, Str&& str, Args&&... args) : Result<void>{Error{}, std::forward<Str>(str)}, m_result{std::forward<Args>(args)...} {};
 101 | +    template<typename Str, typename Prev, typename...Args>
 102 | +    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)...} {};
 103 | +
 104 | +    //! std::optional methods, so Result<T> can be easily swapped for
 105 | +    //! std::optional<T> to add error reporting to existing code or remove it if

jamesob commented at 6:50 PM on July 13, 2022:

I think you meant Result<T>

ryanofsky commented at 7:00 PM on July 13, 2022:

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.

jamesob commented at 7:20 PM on July 13, 2022:

Oh yeah, my mistake, misread :sweat_smile:. Existing comment seems fine.

jamesob commented at 6:53 PM on July 13, 2022: member

Nice! Concept ACK

in src/test/result_tests.cpp:45 in 39769cc059 outdated

  40 | +    return {util::Error{}, Untranslated("status fail"), ret};
  41 | +}
  42 | +
  43 | +util::Result<int> ChainedFailFn(FnStatus arg, int ret)
  44 | +{
  45 | +    return {util::ErrorChain{}, Untranslated("chained fail"), StatusFailFn(arg), ret};

jamesob commented at 7:05 PM on July 13, 2022:

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.

ryanofsky commented at 7:22 PM on July 13, 2022:

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 commented at 7:52 PM on July 13, 2022: contributor

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.

in src/util/result.h:65 in 39769cc059 outdated

  81 | +    //! Success check.
  82 | +    operator bool() const { return m_errors.empty(); }
  83 | +
  84 | +    //! Error retrieval.
  85 | +    const std::vector<bilingual_str>& GetErrors() const { return m_errors; }
  86 | +    std::tuple<const std::vector<bilingual_str>&, const std::vector<bilingual_str>&> GetErrorsAndWarnings() const { return {m_errors, m_warnings}; }

w0xlt commented at 8:01 PM on July 13, 2022:

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

ryanofsky commented at 12:48 PM on July 21, 2022:

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 commented at 3:51 AM on July 14, 2022: contributor

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.

DrahtBot added the label Needs rebase on Jul 14, 2022

in src/util/result.h:41 in 39769cc059 outdated

  44 | +//! Result<void> specialization. Only returns errors and warnings, no values.
  45 | +template<>
  46 | +class Result<void>
  47 | +{
  48 | +protected:
  49 | +    std::vector<bilingual_str> m_errors;

martinus commented at 11:27 AM on July 14, 2022:

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.

ryanofsky commented at 12:47 PM on July 21, 2022:

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

This is a great idea, and implemented in #25665

BResult improvements

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.

dd91f29420

ryanofsky force-pushed on Jul 19, 2022

ryanofsky commented at 4:45 PM on July 19, 2022: contributor

Rebased 39769cc0599a93cbc28e693f6fccbabedae67fb5 -> dd91f294206ac87b213d23bb76656a0a5f0f4781 (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)

DrahtBot removed the label Needs rebase on Jul 19, 2022

in src/util/result.h:84 in dd91f29420

 105 | +    Result(Error, Str&& str, Args&&... args) : Result<void>{Error{}, std::forward<Str>(str)}, m_result{std::forward<Args>(args)...} {};
 106 | +    template<typename Str, typename Prev, typename...Args>
 107 | +    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)...} {};
 108 | +
 109 | +    //! std::optional methods, so Result<T> can be easily swapped for
 110 | +    //! std::optional<T> to add error reporting to existing code or remove it if

MarcoFalke commented at 5:34 PM on July 20, 2022:

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?

ryanofsky commented at 12:51 PM on July 21, 2022:

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

in src/util/result.h:112 in dd91f29420

 133 | + * result information and supports returing `void` and `bilingual_str` results.
 134 | +*/
 135 | +template<class T>
 136 | +class BResult {
 137 | +private:
 138 | +    util::Result<std::optional<T>> m_result;

MarcoFalke commented at 5:37 PM on July 20, 2022:

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.

ryanofsky commented at 12:53 PM on July 21, 2022:

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 6a06a7c3ad264c0ddf4904949839be1ade8011a5 (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 renamed this:
~~BResult improvements~~
BResult improvements, allow returning values on failure
on Jul 21, 2022

ryanofsky commented at 12:55 PM on July 21, 2022: contributor

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

ryanofsky closed this on Jul 21, 2022

bitcoin locked this on Jul 21, 2023