Don't hash lints differently to non-lints. #120490

nnethercote · 2024-01-30T08:41:23Z

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 #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

rustbot · 2024-01-30T08:41:33Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

chenyukang · 2024-01-30T10:34:11Z

ah, my first rustc PR left this surprising behavior 😅 .

I think we may need to investigate why two diagnostics with the same primary message(error: method's name is the same as an existing method in a trait), but have different note.

Give this extra diagnostic will still make users confused, even the note is different, but the second one didn't give more extra information.

chenyukang · 2024-01-30T10:36:54Z

oh, I missed it, they are different line of code, ignore me.

flip1995 · 2024-01-30T11:06:57Z

I think emitting both errors is fine here. I'd say this is rather an issue on the error creation itself. I think it would be better, if the note would list all conflicting implementations and the error would only be emitted once. But that is unrelated to this PR.

bors · 2024-01-30T16:48:08Z

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

estebank · 2024-01-30T20:24:45Z

src/tools/clippy/tests/ui/same_name_method.stderr

+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 {}
+   |         ^^^^^^^^^^^^^^^^


You need to modify the .rs file as well, don't you? Can you locally run python ./x.py test src/tools/clippy to confirm this is actually passing now?

Yeah, I ran the clippy tests before creating the PR, it works without changing the .rs file. If I add the second ERROR line it also works. And if I add a third ERROR line then the test fails.

So I have updated with the second ERROR added, because that's what one would expect. I don't know why it was working without the second ERROR.

We don't enforce ERROR annotations yet. Thanks for adding it!

estebank · 2024-01-30T20:25:21Z

r=me

`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 · 2024-01-30T21:29:25Z

I rebased and added the second ERROR to the clippy test, even though it wasn't necessary, because it's what people would expect.

@bors r=estebank

bors · 2024-01-30T21:29:27Z

📌 Commit 4225a1e has been approved by estebank

It is now in the queue for this repository.

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

Rollup of 12 pull requests Successful merges: - rust-lang#120207 (check `RUST_BOOTSTRAP_CONFIG` in `profile_user_dist` test) - rust-lang#120321 (pattern_analysis: cleanup the contexts) - rust-lang#120323 (On E0277 be clearer about implicit `Sized` bounds on type params and assoc types) - rust-lang#120355 (document `FromIterator for Vec` allocation behaviors) - rust-lang#120396 (Account for unbounded type param receiver in suggestions) - rust-lang#120430 (std: thread_local::register_dtor fix proposal for FreeBSD.) - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg) - rust-lang#120470 (Mark "unused binding" suggestion as maybe incorrect) - rust-lang#120472 (Make duplicate lang items fatal) - rust-lang#120490 (Don't hash lints differently to non-lints.) - rust-lang#120495 (Remove the `abi_amdgpu_kernel` feature) - rust-lang#120501 (rustdoc: Correctly handle attribute merge if this is a glob reexport) Failed merges: - rust-lang#120346 (hir: Refactor getters for owner nodes) r? `@ghost` `@rustbot` modify labels: rollup

Nadrieril · 2024-01-31T08:35:58Z

Got this failure in rollup, which at a glance looks like it's coming from this PR.

Nadrieril · 2024-01-31T11:05:30Z

Welp, apparently came from a different PR

Rollup of 9 pull requests Successful merges: - rust-lang#120207 (check `RUST_BOOTSTRAP_CONFIG` in `profile_user_dist` test) - rust-lang#120321 (pattern_analysis: cleanup the contexts) - rust-lang#120355 (document `FromIterator for Vec` allocation behaviors) - rust-lang#120430 (std: thread_local::register_dtor fix proposal for FreeBSD.) - rust-lang#120469 (Provide more context on derived obligation error primary label) - rust-lang#120472 (Make duplicate lang items fatal) - rust-lang#120490 (Don't hash lints differently to non-lints.) - rust-lang#120495 (Remove the `abi_amdgpu_kernel` feature) - rust-lang#120501 (rustdoc: Correctly handle attribute merge if this is a glob reexport) r? `@ghost` `@rustbot` modify labels: rollup

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`

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

rustbot assigned estebank Jan 30, 2024

rustbot added 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 30, 2024

estebank reviewed Jan 30, 2024

View reviewed changes

nnethercote force-pushed the Diagnostic-hashing branch from 90b68e2 to 4225a1e Compare January 30, 2024 21:25

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 30, 2024

Nadrieril mentioned this pull request Jan 31, 2024

Rollup of 12 pull requests #120519

Closed

Nadrieril mentioned this pull request Jan 31, 2024

Rollup of 9 pull requests #120524

Merged

bors merged commit be4f8e2 into rust-lang:master Jan 31, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 31, 2024

nnethercote deleted the Diagnostic-hashing branch January 31, 2024 22:10

rust-timer mentioned this pull request Feb 9, 2024

Rollup of 11 pull requests #120852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't hash lints differently to non-lints. #120490

Don't hash lints differently to non-lints. #120490

nnethercote commented Jan 30, 2024

rustbot commented Jan 30, 2024

chenyukang commented Jan 30, 2024 •

edited

Loading

chenyukang commented Jan 30, 2024

flip1995 commented Jan 30, 2024

bors commented Jan 30, 2024

estebank Jan 30, 2024

nnethercote Jan 30, 2024

flip1995 Jan 31, 2024

estebank commented Jan 30, 2024

nnethercote commented Jan 30, 2024

bors commented Jan 30, 2024

Nadrieril commented Jan 31, 2024

Nadrieril commented Jan 31, 2024

Don't hash lints differently to non-lints. #120490

Don't hash lints differently to non-lints. #120490

Conversation

nnethercote commented Jan 30, 2024

rustbot commented Jan 30, 2024

chenyukang commented Jan 30, 2024 • edited Loading

chenyukang commented Jan 30, 2024

flip1995 commented Jan 30, 2024

bors commented Jan 30, 2024

estebank Jan 30, 2024

Choose a reason for hiding this comment

nnethercote Jan 30, 2024

Choose a reason for hiding this comment

flip1995 Jan 31, 2024

Choose a reason for hiding this comment

estebank commented Jan 30, 2024

nnethercote commented Jan 30, 2024

bors commented Jan 30, 2024

Nadrieril commented Jan 31, 2024

Nadrieril commented Jan 31, 2024

chenyukang commented Jan 30, 2024 •

edited

Loading