added some example code for llvm::Expected<T>

[kuhnel]

Summary

Since I had some fun understanding how to properly use llvm::Expected<T> I added some code examples that I would have liked to see when learning to use it.

[kuhnel]

removed stray file

[kuhnel]

Argh, the auto formatter seems to have done some magic. I guess I should revert these?

[kuhnel]

undo of auto formatting

[sammccall]

Yes, please revert the unrelated formatting changes.

I agree Error is confusing and better docs might help.
(Not as much as removing the confusing part of the design, but I that seems hard at this point!)

However I don't think the "hump" you experienced can be avoided purely by "recipe-style" documentation, because you can quickly get into a situation where you have to understand Error's checking model to write proper handling code.
By making the examples a bit more complete and adding a little more explanation we can flatten it though!

llvm/include/llvm/Support/Error.h

446

the error() function is a clangd-ism (and was even controversial there!)

The "plain" equivalent is, sadly, return createStringError(inconvertibleErrorCode(), "B must not be zero!");

457

nit: we shouldn't call a variable Error if the type Error is in scope, it's asking for trouble (and occasionally invalid code that some compilers reject and others accept...)

458

IMO the hard part here is that *the error must be consumed*/checked.

takeError doesn't actually do this, after Err = Result.takeError(), Result is empty with Checked=true, but Err is populated and has Checked=false.

Confusingly, most of the things you can actually *do* with an Error (including toString()) will check/consume it. But not all, and in particular if you do nothing with it then ~Error() will crash.

---

Assigning the Error to a separate variable is fairly rare and runs into more of these complications, usually you rather either log or propagate it, in either case using the rvalue directly.

I'd probably replace it with this code + comment:

// We must consume the error. Typically one of:
// - return the error to our caller
// - toString(), when logging
// - consumeError(), to silently swallow the error
// - handleErrors(), to distinguish error types
errs() << "Problem with division " << toString(Result.takeError()) << "\n";

459

generally the "then" block would either propagate the error or otherwise return, so you probably don't want else here.

460

you haven't illustrated how to get the result.

Maybe

outs() << "The answer is " << *Result << "\n";

465

This seems too intrusive/unusual to put inline to me.
And the main thing that is non-obvious is *why* you have to structure your assertions in an unusual way, and that's not explained here.

Consider instead just briefly referencing the EXPECT_THAT_EXPECTED etc macros in llvm/Testing/Support/Error.h which are designed to simplify this a bit.

[kuhnel]

addressed Sam's review comments

[kuhnel]

fixed variable name

[kuhnel]

llvm/include/llvm/Support/Error.h

465

Makes sense.

However EXPECT_THAT_EXPECTED could also use some better documentation :(

[kuhnel]

argh, again fixed autoformatter :(

[sammccall]

Thanks!

llvm/include/llvm/Support/Error.h

464

this won't compile, you need std::move(E)

467

this will crash in the error case, you need to return after the errs() log

[kuhnel]

also added example code for EXPECT_THAT_EXPECTED.

[kuhnel]

fixed compilation issues

llvm/include/llvm/Support/Error.h

464

sorry, I was too lazy to run this through the compiler. Now it compiles and runs...

[sammccall]

llvm/include/llvm/Testing/Support/Error.h

168

marcro -> macro

179

you shouldn't explicitly log the tostring, this should happen automatically

181

this is unidiomatic/incorrect, rather just EXPECT_THAT_EXPECTED(D1, HasValue(2)) in one step

(Note that if you want to write *D1, you need to establish that D1 succeeded, e.g. by using ASSERT_ to bail out early. As it is, if there is an error it'll show one failure and then crash)

187

no, you don't!

[kuhnel]

updated code examples based on Sam's review

Oh my, this is really simple if you know how it's supposed
to work. However my intuition is completely off in trying to
understand the error handling. I hope the examples help
others avoid the pain.

[kuhnel]

fixed typo

[kuhnel]

now fixing arc's way of git commits :(

[dblaikie]

+@lhames for context as the author/owner of llvm::Error and associated things.

Perhaps it'd be handy to have some descriptions of the problems you encountered, @kuhnel, and how you went about trying to resolve them, to help understand where things are lacking.

@sammccall
I agree Error is confusing and better docs might help.
(Not as much as removing the confusing part of the design, but I that seems hard at this point!)

Maybe in this review, or maybe elsewhere: might be good to have some details on your perspective here?

[sammccall]

In D105014#2872674, @dblaikie wrote:

+@lhames for context as the author/owner of llvm::Error and associated things.

Perhaps it'd be handy to have some descriptions of the problems you encountered, @kuhnel, and how you went about trying to resolve them, to help understand where things are lacking.

@sammccall
I agree Error is confusing and better docs might help.
(Not as much as removing the confusing part of the design, but I that seems hard at this point!)

Maybe in this review, or maybe elsewhere: might be good to have some details on your perspective here?

Mostly that the attempt to enforce handling is an interesting idea but a cure worse than the disease in practice. In interacting closely with ~6 developers over the last ~4 years:

• most people were initially confused by this class and took significant time to build up a mental model, and are still fuzzy (e.g. on how it interacts with move semantics). I think this was Christian's experience here.
• In practice, developers who know Error well still sometimes check in code with "unhandled" errors by mistake
• most such code is correct except for the enforcement itself. e.g. test code that's missing a cantFail(...), production code missing a consumeError(Exp.takeError()) after handling failure.
• In practice, these mistake are difficult for reviewers who know Error well to catch
• it causes the Error class to have interface warts (toString() consumes the move-only object, operator== is non-const) that cause surprising ergonomic problems, e.g. requiring special helpers when used from test code

We've talked about the possibility of adding separate Error/Expected types to clangd with similar semantics but no check-enforcement and a simpler payload model. However having to deal with both error types seems too painful.

There's one case that's problematic enough to cause real danger, and has bitten us a couple of times:

• an Expected<T> is obtained and checked for success
• dynamically the value is almost always OK (e.g. except when out of disk space)
• in the failure case, the error is handled but not consumed. This is not caught in tests because the error condition is hard to simulated
• now months later when the error occurs in the wild in a debug build (yes, people seem to use these...) it becomes a crash

---

Separately from this issue, creating/logging errors is pretty clunky in the common case, and error handling is ubiquitous.
I wonder if there's appetite for the error() function from clang-tools-extra/clangd/support/Logger.h to live somewhere common.
(Both error() and the support for errors in log() in that file are pretty useful quality-of-life features, but log() is maybe not well-suited for llvm)

[dblaikie]

In D105014#2873739, @sammccall wrote:

In D105014#2872674, @dblaikie wrote:

+@lhames for context as the author/owner of llvm::Error and associated things.

Perhaps it'd be handy to have some descriptions of the problems you encountered, @kuhnel, and how you went about trying to resolve them, to help understand where things are lacking.

@sammccall
I agree Error is confusing and better docs might help.
(Not as much as removing the confusing part of the design, but I that seems hard at this point!)

Maybe in this review, or maybe elsewhere: might be good to have some details on your perspective here?

Mostly that the attempt to enforce handling is an interesting idea but a cure worse than the disease in practice. In interacting closely with ~6 developers over the last ~4 years:

• most people were initially confused by this class and took significant time to build up a mental model, and are still fuzzy (e.g. on how it interacts with move semantics). I think this was Christian's experience here.
• In practice, developers who know Error well still sometimes check in code with "unhandled" errors by mistake
• most such code is correct except for the enforcement itself. e.g. test code that's missing a cantFail(...), production code missing a consumeError(Exp.takeError()) after handling failure.
• In practice, these mistake are difficult for reviewers who know Error well to catch
• it causes the Error class to have interface warts (toString() consumes the move-only object, operator== is non-const) that cause surprising ergonomic problems, e.g. requiring special helpers when used from test code

We've talked about the possibility of adding separate Error/Expected types to clangd with similar semantics but no check-enforcement and a simpler payload model. However having to deal with both error types seems too painful.

There's one case that's problematic enough to cause real danger, and has bitten us a couple of times:

• an Expected<T> is obtained and checked for success
• dynamically the value is almost always OK (e.g. except when out of disk space)
• in the failure case, the error is handled but not consumed. This is not caught in tests because the error condition is hard to simulated
• now months later when the error occurs in the wild in a debug build (yes, people seem to use these...) it becomes a crash

---

Separately from this issue, creating/logging errors is pretty clunky in the common case, and error handling is ubiquitous.
I wonder if there's appetite for the error() function from clang-tools-extra/clangd/support/Logger.h to live somewhere common.
(Both error() and the support for errors in log() in that file are pretty useful quality-of-life features, but log() is maybe not well-suited for llvm)

Thanks for writing this up - really appreciate it!

I guess the major difference in perspective may be due to different valuation of the cost/"disease" it's intended to prevent - for myself I think the cost is worthwhile but I understand that's not the case for you.

If there are a lot of cases of errors being ignored (consumeError) - maybe some APIs could be changed to not return error, but instead return boolean values? Or have alternative APIs - (in other languages/environments with exceptions you might have "parseInt" that throws if parsing fails and "tryParse" which returns boolean/provides the value via an out parameter, etc).

That it's difficult to spot incorrect usage is part of the justification for the checking - without it, incorrect usage that dropped errors accidentally would go unnoticed. Though if there are ways to make it even easier to not accidentally ignore errors - that'd be great! Going the other way to reducing visibility of dropped errors would be a net loss (by my own values, which as noted, values the cost of lost errors higher than you do - neither position I think is objectively more right).

[lhames]

I missed this thread earlier -- thanks to Dave for pointing me to it.

@kuhnel -- Thanks very much for working on this.

Out of interest, did you see https://llvm.org/docs/ProgrammersManual.html#error-handling ? If not (and if you find it helpful) then maybe we need to make that document more discoverable. If that document is not helpful then we should improve it.

I guess the major difference in perspective may be due to different valuation of the cost/"disease" it's intended to prevent - for myself I think the cost is worthwhile but I understand that's not the case for you.

I've certainly found Error / Expected to be valuable for my work, but I designed them with my use-cases in mind so take that with a grain of salt. ;)

As a general observation: critiques of Error/Expected include the avoidable ergonomic issues (potentially addressable by API improvements), unavoidable ergonomic issues (we really want language support for this, but can't have it), and questions around correct application of Error/Expected -- sometimes error handling is just hard and Error/Expected feel awkward because they force you to confront it, and other times they're just not a good fit for whatever you're trying to do and you have to recognize when they should be dropped in favor of a more appropriate tool.

Especially on that first point (the avoidable ergonomic issues) I think that Error/Expected could benefit from review now that we've gained more practical experience with them. I'm not in a position to drive any such effort, but would be happy to help out.

[kuhnel]

Out of interest, did you see https://llvm.org/docs/ProgrammersManual.html#error-handling ? If not (and if you find it helpful) then maybe we need to make that document more discoverable. If that document is not helpful then we should improve it.

No I wasn't aware of the documentation page and just took a quick look. This looks quite useful. I wish I had known about that part when trying to use Expected<T>.

llvm/include/llvm/Support/Error.h

 }
 
 /// Tagged union holding either a T or a Error.
 ///
 /// This class parallels ErrorOr, but replaces error_code with Error. Since
 /// Error cannot be copied, this class replaces getError() with
 /// takeError(). It also adds an bool errorIsA<ErrT>() method for testing the
 /// error class type.
+///
+/// Example usage of 'Expected<T>' as a function return type:
+///
+///   @code{.cpp}
+///     Expected<int> myDivide(int A, int B) {
+///       if (B == 0) {
+///         // return an Error
+///         return createStringError(inconvertibleErrorCode(),
+///                                  "B must not be zero!");
+///       }
+///       // return an integer
+///       return A / B;
+///     }
+///   @endcode
+///
+///   Checking the results of to a function returning 'Expected<T>':
+///   @code{.cpp}
+///     if (auto E = Result.takeError()) {
+///       // We must consume the error. Typically one of:
+///       // - return the error to our caller
+///       // - toString(), when logging
+///       // - consumeError(), to silently swallow the error
+///       // - handleErrors(), to distinguish error types
+///       errs() << "Problem with division " << toString(std::move(E)) << "\n";
+///       return;
+///     }
+///     // use the result
+///     outs() << "The answer is " << *Result << "\n";
+///   @endcode
+///
+///  For unit-testing a function returning an 'Expceted<T>', see the
+///  'EXPECT_THAT_EXPECTED' macros in llvm/Testing/Support/Error.h
+
 template <class T> class LLVM_NODISCARD Expected {
   template <class T1> friend class ExpectedAsOutParameter;
   template <class OtherT> friend class Expected;
 
   static constexpr bool isRef = std::is_reference<T>::value;
 
   using wrap = std::reference_wrapper<std::remove_reference_t<T>>;
 

llvm/include/llvm/Testing/Support/Error.h

 };
 } // namespace detail
 
 #define EXPECT_THAT_ERROR(Err, Matcher)                                        \
   EXPECT_THAT(llvm::detail::TakeError(Err), Matcher)
 #define ASSERT_THAT_ERROR(Err, Matcher)                                        \
   ASSERT_THAT(llvm::detail::TakeError(Err), Matcher)
 
+/// Helper macro for checking the result of an 'Expected<T>'
+///
+///   @code{.cpp}
+///     // function to be tested
+///     Expected<int> myDivide(int A, int B);
+///
+///     TEST(myDivideTests, GoodAndBad) {
+///       // test good case
+///       // if you only care about success or failure:
+///       EXPECT_THAT_EXPECTED(myDivide(10, 5), Succeeded());
+///       // if you also care about the value:
+///       EXPECT_THAT_EXPECTED(myDivide(10, 5), HasValue(2));
+///
+///       // test the error case
+///       EXPECT_THAT_EXPECTED(myDivide(10, 0), Failed());
+///       // also check the error message
+///       EXPECT_THAT_EXPECTED(myDivide(10, 0),
+///           FailedWithMessage("B must not be zero!"));
+///     }
+///   @endcode
+
 #define EXPECT_THAT_EXPECTED(Err, Matcher)                                     \
   EXPECT_THAT(llvm::detail::TakeExpected(Err), Matcher)
 #define ASSERT_THAT_EXPECTED(Err, Matcher)                                     \
   ASSERT_THAT(llvm::detail::TakeExpected(Err), Matcher)
 
 MATCHER(Succeeded, "") { return arg.Success(); }
 MATCHER(Failed, "") { return !arg.Success(); }