feat: Public Result class encapsulating a result value and a status code

[chandryan]

Provision of a public Result class as described in #193.
The class mostly follows the design of C++'s std::expected.

Summary of changes:

Result:

• moved out of detail namespace and added API comments
• added some implicit conversion operators to T to have similar code on user level as before Results (e.g. direct usage of T and exception on bad status)
• simplified and moved tryInvoke to detail/result_util.h

Async method handling:

• Changed callbacks to retrieve a Result<T> instead of StatusCode and T
• Changed futureToken to return future<Result<T>>

Error handling:

• Mapped alloc errors to UA_STATUSCODE_BADOUTOFMEMORY

[codecov]

Codecov Report

Attention: Patch coverage is 98.75000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 83.90%. Comparing base (dc7c732) to head (eb245c2).
Report is 10 commits behind head on master.

Files	Patch %	Lines
include/open62541pp/Result.h	98.14%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
- Coverage   85.14%   83.90%   -1.25%     
==========================================
  Files          80       78       -2     
  Lines        4356     4181     -175     
  Branches     1746      971     -775     
==========================================
- Hits         3709     3508     -201     
+ Misses        204      198       -6     
- Partials      443      475      +32     

Flag	Coverage Δ
Linux	83.90% <98.75%> (-1.25%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[chandryan]

Hi @lukasberbuer ,
design, implementation updates and tests are done, please check it out and provide your feedback 😀

After incorporating your feedback I'll update the documentation in async_model.md accordingly.

Kind regards

[lukasberbuer]

Hi @chandryan, thank you for your great contribution! 👍

As I mentioned in #193, the Result class should be battle-tested internally before making it part of the public API. Maybe the title of the issue was a little misleading. I see following options:

• move back into detail namespace
• move into experimental namespace
• mark as experimental in the documentation

The Result class has the potential to be the new error handling mechanism but this is quite a disruptive change which affects all functions with client/server communication - both async and sync. Therefore I want to handle this change with care and gain some experiences first.

You already changed the async model to use the Result class. This is too early in my opinion. I would prefer following:

• async callbacks should accept both signatures void(StatusCode, T&) and void(Result<T>&).
• useFuture should return std::future<T>
• useResultFuture (or similar) can be used to return std::future<Result<T>>

[chandryan]

Hi @lukasberbuer !

Indeed I misunderstood "battle-proven", I thought you just meant thinking about the API details of the Result class and making sure tests are complete ^^

Regarding the default interface of sync & async functions:

I was worried about users not wanting to use future<Result<T>> at all because of the disruptive change from programming with exceptions to programming with error codes. I like the idea of using useResultFuture to opt-in for this style even for the long run, this could be an elegant solution to remove the disruption 😄 . For synchronous method calls there could be a useResult tag to stay backwards-compatible with an opt-in for the other style. What do you think about this approach?

Regarding the callback interface:

We could support both interfaces, though I see disadvantages when not using Result<T> and thus I have some doubts about the benefit of this.

I prefer the Result<T> interface over StatusCode and T due to the following reasons:

• it's less error-prone because Result<T> is more explicit about the situations where the value is valid and when not
• it's more efficient because there is no need to default-initialize T when there is no value

Since the callback with StatusCode and T is a quite new feature and hasn't been released in a published version yet, I tend to say it's not disruptive. Or have I missed something, has it been made available before already?

Regarding the options for where to put the Result class:

I would choose experimental over detail for the Result class if it's exposed to the user via the callback interface or tags such as useResultFuture. Explicitly putting it into an experimental namespace would lead to some future disruption to move it out of it again, while only describing it as experimental in the documentation means the information could be missed. My view is the following:

• If the class API is still too immature and subject to changes, an experimental namespace might be better
• If its level of maturity is high enough, marking it as experimental only in the documentation might suffice

[lukasberbuer]

Hi @chandryan,

I think you are right. Keeping both interfaces will make it just more complicated in the long run. I personally favor the Result interface as well for callbacks and futures. Although I am a little afraid of those potential deeply nested template types like std::future<Result<Bitmask<AccessLevel>>> 😅

The roadmap of the new exception handling mechanism is still not clear to me. In the end, every service which might fail could return a Result object. But where to draw the line? Should Client::connect() return a Result<void> or throw an exception?
My opinion in stronger for all functions in the services namespace: They should all return Result objects and should be made noexcept if possible.

Let's keep the PR as it is for now. I will take a deeper dive through the code the next days 😊

[lukasberbuer]

I love it, great work! 🎉
Would you also update the async_model.md?

[lukasberbuer]

Hi @chandryan, could you please merge the master branch again. I had to fix some CI issues:

• wrong coverage report (tests were included) ci: unify test matrix #236
• wrong compilers (clang tests were actually using gcc) ci: fix compiler selection #226

[lukasberbuer]

Currently, Result<void> does not offer a hasValue() member function. Why did you choose not to implement this? Did you want to avoid the additional boolean member variable?

[chandryan]

Currently, Result<void> does not offer a hasValue() member function. Why did you choose not to implement this? Did you want to avoid the additional boolean member variable?

Thanks for spotting this, I forgot to add it! An additional member should not be required, the status code could be queried in a similar fashion as in value().
Actually that reminds me that I was also not sure whether speaking about values for Result<void> was a good idea at all. Adding it to pass the existing tests with less refactoring made it seem like a good idea at some point 🥴 Reconsidering this now I'm also open to change it again.

This has been my thought train:

I looked at std::optional as well as std::expected to find some guidance from the C++ reference; std::optional<void> does not allow T to be void, but std::expected does. I looked closer at std::expected, as that class seems to be closer related to our Result. It still sounds strange to me to ask whether an std::expected<void, E> has a value, I find it more intuitive to ask if it does not have an error. However, at least in that case one state excludes the other, so has_value()==(!has_error()) remains true.

As opposed to std::expected<T,E>, our Result<T> does not have these two mutually exclusive states. There is always a status code and sometimes a value. So we'd have something like hasValue()==(!code().isBad()), which does make sense for Result<T> but to me it doesn't seem very understandable or meaningful to Result<void>.

For Result<void>, maybe we should drop value() and hasValue() in favor of already existing, more expressive alternatives such as code().throwIfBad() and code().isGood()?

[lukasberbuer]

You added the default constructor for Result<T> as a workaround for MSVC.
I was wondering if a default constructor makes sense in general. std::expected defines a default constructor, which default-initializes the expected value. hasValue() will return true.
In our case, the default constructor would default-initialize the std::optional<T> and the status code to UA_STATUSCODE_GOOD. Do you see any issues / disadvantages with a default constructor @chandryan?

[chandryan]

You added the default constructor for Result<T> as a workaround for MSVC. I was wondering if a default constructor makes sense in general. std::expected defines a default constructor, which default-initializes the expected value. hasValue() will return true. In our case, the default constructor would default-initialize the std::optional<T> and the status code to UA_STATUSCODE_GOOD. Do you see any issues / disadvantages with a default constructor @chandryan?

I see no disadvantages, I have added a default constructor as you suggested 👍

[lukasberbuer]

Great, thanks @chandryan. Have you seen the comments in the review? Besides those small adjustments, I would like to merge it as soon as possible 😊

[chandryan]

Great, thanks @chandryan. Have you seen the comments in the review? Besides those small adjustments, I would like to merge it as soon as possible 😊

I currently don't see any open comments I haven't addressed yet, though it's easy to loose orientation without having the possibility to open threads within this page in GitHub 😆

Short summary:

• The last merge from master I had done 2 days ago, the CI issues should already be addressed
• I had added Result<void>::hasValue(); I don't have a strong opinion whether to keep the value API for Result<void> or not, please decide what you prefer 👍
• There is one open comment from me about documentation. Please check it and feel free to decide whether it's worth a separate issue / pull request to provide additional documentation or examples

Regarding review comments: When I look at the top right of this page I see "No reviews". If you have done one, could you double-check whether you submitted it?

[lukasberbuer]

Damn, I forgot to submit 😅

[lukasberbuer]

Ready to merge @chandryan?

[chandryan]

Ready to merge @chandryan?

Yes, please go ahead :-)

[lukasberbuer]

Great work, thank you! 😊