[PATCH] D49013: [Support] Allow formatv() to consume llvm::Error by-value without crashing.

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

Thanks for looking at this, and sorry it's a complicated space.
Context is: I'm trying to move clangd's logging to wrap `formatv` instead of ad-hoc concatenation. One of the motivations is to make dealing with `llvm::Error`/`Expected` less painful. (In hindsight, we maybe should have rolled our own type here maybe, but `Error` is pervasive in our code now).

In https://reviews.llvm.org/D49013#1156544, @zturner wrote:

> There are two more possibilities I can think of.  Re-writing your original code sample in 2 different ways:
>
>   // Method 1
>   llvm::Expected<Foo> MaybeFoo = ...
>   if (!MaybeFoo) {
>     Error E = MaybeFoo.takeError();
>     llvm::errs() << formatv("Error: {0}", E);
>     consumeError(std::move(E));
>   }
>
>
> The former is more verbose and may be the best solution unless you're finding that you need to format errors very very often.  It has the advantage of requiring no code change at all


This is a really common pattern for clangd - in many places we don't have better options in the protocol than logging the error and returning a null response to the client.

So with that context, I think there are at least two problems with method 1:

- it's too verbose/noisy for such a common pattern
- it's error prone: if you forget `consumeError` you get a crash, but only if the error condition actually occurs. If tests don't trigger an error path, it's easy to check in time bomb bugs.

>   // Method 2
>   llvm::Expected<Foo> MaybeFoo = ...
>   if (!MaybeFoo)
>     llvm::errs() << formatv("Error: {0}", fmt_consume_error(MaybeFoo.takeError()));
> 
> 
> The latter is a bit more verbose (perhaps we can make the name more concise), but at least it makes it explicit that the error object will be destroyed as a result of this call.

This seems better, but there are still a couple of problems:

- it's error prone: if you forget the `fmt_consume_error` then it still compiles, but crashes (again, only if the error condition actually occurs).
- it still seems too verbose to me (I don't *want* error consumption to be explicit, I'm already passing by value so it just adds noise). I can't think of a great name to mitigate this :-( However we could provide `fmt_take_error(Expected<T>& X)` equivalent to `fmt_consume_error(X.takeError())` which reduces verbosity at the cost of orthogonality.

> The latter could be done by modeling a `FormatAdapter` after the existing ones like `fmt_repeat`, `fmt_pad`, etc.
> 
> Thoughts?

We can probably find something along these lines (explicit) that will work. My question is *why* :-)

- If you don't think that `formatv(char*, Foo.takeError())`is the best syntax, I'd like to try to convince you otherwise. (Roughly: it's clear and unambiguous, and consistent with `formatv` for other types. It's also consistent with `toString(Error)`.)
- if you don't want to have a special case in the implementation of `formatv` for `Error`, I can agree with that. Obviously reasonable people can disagree about the tradeoffs here. I'd prefer workarounds for `Error` to live in `Error.h`, but I don't see how to do that here.

One point that seems hard to resolve is that we really need to be able to print errors *without* consuming them, but we want to avoid the error-prone patterns above.
i.e. we'd like this to be legal: `formatv("{0}", const Error&)` but this should not be: `formatv("{0}", Error&&)` (unless we make it consume).
This doesn't seem easy to do directly in formatv, maybe we need to write the first `formatv("{0}", fmt_err(const Error&))` (and rename the `operator<<` on `Error` to something else so it can't be selected).

> Currently the toString operation on Error is consuming. Could this just be:
> 
>   errs() << toString(MaybeFoo.takeError());

I don't think `toString` is the right function here (when passing to formatv).

- it's semantically the wrong name: `formatv`'s job is to stringify things, so stringifying them first is confusing and inconsistent with other types.
- we may not actually want to produce the string (e.g. if the `Logger` implementation decides to filter out the `formatv_object_base` based on level)
- `llvm::toString` is dangerously close to the generic `llvm::to_string` which does the same thing without consuming the error.

If this is the desired syntax form, a `FormatAdapter` could give similar syntax with a better name and semantics, I think.


Repository:
  rL LLVM

https://reviews.llvm.org/D49013