[PATCH] D105014: added some example code for llvm::Expected<T>

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

In D105014#2872674 <https://reviews.llvm.org/D105014#2872674>, @dblaikie wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105014/new/

https://reviews.llvm.org/D105014