refactor(non_canonical_impls): lint starting from impls
#15749
Conversation
rustbot
added
the
S-waiting-on-review
rustbot
added
the
S-blocked
|
No changes for e776e42 |
ada4a
force-pushed
the
non_canonical_impls-2
branch
3 times, most recently
from
7b7a795 to
9718c47
Compare
|
The PR was merged, so @rustbot label -S-blocked |
rustbot
removed
the
S-blocked
ada4a
force-pushed
the
non_canonical_impls-2
branch
from
9718c47 to
948e23d
Compare
This comment has been minimized.
This comment has been minimized.
ada4a
force-pushed
the
non_canonical_impls-2
branch
from
948e23d to
2168603
Compare
| && let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id()) | ||
| fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { | ||
| if let ItemKind::Impl(impl_) = item.kind | ||
| && (1..=2).contains(&impl_.items.len()) |
There was a problem hiding this comment.
This needs at least since PartialOrd can have five function.
You'll also want to figure if the trait is one we need to check here before doing anything else. Get the DefId from the path to avoid doing a query.
| .iter() | ||
| .map(|id| cx.tcx.hir_impl_item(*id)) | ||
| .filter_map(|assoc| { | ||
| if let ImplItemKind::Fn(_, assoc_id) = assoc.kind |
| for (assoc, _, block) in assoc_fns { | ||
| // NOTE: we don't bother doing the method name check before the proc-macro check, because we | ||
| // lint both methods of `Clone`, and therefore would need to repeat the proc-macro check anyway | ||
| if !is_from_proc_macro(cx, assoc) { |
There was a problem hiding this comment.
is_from_proc_macro should be moved right before emitting the lint. It's very unlikely to actually filter anything.
| for (assoc, body, block) in assoc_fns { | ||
| // NOTE: `PartialOrd` has methods `lt/le/ge/gt` which we don't lint, so | ||
| // don't bother checking whether _they_ come from a proc-macro | ||
| if assoc.ident.name == sym::partial_cmp && !is_from_proc_macro(cx, assoc) { |
There was a problem hiding this comment.
is_from_proc_macro should be moved right before emitting the lint. It's very unlikely to actually filter anything.
rustbot
added
S-waiting-on-author
The missing external macro test for `non_canonical_clone_impl` was caught thanks to lintcheck -- I went ahead and added all the other combinations of the test while at it
Good idea. I'd like to keep that a separate commit though, if that's okay |
ada4a
force-pushed
the
non_canonical_impls-2
branch
from
2168603 to
e776e42
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Based on #15520 (comment), with the following changes/additions:
Traitenum for quickly filtering out irrelevant impls -- instead, check thetrait_ofbasically right away:DefIds of the relevant traits into the lint passcx.tcx.impl_trait_ref's output for the theDefIdof the trait being implementedDefIds, which should be very cheapself_tyimplements the other relevant trait.changelog: none
Not auto-assigning since @blyxyas and/or @Jarcho will be the ones reviewing it:
r? ghost