Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-27 23:20:39 +00:00
Code Issues Projects Releases Packages Wiki Activity

Auto merge of #9770 - sgued:missnamed-getters, r=llogiq

Browse source 
Add new lint  [`misnamed-getters`]

```
changelog: Add new lint  [`misnamed-getters`]
```

Closes #9769

The current lint matches all methods with a body of just one expression under the form `(&mut?)? <expr>.field` where field doesn't match the name of the method but there is a field of the same type in `<expr>` that matches the name. This allows matching nested structs, for example for newtype wrappers. This may cast the net a bit too wide and cause false positives. I'll run [clippy_lint_tester](https://github.com/mikerite/clippy_lint_tester) on the top crates to see how frequently false positives happen.

There also may be room for improvement by checking that the replacement field would work taking into account implementations of `Deref` and `DerefMut` even if the types don't exactly match but I don't know yet how this could be done.
This commit is contained in:
bors 2022-11-21 19:51:42 +00:00
commit 73efce9ee6
6 changed files with 462 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG.md
View file

@ -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

1
clippy_lints/src/declared_lints.rs
View file

@ -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,

125
clippy_lints/src/functions/misnamed_getters.rs Normal file
View file

@ -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) <self_data>.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 &<expr>, &mut <expr> and <expr>
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);
},
);
}
}

45
clippy_lints/src/functions/mod.rs
View file

@ -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<'_>) {

124
tests/ui/misnamed_getters.rs Normal file
View file

@ -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
}

166
tests/ui/misnamed_getters.stderr Normal file
View file

@ -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