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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Argh, the auto formatter seems to have done some magic. I guess I should revert these?
Comment Actions
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 | ||
|---|---|---|
| 443 ↗ | (On Diff #354849) | 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!"); |
| 454 ↗ | (On Diff #354849) | 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...) |
| 455 ↗ | (On Diff #354849) | 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"; |
| 456 ↗ | (On Diff #354849) | generally the "then" block would either propagate the error or otherwise return, so you probably don't want else here. |
| 457 ↗ | (On Diff #354849) | you haven't illustrated how to get the result. Maybe outs() << "The answer is " << *Result << "\n"; |
| 462 ↗ | (On Diff #354849) | This seems too intrusive/unusual to put inline to me. 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. |
| llvm/include/llvm/Support/Error.h | ||
|---|---|---|
| 462 ↗ | (On Diff #354849) | Makes sense. However EXPECT_THAT_EXPECTED could also use some better documentation :( |
Comment Actions
fixed compilation issues
| llvm/include/llvm/Support/Error.h | ||
|---|---|---|
| 464 ↗ | (On Diff #355472) | sorry, I was too lazy to run this through the compiler. Now it compiles and runs... |
| 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! | |
Comment Actions
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.
Comment Actions
+@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?
Comment Actions
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)
Comment Actions
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).
Comment Actions
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.
Comment Actions
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>.
marcro -> macro