diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f1f73c1fd2..5b6b12c623a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4188,6 +4188,7 @@ Released 2018-09-13 [`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 +[`misnamed_getters`]: https://rust-lang.github.io/rust-clippy/master/index.html#misnamed_getters [`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/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 0d3fc43a644..eb3210946f1 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -177,6 +177,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::from_raw_with_void_ptr::FROM_RAW_WITH_VOID_PTR_INFO, crate::from_str_radix_10::FROM_STR_RADIX_10_INFO, crate::functions::DOUBLE_MUST_USE_INFO, + crate::functions::MISNAMED_GETTERS_INFO, crate::functions::MUST_USE_CANDIDATE_INFO, crate::functions::MUST_USE_UNIT_INFO, crate::functions::NOT_UNSAFE_PTR_ARG_DEREF_INFO, diff --git a/clippy_lints/src/functions/misnamed_getters.rs b/clippy_lints/src/functions/misnamed_getters.rs new file mode 100644 index 00000000000..27acad45ccf --- /dev/null +++ b/clippy_lints/src/functions/misnamed_getters.rs @@ -0,0 +1,125 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::snippet; +use rustc_errors::Applicability; +use rustc_hir::{intravisit::FnKind, Body, ExprKind, FnDecl, HirId, ImplicitSelfKind, Unsafety}; +use rustc_lint::LateContext; +use rustc_middle::ty; +use rustc_span::Span; + +use std::iter; + +use super::MISNAMED_GETTERS; + +pub fn check_fn( + cx: &LateContext<'_>, + kind: FnKind<'_>, + decl: &FnDecl<'_>, + body: &Body<'_>, + span: Span, + _hir_id: HirId, +) { + let FnKind::Method(ref ident, sig) = kind else { + return; + }; + + // Takes only &(mut) self + if decl.inputs.len() != 1 { + return; + } + + let name = ident.name.as_str(); + + let name = match decl.implicit_self { + ImplicitSelfKind::MutRef => { + let Some(name) = name.strip_suffix("_mut") else { + return; + }; + name + }, + ImplicitSelfKind::Imm | ImplicitSelfKind::Mut | ImplicitSelfKind::ImmRef => name, + ImplicitSelfKind::None => return, + }; + + let name = if sig.header.unsafety == Unsafety::Unsafe { + name.strip_suffix("_unchecked").unwrap_or(name) + } else { + name + }; + + // Body must be &(mut) .name + // self_data is not neccessarilly self, to also lint sub-getters, etc… + + let block_expr = if_chain! { + if let ExprKind::Block(block,_) = body.value.kind; + if block.stmts.is_empty(); + if let Some(block_expr) = block.expr; + then { + block_expr + } else { + return; + } + }; + let expr_span = block_expr.span; + + // Accept &, &mut and + let expr = if let ExprKind::AddrOf(_, _, tmp) = block_expr.kind { + tmp + } else { + block_expr + }; + let (self_data, used_ident) = if_chain! { + if let ExprKind::Field(self_data, ident) = expr.kind; + if ident.name.as_str() != name; + then { + (self_data, ident) + } else { + return; + } + }; + + let mut used_field = None; + let mut correct_field = None; + let typeck_results = cx.typeck_results(); + for adjusted_type in iter::once(typeck_results.expr_ty(self_data)) + .chain(typeck_results.expr_adjustments(self_data).iter().map(|adj| adj.target)) + { + let ty::Adt(def,_) = adjusted_type.kind() else { + continue; + }; + + for f in def.all_fields() { + if f.name.as_str() == name { + correct_field = Some(f); + } + if f.name == used_ident.name { + used_field = Some(f); + } + } + } + + let Some(used_field) = used_field else { + // Can happen if the field access is a tuple. We don't lint those because the getter name could not start with a number. + return; + }; + + let Some(correct_field) = correct_field else { + // There is no field corresponding to the getter name. + // FIXME: This can be a false positive if the correct field is reachable trought deeper autodereferences than used_field is + return; + }; + + if cx.tcx.type_of(used_field.did) == cx.tcx.type_of(correct_field.did) { + let left_span = block_expr.span.until(used_ident.span); + let snippet = snippet(cx, left_span, ".."); + let sugg = format!("{snippet}{name}"); + span_lint_and_then( + cx, + MISNAMED_GETTERS, + span, + "getter function appears to return the wrong field", + |diag| { + diag.span_suggestion(expr_span, "consider using", sugg, Applicability::MaybeIncorrect); + }, + ); + } +} diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index ae0e0833446..91e6ffe6447 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -1,3 +1,4 @@ +mod misnamed_getters; mod must_use; mod not_unsafe_ptr_arg_deref; mod result; @@ -260,6 +261,48 @@ declare_clippy_lint! { "function returning `Result` with large `Err` type" } +declare_clippy_lint! { + /// ### What it does + /// Checks for getter methods that return a field that doesn't correspond + /// to the name of the method, when there is a field's whose name matches that of the method. + /// + /// ### Why is this bad? + /// It is most likely that such a method is a bug caused by a typo or by copy-pasting. + /// + /// ### Example + + /// ```rust + /// struct A { + /// a: String, + /// b: String, + /// } + /// + /// impl A { + /// fn a(&self) -> &str{ + /// &self.b + /// } + /// } + + /// ``` + /// Use instead: + /// ```rust + /// struct A { + /// a: String, + /// b: String, + /// } + /// + /// impl A { + /// fn a(&self) -> &str{ + /// &self.a + /// } + /// } + /// ``` + #[clippy::version = "1.67.0"] + pub MISNAMED_GETTERS, + suspicious, + "getter method returning the wrong field" +} + #[derive(Copy, Clone)] pub struct Functions { too_many_arguments_threshold: u64, @@ -286,6 +329,7 @@ impl_lint_pass!(Functions => [ MUST_USE_CANDIDATE, RESULT_UNIT_ERR, RESULT_LARGE_ERR, + MISNAMED_GETTERS, ]); impl<'tcx> LateLintPass<'tcx> for Functions { @@ -301,6 +345,7 @@ impl<'tcx> LateLintPass<'tcx> for Functions { too_many_arguments::check_fn(cx, kind, decl, span, hir_id, self.too_many_arguments_threshold); too_many_lines::check_fn(cx, kind, span, body, self.too_many_lines_threshold); not_unsafe_ptr_arg_deref::check_fn(cx, kind, decl, body, hir_id); + misnamed_getters::check_fn(cx, kind, decl, body, span, hir_id); } fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { diff --git a/tests/ui/misnamed_getters.rs b/tests/ui/misnamed_getters.rs new file mode 100644 index 00000000000..03e7dac7df9 --- /dev/null +++ b/tests/ui/misnamed_getters.rs @@ -0,0 +1,124 @@ +#![allow(unused)] +#![warn(clippy::misnamed_getters)] + +struct A { + a: u8, + b: u8, + c: u8, +} + +impl A { + fn a(&self) -> &u8 { + &self.b + } + fn a_mut(&mut self) -> &mut u8 { + &mut self.b + } + + fn b(self) -> u8 { + self.a + } + + fn b_mut(&mut self) -> &mut u8 { + &mut self.a + } + + fn c(&self) -> &u8 { + &self.b + } + + fn c_mut(&mut self) -> &mut u8 { + &mut self.a + } +} + +union B { + a: u8, + b: u8, +} + +impl B { + unsafe fn a(&self) -> &u8 { + &self.b + } + unsafe fn a_mut(&mut self) -> &mut u8 { + &mut self.b + } + + unsafe fn b(self) -> u8 { + self.a + } + + unsafe fn b_mut(&mut self) -> &mut u8 { + &mut self.a + } + + unsafe fn c(&self) -> &u8 { + &self.b + } + + unsafe fn c_mut(&mut self) -> &mut u8 { + &mut self.a + } + + unsafe fn a_unchecked(&self) -> &u8 { + &self.b + } + unsafe fn a_unchecked_mut(&mut self) -> &mut u8 { + &mut self.b + } + + unsafe fn b_unchecked(self) -> u8 { + self.a + } + + unsafe fn b_unchecked_mut(&mut self) -> &mut u8 { + &mut self.a + } + + unsafe fn c_unchecked(&self) -> &u8 { + &self.b + } + + unsafe fn c_unchecked_mut(&mut self) -> &mut u8 { + &mut self.a + } +} + +struct D { + d: u8, + inner: A, +} + +impl core::ops::Deref for D { + type Target = A; + fn deref(&self) -> &A { + &self.inner + } +} + +impl core::ops::DerefMut for D { + fn deref_mut(&mut self) -> &mut A { + &mut self.inner + } +} + +impl D { + fn a(&self) -> &u8 { + &self.b + } + fn a_mut(&mut self) -> &mut u8 { + &mut self.b + } + + fn d(&self) -> &u8 { + &self.b + } + fn d_mut(&mut self) -> &mut u8 { + &mut self.b + } +} + +fn main() { + // test code goes here +} diff --git a/tests/ui/misnamed_getters.stderr b/tests/ui/misnamed_getters.stderr new file mode 100644 index 00000000000..1e38a83d019 --- /dev/null +++ b/tests/ui/misnamed_getters.stderr @@ -0,0 +1,166 @@ +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:11:5 + | +LL | / fn a(&self) -> &u8 { +LL | | &self.b + | | ------- help: consider using: `&self.a` +LL | | } + | |_____^ + | + = note: `-D clippy::misnamed-getters` implied by `-D warnings` + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:14:5 + | +LL | / fn a_mut(&mut self) -> &mut u8 { +LL | | &mut self.b + | | ----------- help: consider using: `&mut self.a` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:18:5 + | +LL | / fn b(self) -> u8 { +LL | | self.a + | | ------ help: consider using: `self.b` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:22:5 + | +LL | / fn b_mut(&mut self) -> &mut u8 { +LL | | &mut self.a + | | ----------- help: consider using: `&mut self.b` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:26:5 + | +LL | / fn c(&self) -> &u8 { +LL | | &self.b + | | ------- help: consider using: `&self.c` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:30:5 + | +LL | / fn c_mut(&mut self) -> &mut u8 { +LL | | &mut self.a + | | ----------- help: consider using: `&mut self.c` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:41:5 + | +LL | / unsafe fn a(&self) -> &u8 { +LL | | &self.b + | | ------- help: consider using: `&self.a` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:44:5 + | +LL | / unsafe fn a_mut(&mut self) -> &mut u8 { +LL | | &mut self.b + | | ----------- help: consider using: `&mut self.a` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:48:5 + | +LL | / unsafe fn b(self) -> u8 { +LL | | self.a + | | ------ help: consider using: `self.b` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:52:5 + | +LL | / unsafe fn b_mut(&mut self) -> &mut u8 { +LL | | &mut self.a + | | ----------- help: consider using: `&mut self.b` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:64:5 + | +LL | / unsafe fn a_unchecked(&self) -> &u8 { +LL | | &self.b + | | ------- help: consider using: `&self.a` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:67:5 + | +LL | / unsafe fn a_unchecked_mut(&mut self) -> &mut u8 { +LL | | &mut self.b + | | ----------- help: consider using: `&mut self.a` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:71:5 + | +LL | / unsafe fn b_unchecked(self) -> u8 { +LL | | self.a + | | ------ help: consider using: `self.b` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:75:5 + | +LL | / unsafe fn b_unchecked_mut(&mut self) -> &mut u8 { +LL | | &mut self.a + | | ----------- help: consider using: `&mut self.b` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:107:5 + | +LL | / fn a(&self) -> &u8 { +LL | | &self.b + | | ------- help: consider using: `&self.a` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:110:5 + | +LL | / fn a_mut(&mut self) -> &mut u8 { +LL | | &mut self.b + | | ----------- help: consider using: `&mut self.a` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:114:5 + | +LL | / fn d(&self) -> &u8 { +LL | | &self.b + | | ------- help: consider using: `&self.d` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:117:5 + | +LL | / fn d_mut(&mut self) -> &mut u8 { +LL | | &mut self.b + | | ----------- help: consider using: `&mut self.d` +LL | | } + | |_____^ + +error: aborting due to 18 previous errors +