This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kuhnel on Jun 28 2021, 5:19 AM.

Details

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.

Diff Detail

Event Timeline

kuhnel created this revision.Jun 28 2021, 5:19 AM
kuhnel requested review of this revision.Jun 28 2021, 5:19 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 28 2021, 5:19 AM
kuhnel updated this revision to Diff 354849.Jun 28 2021, 5:20 AM

removed stray file

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

kuhnel updated this revision to Diff 354855.Jun 28 2021, 5:30 AM

undo of auto formatting

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 updated this revision to Diff 355470.Jun 30 2021, 1:23 AM

addressed Sam's review comments

kuhnel updated this revision to Diff 355471.Jun 30 2021, 1:24 AM
kuhnel marked 2 inline comments as done.

fixed variable name

kuhnel marked 4 inline comments as done.Jun 30 2021, 1:25 AM
kuhnel added inline comments.
llvm/include/llvm/Support/Error.h
465

Makes sense.

However EXPECT_THAT_EXPECTED could also use some better documentation :(

kuhnel updated this revision to Diff 355472.Jun 30 2021, 1:30 AM
kuhnel marked an inline comment as done.

argh, again fixed autoformatter :(

sammccall accepted this revision.Jun 30 2021, 1:39 AM

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

This revision is now accepted and ready to land.Jun 30 2021, 1:39 AM
kuhnel updated this revision to Diff 355481.Jun 30 2021, 2:02 AM

also added example code for EXPECT_THAT_EXPECTED.

kuhnel updated this revision to Diff 355485.Jun 30 2021, 2:37 AM
kuhnel marked 2 inline comments as done.

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 added inline comments.Jun 30 2021, 2:59 AM
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 this revision to Diff 355507.Jun 30 2021, 4:57 AM
kuhnel marked 4 inline comments as done.

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 updated this revision to Diff 355508.Jun 30 2021, 4:58 AM

fixed typo

kuhnel updated this revision to Diff 355510.Jun 30 2021, 5:00 AM

now fixing arc's way of git commits :(

sammccall accepted this revision.Jun 30 2021, 7:19 AM
This revision was automatically updated to reflect the committed changes.

+@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?

+@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)

+@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 added a comment.Sep 4 2021, 7:44 PM

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.

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