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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D105014#2873739 <https://reviews.llvm.org/D105014#2873739>, @sammccall wrote:

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105014