Skip to content

Commit

Permalink
Don't hash lints differently to non-lints.
Browse files Browse the repository at this point in the history
`Diagnostic::keys`, which is used for hashing and equating diagnostics,
has a surprising behaviour: it ignores children, but only for lints.
This was added in rust-lang#88493 to fix some duplicated diagnostics, but it
doesn't seem necessary any more.

This commit removes the special case and only four tests have changed
output, with additional errors. And those additional errors aren't
exact duplicates, they're just similar. For example, in
src/tools/clippy/tests/ui/same_name_method.rs we currently have this
error:
```
error: method's name is the same as an existing method in a trait
  --> $DIR/same_name_method.rs:75:13
   |
LL |             fn foo() {}
   |             ^^^^^^^^^^^
   |
note: existing `foo` defined here
  --> $DIR/same_name_method.rs:79:9
   |
LL |         impl T1 for S {}
   |         ^^^^^^^^^^^^^^^^
```
and with this change we also get this error:
```
error: method's name is the same as an existing method in a trait
  --> $DIR/same_name_method.rs:75:13
   |
LL |             fn foo() {}
   |             ^^^^^^^^^^^
   |
note: existing `foo` defined here
  --> $DIR/same_name_method.rs:81:9
   |
LL |         impl T2 for S {}
   |         ^^^^^^^^^^^^^^^^
```
I think printing this second argument is reasonable, possibly even
preferable to hiding it. And the other cases are similar.
  • Loading branch information
nnethercote committed Jan 30, 2024
1 parent f3d71c9 commit 4225a1e
Show file tree
Hide file tree
Showing 6 changed files with 23 additions and 15 deletions.
16 changes: 8 additions & 8 deletions compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,6 @@ pub struct Diagnostic {
/// `span` if there is one. Otherwise, it is `DUMMY_SP`.
pub sort_span: Span,

/// If diagnostic is from Lint, custom hash function ignores children.
/// Otherwise hash is based on the all the fields.
pub is_lint: Option<IsLint>,

/// With `-Ztrack_diagnostics` enabled,
Expand Down Expand Up @@ -980,22 +978,24 @@ impl Diagnostic {
) -> (
&Level,
&[(DiagnosticMessage, Style)],
Vec<(&Cow<'static, str>, &DiagnosticArgValue)>,
&Option<ErrCode>,
&Option<IsLint>,
&MultiSpan,
&[SubDiagnostic],
&Result<Vec<CodeSuggestion>, SuggestionsDisabled>,
Option<&[SubDiagnostic]>,
Vec<(&DiagnosticArgName, &DiagnosticArgValue)>,
&Option<IsLint>,
) {
(
&self.level,
&self.messages,
self.args().collect(),
&self.code,
&self.is_lint,
&self.span,
&self.children,
&self.suggestions,
(if self.is_lint.is_some() { None } else { Some(&self.children) }),
self.args().collect(),
// omit self.sort_span
&self.is_lint,
// omit self.emitted_at
)
}
}
Expand Down
1 change: 1 addition & 0 deletions src/tools/clippy/tests/ui/same_name_method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ mod should_lint {
impl S {
fn foo() {}
//~^ ERROR: method's name is the same as an existing method in a trait
//~| ERROR: method's name is the same as an existing method in a trait
}

impl T1 for S {}
Expand Down
16 changes: 14 additions & 2 deletions src/tools/clippy/tests/ui/same_name_method.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,22 @@ LL | fn foo() {}
| ^^^^^^^^^^^
|
note: existing `foo` defined here
--> $DIR/same_name_method.rs:79:9
--> $DIR/same_name_method.rs:80:9
|
LL | impl T1 for S {}
| ^^^^^^^^^^^^^^^^

error: aborting due to 5 previous errors
error: method's name is the same as an existing method in a trait
--> $DIR/same_name_method.rs:75:13
|
LL | fn foo() {}
| ^^^^^^^^^^^
|
note: existing `foo` defined here
--> $DIR/same_name_method.rs:82:9
|
LL | impl T2 for S {}
| ^^^^^^^^^^^^^^^^

error: aborting due to 6 previous errors

3 changes: 0 additions & 3 deletions tests/rustdoc-ui/unescaped_backticks.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,6 @@ LL | | /// level changes.
= help: if you meant to use a literal backtick, escape it
change: or `None` if it isn't.
to this: or `None\` if it isn't.
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: unescaped backtick
--> $DIR/unescaped_backticks.rs:323:5
Expand All @@ -322,7 +321,6 @@ LL | | /// level changes.
= help: if you meant to use a literal backtick, escape it
change: `on_event` should be called.
to this: `on_event\` should be called.
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: unescaped backtick
--> $DIR/unescaped_backticks.rs:323:5
Expand All @@ -342,7 +340,6 @@ LL | | /// level changes.
= help: if you meant to use a literal backtick, escape it
change: [`rebuild_interest_cache`][rebuild] is called after the value of the max
to this: [`rebuild_interest_cache\`][rebuild] is called after the value of the max
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: unescaped backtick
--> $DIR/unescaped_backticks.rs:349:56
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ help: the constant being evaluated
|
LL | const Y: u32 = simple_loop(35);
| ^^^^^^^^^^^^
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

warning: constant evaluation is taking a long time
--> $DIR/ctfe-simple-loop.rs:9:5
Expand Down
1 change: 0 additions & 1 deletion tests/ui/imports/ambiguous-9.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ note: `date_range` could also refer to the function imported here
LL | use prelude::*;
| ^^^^^^^^^^
= help: consider adding an explicit import of `date_range` to disambiguate
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

warning: 4 warnings emitted

0 comments on commit 4225a1e

Please sign in to comment.