From 58cd01c2fcda07f97efcd908b50e5256cf084593 Mon Sep 17 00:00:00 2001 From: Ariel Uy Date: Fri, 13 May 2022 23:48:52 -0700 Subject: [PATCH] Add new lint mismatching_type_param_order Add new lint for checking if type parameters are consistent between type definitions and impl blocks. --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_pedantic.rs | 1 + clippy_lints/src/lib.rs | 2 + .../src/mismatching_type_param_order.rs | 116 ++++++++++++++++++ tests/ui/mismatching_type_param_order.rs | 60 +++++++++ tests/ui/mismatching_type_param_order.stderr | 83 +++++++++++++ 7 files changed, 264 insertions(+) create mode 100644 clippy_lints/src/mismatching_type_param_order.rs create mode 100644 tests/ui/mismatching_type_param_order.rs create mode 100644 tests/ui/mismatching_type_param_order.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c7ebfdf035..0a44ffdd3d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3560,6 +3560,7 @@ Released 2018-09-13 [`min_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_max [`misaligned_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#misaligned_transmute [`mismatched_target_os`]: https://rust-lang.github.io/rust-clippy/master/index.html#mismatched_target_os +[`mismatching_type_param_order`]: https://rust-lang.github.io/rust-clippy/master/index.html#mismatching_type_param_order [`misrefactored_assign_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#misrefactored_assign_op [`missing_const_for_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn [`missing_docs_in_private_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_docs_in_private_items diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index ea8040a319b..29bfc660d29 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -380,6 +380,7 @@ store.register_lints(&[ misc_early::UNNEEDED_WILDCARD_PATTERN, misc_early::UNSEPARATED_LITERAL_SUFFIX, misc_early::ZERO_PREFIXED_LITERAL, + mismatching_type_param_order::MISMATCHING_TYPE_PARAM_ORDER, missing_const_for_fn::MISSING_CONST_FOR_FN, missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS, missing_enforced_import_rename::MISSING_ENFORCED_IMPORT_RENAMES, diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs index 5684972b8be..2e47a287d5c 100644 --- a/clippy_lints/src/lib.register_pedantic.rs +++ b/clippy_lints/src/lib.register_pedantic.rs @@ -67,6 +67,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![ LintId::of(methods::UNNECESSARY_JOIN), LintId::of(misc::FLOAT_CMP), LintId::of(misc::USED_UNDERSCORE_BINDING), + LintId::of(mismatching_type_param_order::MISMATCHING_TYPE_PARAM_ORDER), LintId::of(mut_mut::MUT_MUT), LintId::of(needless_bitwise_bool::NEEDLESS_BITWISE_BOOL), LintId::of(needless_continue::NEEDLESS_CONTINUE), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 89b65ce5469..6c3d7594f5c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -298,6 +298,7 @@ mod methods; mod minmax; mod misc; mod misc_early; +mod mismatching_type_param_order; mod missing_const_for_fn; mod missing_doc; mod missing_enforced_import_rename; @@ -917,6 +918,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_early_pass(|| Box::new(unused_rounding::UnusedRounding)); store.register_early_pass(move || Box::new(almost_complete_letter_range::AlmostCompleteLetterRange::new(msrv))); store.register_late_pass(|| Box::new(swap_ptr_to_ref::SwapPtrToRef)); + store.register_late_pass(|| Box::new(mismatching_type_param_order::TypeParamMismatch)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/mismatching_type_param_order.rs b/clippy_lints/src/mismatching_type_param_order.rs new file mode 100644 index 00000000000..d466d54a6ba --- /dev/null +++ b/clippy_lints/src/mismatching_type_param_order.rs @@ -0,0 +1,116 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use rustc_data_structures::fx::FxHashMap; +use rustc_hir::def::{DefKind, Res}; +use rustc_hir::{GenericArg, Item, ItemKind, QPath, Ty, TyKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::GenericParamDefKind; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for type parameters which are positioned inconsistently between + /// a type definition and impl block. Specifically, a paramater in an impl + /// block which has the same name as a parameter in the type def, but is in + /// a different place. + /// + /// ### Why is this bad? + /// Type parameters are determined by their position rather than name. + /// Naming type parameters inconsistently may cause you to refer to the + /// wrong type parameter. + /// + /// ### Example + /// ```rust + /// struct Foo { + /// x: A, + /// y: B, + /// } + /// // inside the impl, B refers to Foo::A + /// impl Foo {} + /// ``` + /// Use instead: + /// ```rust + /// struct Foo { + /// x: A, + /// y: B, + /// } + /// impl Foo {} + /// ``` + #[clippy::version = "1.62.0"] + pub MISMATCHING_TYPE_PARAM_ORDER, + pedantic, + "type parameter positioned inconsistently between type def and impl block" +} +declare_lint_pass!(TypeParamMismatch => [MISMATCHING_TYPE_PARAM_ORDER]); + +impl<'tcx> LateLintPass<'tcx> for TypeParamMismatch { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { + if_chain! { + if !item.span.from_expansion(); + if let ItemKind::Impl(imp) = &item.kind; + if let TyKind::Path(QPath::Resolved(_, path)) = &imp.self_ty.kind; + if let Some(segment) = path.segments.iter().next(); + if let Some(generic_args) = segment.args; + if !generic_args.args.is_empty(); + then { + // get the name and span of the generic parameters in the Impl + let impl_params = generic_args.args.iter() + .filter_map(|p| + match p { + GenericArg::Type(Ty {kind: TyKind::Path(QPath::Resolved(_, path)), ..}) => + Some((path.segments[0].ident.to_string(), path.span)), + _ => None, + } + ); + + // find the type that the Impl is for + // only lint on struct/enum/union for now + let defid = match path.res { + Res::Def(DefKind::Struct | DefKind::Enum | DefKind::Union, defid) => defid, + _ => return, + }; + + // get the names of the generic parameters in the type + let type_params = &cx.tcx.generics_of(defid).params; + let type_param_names: Vec<_> = type_params.iter() + .filter_map(|p| + match p.kind { + GenericParamDefKind::Type {..} => Some(p.name.to_string()), + _ => None, + } + ).collect(); + // hashmap of name -> index for mismatch_param_name + let type_param_names_hashmap: FxHashMap<&String, usize> = + type_param_names.iter().enumerate().map(|(i, param)| (param, i)).collect(); + + let type_name = segment.ident; + for (i, (impl_param_name, impl_param_span)) in impl_params.enumerate() { + if mismatch_param_name(i, &impl_param_name, &type_param_names_hashmap) { + let msg = format!("`{}` has a similarly named generic type parameter `{}` in its declaration, but in a different order", + type_name, impl_param_name); + let help = format!("try `{}`, or a name that does not conflict with `{}`'s generic params", + type_param_names[i], type_name); + span_lint_and_help( + cx, + MISMATCHING_TYPE_PARAM_ORDER, + impl_param_span, + &msg, + None, + &help + ); + } + } + } + } + } +} + +// Checks if impl_param_name is the same as one of type_param_names, +// and is in a different position +fn mismatch_param_name(i: usize, impl_param_name: &String, type_param_names: &FxHashMap<&String, usize>) -> bool { + if let Some(j) = type_param_names.get(impl_param_name) { + if i != *j { + return true; + } + } + false +} diff --git a/tests/ui/mismatching_type_param_order.rs b/tests/ui/mismatching_type_param_order.rs new file mode 100644 index 00000000000..8f286c9304c --- /dev/null +++ b/tests/ui/mismatching_type_param_order.rs @@ -0,0 +1,60 @@ +#![warn(clippy::mismatching_type_param_order)] +#![allow(clippy::blacklisted_name)] + +fn main() { + struct Foo { + x: A, + y: B, + } + + // lint on both params + impl Foo {} + + // lint on the 2nd param + impl Foo {} + + // should not lint + impl Foo {} + + struct FooLifetime<'l, 'm, A, B> { + x: &'l A, + y: &'m B, + } + + // should not lint on lifetimes + impl<'m, 'l, B, A> FooLifetime<'m, 'l, B, A> {} + + struct Bar { + x: i32, + } + + // should not lint + impl Bar {} + + // also works for enums + enum FooEnum { + X(A), + Y(B), + Z(C), + } + + impl FooEnum {} + + // also works for unions + union FooUnion + where + B: Copy, + { + x: A, + y: B, + } + + impl FooUnion where A: Copy {} + + impl FooUnion + where + A: Copy, + B: Copy, + { + } +} diff --git a/tests/ui/mismatching_type_param_order.stderr b/tests/ui/mismatching_type_param_order.stderr new file mode 100644 index 00000000000..cb720256c50 --- /dev/null +++ b/tests/ui/mismatching_type_param_order.stderr @@ -0,0 +1,83 @@ +error: `Foo` has a similarly named generic type parameter `B` in its declaration, but in a different order + --> $DIR/mismatching_type_param_order.rs:11:20 + | +LL | impl Foo {} + | ^ + | + = note: `-D clippy::mismatching-type-param-order` implied by `-D warnings` + = help: try `A`, or a name that does not conflict with `Foo`'s generic params + +error: `Foo` has a similarly named generic type parameter `A` in its declaration, but in a different order + --> $DIR/mismatching_type_param_order.rs:11:23 + | +LL | impl Foo {} + | ^ + | + = help: try `B`, or a name that does not conflict with `Foo`'s generic params + +error: `Foo` has a similarly named generic type parameter `A` in its declaration, but in a different order + --> $DIR/mismatching_type_param_order.rs:14:23 + | +LL | impl Foo {} + | ^ + | + = help: try `B`, or a name that does not conflict with `Foo`'s generic params + +error: `FooLifetime` has a similarly named generic type parameter `B` in its declaration, but in a different order + --> $DIR/mismatching_type_param_order.rs:25:44 + | +LL | impl<'m, 'l, B, A> FooLifetime<'m, 'l, B, A> {} + | ^ + | + = help: try `A`, or a name that does not conflict with `FooLifetime`'s generic params + +error: `FooLifetime` has a similarly named generic type parameter `A` in its declaration, but in a different order + --> $DIR/mismatching_type_param_order.rs:25:47 + | +LL | impl<'m, 'l, B, A> FooLifetime<'m, 'l, B, A> {} + | ^ + | + = help: try `B`, or a name that does not conflict with `FooLifetime`'s generic params + +error: `FooEnum` has a similarly named generic type parameter `C` in its declaration, but in a different order + --> $DIR/mismatching_type_param_order.rs:41:27 + | +LL | impl FooEnum {} + | ^ + | + = help: try `A`, or a name that does not conflict with `FooEnum`'s generic params + +error: `FooEnum` has a similarly named generic type parameter `A` in its declaration, but in a different order + --> $DIR/mismatching_type_param_order.rs:41:30 + | +LL | impl FooEnum {} + | ^ + | + = help: try `B`, or a name that does not conflict with `FooEnum`'s generic params + +error: `FooEnum` has a similarly named generic type parameter `B` in its declaration, but in a different order + --> $DIR/mismatching_type_param_order.rs:41:33 + | +LL | impl FooEnum {} + | ^ + | + = help: try `C`, or a name that does not conflict with `FooEnum`'s generic params + +error: `FooUnion` has a similarly named generic type parameter `B` in its declaration, but in a different order + --> $DIR/mismatching_type_param_order.rs:52:31 + | +LL | impl FooUnion where A: Copy {} + | ^ + | + = help: try `A`, or a name that does not conflict with `FooUnion`'s generic params + +error: `FooUnion` has a similarly named generic type parameter `A` in its declaration, but in a different order + --> $DIR/mismatching_type_param_order.rs:52:34 + | +LL | impl FooUnion where A: Copy {} + | ^ + | + = help: try `B`, or a name that does not conflict with `FooUnion`'s generic params + +error: aborting due to 10 previous errors +