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: > >> +@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
