Remove the sym::Deref diagnostic item#148033
Conversation
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
sym::Defer diagnostic itemsym::Deref diagnostic item
The `Deref` trait is already as lang item, there is no need for it to be a diagnostic item as well.
9d89069 to
8275f31
Compare
You fixing the typo in the PR title made me notice it was present (twice!) in the commit message. I've pushed a commit with the typo fixed ( |
That was @lqd. I admit that I did not review the commit messages... :) |
There was a problem hiding this comment.
looking at the changes, while I think avoiding duplication between diagnostic and lang items is good, the changes themselves are... not amazing.
I feel like ideally code shouldn't have to care about the difference between lang and diagnostics items. What are the meaningful differences between em
- lang item use an enum preventing accidentally referring to an item that doesn't actually exist, while diagnostic items theoretically allow being introduced in third party crates without rustc knowing about it (just for clippy)
- being a diagnostic item should not impact happy path behavior in any way. it should only matter for diagnostics, while lang items are more interesting as they also impact semantics
I guess this is fine and don't have a great suggestion here. I think some API which fetches either the diagnostic or the lang item name would be nice so that the set of diagnostic items is always a super set of all lang items. But 🤷 on how to do that and think it doesn't have to happen rn. Would be happy if you (or somebody else) wants to look into that separately from this PR
| if !matches!(trait_, sym::Borrow | sym::Clone | sym::Deref) { | ||
| return; | ||
| let trait_ = match cx.tcx.get_diagnostic_name(trait_id) { | ||
| Some(trait_ @ (sym::Borrow | sym::Clone)) => trait_, |
There was a problem hiding this comment.
the same is also true for Clone 🤔
There was a problem hiding this comment.
the same as in Clone is also both langitem and diagitem? And so should perhaps lose its diagitem status?
There was a problem hiding this comment.
I'll propose a unified way in this PR.
@rustbot author
|
@samueltardieu any updates on this? thanks |
The
Dereftrait is already as lang item, there is no need for it to be a diagnostic item as well.r? lcnr