From 31b83d0895d37dc8a37e195f75bb9fe7de2c5e37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Tue, 1 Nov 2022 18:39:36 +0100 Subject: [PATCH 01/14] Add missnamed_getters lint --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + .../src/functions/missnamed_getters.rs | 123 ++++++++++++++++++ clippy_lints/src/functions/mod.rs | 22 ++++ tests/ui/missnamed_getters.rs | 28 ++++ tests/ui/missnamed_getters.stderr | 16 +++ 6 files changed, 191 insertions(+) create mode 100644 clippy_lints/src/functions/missnamed_getters.rs create mode 100644 tests/ui/missnamed_getters.rs create mode 100644 tests/ui/missnamed_getters.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f1f73c1f..180e7d8be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4198,6 +4198,7 @@ Released 2018-09-13 [`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc [`missing_spin_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_spin_loop [`missing_trait_methods`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_trait_methods +[`missnamed_getters`]: https://rust-lang.github.io/rust-clippy/master/index.html#missnamed_getters [`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes [`mixed_case_hex_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_case_hex_literals [`mixed_read_write_in_expression`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_read_write_in_expression diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 0d3fc43a6..0c9ae6380 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -184,6 +184,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::functions::RESULT_UNIT_ERR_INFO, crate::functions::TOO_MANY_ARGUMENTS_INFO, crate::functions::TOO_MANY_LINES_INFO, + crate::functions::MISSNAMED_GETTERS_INFO, crate::future_not_send::FUTURE_NOT_SEND_INFO, crate::if_let_mutex::IF_LET_MUTEX_INFO, crate::if_not_else::IF_NOT_ELSE_INFO, diff --git a/clippy_lints/src/functions/missnamed_getters.rs b/clippy_lints/src/functions/missnamed_getters.rs new file mode 100644 index 000000000..c522bb780 --- /dev/null +++ b/clippy_lints/src/functions/missnamed_getters.rs @@ -0,0 +1,123 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet; +use rustc_errors::Applicability; +use rustc_hir::{intravisit::FnKind, Body, ExprKind, FnDecl, HirId, ImplicitSelfKind}; +use rustc_lint::LateContext; +use rustc_middle::ty; +use rustc_span::Span; + +use super::MISSNAMED_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 sig.decl.implicit_self { + ImplicitSelfKind::ImmRef => name, + ImplicitSelfKind::MutRef => { + let Some(name) = name.strip_suffix("_mut") else { + return; + }; + name + }, + _ => return, + }; + + // Body must be &(mut) .name + // self_data is not neccessarilly self + let (self_data, used_ident, span) = if_chain! { + if let ExprKind::Block(block,_) = body.value.kind; + if block.stmts.is_empty(); + if let Some(block_expr) = block.expr; + // replace with while for as many addrof needed + if let ExprKind::AddrOf(_,_, expr) = block_expr.kind; + if let ExprKind::Field(self_data, ident) = expr.kind; + if ident.name.as_str() != name; + then { + (self_data,ident,block_expr.span) + } else { + return; + } + }; + + let ty = cx.typeck_results().expr_ty(self_data); + + let def = { + let mut kind = ty.kind(); + loop { + match kind { + ty::Adt(def, _) => break def, + ty::Ref(_, ty, _) => kind = ty.kind(), + // We don't do tuples because the function name cannot be a number + _ => return, + } + } + }; + + let variants = def.variants(); + + // We're accessing a field, so it should be an union or a struct and have one and only one variant + if variants.len() != 1 { + if cfg!(debug_assertions) { + panic!("Struct or union expected to have only one variant"); + } else { + // Don't ICE when possible + return; + } + } + + let first = variants.last().unwrap(); + let fields = &variants[first]; + + let mut used_field = None; + let mut correct_field = None; + for f in &fields.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 { + if cfg!(debug_assertions) { + panic!("Struct doesn't contain the correct field"); + } else { + // Don't ICE when possible + return; + } + }; + let Some(correct_field) = correct_field else { + return; + }; + + if cx.tcx.type_of(used_field.did) == cx.tcx.type_of(correct_field.did) { + let snippet = snippet(cx, span, ".."); + let sugg = format!("{}{name}", snippet.strip_suffix(used_field.name.as_str()).unwrap()); + span_lint_and_sugg( + cx, + MISSNAMED_GETTERS, + span, + "getter function appears to return the wrong field", + "consider using", + sugg, + Applicability::MaybeIncorrect, + ); + } +} diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index ae0e08334..726df0244 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -1,3 +1,4 @@ +mod missnamed_getters; mod must_use; mod not_unsafe_ptr_arg_deref; mod result; @@ -260,6 +261,25 @@ declare_clippy_lint! { "function returning `Result` with large `Err` type" } +declare_clippy_lint! { + /// ### What it does + /// + /// ### Why is this bad? + /// + /// ### Example + /// ```rust + /// // example code where clippy issues a warning + /// ``` + /// Use instead: + /// ```rust + /// // example code which does not raise clippy warning + /// ``` + #[clippy::version = "1.66.0"] + pub MISSNAMED_GETTERS, + suspicious, + "default lint description" +} + #[derive(Copy, Clone)] pub struct Functions { too_many_arguments_threshold: u64, @@ -286,6 +306,7 @@ impl_lint_pass!(Functions => [ MUST_USE_CANDIDATE, RESULT_UNIT_ERR, RESULT_LARGE_ERR, + MISSNAMED_GETTERS, ]); impl<'tcx> LateLintPass<'tcx> for Functions { @@ -301,6 +322,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); + missnamed_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/missnamed_getters.rs b/tests/ui/missnamed_getters.rs new file mode 100644 index 000000000..b47f6edc5 --- /dev/null +++ b/tests/ui/missnamed_getters.rs @@ -0,0 +1,28 @@ +#![allow(unused)] +#![warn(clippy::missnamed_getters)] + +struct A { + a: u8, + b: u8, +} + +impl A { + fn a(&self) -> &u8 { + &self.b + } +} + +union B { + a: u8, + b: u8, +} + +impl B { + unsafe fn a(&self) -> &u8 { + &self.b + } +} + +fn main() { + // test code goes here +} diff --git a/tests/ui/missnamed_getters.stderr b/tests/ui/missnamed_getters.stderr new file mode 100644 index 000000000..8e31a42b9 --- /dev/null +++ b/tests/ui/missnamed_getters.stderr @@ -0,0 +1,16 @@ +error: getter function appears to return the wrong field + --> $DIR/missnamed_getters.rs:11:9 + | +LL | &self.b + | ^^^^^^^ help: consider using: `&self.a` + | + = note: `-D clippy::missnamed-getters` implied by `-D warnings` + +error: getter function appears to return the wrong field + --> $DIR/missnamed_getters.rs:22:9 + | +LL | &self.b + | ^^^^^^^ help: consider using: `&self.a` + +error: aborting due to 2 previous errors + From 9891af348c01f8cf14ab1e10754faccad04fcaa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Tue, 1 Nov 2022 19:07:51 +0100 Subject: [PATCH 02/14] missnamed_getters: Match owned methods --- .../src/functions/missnamed_getters.rs | 27 +++++++--- tests/ui/missnamed_getters.rs | 39 ++++++++++++++ tests/ui/missnamed_getters.stderr | 54 +++++++++++++++++-- 3 files changed, 110 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/functions/missnamed_getters.rs b/clippy_lints/src/functions/missnamed_getters.rs index c522bb780..1f4eefc62 100644 --- a/clippy_lints/src/functions/missnamed_getters.rs +++ b/clippy_lints/src/functions/missnamed_getters.rs @@ -35,21 +35,34 @@ pub fn check_fn( }; name }, + ImplicitSelfKind::Imm | ImplicitSelfKind::Mut => name, _ => return, }; // Body must be &(mut) .name - // self_data is not neccessarilly self - let (self_data, used_ident, span) = if_chain! { + // 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; - // replace with while for as many addrof needed - if let ExprKind::AddrOf(_,_, expr) = block_expr.kind; + then { + block_expr + } else { + return; + } + }; + let expr_span = block_expr.span; + + let mut expr = block_expr; + if let ExprKind::AddrOf(_, _, tmp) = expr.kind { + expr = tmp; + } + 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,block_expr.span) + (self_data,ident) } else { return; } @@ -108,12 +121,12 @@ pub fn check_fn( }; if cx.tcx.type_of(used_field.did) == cx.tcx.type_of(correct_field.did) { - let snippet = snippet(cx, span, ".."); + let snippet = snippet(cx, expr_span, ".."); let sugg = format!("{}{name}", snippet.strip_suffix(used_field.name.as_str()).unwrap()); span_lint_and_sugg( cx, MISSNAMED_GETTERS, - span, + expr_span, "getter function appears to return the wrong field", "consider using", sugg, diff --git a/tests/ui/missnamed_getters.rs b/tests/ui/missnamed_getters.rs index b47f6edc5..f9c2351f8 100644 --- a/tests/ui/missnamed_getters.rs +++ b/tests/ui/missnamed_getters.rs @@ -4,12 +4,32 @@ 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 { @@ -21,6 +41,25 @@ 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 + } } fn main() { diff --git a/tests/ui/missnamed_getters.stderr b/tests/ui/missnamed_getters.stderr index 8e31a42b9..276096ade 100644 --- a/tests/ui/missnamed_getters.stderr +++ b/tests/ui/missnamed_getters.stderr @@ -1,5 +1,5 @@ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:11:9 + --> $DIR/missnamed_getters.rs:12:9 | LL | &self.b | ^^^^^^^ help: consider using: `&self.a` @@ -7,10 +7,58 @@ LL | &self.b = note: `-D clippy::missnamed-getters` implied by `-D warnings` error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:22:9 + --> $DIR/missnamed_getters.rs:15:9 + | +LL | &mut self.b + | ^^^^^^^^^^^ help: consider using: `&mut self.a` + +error: getter function appears to return the wrong field + --> $DIR/missnamed_getters.rs:19:9 + | +LL | self.a + | ^^^^^^ help: consider using: `self.b` + +error: getter function appears to return the wrong field + --> $DIR/missnamed_getters.rs:23:9 + | +LL | &mut self.a + | ^^^^^^^^^^^ help: consider using: `&mut self.b` + +error: getter function appears to return the wrong field + --> $DIR/missnamed_getters.rs:27:9 + | +LL | &self.b + | ^^^^^^^ help: consider using: `&self.c` + +error: getter function appears to return the wrong field + --> $DIR/missnamed_getters.rs:31:9 + | +LL | &mut self.a + | ^^^^^^^^^^^ help: consider using: `&mut self.c` + +error: getter function appears to return the wrong field + --> $DIR/missnamed_getters.rs:42:9 | LL | &self.b | ^^^^^^^ help: consider using: `&self.a` -error: aborting due to 2 previous errors +error: getter function appears to return the wrong field + --> $DIR/missnamed_getters.rs:45:9 + | +LL | &mut self.b + | ^^^^^^^^^^^ help: consider using: `&mut self.a` + +error: getter function appears to return the wrong field + --> $DIR/missnamed_getters.rs:49:9 + | +LL | self.a + | ^^^^^^ help: consider using: `self.b` + +error: getter function appears to return the wrong field + --> $DIR/missnamed_getters.rs:53:9 + | +LL | &mut self.a + | ^^^^^^^^^^^ help: consider using: `&mut self.b` + +error: aborting due to 10 previous errors From ddc49966dc4552b77582cf7e5aace8ac97d736fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Tue, 1 Nov 2022 19:28:06 +0100 Subject: [PATCH 03/14] Fix suggestion to point to the whole method --- .../src/functions/missnamed_getters.rs | 22 ++--- tests/ui/missnamed_getters.stderr | 90 ++++++++++++------- 2 files changed, 72 insertions(+), 40 deletions(-) diff --git a/clippy_lints/src/functions/missnamed_getters.rs b/clippy_lints/src/functions/missnamed_getters.rs index 1f4eefc62..60922fb4e 100644 --- a/clippy_lints/src/functions/missnamed_getters.rs +++ b/clippy_lints/src/functions/missnamed_getters.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; +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}; @@ -13,7 +13,7 @@ pub fn check_fn( kind: FnKind<'_>, decl: &FnDecl<'_>, body: &Body<'_>, - _span: Span, + span: Span, _hir_id: HirId, ) { let FnKind::Method(ref ident, sig) = kind else { @@ -55,6 +55,7 @@ pub fn check_fn( let expr_span = block_expr.span; let mut expr = block_expr; + // Accept &, &mut and if let ExprKind::AddrOf(_, _, tmp) = expr.kind { expr = tmp; } @@ -62,7 +63,7 @@ pub fn check_fn( if let ExprKind::Field(self_data, ident) = expr.kind; if ident.name.as_str() != name; then { - (self_data,ident) + (self_data, ident) } else { return; } @@ -121,16 +122,17 @@ pub fn check_fn( }; if cx.tcx.type_of(used_field.did) == cx.tcx.type_of(correct_field.did) { - let snippet = snippet(cx, expr_span, ".."); - let sugg = format!("{}{name}", snippet.strip_suffix(used_field.name.as_str()).unwrap()); - span_lint_and_sugg( + 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, MISSNAMED_GETTERS, - expr_span, + span, "getter function appears to return the wrong field", - "consider using", - sugg, - Applicability::MaybeIncorrect, + |diag| { + diag.span_suggestion(expr_span, "consider using", sugg, Applicability::MaybeIncorrect); + }, ); } } diff --git a/tests/ui/missnamed_getters.stderr b/tests/ui/missnamed_getters.stderr index 276096ade..2e3d9df34 100644 --- a/tests/ui/missnamed_getters.stderr +++ b/tests/ui/missnamed_getters.stderr @@ -1,64 +1,94 @@ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:12:9 + --> $DIR/missnamed_getters.rs:11:5 | -LL | &self.b - | ^^^^^^^ help: consider using: `&self.a` +LL | / fn a(&self) -> &u8 { +LL | | &self.b + | | ------- help: consider using: `&self.a` +LL | | } + | |_____^ | = note: `-D clippy::missnamed-getters` implied by `-D warnings` error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:15:9 + --> $DIR/missnamed_getters.rs:14:5 | -LL | &mut self.b - | ^^^^^^^^^^^ help: consider using: `&mut self.a` +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/missnamed_getters.rs:19:9 + --> $DIR/missnamed_getters.rs:18:5 | -LL | self.a - | ^^^^^^ help: consider using: `self.b` +LL | / fn b(self) -> u8 { +LL | | self.a + | | ------ help: consider using: `self.b` +LL | | } + | |_____^ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:23:9 + --> $DIR/missnamed_getters.rs:22:5 | -LL | &mut self.a - | ^^^^^^^^^^^ help: consider using: `&mut self.b` +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/missnamed_getters.rs:27:9 + --> $DIR/missnamed_getters.rs:26:5 | -LL | &self.b - | ^^^^^^^ help: consider using: `&self.c` +LL | / fn c(&self) -> &u8 { +LL | | &self.b + | | ------- help: consider using: `&self.c` +LL | | } + | |_____^ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:31:9 + --> $DIR/missnamed_getters.rs:30:5 | -LL | &mut self.a - | ^^^^^^^^^^^ help: consider using: `&mut self.c` +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/missnamed_getters.rs:42:9 + --> $DIR/missnamed_getters.rs:41:5 | -LL | &self.b - | ^^^^^^^ help: consider using: `&self.a` +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/missnamed_getters.rs:45:9 + --> $DIR/missnamed_getters.rs:44:5 | -LL | &mut self.b - | ^^^^^^^^^^^ help: consider using: `&mut self.a` +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/missnamed_getters.rs:49:9 + --> $DIR/missnamed_getters.rs:48:5 | -LL | self.a - | ^^^^^^ help: consider using: `self.b` +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/missnamed_getters.rs:53:9 + --> $DIR/missnamed_getters.rs:52:5 | -LL | &mut self.a - | ^^^^^^^^^^^ help: consider using: `&mut self.b` +LL | / unsafe fn b_mut(&mut self) -> &mut u8 { +LL | | &mut self.a + | | ----------- help: consider using: `&mut self.b` +LL | | } + | |_____^ error: aborting due to 10 previous errors From 81d459083499c35700f3d7fa2b28cdae2f35a636 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Tue, 1 Nov 2022 19:31:47 +0100 Subject: [PATCH 04/14] missnamed_getters: use all_fields iterator --- clippy_lints/src/functions/missnamed_getters.rs | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/clippy_lints/src/functions/missnamed_getters.rs b/clippy_lints/src/functions/missnamed_getters.rs index 60922fb4e..306dccdbd 100644 --- a/clippy_lints/src/functions/missnamed_getters.rs +++ b/clippy_lints/src/functions/missnamed_getters.rs @@ -83,24 +83,9 @@ pub fn check_fn( } }; - let variants = def.variants(); - - // We're accessing a field, so it should be an union or a struct and have one and only one variant - if variants.len() != 1 { - if cfg!(debug_assertions) { - panic!("Struct or union expected to have only one variant"); - } else { - // Don't ICE when possible - return; - } - } - - let first = variants.last().unwrap(); - let fields = &variants[first]; - let mut used_field = None; let mut correct_field = None; - for f in &fields.fields { + for f in def.all_fields() { if f.name.as_str() == name { correct_field = Some(f); } From 5fa0e07cdf5e6798a71ff252f14a2713699ae99d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Tue, 1 Nov 2022 21:36:18 +0100 Subject: [PATCH 05/14] Document missname_getters --- clippy_lints/src/declared_lints.rs | 2 +- clippy_lints/src/functions/mod.rs | 21 +++++++++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 0c9ae6380..bccf84259 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::MISSNAMED_GETTERS_INFO, crate::functions::MUST_USE_CANDIDATE_INFO, crate::functions::MUST_USE_UNIT_INFO, crate::functions::NOT_UNSAFE_PTR_ARG_DEREF_INFO, @@ -184,7 +185,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::functions::RESULT_UNIT_ERR_INFO, crate::functions::TOO_MANY_ARGUMENTS_INFO, crate::functions::TOO_MANY_LINES_INFO, - crate::functions::MISSNAMED_GETTERS_INFO, crate::future_not_send::FUTURE_NOT_SEND_INFO, crate::if_let_mutex::IF_LET_MUTEX_INFO, crate::if_not_else::IF_NOT_ELSE_INFO, diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index 726df0244..87792899b 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -263,21 +263,38 @@ declare_clippy_lint! { 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 + /// } + /// } /// // example code where clippy issues a warning /// ``` /// Use instead: /// ```rust - /// // example code which does not raise clippy warning + /// impl A { + /// fn a(&self) -> &str{ + /// self.a + /// } + /// } /// ``` #[clippy::version = "1.66.0"] pub MISSNAMED_GETTERS, suspicious, - "default lint description" + "getter method returning the wrong field" } #[derive(Copy, Clone)] From 3e2e81b2db9bbacc826ac429b52ac1173356a2e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Tue, 1 Nov 2022 21:49:20 +0100 Subject: [PATCH 06/14] Fix internal warnings --- clippy_lints/src/functions/missnamed_getters.rs | 5 ++--- clippy_lints/src/functions/mod.rs | 9 +++++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/functions/missnamed_getters.rs b/clippy_lints/src/functions/missnamed_getters.rs index 306dccdbd..3d78679b5 100644 --- a/clippy_lints/src/functions/missnamed_getters.rs +++ b/clippy_lints/src/functions/missnamed_getters.rs @@ -28,15 +28,14 @@ pub fn check_fn( let name = ident.name.as_str(); let name = match sig.decl.implicit_self { - ImplicitSelfKind::ImmRef => name, ImplicitSelfKind::MutRef => { let Some(name) = name.strip_suffix("_mut") else { return; }; name }, - ImplicitSelfKind::Imm | ImplicitSelfKind::Mut => name, - _ => return, + ImplicitSelfKind::Imm | ImplicitSelfKind::Mut | ImplicitSelfKind::ImmRef => name, + ImplicitSelfKind::None => return, }; // Body must be &(mut) .name diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index 87792899b..e4181a164 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -278,16 +278,21 @@ declare_clippy_lint! { /// /// impl A { /// fn a(&self) -> &str{ - /// self.b + /// &self.b /// } /// } /// // example code where clippy issues a warning /// ``` /// Use instead: /// ```rust + /// struct A { + /// a: String, + /// b: String, + /// } + /// /// impl A { /// fn a(&self) -> &str{ - /// self.a + /// &self.a /// } /// } /// ``` From 3428da6e00c025b3b3141a9fe7c65ee5008e07f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Wed, 2 Nov 2022 09:01:33 +0100 Subject: [PATCH 07/14] Fix typo missnamed -> misnamed --- CHANGELOG.md | 2 +- clippy_lints/src/declared_lints.rs | 2 +- ...ssnamed_getters.rs => misnamed_getters.rs} | 4 ++-- clippy_lints/src/functions/mod.rs | 8 +++---- ...ssnamed_getters.rs => misnamed_getters.rs} | 2 +- ...getters.stderr => misnamed_getters.stderr} | 22 +++++++++---------- 6 files changed, 20 insertions(+), 20 deletions(-) rename clippy_lints/src/functions/{missnamed_getters.rs => misnamed_getters.rs} (98%) rename tests/ui/{missnamed_getters.rs => misnamed_getters.rs} (96%) rename tests/ui/{missnamed_getters.stderr => misnamed_getters.stderr} (83%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 180e7d8be..5b6b12c62 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 @@ -4198,7 +4199,6 @@ Released 2018-09-13 [`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc [`missing_spin_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_spin_loop [`missing_trait_methods`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_trait_methods -[`missnamed_getters`]: https://rust-lang.github.io/rust-clippy/master/index.html#missnamed_getters [`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes [`mixed_case_hex_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_case_hex_literals [`mixed_read_write_in_expression`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_read_write_in_expression diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index bccf84259..eb3210946 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -177,7 +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::MISSNAMED_GETTERS_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/missnamed_getters.rs b/clippy_lints/src/functions/misnamed_getters.rs similarity index 98% rename from clippy_lints/src/functions/missnamed_getters.rs rename to clippy_lints/src/functions/misnamed_getters.rs index 3d78679b5..3859c7a62 100644 --- a/clippy_lints/src/functions/missnamed_getters.rs +++ b/clippy_lints/src/functions/misnamed_getters.rs @@ -6,7 +6,7 @@ use rustc_lint::LateContext; use rustc_middle::ty; use rustc_span::Span; -use super::MISSNAMED_GETTERS; +use super::MISNAMED_GETTERS; pub fn check_fn( cx: &LateContext<'_>, @@ -111,7 +111,7 @@ pub fn check_fn( let sugg = format!("{snippet}{name}"); span_lint_and_then( cx, - MISSNAMED_GETTERS, + MISNAMED_GETTERS, span, "getter function appears to return the wrong field", |diag| { diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index e4181a164..3478fdd86 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -1,4 +1,4 @@ -mod missnamed_getters; +mod misnamed_getters; mod must_use; mod not_unsafe_ptr_arg_deref; mod result; @@ -297,7 +297,7 @@ declare_clippy_lint! { /// } /// ``` #[clippy::version = "1.66.0"] - pub MISSNAMED_GETTERS, + pub MISNAMED_GETTERS, suspicious, "getter method returning the wrong field" } @@ -328,7 +328,7 @@ impl_lint_pass!(Functions => [ MUST_USE_CANDIDATE, RESULT_UNIT_ERR, RESULT_LARGE_ERR, - MISSNAMED_GETTERS, + MISNAMED_GETTERS, ]); impl<'tcx> LateLintPass<'tcx> for Functions { @@ -344,7 +344,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); - missnamed_getters::check_fn(cx, kind, decl, body, span, 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/missnamed_getters.rs b/tests/ui/misnamed_getters.rs similarity index 96% rename from tests/ui/missnamed_getters.rs rename to tests/ui/misnamed_getters.rs index f9c2351f8..7d490f4b1 100644 --- a/tests/ui/missnamed_getters.rs +++ b/tests/ui/misnamed_getters.rs @@ -1,5 +1,5 @@ #![allow(unused)] -#![warn(clippy::missnamed_getters)] +#![warn(clippy::misnamed_getters)] struct A { a: u8, diff --git a/tests/ui/missnamed_getters.stderr b/tests/ui/misnamed_getters.stderr similarity index 83% rename from tests/ui/missnamed_getters.stderr rename to tests/ui/misnamed_getters.stderr index 2e3d9df34..f4a059ab9 100644 --- a/tests/ui/missnamed_getters.stderr +++ b/tests/ui/misnamed_getters.stderr @@ -1,5 +1,5 @@ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:11:5 + --> $DIR/misnamed_getters.rs:11:5 | LL | / fn a(&self) -> &u8 { LL | | &self.b @@ -7,10 +7,10 @@ LL | | &self.b LL | | } | |_____^ | - = note: `-D clippy::missnamed-getters` implied by `-D warnings` + = note: `-D clippy::misnamed-getters` implied by `-D warnings` error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:14:5 + --> $DIR/misnamed_getters.rs:14:5 | LL | / fn a_mut(&mut self) -> &mut u8 { LL | | &mut self.b @@ -19,7 +19,7 @@ LL | | } | |_____^ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:18:5 + --> $DIR/misnamed_getters.rs:18:5 | LL | / fn b(self) -> u8 { LL | | self.a @@ -28,7 +28,7 @@ LL | | } | |_____^ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:22:5 + --> $DIR/misnamed_getters.rs:22:5 | LL | / fn b_mut(&mut self) -> &mut u8 { LL | | &mut self.a @@ -37,7 +37,7 @@ LL | | } | |_____^ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:26:5 + --> $DIR/misnamed_getters.rs:26:5 | LL | / fn c(&self) -> &u8 { LL | | &self.b @@ -46,7 +46,7 @@ LL | | } | |_____^ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:30:5 + --> $DIR/misnamed_getters.rs:30:5 | LL | / fn c_mut(&mut self) -> &mut u8 { LL | | &mut self.a @@ -55,7 +55,7 @@ LL | | } | |_____^ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:41:5 + --> $DIR/misnamed_getters.rs:41:5 | LL | / unsafe fn a(&self) -> &u8 { LL | | &self.b @@ -64,7 +64,7 @@ LL | | } | |_____^ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:44:5 + --> $DIR/misnamed_getters.rs:44:5 | LL | / unsafe fn a_mut(&mut self) -> &mut u8 { LL | | &mut self.b @@ -73,7 +73,7 @@ LL | | } | |_____^ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:48:5 + --> $DIR/misnamed_getters.rs:48:5 | LL | / unsafe fn b(self) -> u8 { LL | | self.a @@ -82,7 +82,7 @@ LL | | } | |_____^ error: getter function appears to return the wrong field - --> $DIR/missnamed_getters.rs:52:5 + --> $DIR/misnamed_getters.rs:52:5 | LL | / unsafe fn b_mut(&mut self) -> &mut u8 { LL | | &mut self.a From a867c17ab36926a575a57e24a03e99db672055f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Wed, 2 Nov 2022 19:02:46 +0100 Subject: [PATCH 08/14] Improve code --- .../src/functions/misnamed_getters.rs | 13 ++++++----- tests/ui/misnamed_getters.rs | 23 +++++++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/functions/misnamed_getters.rs b/clippy_lints/src/functions/misnamed_getters.rs index 3859c7a62..0d50ec379 100644 --- a/clippy_lints/src/functions/misnamed_getters.rs +++ b/clippy_lints/src/functions/misnamed_getters.rs @@ -16,7 +16,7 @@ pub fn check_fn( span: Span, _hir_id: HirId, ) { - let FnKind::Method(ref ident, sig) = kind else { + let FnKind::Method(ref ident, _) = kind else { return; }; @@ -27,7 +27,7 @@ pub fn check_fn( let name = ident.name.as_str(); - let name = match sig.decl.implicit_self { + let name = match decl.implicit_self { ImplicitSelfKind::MutRef => { let Some(name) = name.strip_suffix("_mut") else { return; @@ -53,11 +53,12 @@ pub fn check_fn( }; let expr_span = block_expr.span; - let mut expr = block_expr; // Accept &, &mut and - if let ExprKind::AddrOf(_, _, tmp) = expr.kind { - expr = tmp; - } + 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; diff --git a/tests/ui/misnamed_getters.rs b/tests/ui/misnamed_getters.rs index 7d490f4b1..6f1618182 100644 --- a/tests/ui/misnamed_getters.rs +++ b/tests/ui/misnamed_getters.rs @@ -60,6 +60,29 @@ impl 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 + } } fn main() { From 3f1a186bd1fd74ad3b25bf5c642e86c8913a1149 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Wed, 2 Nov 2022 19:11:27 +0100 Subject: [PATCH 09/14] misnamed_getters: Trigger on unsafe with _unchecked --- .../src/functions/misnamed_getters.rs | 10 ++++- tests/ui/misnamed_getters.stderr | 38 ++++++++++++++++++- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/functions/misnamed_getters.rs b/clippy_lints/src/functions/misnamed_getters.rs index 0d50ec379..599269e2d 100644 --- a/clippy_lints/src/functions/misnamed_getters.rs +++ b/clippy_lints/src/functions/misnamed_getters.rs @@ -1,7 +1,7 @@ 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}; +use rustc_hir::{intravisit::FnKind, Body, ExprKind, FnDecl, HirId, ImplicitSelfKind, Unsafety}; use rustc_lint::LateContext; use rustc_middle::ty; use rustc_span::Span; @@ -16,7 +16,7 @@ pub fn check_fn( span: Span, _hir_id: HirId, ) { - let FnKind::Method(ref ident, _) = kind else { + let FnKind::Method(ref ident, sig) = kind else { return; }; @@ -38,6 +38,12 @@ pub fn check_fn( 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… diff --git a/tests/ui/misnamed_getters.stderr b/tests/ui/misnamed_getters.stderr index f4a059ab9..feefb95ab 100644 --- a/tests/ui/misnamed_getters.stderr +++ b/tests/ui/misnamed_getters.stderr @@ -90,5 +90,41 @@ LL | | &mut self.a LL | | } | |_____^ -error: aborting due to 10 previous errors +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: aborting due to 14 previous errors From 77374a95272dfb8fcbc75cd074b4e61b1890c724 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sat, 12 Nov 2022 22:33:46 +0100 Subject: [PATCH 10/14] Add failing test --- tests/ui/misnamed_getters.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/ui/misnamed_getters.rs b/tests/ui/misnamed_getters.rs index 6f1618182..2c451bd3a 100644 --- a/tests/ui/misnamed_getters.rs +++ b/tests/ui/misnamed_getters.rs @@ -85,6 +85,18 @@ impl B { } } +struct C { + inner: Box, +} +impl C { + unsafe fn a(&self) -> &u8 { + &self.inner.b + } + unsafe fn a_mut(&mut self) -> &mut u8 { + &mut self.inner.b + } +} + fn main() { // test code goes here } From d9993cb133909eb17705cd9dc518c2c213fc779b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sun, 20 Nov 2022 12:20:16 +0100 Subject: [PATCH 11/14] Remove error when fields use autoderef --- clippy_lints/src/functions/misnamed_getters.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/functions/misnamed_getters.rs b/clippy_lints/src/functions/misnamed_getters.rs index 599269e2d..c55390105 100644 --- a/clippy_lints/src/functions/misnamed_getters.rs +++ b/clippy_lints/src/functions/misnamed_getters.rs @@ -101,16 +101,14 @@ pub fn check_fn( } let Some(used_field) = used_field else { - if cfg!(debug_assertions) { - panic!("Struct doesn't contain the correct field"); - } else { - // Don't ICE when possible - return; - } - }; + // FIXME: This can be reached if the field access uses autoderef. + // `dec.all_fields()` should be replaced by something that uses autoderef. + return; + }; + let Some(correct_field) = correct_field else { 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); From 6178ddaded7f73ba413475ea27ac98336069cb11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sun, 20 Nov 2022 13:46:30 +0100 Subject: [PATCH 12/14] misname-getters: Fix documentation --- clippy_lints/src/functions/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index 3478fdd86..91e6ffe64 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -270,6 +270,7 @@ declare_clippy_lint! { /// 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, @@ -281,7 +282,7 @@ declare_clippy_lint! { /// &self.b /// } /// } - /// // example code where clippy issues a warning + /// ``` /// Use instead: /// ```rust @@ -296,7 +297,7 @@ declare_clippy_lint! { /// } /// } /// ``` - #[clippy::version = "1.66.0"] + #[clippy::version = "1.67.0"] pub MISNAMED_GETTERS, suspicious, "getter method returning the wrong field" From 0411edfbbda82407c956b69db5bf687f8749766e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sun, 20 Nov 2022 15:34:56 +0100 Subject: [PATCH 13/14] Improve diagnostic for cases where autoderef is used --- .../src/functions/misnamed_getters.rs | 4 ++-- tests/ui/misnamed_getters.stderr | 20 ++++++++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/functions/misnamed_getters.rs b/clippy_lints/src/functions/misnamed_getters.rs index c55390105..5424c0eec 100644 --- a/clippy_lints/src/functions/misnamed_getters.rs +++ b/clippy_lints/src/functions/misnamed_getters.rs @@ -75,7 +75,7 @@ pub fn check_fn( } }; - let ty = cx.typeck_results().expr_ty(self_data); + let ty = cx.typeck_results().expr_ty_adjusted(self_data); let def = { let mut kind = ty.kind(); @@ -102,7 +102,7 @@ pub fn check_fn( let Some(used_field) = used_field else { // FIXME: This can be reached if the field access uses autoderef. - // `dec.all_fields()` should be replaced by something that uses autoderef. + // `dec.all_fields()` should be replaced by something that uses autoderef on the unajusted type of `self_data` return; }; diff --git a/tests/ui/misnamed_getters.stderr b/tests/ui/misnamed_getters.stderr index feefb95ab..521029589 100644 --- a/tests/ui/misnamed_getters.stderr +++ b/tests/ui/misnamed_getters.stderr @@ -126,5 +126,23 @@ LL | | &mut self.a LL | | } | |_____^ -error: aborting due to 14 previous errors +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:92:5 + | +LL | / unsafe fn a(&self) -> &u8 { +LL | | &self.inner.b + | | ------------- help: consider using: `&self.inner.a` +LL | | } + | |_____^ + +error: getter function appears to return the wrong field + --> $DIR/misnamed_getters.rs:95:5 + | +LL | / unsafe fn a_mut(&mut self) -> &mut u8 { +LL | | &mut self.inner.b + | | ----------------- help: consider using: `&mut self.inner.a` +LL | | } + | |_____^ + +error: aborting due to 16 previous errors From 1f2f50c34eb304ce56213dad70ccdeb2f2901512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sun, 20 Nov 2022 17:03:53 +0100 Subject: [PATCH 14/14] Fix many false negatives caused by autoderef --- .../src/functions/misnamed_getters.rs | 44 +++++++++---------- tests/ui/misnamed_getters.rs | 36 ++++++++++++--- tests/ui/misnamed_getters.stderr | 36 +++++++++++---- 3 files changed, 77 insertions(+), 39 deletions(-) diff --git a/clippy_lints/src/functions/misnamed_getters.rs b/clippy_lints/src/functions/misnamed_getters.rs index 5424c0eec..27acad45c 100644 --- a/clippy_lints/src/functions/misnamed_getters.rs +++ b/clippy_lints/src/functions/misnamed_getters.rs @@ -6,6 +6,8 @@ use rustc_lint::LateContext; use rustc_middle::ty; use rustc_span::Span; +use std::iter; + use super::MISNAMED_GETTERS; pub fn check_fn( @@ -75,39 +77,35 @@ pub fn check_fn( } }; - let ty = cx.typeck_results().expr_ty_adjusted(self_data); - - let def = { - let mut kind = ty.kind(); - loop { - match kind { - ty::Adt(def, _) => break def, - ty::Ref(_, ty, _) => kind = ty.kind(), - // We don't do tuples because the function name cannot be a number - _ => return, - } - } - }; - let mut used_field = None; let mut correct_field = None; - 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 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 { - // FIXME: This can be reached if the field access uses autoderef. - // `dec.all_fields()` should be replaced by something that uses autoderef on the unajusted type of `self_data` + // 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 { - return; + // 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) { diff --git a/tests/ui/misnamed_getters.rs b/tests/ui/misnamed_getters.rs index 2c451bd3a..03e7dac7d 100644 --- a/tests/ui/misnamed_getters.rs +++ b/tests/ui/misnamed_getters.rs @@ -85,15 +85,37 @@ impl B { } } -struct C { - inner: Box, +struct D { + d: u8, + inner: A, } -impl C { - unsafe fn a(&self) -> &u8 { - &self.inner.b + +impl core::ops::Deref for D { + type Target = A; + fn deref(&self) -> &A { + &self.inner } - unsafe fn a_mut(&mut self) -> &mut u8 { - &mut self.inner.b +} + +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 } } diff --git a/tests/ui/misnamed_getters.stderr b/tests/ui/misnamed_getters.stderr index 521029589..1e38a83d0 100644 --- a/tests/ui/misnamed_getters.stderr +++ b/tests/ui/misnamed_getters.stderr @@ -127,22 +127,40 @@ LL | | } | |_____^ error: getter function appears to return the wrong field - --> $DIR/misnamed_getters.rs:92:5 + --> $DIR/misnamed_getters.rs:107:5 | -LL | / unsafe fn a(&self) -> &u8 { -LL | | &self.inner.b - | | ------------- help: consider using: `&self.inner.a` +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:95:5 + --> $DIR/misnamed_getters.rs:110:5 | -LL | / unsafe fn a_mut(&mut self) -> &mut u8 { -LL | | &mut self.inner.b - | | ----------------- help: consider using: `&mut self.inner.a` +LL | / fn a_mut(&mut self) -> &mut u8 { +LL | | &mut self.b + | | ----------- help: consider using: `&mut self.a` LL | | } | |_____^ -error: aborting due to 16 previous errors +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