Mirrors/rust
1
0
Fork 0
mirror of https://github.com/rust-lang/rust.git synced 2026-05-15 20:45:45 +03:00
Code Issues Packages Projects Releases Wiki Activity

refactor(non_canonical_impls): lint starting from impls (#15749)

Browse Source 
Based on
https://github.com/rust-lang/rust-clippy/pull/15520#discussion_r2372599748,
with the following changes/additions:
- No longer use the `Trait` enum for quickly filtering out irrelevant
impls -- instead, check the `trait_of` basically right away:
    1. pre-fetch `DefId`s of the relevant traits into the lint pass
2. reuse `cx.tcx.impl_trait_ref`'s output for the the `DefId` of the
trait being implemented
    3. compare those `DefId`s, which should be very cheap
- Next, check whether `self_ty` implements the other relevant trait.
- Pre-filter impl items in the same (lazy) iterator, but delay the
proc-macro check as much as possible

changelog: none

Not auto-assigning since @blyxyas and/or @Jarcho will be the ones
reviewing it:
r? ghost
This commit is contained in:
Jason Newcomb
2025-10-15 02:20:21 +00:00
committed by GitHub
parent d230acd9cf 3c02c0ef20
commit ec8e8fde91
8 changed files with 357 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files
clippy_lints/src/lib.rs
+1 -1
View File
@@ -765,7 +765,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
store.register_late_pass(move |_| Box::new(large_stack_frames::LargeStackFrames::new(conf)));
store.register_late_pass(|_| Box::new(single_range_in_vec_init::SingleRangeInVecInit));
store.register_late_pass(move |_| Box::new(needless_pass_by_ref_mut::NeedlessPassByRefMut::new(conf)));
store.register_late_pass(|_| Box::new(non_canonical_impls::NonCanonicalImpls));
store.register_late_pass(|tcx| Box::new(non_canonical_impls::NonCanonicalImpls::new(tcx)));
store.register_late_pass(move |_| Box::new(single_call_fn::SingleCallFn::new(conf)));
store.register_early_pass(move || Box::new(raw_strings::RawStrings::new(conf)));
store.register_late_pass(move |_| Box::new(legacy_numeric_constants::LegacyNumericConstants::new(conf)));
clippy_lints/src/non_canonical_impls.rs
+96 -34
View File
@@ -3,10 +3,11 @@
use clippy_utils::ty::implements_trait;
use clippy_utils::{is_from_proc_macro, last_path_segment, std_or_core};
use rustc_errors::Applicability;
use rustc_hir::{Block, Body, Expr, ExprKind, ImplItem, ImplItemKind, Item, LangItem, Node, UnOp};
use rustc_hir::def_id::DefId;
use rustc_hir::{Block, Body, Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, LangItem, UnOp};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::ty::{EarlyBinder, TraitRef, TypeckResults};
use rustc_session::declare_lint_pass;
use rustc_middle::ty::{EarlyBinder, TyCtxt, TypeckResults};
use rustc_session::impl_lint_pass;
use rustc_span::sym;
use rustc_span::symbol::kw;
@@ -107,33 +108,96 @@
suspicious,
"non-canonical implementation of `PartialOrd` on an `Ord` type"
}
declare_lint_pass!(NonCanonicalImpls => [NON_CANONICAL_CLONE_IMPL, NON_CANONICAL_PARTIAL_ORD_IMPL]);
impl_lint_pass!(NonCanonicalImpls => [NON_CANONICAL_CLONE_IMPL, NON_CANONICAL_PARTIAL_ORD_IMPL]);
#[expect(
clippy::struct_field_names,
reason = "`_trait` suffix is meaningful on its own, \
and creating an inner `StoredTraits` struct would just add a level of indirection"
)]
pub(crate) struct NonCanonicalImpls {
partial_ord_trait: Option<DefId>,
ord_trait: Option<DefId>,
clone_trait: Option<DefId>,
copy_trait: Option<DefId>,
}
impl NonCanonicalImpls {
pub(crate) fn new(tcx: TyCtxt<'_>) -> Self {
let lang_items = tcx.lang_items();
Self {
partial_ord_trait: lang_items.partial_ord_trait(),
ord_trait: tcx.get_diagnostic_item(sym::Ord),
clone_trait: lang_items.clone_trait(),
copy_trait: lang_items.copy_trait(),
}
}
}
/// The traits that this lint looks at
enum Trait {
Clone,
PartialOrd,
}
impl LateLintPass<'_> for NonCanonicalImpls {
fn check_impl_item<'tcx>(&mut self, cx: &LateContext<'tcx>, impl_item: &ImplItem<'tcx>) {
if let ImplItemKind::Fn(_, impl_item_id) = impl_item.kind
&& let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id())
&& let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder)
&& let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id)
// NOTE: check this early to avoid expensive checks that come after this one
&& matches!(trait_name, Some(sym::Clone | sym::PartialOrd))
fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
if let ItemKind::Impl(impl_) = item.kind
// Both `PartialOrd` and `Clone` have one required method, and `PartialOrd` can have 5 methods in total
&& (1..=5).contains(&impl_.items.len())
&& let Some(of_trait) = impl_.of_trait
&& let Some(trait_did) = of_trait.trait_ref.trait_def_id()
// Check this early to hopefully bail out as soon as possible
&& let trait_ = if Some(trait_did) == self.clone_trait {
Trait::Clone
} else if Some(trait_did) == self.partial_ord_trait {
Trait::PartialOrd
} else {
return;
}
&& !cx.tcx.is_automatically_derived(item.owner_id.to_def_id())
&& let body = cx.tcx.hir_body(impl_item_id)
&& let ExprKind::Block(block, ..) = body.value.kind
&& !block.span.in_external_macro(cx.sess().source_map())
&& !is_from_proc_macro(cx, impl_item)
{
if trait_name == Some(sym::Clone)
&& let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy)
&& implements_trait(cx, trait_impl.self_ty(), copy_def_id, &[])
{
check_clone_on_copy(cx, impl_item, block);
} else if trait_name == Some(sym::PartialOrd)
&& impl_item.ident.name == sym::partial_cmp
&& let Some(ord_def_id) = cx.tcx.get_diagnostic_item(sym::Ord)
&& implements_trait(cx, trait_impl.self_ty(), ord_def_id, &[])
{
check_partial_ord_on_ord(cx, impl_item, item, &trait_impl, body, block);
let mut assoc_fns = impl_
.items
.iter()
.map(|id| cx.tcx.hir_impl_item(*id))
.filter_map(|assoc| {
if let ImplItemKind::Fn(_, body_id) = assoc.kind
&& let body = cx.tcx.hir_body(body_id)
&& let ExprKind::Block(block, ..) = body.value.kind
&& !block.span.in_external_macro(cx.sess().source_map())
{
Some((assoc, body, block))
} else {
None
}
});
match trait_ {
Trait::Clone => {
if let Some(copy_trait) = self.copy_trait
&& let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder)
&& implements_trait(cx, trait_impl.self_ty(), copy_trait, &[])
{
for (assoc, _, block) in assoc_fns {
check_clone_on_copy(cx, assoc, block);
}
}
},
Trait::PartialOrd => {
if let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder)
// If `Self` and `Rhs` are not the same type, then a corresponding `Ord` impl is not possible,
// since it doesn't have an `Rhs`
&& let [lhs, rhs] = trait_impl.args.as_slice()
&& lhs == rhs
&& let Some(ord_trait) = self.ord_trait
&& implements_trait(cx, trait_impl.self_ty(), ord_trait, &[])
&& let Some((assoc, body, block)) =
assoc_fns.find(|(assoc, _, _)| assoc.ident.name == sym::partial_cmp)
{
check_partial_ord_on_ord(cx, assoc, item, body, block);
}
},
}
}
}
@@ -151,6 +215,10 @@ fn check_clone_on_copy(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, block: &B
return;
}
if is_from_proc_macro(cx, impl_item) {
return;
}
span_lint_and_sugg(
cx,
NON_CANONICAL_CLONE_IMPL,
@@ -162,7 +230,7 @@ fn check_clone_on_copy(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, block: &B
);
}
if impl_item.ident.name == sym::clone_from {
if impl_item.ident.name == sym::clone_from && !is_from_proc_macro(cx, impl_item) {
span_lint_and_sugg(
cx,
NON_CANONICAL_CLONE_IMPL,
@@ -179,7 +247,6 @@ fn check_partial_ord_on_ord<'tcx>(
cx: &LateContext<'tcx>,
impl_item: &ImplItem<'_>,
item: &Item<'_>,
trait_impl: &TraitRef<'_>,
body: &Body<'_>,
block: &Block<'tcx>,
) {
@@ -204,12 +271,7 @@ fn check_partial_ord_on_ord<'tcx>(
&& expr_is_cmp(cx, ret, impl_item, &mut needs_fully_qualified)
{
return;
}
// If `Self` and `Rhs` are not the same type, bail. This makes creating a valid
// suggestion tons more complex.
else if let [lhs, rhs, ..] = trait_impl.args.as_slice()
&& lhs != rhs
{
} else if is_from_proc_macro(cx, impl_item) {
return;
}
tests/ui/non_canonical_clone_impl.fixed
+40 -13
View File
@@ -4,7 +4,7 @@
#![no_main]
extern crate proc_macros;
use proc_macros::with_span;
use proc_macros::inline_macros;
// lint
@@ -100,18 +100,45 @@ impl<A: Copy> Clone for Uwu<A> {
impl<A: std::fmt::Debug + Copy + Clone> Copy for Uwu<A> {}
// should skip proc macros, see https://github.com/rust-lang/rust-clippy/issues/12788
#[derive(proc_macro_derive::NonCanonicalClone)]
pub struct G;
#[inline_macros]
mod issue12788 {
use proc_macros::{external, with_span};
with_span!(
span
// lint -- not an external macro
inline!(
#[derive(Copy)]
pub struct A;
#[derive(Copy)]
struct H;
impl Clone for H {
fn clone(&self) -> Self {
todo!()
impl Clone for A {
fn clone(&self) -> Self { *self }
}
}
);
);
// do not lint -- should skip external macros
external!(
#[derive(Copy)]
pub struct B;
impl Clone for B {
fn clone(&self) -> Self {
todo!()
}
}
);
// do not lint -- should skip proc macros
#[derive(proc_macro_derive::NonCanonicalClone)]
pub struct C;
with_span!(
span
#[derive(Copy)]
struct D;
impl Clone for D {
fn clone(&self) -> Self {
todo!()
}
}
);
}
tests/ui/non_canonical_clone_impl.rs
+43 -13
View File
@@ -4,7 +4,7 @@
#![no_main]
extern crate proc_macros;
use proc_macros::with_span;
use proc_macros::inline_macros;
// lint
@@ -114,18 +114,48 @@ fn clone_from(&mut self, source: &Self) {
impl<A: std::fmt::Debug + Copy + Clone> Copy for Uwu<A> {}
// should skip proc macros, see https://github.com/rust-lang/rust-clippy/issues/12788
#[derive(proc_macro_derive::NonCanonicalClone)]
pub struct G;
#[inline_macros]
mod issue12788 {
use proc_macros::{external, with_span};
with_span!(
span
// lint -- not an external macro
inline!(
#[derive(Copy)]
pub struct A;
#[derive(Copy)]
struct H;
impl Clone for H {
fn clone(&self) -> Self {
todo!()
impl Clone for A {
fn clone(&self) -> Self {
//~^ non_canonical_clone_impl
todo!()
}
}
}
);
);
// do not lint -- should skip external macros
external!(
#[derive(Copy)]
pub struct B;
impl Clone for B {
fn clone(&self) -> Self {
todo!()
}
}
);
// do not lint -- should skip proc macros
#[derive(proc_macro_derive::NonCanonicalClone)]
pub struct C;
with_span!(
span
#[derive(Copy)]
struct D;
impl Clone for D {
fn clone(&self) -> Self {
todo!()
}
}
);
}
tests/ui/non_canonical_clone_impl.stderr
+13 -1
View File
@@ -41,5 +41,17 @@ LL | | *self = source.clone();
LL | | }
| |_____^ help: remove it
error: aborting due to 4 previous errors
error: non-canonical implementation of `clone` on a `Copy` type
--> tests/ui/non_canonical_clone_impl.rs:127:37
|
LL | fn clone(&self) -> Self {
| _____________________________________^
LL | |
LL | | todo!()
LL | | }
| |_____________^ help: change this to: `{ *self }`
|
= note: this error originates in the macro `__inline_mac_mod_issue12788` (in Nightly builds, run with -Z macro-backtrace for more info)
error: aborting due to 5 previous errors
tests/ui/non_canonical_partial_ord_impl.fixed
+71
View File
@@ -1,5 +1,9 @@
//@aux-build:proc_macro_derive.rs
#![no_main]
extern crate proc_macros;
use proc_macros::inline_macros;
use std::cmp::Ordering;
// lint
@@ -163,6 +167,73 @@ impl PartialOrd for I {
}
}
#[inline_macros]
mod issue12788 {
use std::cmp::Ordering;
use proc_macros::{external, with_span};
// lint -- not an external macro
inline!(
#[derive(PartialEq, Eq)]
pub struct A;
impl Ord for A {
fn cmp(&self, other: &Self) -> Ordering {
todo!();
}
}
impl PartialOrd for A {
//~^ non_canonical_partial_ord_impl
fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) }
}
);
// do not lint -- should skip external macros
external!(
#[derive(PartialEq, Eq)]
pub struct B;
impl Ord for B {
fn cmp(&self, other: &Self) -> Ordering {
todo!();
}
}
impl PartialOrd for B {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
todo!();
}
}
);
// do not lint -- should skip proc macros
#[derive(proc_macro_derive::NonCanonicalClone)]
pub struct C;
with_span!(
span
#[derive(PartialEq, Eq)]
pub struct D;
impl Ord for D {
fn cmp(&self, other: &Self) -> Ordering {
todo!();
}
}
impl PartialOrd for D {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
todo!();
}
}
);
}
// #13640, do not lint
#[derive(Eq, PartialEq)]
tests/ui/non_canonical_partial_ord_impl.rs
+73
View File
@@ -1,5 +1,9 @@
//@aux-build:proc_macro_derive.rs
#![no_main]
extern crate proc_macros;
use proc_macros::inline_macros;
use std::cmp::Ordering;
// lint
@@ -167,6 +171,75 @@ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
}
}
#[inline_macros]
mod issue12788 {
use std::cmp::Ordering;
use proc_macros::{external, with_span};
// lint -- not an external macro
inline!(
#[derive(PartialEq, Eq)]
pub struct A;
impl Ord for A {
fn cmp(&self, other: &Self) -> Ordering {
todo!();
}
}
impl PartialOrd for A {
//~^ non_canonical_partial_ord_impl
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
todo!();
}
}
);
// do not lint -- should skip external macros
external!(
#[derive(PartialEq, Eq)]
pub struct B;
impl Ord for B {
fn cmp(&self, other: &Self) -> Ordering {
todo!();
}
}
impl PartialOrd for B {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
todo!();
}
}
);
// do not lint -- should skip proc macros
#[derive(proc_macro_derive::NonCanonicalClone)]
pub struct C;
with_span!(
span
#[derive(PartialEq, Eq)]
pub struct D;
impl Ord for D {
fn cmp(&self, other: &Self) -> Ordering {
todo!();
}
}
impl PartialOrd for D {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
todo!();
}
}
);
}
// #13640, do not lint
#[derive(Eq, PartialEq)]
tests/ui/non_canonical_partial_ord_impl.stderr
+20 -5
View File
@@ -1,5 +1,5 @@
error: non-canonical implementation of `partial_cmp` on an `Ord` type
--> tests/ui/non_canonical_partial_ord_impl.rs:16:1
--> tests/ui/non_canonical_partial_ord_impl.rs:20:1
|
LL | / impl PartialOrd for A {
LL | |
@@ -15,7 +15,7 @@ LL | | }
= help: to override `-D warnings` add `#[allow(clippy::non_canonical_partial_ord_impl)]`
error: non-canonical implementation of `partial_cmp` on an `Ord` type
--> tests/ui/non_canonical_partial_ord_impl.rs:51:1
--> tests/ui/non_canonical_partial_ord_impl.rs:55:1
|
LL | / impl PartialOrd for C {
LL | |
@@ -32,7 +32,22 @@ LL + fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp
|
error: non-canonical implementation of `partial_cmp` on an `Ord` type
--> tests/ui/non_canonical_partial_ord_impl.rs:198:1
--> tests/ui/non_canonical_partial_ord_impl.rs:191:9
|
LL | / impl PartialOrd for A {
LL | |
LL | | fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
| | _____________________________________________________________________-
LL | || todo!();
LL | || }
| ||_____________- help: change this to: `{ Some(self.cmp(other)) }`
LL | | }
| |__________^
|
= note: this error originates in the macro `__inline_mac_mod_issue12788` (in Nightly builds, run with -Z macro-backtrace for more info)
error: non-canonical implementation of `partial_cmp` on an `Ord` type
--> tests/ui/non_canonical_partial_ord_impl.rs:271:1
|
LL | / impl PartialOrd for K {
LL | |
@@ -45,7 +60,7 @@ LL | | }
| |__^
error: non-canonical implementation of `partial_cmp` on an `Ord` type
--> tests/ui/non_canonical_partial_ord_impl.rs:216:1
--> tests/ui/non_canonical_partial_ord_impl.rs:289:1
|
LL | / impl PartialOrd for L {
LL | |
@@ -57,5 +72,5 @@ LL | || }
LL | | }
| |__^
error: aborting due to 4 previous errors
error: aborting due to 5 previous errors