Remove Session::one_time_diagnostic
#95149
Conversation
Taking a Diagnostic by move would break the usual pattern `diag.label(..).emit()`.
rustbot
added
the
T-compiler
|
Some changes occurred in src/tools/clippy. cc @rust-lang/clippy Some changes occurred in src/tools/rustfmt. |
|
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // FIXME(eddyb) this should ideally take `diagnostic` by value. | ||
| fn emit_diagnostic(&mut self, diagnostic: &Diagnostic) -> Option<ErrorGuaranteed> { | ||
| fn emit_diagnostic(&mut self, diagnostic: &mut Diagnostic) -> Option<ErrorGuaranteed> { |
There was a problem hiding this comment.
Would fixing the FIXME be possible as part of this PR, or are there big consequences that I'm failing to think of?
There was a problem hiding this comment.
This requires to make the DiagnosticBuilder::emit method take self by move, which breaks struct_err(..).label(..).emit() chains.
There was a problem hiding this comment.
Not exactly, I decided we can't make .emit() take &mut self for that reason, but once .emit() is called the DiagnosticBuilder state changes to ignore the Diagnostic on further .emit(), so we could be replacing the Diagnostic with a dummy one (or even having Option around it but that could slow down the Deref/DerefMut impl a bunch).
| note: the lint level is defined here | ||
| --> $DIR/lint-tool-test.rs:14:9 | ||
| | | ||
| LL | #![deny(clippy_group)] | ||
| | ^^^^^^^^^^^^ |
There was a problem hiding this comment.
This seems to be a small issue: for lints the current deduplication strategy is (lint_id, group_id), where the new deduplication strategy is only group_id. Fixing this might be "as simple" as changing the hashing to also account for the subdiagnostic's parent Diagnostic::code.
There was a problem hiding this comment.
Oh! Wait, the note is still being emitted, but now we don't point at the group. That's perfectly fine.
|
rustdoc-ui tests have been blessed, with the expected behaviour. |
|
@bors r=estebank |
|
📌 Commit f7d5b7a has been approved by |
bors
added
S-waiting-on-bors
Remove `Session::one_time_diagnostic`
This is untracked mutable state, which modified the behaviour of queries.
It was used for 2 things: some full-blown errors, but mostly for lint declaration notes ("the lint level is defined here" notes).
It is replaced by the diagnostic deduplication infra which already exists in the diagnostic emitter.
A new diagnostic level `OnceNote` is introduced specifically for lint notes, to deduplicate subdiagnostics.
As a drive-by, diagnostic emission takes a `&mut` to allow dropping the `SubDiagnostic`s.
|
⌛ Testing commit f7d5b7a with merge beeb270e17dcf0662d43128e69a07059d98374e0... |
Remove `Session::one_time_diagnostic`
This is untracked mutable state, which modified the behaviour of queries.
It was used for 2 things: some full-blown errors, but mostly for lint declaration notes ("the lint level is defined here" notes).
It is replaced by the diagnostic deduplication infra which already exists in the diagnostic emitter.
A new diagnostic level `OnceNote` is introduced specifically for lint notes, to deduplicate subdiagnostics.
As a drive-by, diagnostic emission takes a `&mut` to allow dropping the `SubDiagnostic`s.
|
@bors retry (included in rollup so yielding) |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (c749254): comparison url. Summary: This benchmark run did not return any relevant results. 13 results were found to be statistically significant but too small to be relevant. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Remove `Session::one_time_diagnostic`
This is untracked mutable state, which modified the behaviour of queries.
It was used for 2 things: some full-blown errors, but mostly for lint declaration notes ("the lint level is defined here" notes).
It is replaced by the diagnostic deduplication infra which already exists in the diagnostic emitter.
A new diagnostic level `OnceNote` is introduced specifically for lint notes, to deduplicate subdiagnostics.
As a drive-by, diagnostic emission takes a `&mut` to allow dropping the `SubDiagnostic`s.
This is untracked mutable state, which modified the behaviour of queries.
It was used for 2 things: some full-blown errors, but mostly for lint declaration notes ("the lint level is defined here" notes).
It is replaced by the diagnostic deduplication infra which already exists in the diagnostic emitter.
A new diagnostic level
OnceNoteis introduced specifically for lint notes, to deduplicate subdiagnostics.As a drive-by, diagnostic emission takes a
&mutto allow dropping theSubDiagnostics.