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

Fix #88256 remove duplicated diagnostics #88493

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

chenyukang
Copy link
Member

@chenyukang chenyukang commented Aug 30, 2021

Fix #88256

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2021
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one partial regression and I have a theory for how we could avoid it. Could you try it out?

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you squash all the commits?

r=me after addressing the comments.

compiler/rustc_errors/src/diagnostic.rs Outdated Show resolved Hide resolved
compiler/rustc_errors/src/diagnostic_builder.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/lint.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@chenyukang
Copy link
Member Author

r? @estebank

@estebank
Copy link
Contributor

estebank commented Sep 5, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Sep 5, 2021

📌 Commit ca27f03 has been approved by estebank

@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 Sep 5, 2021
@bors
Copy link
Contributor

bors commented Sep 6, 2021

⌛ Testing commit ca27f03 with merge 8f3aa5e...

@bors
Copy link
Contributor

bors commented Sep 6, 2021

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 8f3aa5e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 6, 2021
@bors bors merged commit 8f3aa5e into rust-lang:master Sep 6, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 6, 2021
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 8, 2021
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Oct 20, 2021
nnethercote added a commit to nnethercote/rust that referenced this pull request Jan 30, 2024
`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.
nnethercote added a commit to nnethercote/rust that referenced this pull request Jan 30, 2024
`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.
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 31, 2024
…estebank

Don't hash lints differently to non-lints.

`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.

r? `@estebank`
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 31, 2024
…estebank

Don't hash lints differently to non-lints.

`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.

r? `@estebank`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2024
Rollup merge of rust-lang#120490 - nnethercote:Diagnostic-hashing, r=estebank

Don't hash lints differently to non-lints.

`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.

r? `@estebank`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 8, 2024
`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.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 8, 2024
…estebank

Don't hash lints differently to non-lints.

`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.

r? `@estebank`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary parentheses warning is printed twice
6 participants