From 05ebce8e1a973907db7d3e89e4738a095c250371 Mon Sep 17 00:00:00 2001 From: Ruairidh Williamson Date: Sat, 21 Sep 2024 00:54:20 +0100 Subject: [PATCH 1/2] Add lint unused_trait_names --- CHANGELOG.md | 1 + book/src/lint_configuration.md | 1 + clippy_config/src/conf.rs | 1 + clippy_config/src/msrvs.rs | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/unused_trait_names.rs | 94 ++++++++++++++++++++++++++ 7 files changed, 101 insertions(+) create mode 100644 clippy_lints/src/unused_trait_names.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e0aaa25a..41a86e8ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6063,6 +6063,7 @@ Released 2018-09-13 [`unused_result_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_result_ok [`unused_rounding`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_rounding [`unused_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_self +[`unused_trait_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_trait_names [`unused_unit`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_unit [`unusual_byte_groupings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unusual_byte_groupings [`unwrap_in_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_in_result diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 783487975..91159bc79 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -727,6 +727,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio * [`uninlined_format_args`](https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args) * [`unnecessary_lazy_evaluations`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations) * [`unnested_or_patterns`](https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns) +* [`unused_trait_names`](https://rust-lang.github.io/rust-clippy/master/index.html#unused_trait_names) * [`use_self`](https://rust-lang.github.io/rust-clippy/master/index.html#use_self) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 6c1dd2325..1765cf87d 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -563,6 +563,7 @@ define_Conf! { uninlined_format_args, unnecessary_lazy_evaluations, unnested_or_patterns, + unused_trait_names, use_self, )] msrv: Msrv = Msrv::empty(), diff --git a/clippy_config/src/msrvs.rs b/clippy_config/src/msrvs.rs index a70effd63..a6cba8499 100644 --- a/clippy_config/src/msrvs.rs +++ b/clippy_config/src/msrvs.rs @@ -49,6 +49,7 @@ msrv_aliases! { 1,36,0 { ITERATOR_COPIED } 1,35,0 { OPTION_COPIED, RANGE_CONTAINS } 1,34,0 { TRY_FROM } + 1,33,0 { UNDERSCORE_IMPORTS } 1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES } 1,29,0 { ITER_FLATTEN } 1,28,0 { FROM_BOOL } diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 824f57cec..2b229d2fe 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -748,6 +748,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::unused_result_ok::UNUSED_RESULT_OK_INFO, crate::unused_rounding::UNUSED_ROUNDING_INFO, crate::unused_self::UNUSED_SELF_INFO, + crate::unused_trait_names::UNUSED_TRAIT_NAMES_INFO, crate::unused_unit::UNUSED_UNIT_INFO, crate::unwrap::PANICKING_UNWRAP_INFO, crate::unwrap::UNNECESSARY_UNWRAP_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3604090b6..b21d3f8d0 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -376,6 +376,7 @@ mod unused_peekable; mod unused_result_ok; mod unused_rounding; mod unused_self; +mod unused_trait_names; mod unused_unit; mod unwrap; mod unwrap_in_result; @@ -942,5 +943,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| Box::new(manual_div_ceil::ManualDivCeil::new(conf))); store.register_late_pass(|_| Box::new(manual_is_power_of_two::ManualIsPowerOfTwo)); store.register_late_pass(|_| Box::new(non_zero_suggestions::NonZeroSuggestions)); + store.register_late_pass(move |_| Box::new(unused_trait_names::UnusedTraitNames::new(conf))); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/unused_trait_names.rs b/clippy_lints/src/unused_trait_names.rs new file mode 100644 index 000000000..c1cf58dcf --- /dev/null +++ b/clippy_lints/src/unused_trait_names.rs @@ -0,0 +1,94 @@ +use clippy_config::msrvs::{self, Msrv}; +use clippy_config::Conf; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_from_proc_macro; +use clippy_utils::source::snippet_opt; +use rustc_errors::Applicability; +use rustc_hir::def::{DefKind, Res}; +use rustc_hir::{Item, ItemKind, UseKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext as _}; +use rustc_middle::lint::in_external_macro; +use rustc_middle::ty::Visibility; +use rustc_session::impl_lint_pass; +use rustc_span::symbol::kw; + +declare_clippy_lint! { + /// ### What it does + /// Checks for `use Trait` where the Trait is only used for its methods and not referenced by a path directly. + /// + /// ### Why is this bad? + /// Traits imported that aren't used directly can be imported anonymously with `use Trait as _`. + /// It is more explicit, avoids polluting the current scope with unused names and can be useful to show which imports are required for traits. + /// + /// ### Example + /// ```no_run + /// use std::fmt::Write; + /// + /// fn main() { + /// let mut s = String::new(); + /// let _ = write!(s, "hello, world!"); + /// println!("{s}"); + /// } + /// ``` + /// Use instead: + /// ```no_run + /// use std::fmt::Write as _; + /// + /// fn main() { + /// let mut s = String::new(); + /// let _ = write!(s, "hello, world!"); + /// println!("{s}"); + /// } + /// ``` + #[clippy::version = "1.83.0"] + pub UNUSED_TRAIT_NAMES, + restriction, + "use items that import a trait but only use it anonymously" +} + +pub struct UnusedTraitNames { + msrv: Msrv, +} + +impl UnusedTraitNames { + pub fn new(conf: &'static Conf) -> Self { + Self { + msrv: conf.msrv.clone(), + } + } +} + +impl_lint_pass!(UnusedTraitNames => [UNUSED_TRAIT_NAMES]); + +impl<'tcx> LateLintPass<'tcx> for UnusedTraitNames { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { + if self.msrv.meets(msrvs::UNDERSCORE_IMPORTS) + && !in_external_macro(cx.sess(), item.span) + && let ItemKind::Use(path, UseKind::Single) = item.kind + // Ignore imports that already use Underscore + && item.ident.name != kw::Underscore + // Only check traits + && let Some(Res::Def(DefKind::Trait, _)) = path.res.first() + && cx.tcx.maybe_unused_trait_imports(()).contains(&item.owner_id.def_id) + // Only check this import if it is visible to its module only (no pub, pub(crate), ...) + && let module = cx.tcx.parent_module_from_def_id(item.owner_id.def_id) + && cx.tcx.visibility(item.owner_id.def_id) == Visibility::Restricted(module.to_def_id()) + && let Some(last_segment) = path.segments.last() + && let Some(snip) = snippet_opt(cx, last_segment.ident.span) + && !is_from_proc_macro(cx, &last_segment.ident) + { + let complete_span = last_segment.ident.span.to(item.ident.span); + span_lint_and_sugg( + cx, + UNUSED_TRAIT_NAMES, + complete_span, + "importing trait that is only used anonymously", + "use", + format!("{snip} as _"), + Applicability::MachineApplicable, + ); + } + } + + extract_msrv_attr!(LateContext); +} From 739ef7bf0d5686ef10e7700f34e34995936e00ff Mon Sep 17 00:00:00 2001 From: Ruairidh Williamson Date: Sat, 21 Sep 2024 00:54:56 +0100 Subject: [PATCH 2/2] Add unused_trait_names tests --- clippy_lints/src/attrs/useless_attribute.rs | 1 + tests/ui/unused_trait_names.fixed | 286 ++++++++++++++++++++ tests/ui/unused_trait_names.rs | 286 ++++++++++++++++++++ tests/ui/unused_trait_names.stderr | 73 +++++ 4 files changed, 646 insertions(+) create mode 100644 tests/ui/unused_trait_names.fixed create mode 100644 tests/ui/unused_trait_names.rs create mode 100644 tests/ui/unused_trait_names.stderr diff --git a/clippy_lints/src/attrs/useless_attribute.rs b/clippy_lints/src/attrs/useless_attribute.rs index 67ba605a5..1296b59f2 100644 --- a/clippy_lints/src/attrs/useless_attribute.rs +++ b/clippy_lints/src/attrs/useless_attribute.rs @@ -51,6 +51,7 @@ pub(super) fn check(cx: &LateContext<'_>, item: &Item<'_>, attrs: &[Attribute]) | "module_name_repetitions" | "single_component_path_imports" | "disallowed_types" + | "unused_trait_names" ) }) { return; diff --git a/tests/ui/unused_trait_names.fixed b/tests/ui/unused_trait_names.fixed new file mode 100644 index 000000000..7dfd0db65 --- /dev/null +++ b/tests/ui/unused_trait_names.fixed @@ -0,0 +1,286 @@ +//@aux-build:proc_macros.rs + +#![allow(unused)] +#![warn(clippy::unused_trait_names)] +#![feature(decl_macro)] + +extern crate proc_macros; + +fn main() {} + +fn bad() { + use std::any::Any as _; + + println!("{:?}", "foo".type_id()); +} + +fn good() { + use std::any::Any as _; + + println!("{:?}", "foo".type_id()); +} + +fn used_good() { + use std::any::Any; + + println!("{:?}", Any::type_id("foo")); + println!("{:?}", "foo".type_id()); +} + +fn multi_bad() { + use std::any::{self, Any as _, TypeId}; + + println!("{:?}", "foo".type_id()); +} + +fn multi_good() { + use std::any::{self, Any as _, TypeId}; + + println!("{:?}", "foo".type_id()); +} + +fn renamed_bad() { + use std::any::Any as _; + + println!("{:?}", "foo".type_id()); +} + +fn multi_renamed_bad() { + use std::any::{Any as _, TypeId as MyTypeId}; + + println!("{:?}", "foo".type_id()); +} + +mod pub_good { + pub use std::any::Any; + + fn foo() { + println!("{:?}", "foo".type_id()); + } +} + +mod used_mod_good { + use std::any::Any; + + fn foo() { + println!("{:?}", Any::type_id("foo")); + } +} + +mod mod_import_bad { + fn mod_import_bad() { + use std::any::Any as _; + + println!("{:?}", "foo".type_id()); + } +} + +mod nested_mod_used_good1 { + use std::any::Any; + + mod foo { + fn foo() { + super::Any::type_id("foo"); + } + } +} + +mod nested_mod_used_good2 { + use std::any::Any; + + mod foo { + use super::Any; + + fn foo() { + Any::type_id("foo"); + } + } +} + +mod nested_mod_used_good3 { + use std::any::Any; + + mod foo { + use crate::nested_mod_used_good3::Any; + + fn foo() { + println!("{:?}", Any::type_id("foo")); + } + } +} + +mod nested_mod_used_bad { + use std::any::Any as _; + + fn bar() { + println!("{:?}", "foo".type_id()); + } + + mod foo { + use std::any::Any; + + fn foo() { + println!("{:?}", Any::type_id("foo")); + } + } +} + +// More complex example where `use std::any::Any;` should be anonymised but `use std::any::Any as +// MyAny;` should not as it is used by a sub module. Even though if you removed `use std::any::Any;` +// the code would still compile. +mod nested_mod_used_bad1 { + use std::any::Any as _; + + use std::any::Any as MyAny; + + fn baz() { + println!("{:?}", "baz".type_id()); + } + + mod foo { + use crate::nested_mod_used_bad1::MyAny; + + fn foo() { + println!("{:?}", MyAny::type_id("foo")); + } + } +} + +// Example of nested import with an unused import to try and trick it +mod nested_mod_used_good5 { + use std::any::Any; + + mod foo { + use std::any::Any; + + fn baz() { + println!("{:?}", "baz".type_id()); + } + + mod bar { + use crate::nested_mod_used_good5::foo::Any; + + fn foo() { + println!("{:?}", Any::type_id("foo")); + } + } + } +} + +mod simple_trait { + pub trait MyTrait { + fn do_things(&self); + } + + pub struct MyStruct; + + impl MyTrait for MyStruct { + fn do_things(&self) {} + } +} + +// Underscore imports were stabilized in 1.33 +#[clippy::msrv = "1.32"] +fn msrv_1_32() { + use simple_trait::{MyStruct, MyTrait}; + MyStruct.do_things(); +} + +#[clippy::msrv = "1.33"] +fn msrv_1_33() { + use simple_trait::{MyStruct, MyTrait as _}; + MyStruct.do_things(); +} + +mod lint_inside_macro_expansion_bad { + macro_rules! foo { + () => { + use std::any::Any as _; + fn bar() { + "bar".type_id(); + } + }; + } + + foo!(); +} + +mod macro_and_trait_same_name { + pub macro Foo() {} + pub trait Foo { + fn bar(&self); + } + impl Foo for () { + fn bar(&self) {} + } +} + +fn call_macro_and_trait_good() { + // importing trait and macro but only using macro by path won't allow us to change this to + // `use macro_and_trait_same_name::Foo as _;` + use macro_and_trait_same_name::Foo; + Foo!(); + ().bar(); +} + +proc_macros::external!( + fn ignore_inside_external_proc_macro() { + use std::any::Any; + "foo".type_id(); + } +); + +proc_macros::with_span!( + span + + fn ignore_inside_with_span_proc_macro() { + use std::any::Any; + "foo".type_id(); + } +); + +// This should warn the import is unused but should not trigger unused_trait_names +#[warn(unused)] +mod unused_import { + +} + +#[allow(clippy::unused_trait_names)] +fn allow_lint_fn() { + use std::any::Any; + + "foo".type_id(); +} + +#[allow(clippy::unused_trait_names)] +mod allow_lint_mod { + use std::any::Any; + + fn foo() { + "foo".type_id(); + } +} + +mod allow_lint_import { + #[allow(clippy::unused_trait_names)] + use std::any::Any; + + fn foo() { + "foo".type_id(); + } +} + +// Limitation: Suggests `use std::any::Any as _::{self};` which looks weird +// fn use_trait_self_good() { +// use std::any::Any::{self}; +// "foo".type_id(); +// } + +// Limitation: Suggests `use std::any::{Any as _, Any as _};` +// mod repeated_renamed { +// use std::any::{Any, Any as MyAny}; + +// fn foo() { +// "foo".type_id(); +// } +// } diff --git a/tests/ui/unused_trait_names.rs b/tests/ui/unused_trait_names.rs new file mode 100644 index 000000000..ce44eedbc --- /dev/null +++ b/tests/ui/unused_trait_names.rs @@ -0,0 +1,286 @@ +//@aux-build:proc_macros.rs + +#![allow(unused)] +#![warn(clippy::unused_trait_names)] +#![feature(decl_macro)] + +extern crate proc_macros; + +fn main() {} + +fn bad() { + use std::any::Any; + + println!("{:?}", "foo".type_id()); +} + +fn good() { + use std::any::Any as _; + + println!("{:?}", "foo".type_id()); +} + +fn used_good() { + use std::any::Any; + + println!("{:?}", Any::type_id("foo")); + println!("{:?}", "foo".type_id()); +} + +fn multi_bad() { + use std::any::{self, Any, TypeId}; + + println!("{:?}", "foo".type_id()); +} + +fn multi_good() { + use std::any::{self, Any as _, TypeId}; + + println!("{:?}", "foo".type_id()); +} + +fn renamed_bad() { + use std::any::Any as MyAny; + + println!("{:?}", "foo".type_id()); +} + +fn multi_renamed_bad() { + use std::any::{Any as MyAny, TypeId as MyTypeId}; + + println!("{:?}", "foo".type_id()); +} + +mod pub_good { + pub use std::any::Any; + + fn foo() { + println!("{:?}", "foo".type_id()); + } +} + +mod used_mod_good { + use std::any::Any; + + fn foo() { + println!("{:?}", Any::type_id("foo")); + } +} + +mod mod_import_bad { + fn mod_import_bad() { + use std::any::Any; + + println!("{:?}", "foo".type_id()); + } +} + +mod nested_mod_used_good1 { + use std::any::Any; + + mod foo { + fn foo() { + super::Any::type_id("foo"); + } + } +} + +mod nested_mod_used_good2 { + use std::any::Any; + + mod foo { + use super::Any; + + fn foo() { + Any::type_id("foo"); + } + } +} + +mod nested_mod_used_good3 { + use std::any::Any; + + mod foo { + use crate::nested_mod_used_good3::Any; + + fn foo() { + println!("{:?}", Any::type_id("foo")); + } + } +} + +mod nested_mod_used_bad { + use std::any::Any; + + fn bar() { + println!("{:?}", "foo".type_id()); + } + + mod foo { + use std::any::Any; + + fn foo() { + println!("{:?}", Any::type_id("foo")); + } + } +} + +// More complex example where `use std::any::Any;` should be anonymised but `use std::any::Any as +// MyAny;` should not as it is used by a sub module. Even though if you removed `use std::any::Any;` +// the code would still compile. +mod nested_mod_used_bad1 { + use std::any::Any; + + use std::any::Any as MyAny; + + fn baz() { + println!("{:?}", "baz".type_id()); + } + + mod foo { + use crate::nested_mod_used_bad1::MyAny; + + fn foo() { + println!("{:?}", MyAny::type_id("foo")); + } + } +} + +// Example of nested import with an unused import to try and trick it +mod nested_mod_used_good5 { + use std::any::Any; + + mod foo { + use std::any::Any; + + fn baz() { + println!("{:?}", "baz".type_id()); + } + + mod bar { + use crate::nested_mod_used_good5::foo::Any; + + fn foo() { + println!("{:?}", Any::type_id("foo")); + } + } + } +} + +mod simple_trait { + pub trait MyTrait { + fn do_things(&self); + } + + pub struct MyStruct; + + impl MyTrait for MyStruct { + fn do_things(&self) {} + } +} + +// Underscore imports were stabilized in 1.33 +#[clippy::msrv = "1.32"] +fn msrv_1_32() { + use simple_trait::{MyStruct, MyTrait}; + MyStruct.do_things(); +} + +#[clippy::msrv = "1.33"] +fn msrv_1_33() { + use simple_trait::{MyStruct, MyTrait}; + MyStruct.do_things(); +} + +mod lint_inside_macro_expansion_bad { + macro_rules! foo { + () => { + use std::any::Any; + fn bar() { + "bar".type_id(); + } + }; + } + + foo!(); +} + +mod macro_and_trait_same_name { + pub macro Foo() {} + pub trait Foo { + fn bar(&self); + } + impl Foo for () { + fn bar(&self) {} + } +} + +fn call_macro_and_trait_good() { + // importing trait and macro but only using macro by path won't allow us to change this to + // `use macro_and_trait_same_name::Foo as _;` + use macro_and_trait_same_name::Foo; + Foo!(); + ().bar(); +} + +proc_macros::external!( + fn ignore_inside_external_proc_macro() { + use std::any::Any; + "foo".type_id(); + } +); + +proc_macros::with_span!( + span + + fn ignore_inside_with_span_proc_macro() { + use std::any::Any; + "foo".type_id(); + } +); + +// This should warn the import is unused but should not trigger unused_trait_names +#[warn(unused)] +mod unused_import { + use std::any::Any; +} + +#[allow(clippy::unused_trait_names)] +fn allow_lint_fn() { + use std::any::Any; + + "foo".type_id(); +} + +#[allow(clippy::unused_trait_names)] +mod allow_lint_mod { + use std::any::Any; + + fn foo() { + "foo".type_id(); + } +} + +mod allow_lint_import { + #[allow(clippy::unused_trait_names)] + use std::any::Any; + + fn foo() { + "foo".type_id(); + } +} + +// Limitation: Suggests `use std::any::Any as _::{self};` which looks weird +// fn use_trait_self_good() { +// use std::any::Any::{self}; +// "foo".type_id(); +// } + +// Limitation: Suggests `use std::any::{Any as _, Any as _};` +// mod repeated_renamed { +// use std::any::{Any, Any as MyAny}; + +// fn foo() { +// "foo".type_id(); +// } +// } diff --git a/tests/ui/unused_trait_names.stderr b/tests/ui/unused_trait_names.stderr new file mode 100644 index 000000000..f59d8f58a --- /dev/null +++ b/tests/ui/unused_trait_names.stderr @@ -0,0 +1,73 @@ +error: unused import: `std::any::Any` + --> tests/ui/unused_trait_names.rs:245:9 + | +LL | use std::any::Any; + | ^^^^^^^^^^^^^ + | + = note: `-D unused-imports` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(unused_imports)]` + +error: importing trait that is only used anonymously + --> tests/ui/unused_trait_names.rs:12:19 + | +LL | use std::any::Any; + | ^^^ help: use: `Any as _` + | + = note: `-D clippy::unused-trait-names` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unused_trait_names)]` + +error: importing trait that is only used anonymously + --> tests/ui/unused_trait_names.rs:31:26 + | +LL | use std::any::{self, Any, TypeId}; + | ^^^ help: use: `Any as _` + +error: importing trait that is only used anonymously + --> tests/ui/unused_trait_names.rs:43:19 + | +LL | use std::any::Any as MyAny; + | ^^^^^^^^^^^^ help: use: `Any as _` + +error: importing trait that is only used anonymously + --> tests/ui/unused_trait_names.rs:49:20 + | +LL | use std::any::{Any as MyAny, TypeId as MyTypeId}; + | ^^^^^^^^^^^^ help: use: `Any as _` + +error: importing trait that is only used anonymously + --> tests/ui/unused_trait_names.rs:72:23 + | +LL | use std::any::Any; + | ^^^ help: use: `Any as _` + +error: importing trait that is only used anonymously + --> tests/ui/unused_trait_names.rs:113:19 + | +LL | use std::any::Any; + | ^^^ help: use: `Any as _` + +error: importing trait that is only used anonymously + --> tests/ui/unused_trait_names.rs:132:19 + | +LL | use std::any::Any; + | ^^^ help: use: `Any as _` + +error: importing trait that is only used anonymously + --> tests/ui/unused_trait_names.rs:191:34 + | +LL | use simple_trait::{MyStruct, MyTrait}; + | ^^^^^^^ help: use: `MyTrait as _` + +error: importing trait that is only used anonymously + --> tests/ui/unused_trait_names.rs:198:27 + | +LL | use std::any::Any; + | ^^^ help: use: `Any as _` +... +LL | foo!(); + | ------ in this macro invocation + | + = note: this error originates in the macro `foo` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 10 previous errors +