rust-clippy icon indicating copy to clipboard operation
rust-clippy copied to clipboard

Resolve duplicate diag message for: tests/ui/renamed_builtin_attr.rs

Open grtn316 opened this issue 1 year ago • 6 comments

This PR is to resolve duplicate diag messages for the renamed_builtin_attr UI test which is related to: #12379

This was caused by multiple functions calling the get_attr function resulting in the deprecated attribute to be reevaluated.

I added a state to the attr.rs file to track if the warning has already been emitted and skip the evaluation if the attribute warning has already been emitted.

changelog: [renamed_builtin_attr]: Fix duplicate diagnostics

grtn316 avatar Jun 19 '24 15:06 grtn316

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot avatar Jun 19 '24 15:06 rustbot

This one is a bit of an edge case compared to the others since it's not being caused by a single lint. Since the deduplication here is similar to what the diagnostic infrastructure is doing internally it might be worth keeping this one as a duplicate emitter

Alexendoo avatar Jun 19 '24 22:06 Alexendoo

r? @Alexendoo

Manishearth avatar Jun 20 '24 01:06 Manishearth

This one is a bit of an edge case compared to the others since it's not being caused by a single lint. Since the deduplication here is similar to what the diagnostic infrastructure is doing internally it might be worth keeping this one as a duplicate emitter

Hi @Alexendoo. The only thing worth mentioning is that currently, without this fix, each deprecated attribute will be evaluated 4x which means 4 messages per attribute will be emitted. With this fix it will only be emitted once. More of a cosmetic thing but wanted to mention just in case.

grtn316 avatar Jun 20 '24 15:06 grtn316

:umbrella: The latest upstream changes (presumably #12999) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Jun 27 '24 17:06 bors

UI wise duplicates are hidden by default, we set -Zdeduplicate-diagnostics=no to show duplicates as it may indicate an issue. You're right though that it's still being evaluated repeatedly which is wasteful, we could move the check into e.g. https://github.com/rust-lang/rust-clippy/blob/68a799aea9b65e2444fbecfe32217ce7d5a3604f/clippy_lints/src/attrs/mod.rs#L532

so it's only happening once per attribute

Alexendoo avatar Jun 27 '24 17:06 Alexendoo

@rustbot author

Alexendoo avatar Jul 17 '24 14:07 Alexendoo

:umbrella: The latest upstream changes (possibly d28d2344d000aa96bef729cf408731f952f71fb0) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot avatar Mar 31 '25 23:03 rustbot

@grtn316 Hi, this is a ping from triage, would you mind resolving the merge conflicts and take look of the last comment by 'Alexendoo'?

J-ZhengLi avatar Sep 03 '25 02:09 J-ZhengLi

Closing this since there's been no response. @grtn316 if you wish to come back to this feel free to reopen this or create a new PR.

Jarcho avatar Oct 01 '25 20:10 Jarcho