Consuming `emit` #119606

It seems like a bad idea, just asking for diagnostics to be emitted multiple times.

This works for most of its call sites. This is nice, because `emit` very much makes sense as a consuming operation -- indeed, `DiagnosticBuilderState` exists to ensure no diagnostic is emitted twice, but it uses runtime checks. For the small number of call sites where a consuming emit doesn't work, the commit adds `DiagnosticBuilder::emit_without_consuming`. (This will be removed in subsequent commits.) Likewise, `emit_unless` becomes consuming. And `delay_as_bug` becomes consuming, while `delay_as_bug_without_consuming` is added (which will also be removed in subsequent commits.) All this requires significant changes to `DiagnosticBuilder`'s chaining methods. Currently `DiagnosticBuilder` method chaining uses a non-consuming `&mut self -> &mut Self` style, which allows chaining to be used when the chain ends in `emit()`, like so: ``` struct_err(msg).span(span).emit(); ``` But it doesn't work when producing a `DiagnosticBuilder` value, requiring this: ``` let mut err = self.struct_err(msg); err.span(span); err ``` This style of chaining won't work with consuming `emit` though. For that, we need to use to a `self -> Self` style. That also would allow `DiagnosticBuilder` production to be chained, e.g.: ``` self.struct_err(msg).span(span) ``` However, removing the `&mut self -> &mut Self` style would require that individual modifications of a `DiagnosticBuilder` go from this: ``` err.span(span); ``` to this: ``` err = err.span(span); ``` There are *many* such places. I have a high tolerance for tedious refactorings, but even I gave up after a long time trying to convert them all. Instead, this commit has it both ways: the existing `&mut self -> Self` chaining methods are kept, and new `self -> Self` chaining methods are added, all of which have a `_mv` suffix (short for "move"). Changes to the existing `forward!` macro lets this happen with very little additional boilerplate code. I chose to add the suffix to the new chaining methods rather than the existing ones, because the number of changes required is much smaller that way. This doubled chainging is a bit clumsy, but I think it is worthwhile because it allows a *lot* of good things to subsequently happen. In this commit, there are many `mut` qualifiers removed in places where diagnostics are emitted without being modified. In subsequent commits: - chaining can be used more, making the code more concise; - more use of chaining also permits the removal of redundant diagnostic APIs like `struct_err_with_code`, which can be replaced easily with `struct_err` + `code_mv`; - `emit_without_diagnostic` can be removed, which simplifies a lot of machinery, removing the need for `DiagnosticBuilderState`.

To avoid the use of a mutable local variable, and because it reads more nicely.

These all have relatively low use, and can be perfectly emulated with a simpler construction method combined with `code` or `code_mv`.

In this parsing recovery function, we only need to emit the previously obtained error message and mark `expr` as erroneous in the case where we actually recover.

Instead of taking `seq` as a mutable reference, `maybe_recover_struct_lit_bad_delims` now consumes `seq` on the recovery path, and returns `seq` unchanged on the non-recovery path. The commit also combines an `if` and a `match` to merge two identical paths. Also change `recover_seq_parse_error` so it receives a `PErr` instead of a `PResult`, because all the call sites now handle the `Ok`/`Err` distinction themselves.

It's not clear why this was here, because the created error is returned as a normal error anyway. Nor is it clear why removing the call works. The change doesn't affect any tests; `tests/ui/parser/issues/issue-102182-impl-trait-recover.rs` looks like the only test that could have been affected.

The old code was very hard to understand, involving an `emit_without_consuming` call *and* a `delay_as_bug_without_consuming` call. With slight changes both calls can be avoided. Not creating the error until later is crucial, as is the early return in the `if recovered` block. It took me some time to come up with this reworking -- it went through intermediate states much further from the original code than this final version -- and it's isn't obvious at a glance that it is equivalent. But I think it is, and the unchanged test behaviour is good supporting evidence. The commit also changes `check_trailing_angle_brackets` to return `Option<ErrorGuaranteed>`. This provides a stricter proof that it emitted an error message than asserting `dcx.has_errors().is_some()`, which would succeed if any error had previously been emitted anywhere.

A nice cleanup: it's now impossible to directly emit a `DiagnosticBuilder` without consuming it.

The existing uses are replaced in one of three ways. - In a function that also has calls to `emit`, just rearrange the code so that exactly one of `delay_as_bug` or `emit` is called on every path. - In a function returning a `DiagnosticBuilder`, use `downgrade_to_delayed_bug`. That's good enough because it will get emitted later anyway. - In `unclosed_delim_err`, one set of errors is being replaced with another set, so just cancel the original errors.

This is now possible, thanks to changes in previous commits.

Currently it's used for two dynamic checks: - When a diagnostic is emitted, has it been emitted before? - When a diagnostic is dropped, has it been emitted/cancelled? The first check is no longer need, because `emit` is consuming, so it's impossible to emit a `DiagnosticBuilder` twice. The second check is still needed. This commit replaces `DiagnosticBuilderState` with a simpler `Option<Box<Diagnostic>>`, which is enough for the second check: functions like `emit` and `cancel` can take the `Diagnostic` and then `drop` can check that the `Diagnostic` was taken. The `DiagCtxt` reference from `DiagnosticBuilderState` is now stored as its own field, removing the need for the `dcx` method. As well as making the code shorter and simpler, the commit removes: - One (deprecated) `ErrorGuaranteed::unchecked_claim_error_was_emitted` call. - Two `FIXME(eddyb)` comments that are no longer relevant. - The use of a dummy `Diagnostic` in `into_diagnostic`. Nice!

They are no longer used, because `{DiagCtxt,DiagCtxtInner}::emit_diagnostic` are used everywhere instead. This also means `track_diagnostic` can become consuming.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consuming `emit` #119606

Consuming `emit` #119606

Commits on Jan 8, 2024

Consuming emit #119606

Consuming emit #119606

Commits on Jan 8, 2024

Consuming `emit` #119606

Consuming `emit` #119606