Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consuming emit #119606

Merged
merged 14 commits into from
Jan 8, 2024
Merged

Consuming emit #119606

merged 14 commits into from
Jan 8, 2024

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jan 5, 2024

This PR makes DiagnosticBuilder::emit consuming, i.e. take self instead of &mut self. This is good because it doesn't make sense to emit a diagnostic twice.

This requires some changes to DiagnosticBuilder method changing -- every existing non-consuming chaining method gets a new consuming partner with a _mv suffix -- but permits a host of beneficial follow-up changes: more concise code through more chaining, removal of redundant diagnostic construction API methods, and removal of machinery to track the possibility of a diagnostic being emitted multiple times.

r? @compiler-errors

@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2024

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Type relation code was changed

cc @compiler-errors, @lcnr

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

@nnethercote nnethercote marked this pull request as draft January 5, 2024 07:27
@nnethercote
Copy link
Contributor Author

Draft code only for now, apologies for the notifications.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 5, 2024

☔ The latest upstream changes (presumably #119621) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

It seems like a bad idea, just asking for diagnostics to be emitted
multiple times.
@rust-log-analyzer

This comment has been minimized.

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.
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.
@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 8, 2024
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@oli-obk
Copy link
Contributor

oli-obk commented Jan 8, 2024

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 8, 2024
@bors
Copy link
Contributor

bors commented Jan 8, 2024

⌛ Testing commit db09eb2 with merge ca663b0...

@bors
Copy link
Contributor

bors commented Jan 8, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing ca663b0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 8, 2024
@bors bors merged commit ca663b0 into rust-lang:master Jan 8, 2024
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 8, 2024
@nnethercote nnethercote deleted the consuming-emit branch January 8, 2024 20:57
nnethercote added a commit to nnethercote/rust that referenced this pull request Jan 8, 2024
In rust-lang#119606 I added them and used a `_mv` suffix, but that wasn't great.

A `with_` prefix has three different existing uses.
- Constructors, e.g. `Vec::with_capacity`.
- Wrappers that provide an environment to execute some code, e.g.
  `with_session_globals`.
- Consuming chaining methods, e.g. `Span::with_{lo,hi,ctxt}`.

The third case is exactly what we want, so this commit changes
`DiagnosticBuilder::foo_mv` to `DiagnosticBuilder::with_foo`.

Thanks to @compiler-errors for the suggestion.
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ca663b0): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.7% [1.7%, 1.7%] 1
Regressions ❌
(secondary)
10.4% [10.4%, 10.4%] 1
Improvements ✅
(primary)
-0.5% [-0.6%, -0.4%] 2
Improvements ✅
(secondary)
-3.0% [-4.6%, -1.4%] 2
All ❌✅ (primary) 0.2% [-0.6%, 1.7%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.1% [-1.3%, -0.9%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.1% [-1.3%, -0.9%] 2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 667.831s -> 668.073s (0.04%)
Artifact size: 308.39 MiB -> 308.47 MiB (0.03%)

nnethercote added a commit to nnethercote/rust that referenced this pull request Jan 9, 2024
In rust-lang#119606 I added them and used a `_mv` suffix, but that wasn't great.

A `with_` prefix has three different existing uses.
- Constructors, e.g. `Vec::with_capacity`.
- Wrappers that provide an environment to execute some code, e.g.
  `with_session_globals`.
- Consuming chaining methods, e.g. `Span::with_{lo,hi,ctxt}`.

The third case is exactly what we want, so this commit changes
`DiagnosticBuilder::foo_mv` to `DiagnosticBuilder::with_foo`.

Thanks to @compiler-errors for the suggestion.
nnethercote added a commit to nnethercote/rust that referenced this pull request Jan 9, 2024
In rust-lang#119606 I added them and used a `_mv` suffix, but that wasn't great.

A `with_` prefix has three different existing uses.
- Constructors, e.g. `Vec::with_capacity`.
- Wrappers that provide an environment to execute some code, e.g.
  `with_session_globals`.
- Consuming chaining methods, e.g. `Span::with_{lo,hi,ctxt}`.

The third case is exactly what we want, so this commit changes
`DiagnosticBuilder::foo_mv` to `DiagnosticBuilder::with_foo`.

Thanks to @compiler-errors for the suggestion.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 11, 2024
In rust-lang#119606 I added them and used a `_mv` suffix, but that wasn't great.

A `with_` prefix has three different existing uses.
- Constructors, e.g. `Vec::with_capacity`.
- Wrappers that provide an environment to execute some code, e.g.
  `with_session_globals`.
- Consuming chaining methods, e.g. `Span::with_{lo,hi,ctxt}`.

The third case is exactly what we want, so this commit changes
`DiagnosticBuilder::foo_mv` to `DiagnosticBuilder::with_foo`.

Thanks to @compiler-errors for the suggestion.
feliperodri pushed a commit to model-checking/kani that referenced this pull request Jan 18, 2024
Fixes were done to address the following upstream changes:

- rust-lang/rust#119606
- rust-lang/rust#119751
- rust-lang/rust#120025
- rust-lang/rust#116520

Resolves #2971 

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 2, 2024
All the other `emit`/`emit_diagnostic` methods were recently made
consuming (e.g. rust-lang#119606), but this one wasn't. But it makes sense.

Much of this is straightforward, and lots of `clone` calls are avoided.
There are a couple of tricky bits.
- `Emitter::primary_span_formatted` no longer takes a `Diagnostic` and
  returns a pair. Instead it takes the two fields from `Diagnostic` that
  it used (`span` and `suggestions`) as `&mut`, and modifies them. This
  is necessary to avoid the cloning of `diag.children` in two emitters.
- `from_errors_diagnostic` is rearranged so various used of `diag` now
  occur before the consuming `emit_diagnostic` call.
nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 2, 2024
All the other `emit`/`emit_diagnostic` methods were recently made
consuming (e.g. rust-lang#119606), but this one wasn't. But it makes sense to.

Much of this is straightforward, and lots of `clone` calls are avoided.
There are a couple of tricky bits.
- `Emitter::primary_span_formatted` no longer takes a `Diagnostic` and
  returns a pair. Instead it takes the two fields from `Diagnostic` that
  it used (`span` and `suggestions`) as `&mut`, and modifies them. This
  is necessary to avoid the cloning of `diag.children` in two emitters.
- `from_errors_diagnostic` is rearranged so various uses of `diag` occur
  before the consuming `emit_diagnostic` call.
nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 2, 2024
All the other `emit`/`emit_diagnostic` methods were recently made
consuming (e.g. rust-lang#119606), but this one wasn't. But it makes sense to.

Much of this is straightforward, and lots of `clone` calls are avoided.
There are a couple of tricky bits.
- `Emitter::primary_span_formatted` no longer takes a `Diagnostic` and
  returns a pair. Instead it takes the two fields from `Diagnostic` that
  it used (`span` and `suggestions`) as `&mut`, and modifies them. This
  is necessary to avoid the cloning of `diag.children` in two emitters.
- `from_errors_diagnostic` is rearranged so various uses of `diag` occur
  before the consuming `emit_diagnostic` call.
nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 5, 2024
All the other `emit`/`emit_diagnostic` methods were recently made
consuming (e.g. rust-lang#119606), but this one wasn't. But it makes sense to.

Much of this is straightforward, and lots of `clone` calls are avoided.
There are a couple of tricky bits.
- `Emitter::primary_span_formatted` no longer takes a `Diagnostic` and
  returns a pair. Instead it takes the two fields from `Diagnostic` that
  it used (`span` and `suggestions`) as `&mut`, and modifies them. This
  is necessary to avoid the cloning of `diag.children` in two emitters.
- `from_errors_diagnostic` is rearranged so various uses of `diag` occur
  before the consuming `emit_diagnostic` call.
nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 5, 2024
All the other `emit`/`emit_diagnostic` methods were recently made
consuming (e.g. rust-lang#119606), but this one wasn't. But it makes sense to.

Much of this is straightforward, and lots of `clone` calls are avoided.
There are a couple of tricky bits.
- `Emitter::primary_span_formatted` no longer takes a `Diagnostic` and
  returns a pair. Instead it takes the two fields from `Diagnostic` that
  it used (`span` and `suggestions`) as `&mut`, and modifies them. This
  is necessary to avoid the cloning of `diag.children` in two emitters.
- `from_errors_diagnostic` is rearranged so various uses of `diag` occur
  before the consuming `emit_diagnostic` call.
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jun 22, 2024
All the other `emit`/`emit_diagnostic` methods were recently made
consuming (e.g. rust-lang#119606), but this one wasn't. But it makes sense to.

Much of this is straightforward, and lots of `clone` calls are avoided.
There are a couple of tricky bits.
- `Emitter::primary_span_formatted` no longer takes a `Diagnostic` and
  returns a pair. Instead it takes the two fields from `Diagnostic` that
  it used (`span` and `suggestions`) as `&mut`, and modifies them. This
  is necessary to avoid the cloning of `diag.children` in two emitters.
- `from_errors_diagnostic` is rearranged so various uses of `diag` occur
  before the consuming `emit_diagnostic` call.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants