diff --git a/CHANGELOG.md b/CHANGELOG.md index 37d46d349667..7b98ac5fa416 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6523,6 +6523,7 @@ Released 2018-09-13 [`mem_replace_option_with_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_some [`mem_replace_with_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_default [`mem_replace_with_uninit`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_uninit +[`method_without_self_relation`]: https://rust-lang.github.io/rust-clippy/master/index.html#method_without_self_relation [`min_ident_chars`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_ident_chars [`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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 375d179681da..703d82645a1d 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -425,6 +425,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::methods::MAP_IDENTITY_INFO, crate::methods::MAP_UNWRAP_OR_INFO, crate::methods::MAP_WITH_UNUSED_ARGUMENT_OVER_RANGES_INFO, + crate::methods::METHOD_WITHOUT_SELF_RELATION_INFO, crate::methods::MUT_MUTEX_LOCK_INFO, crate::methods::NAIVE_BYTECOUNT_INFO, crate::methods::NEEDLESS_AS_BYTES_INFO, diff --git a/clippy_lints/src/methods/method_without_self_relation.rs b/clippy_lints/src/methods/method_without_self_relation.rs new file mode 100644 index 000000000000..68e7103b3bf6 --- /dev/null +++ b/clippy_lints/src/methods/method_without_self_relation.rs @@ -0,0 +1,126 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use rustc_hir::{FnSig, ImplItem, ImplItemKind}; +use rustc_lint::LateContext; +use rustc_middle::ty::{self, Ty}; +use rustc_span::Span; + +use super::METHOD_WITHOUT_SELF_RELATION; + +/// Check if a type contains a reference to `Self` anywhere in its structure. +/// This includes direct references and generic parameters. +fn contains_self<'tcx>(ty: Ty<'tcx>, self_ty: Ty<'tcx>) -> bool { + // Direct comparison with Self type + if ty == self_ty { + return true; + } + + match ty.kind() { + // Check if this is a reference or raw pointer to Self + ty::Ref(_, inner_ty, _) | ty::RawPtr(inner_ty, _) => contains_self(*inner_ty, self_ty), + + // Check generic types like Option, Vec, Result, etc. + ty::Adt(_, args) => args.types().any(|arg_ty| contains_self(arg_ty, self_ty)), + + // Check tuples like (Self, i32) or (String, Self) + ty::Tuple(types) => types.iter().any(|ty| contains_self(ty, self_ty)), + + // Check array types like [Self; 10] + ty::Array(elem_ty, _) | ty::Slice(elem_ty) => contains_self(*elem_ty, self_ty), + + // Check function pointer types + ty::FnPtr(sig, _) => { + let sig = sig.skip_binder(); + sig.inputs().iter().any(|&ty| contains_self(ty, self_ty)) || contains_self(sig.output(), self_ty) + }, + + // Check closures (uncommon but possible) + ty::Closure(_, args) => { + args.as_closure() + .sig() + .inputs() + .skip_binder() + .iter() + .any(|&ty| contains_self(ty, self_ty)) + || contains_self(args.as_closure().sig().output().skip_binder(), self_ty) + }, + + // Check opaque types (impl Trait, async fn return types) + ty::Alias(ty::AliasTyKind::Opaque, alias_ty) => { + // Check the bounds of the opaque type + alias_ty.args.types().any(|arg_ty| contains_self(arg_ty, self_ty)) + }, + + // Check trait objects (dyn Trait) + ty::Dynamic(predicates, _) => { + use rustc_middle::ty::ExistentialPredicate; + // Check if any of the trait bounds reference Self + predicates.iter().any(|predicate| match predicate.skip_binder() { + ExistentialPredicate::Trait(trait_ref) => { + trait_ref.args.types().any(|arg_ty| contains_self(arg_ty, self_ty)) + }, + ExistentialPredicate::Projection(projection) => { + projection.args.types().any(|arg_ty| contains_self(arg_ty, self_ty)) + || contains_self(projection.term.as_type().unwrap(), self_ty) + }, + ExistentialPredicate::AutoTrait(_) => false, + }) + }, + + _ => false, + } +} + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + impl_item: &'tcx ImplItem<'_>, + self_ty: Ty<'tcx>, + implements_trait: bool, +) { + if let ImplItemKind::Fn(ref sig, _) = impl_item.kind { + // Don't lint trait implementations - these methods are defined by the trait + if implements_trait { + return; + } + + // Get the method signature from the type system + let method_sig = cx.tcx.fn_sig(impl_item.owner_id).instantiate_identity(); + let method_sig = method_sig.skip_binder(); + + // Check if there's a self parameter (self, &self, &mut self, self: Arc, etc.) + if has_self_parameter(sig) { + return; + } + + // Check all input parameters for Self references + for ¶m_ty in method_sig.inputs() { + if contains_self(param_ty, self_ty) { + return; + } + } + + // Check return type for Self references + let return_ty = method_sig.output(); + if contains_self(return_ty, self_ty) { + return; + } + + // If we reach here, the method has no relationship to Self + emit_lint(cx, impl_item.span, impl_item.ident.name.as_str()); + } +} + +/// Check if the function signature has an explicit self parameter +fn has_self_parameter(sig: &FnSig<'_>) -> bool { + sig.decl.implicit_self.has_implicit_self() +} + +fn emit_lint(cx: &LateContext<'_>, span: Span, method_name: &str) { + span_lint_and_help( + cx, + METHOD_WITHOUT_SELF_RELATION, + span, + format!("method `{method_name}` has no relationship to `Self`"), + None, + "consider making this a standalone function instead of a method", + ); +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c9066be51c44..f4aaa22f9f28 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -74,6 +74,7 @@ mod map_flatten; mod map_identity; mod map_unwrap_or; mod map_with_unused_argument_over_ranges; +mod method_without_self_relation; mod mut_mutex_lock; mod needless_as_bytes; mod needless_character_iteration; @@ -1162,6 +1163,74 @@ declare_clippy_lint! { "not returning type containing `Self` in a `new` method" } +declare_clippy_lint! { + /// ### What it does + /// Checks for methods in impl blocks that have no relationship to `Self`. + /// + /// ### Why is this bad? + /// Methods that don't use `Self` in their signature (parameters or return type) + /// are not actually methods but rather associated functions. They would be + /// better expressed as standalone functions, making the code more modular + /// and easier to discover. + /// + /// ### Known issues + /// This lint is intentionally set to `restriction` category because there are + /// valid reasons to keep functions within an impl block: + /// - Helper functions that logically belong to the type + /// - Functions meant to be used through the type's namespace + /// - Private implementation details that should be grouped with the type + /// + /// ### Example + /// ```no_run + /// struct Calculator; + /// + /// impl Calculator { + /// // Bad: No relationship to Self + /// fn add(a: i32, b: i32) -> i32 { + /// a + b + /// } + /// } + /// ``` + /// Use instead: + /// ```no_run + /// struct Calculator; + /// + /// // Good: Standalone function + /// fn add(a: i32, b: i32) -> i32 { + /// a + b + /// } + /// ``` + /// + /// Methods with Self references are fine: + /// ```no_run + /// #[derive(Default)] + /// struct Calculator { + /// precision: u32, + /// } + /// + /// impl Calculator { + /// // Good: Uses &self + /// fn add(&self, a: i32, b: i32) -> i32 { + /// a + b + /// } + /// + /// // Good: Returns Self + /// fn new(precision: u32) -> Self { + /// Self { precision } + /// } + /// + /// // Good: Takes Self in generic parameter + /// fn from_option(opt: Option) -> Self { + /// opt.unwrap_or_default() + /// } + /// } + /// ``` + #[clippy::version = "1.92.0"] + pub METHOD_WITHOUT_SELF_RELATION, + restriction, + "methods in impl blocks that have no relationship to `Self`" +} + declare_clippy_lint! { /// ### What it does /// Checks for calling `.step_by(0)` on iterators which panics. @@ -4720,6 +4789,7 @@ impl_lint_pass!(Methods => [ FLAT_MAP_OPTION, INEFFICIENT_TO_STRING, NEW_RET_NO_SELF, + METHOD_WITHOUT_SELF_RELATION, SINGLE_CHAR_ADD_STR, SEARCH_IS_SOME, FILTER_NEXT, @@ -4929,6 +4999,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods { } new_ret_no_self::check_impl_item(cx, impl_item, self_ty, implements_trait); + method_without_self_relation::check(cx, impl_item, self_ty, implements_trait); } } diff --git a/tests/ui/explicit_write_in_test.stderr b/tests/ui/explicit_write_in_test.stderr deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/tests/ui/method_without_self_relation.rs b/tests/ui/method_without_self_relation.rs new file mode 100644 index 000000000000..0cd135c80e18 --- /dev/null +++ b/tests/ui/method_without_self_relation.rs @@ -0,0 +1,382 @@ +#![warn(clippy::method_without_self_relation)] +#![allow(dead_code, unused_variables)] + +use std::pin::Pin; +use std::sync::Arc; + +#[derive(Clone)] +struct Calculator { + precision: u32, +} + +impl Calculator { + // Should trigger lint - no Self relation at all + fn add(a: i32, b: i32) -> i32 { + //~^ ERROR: method `add` has no relationship to `Self` + a + b + } + + // Should trigger lint - no Self relation + fn multiply(x: i32, y: i32) -> i32 { + //~^ ERROR: method `multiply` has no relationship to `Self` + x * y + } + + // Should NOT trigger - has &self + fn get_precision(&self) -> u32 { + self.precision + } + + // Should NOT trigger - has &mut self + fn set_precision(&mut self, precision: u32) { + self.precision = precision; + } + + // Should NOT trigger - has self + fn consume(self) -> u32 { + self.precision + } + + // Should NOT trigger - returns Self + fn new(precision: u32) -> Self { + Self { precision } + } + + // Should NOT trigger - takes Self as parameter + fn from_self(other: Self) -> u32 { + other.precision + } + + // Should NOT trigger - uses Self in Option + fn from_option(opt: Option) -> u32 { + opt.map(|s| s.precision).unwrap_or(0) + } + + // Should NOT trigger - returns Option + fn maybe_new(precision: u32) -> Option { + if precision > 0 { Some(Self { precision }) } else { None } + } + + // Should NOT trigger - uses Result + fn try_new(precision: u32) -> Result { + if precision > 0 { + Ok(Self { precision }) + } else { + Err("Invalid precision".to_string()) + } + } + + // Should NOT trigger - returns Result with Self in error + fn result_with_self_error(precision: u32) -> Result { + if precision == 0 { + Err(Self { precision: 1 }) + } else { + Ok(precision as i32) + } + } + + // Should NOT trigger - Self in Vec + fn many(count: usize, precision: u32) -> Vec { + vec![Self { precision }; count] + } + + // Should NOT trigger - &Self parameter + fn compare(a: &Self, b: &Self) -> bool { + a.precision == b.precision + } + + // Should NOT trigger - exotic self receiver + fn from_arc(self: Arc) -> u32 { + self.precision + } + + // Should NOT trigger - Pin<&mut Self> + fn pinned_method(self: Pin<&mut Self>) { + // Do something + } + + // Should NOT trigger - Self in tuple + fn tuple_with_self(x: i32) -> (i32, Self) { + (x, Self { precision: x as u32 }) + } + + // Should NOT trigger - Self in nested generic + fn nested_generic() -> Result, String> { + Ok(Some(Self { precision: 1 })) + } + + // Should trigger - only uses primitive types + fn helper(x: i32, y: i32, z: i32) -> i32 { + //~^ ERROR: method `helper` has no relationship to `Self` + x + y + z + } + + // Should trigger - String operations, no Self + fn format_string(s: &str) -> String { + //~^ ERROR: method `format_string` has no relationship to `Self` + format!("Formatted: {}", s) + } +} + +// Test with trait implementations +trait Display { + fn display(&self); +} + +impl Display for Calculator { + // Should NOT trigger - trait implementation + fn display(&self) { + println!("Precision: {}", self.precision); + } +} + +// Test trait implementation without self parameter +trait UtilityTrait { + fn process_data(x: i32, y: i32) -> i32; +} + +struct UtilityImpl; + +impl UtilityTrait for UtilityImpl { + // Should NOT trigger - this is a trait implementation + // Even though it has no Self relation, it's required by the trait + fn process_data(x: i32, y: i32) -> i32 { + x + y + } +} + +// Test with generic type +struct Container { + value: T, +} + +impl Container { + // Should trigger - no relationship to Self or T + fn static_helper(x: i32, y: i32) -> i32 { + //~^ ERROR: method `static_helper` has no relationship to `Self` + x * y + } + + // Should NOT trigger - returns Self + fn new(value: T) -> Self { + Self { value } + } + + // Should NOT trigger - takes Self + fn unwrap(self) -> T { + self.value + } + + // Should NOT trigger - takes &Self + fn get(&self) -> &T { + &self.value + } +} + +// Test with zero-sized type +struct Utility; + +impl Utility { + // Should trigger - no Self relation + fn process(data: &str) -> String { + //~^ ERROR: method `process` has no relationship to `Self` + data.to_uppercase() + } + + // Should NOT trigger - returns Self + fn instance() -> Self { + Self + } +} + +// Test with tuple struct +struct Point(i32, i32); + +impl Point { + // Should trigger - no Self relation + fn distance_formula(x1: i32, y1: i32, x2: i32, y2: i32) -> f64 { + //~^ ERROR: method `distance_formula` has no relationship to `Self` + (((x2 - x1).pow(2) + (y2 - y1).pow(2)) as f64).sqrt() + } + + // Should NOT trigger - returns Self + fn new(x: i32, y: i32) -> Self { + Self(x, y) + } + + // Should NOT trigger - takes &self + fn x(&self) -> i32 { + self.0 + } +} + +// Test with array of Self +struct Matrix; + +impl Matrix { + // Should NOT trigger - returns array containing Self + fn identity_matrices(count: usize) -> [Self; 3] { + [Self, Self, Self] + } + + // Should trigger - returns array of primitives + fn numbers() -> [i32; 3] { + //~^ ERROR: method `numbers` has no relationship to `Self` + [1, 2, 3] + } +} + +// Test with function pointers +struct Callbacks; + +impl Callbacks { + // Should NOT trigger - function pointer references Self + fn get_constructor() -> fn(i32) -> Self { + |_| Self + } + + // Should trigger - function pointer with no Self + fn get_adder() -> fn(i32, i32) -> i32 { + //~^ ERROR: method `get_adder` has no relationship to `Self` + |a, b| a + b + } +} + +// Test async functions +struct AsyncStruct; + +impl AsyncStruct { + // Should trigger - async function with no Self relation + async fn async_no_self(x: i32) -> i32 { + //~^ ERROR: method `async_no_self` has no relationship to `Self` + x + 1 + } + + // Should NOT trigger - async function taking Self parameter + async fn async_consume(s: Self) -> i32 { + 42 + } + + // Should NOT trigger - async with self receiver + async fn async_method(&self) -> i32 { + 42 + } +} + +// Test impl Trait return types +struct ImplTraitStruct; + +impl ImplTraitStruct { + // Should trigger - impl Trait with no Self + fn iter_no_self() -> impl Iterator { + //~^ ERROR: method `iter_no_self` has no relationship to `Self` + std::iter::once(42) + } + + // Should NOT trigger - impl Trait taking Self in parameter + fn from_iter(items: impl Iterator) -> Vec { + items.collect() + } +} + +// Test raw struct name (should be treated as Self) +struct RawStructName { + value: i32, +} + +impl RawStructName { + // Should NOT trigger - returns the raw struct type name + fn create() -> RawStructName { + RawStructName { value: 42 } + } + + // Should NOT trigger - takes raw struct type name + fn process(r: RawStructName) -> i32 { + r.value + } + + // Should NOT trigger - raw struct name in generic + fn wrap() -> Option { + Some(RawStructName { value: 42 }) + } +} + +// Test Box, Rc, and other smart pointers +use std::marker::PhantomData; +use std::rc::Rc; + +struct SmartPointers; + +impl SmartPointers { + // Should NOT trigger - Box + fn boxed() -> Box { + Box::new(Self) + } + + // Should NOT trigger - Rc + fn rced() -> Rc { + Rc::new(Self) + } + + // Should NOT trigger - takes Box + fn unbox(b: Box) { + drop(b); + } + + // Should trigger - Box with no Self + fn boxed_int() -> Box { + //~^ ERROR: method `boxed_int` has no relationship to `Self` + Box::new(42) + } +} + +// Test PhantomData +struct PhantomStruct(PhantomData); + +impl PhantomStruct { + // Should NOT trigger - PhantomData + fn with_phantom() -> PhantomData { + PhantomData + } + + // Should trigger - PhantomData with different type + fn phantom_other() -> PhantomData { + //~^ ERROR: method `phantom_other` has no relationship to `Self` + PhantomData + } +} + +// Test const generics +#[derive(Copy, Clone)] +struct ConstGenericStruct; + +impl ConstGenericStruct { + // Should NOT trigger - array with Self + fn array_of_self() -> [Self; N] { + [Self; N] + } + + // Should trigger - array with no Self + fn array_of_int() -> [i32; N] { + //~^ ERROR: method `array_of_int` has no relationship to `Self` + [0; N] + } +} + +// Test trait objects +struct TraitObjectStruct; + +impl TraitObjectStruct { + // Should NOT trigger - trait object taking Self + fn callback_with_self() -> Box i32> { + Box::new(|_| 42) + } + + // Should trigger - trait object with no Self + fn callback_no_self() -> Box i32> { + //~^ ERROR: method `callback_no_self` has no relationship to `Self` + Box::new(|x| x + 1) + } +} + +fn main() {} diff --git a/tests/ui/method_without_self_relation.stderr b/tests/ui/method_without_self_relation.stderr new file mode 100644 index 000000000000..733ed7947ef2 --- /dev/null +++ b/tests/ui/method_without_self_relation.stderr @@ -0,0 +1,169 @@ +error: method `add` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:14:5 + | +LL | / fn add(a: i32, b: i32) -> i32 { +LL | | +LL | | a + b +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + = note: `-D clippy::method-without-self-relation` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::method_without_self_relation)]` + +error: method `multiply` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:20:5 + | +LL | / fn multiply(x: i32, y: i32) -> i32 { +LL | | +LL | | x * y +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `helper` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:109:5 + | +LL | / fn helper(x: i32, y: i32, z: i32) -> i32 { +LL | | +LL | | x + y + z +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `format_string` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:115:5 + | +LL | / fn format_string(s: &str) -> String { +LL | | +LL | | format!("Formatted: {}", s) +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `static_helper` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:155:5 + | +LL | / fn static_helper(x: i32, y: i32) -> i32 { +LL | | +LL | | x * y +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `process` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:181:5 + | +LL | / fn process(data: &str) -> String { +LL | | +LL | | data.to_uppercase() +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `distance_formula` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:197:5 + | +LL | / fn distance_formula(x1: i32, y1: i32, x2: i32, y2: i32) -> f64 { +LL | | +LL | | (((x2 - x1).pow(2) + (y2 - y1).pow(2)) as f64).sqrt() +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `numbers` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:223:5 + | +LL | / fn numbers() -> [i32; 3] { +LL | | +LL | | [1, 2, 3] +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `get_adder` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:239:5 + | +LL | / fn get_adder() -> fn(i32, i32) -> i32 { +LL | | +LL | | |a, b| a + b +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `async_no_self` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:250:5 + | +LL | / async fn async_no_self(x: i32) -> i32 { +LL | | +LL | | x + 1 +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `iter_no_self` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:271:5 + | +LL | / fn iter_no_self() -> impl Iterator { +LL | | +LL | | std::iter::once(42) +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `boxed_int` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:327:5 + | +LL | / fn boxed_int() -> Box { +LL | | +LL | | Box::new(42) +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `phantom_other` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:343:5 + | +LL | / fn phantom_other() -> PhantomData { +LL | | +LL | | PhantomData +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `array_of_int` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:360:5 + | +LL | / fn array_of_int() -> [i32; N] { +LL | | +LL | | [0; N] +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: method `callback_no_self` has no relationship to `Self` + --> tests/ui/method_without_self_relation.rs:376:5 + | +LL | / fn callback_no_self() -> Box i32> { +LL | | +LL | | Box::new(|x| x + 1) +LL | | } + | |_____^ + | + = help: consider making this a standalone function instead of a method + +error: aborting due to 15 previous errors +