mirror of
https://github.com/rust-lang/rust.git
synced 2026-05-28 20:16:58 +03:00
overhaul mut_mut (#15417)
This PR: - adds structured suggestions to all 3 lint scenarios - adds more examples of linted patterns, and recommended fixes - moves some test cases to `_unfixable.rs`, to allow running rustfix on the main file - stops the lint from firing multiple times on types like `&mut &mut &mut T` Open questions: - I'd like to add a note explaining why chained `&mut` are useless.. but can't really come up with something consice. I very much don't like the one in the docs -- it's a bit too condescending imo. - see comments in the diff for more I do realize that the PR ended up being quite wide-scoped, but that's primarily because once I added structured suggestions to one of the lint cases, `ui_test` started complaining about warnings remaining after the rustfix run, which of course had to with the fact that the other two lint cases didn't actually fix the code they linted, only warned. I _guess_ `ui_test` could be smarter about this, by understanding that if a warning was created without a suggestion, then that warning will still remain in the fixed file. But oh well. changelog: [`mut_mut`]: add structured suggestions, improve docs fixes rust-lang/rust-clippy#13021
This commit is contained in:
Alejandra González
@@ -482,7 +482,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
|
||||
store.register_late_pass(|_| Box::new(needless_for_each::NeedlessForEach));
|
||||
store.register_late_pass(|_| Box::new(misc::LintPass));
|
||||
store.register_late_pass(|_| Box::new(eta_reduction::EtaReduction));
|
||||
store.register_late_pass(|_| Box::new(mut_mut::MutMut));
|
||||
store.register_late_pass(|_| Box::new(mut_mut::MutMut::default()));
|
||||
store.register_late_pass(|_| Box::new(unnecessary_mut_passed::UnnecessaryMutPassed));
|
||||
store.register_late_pass(|_| Box::<significant_drop_tightening::SignificantDropTightening<'_>>::default());
|
||||
store.register_late_pass(|_| Box::new(len_zero::LenZero));
|
||||
|
||||
+111
-24
@@ -1,31 +1,51 @@
|
||||
use clippy_utils::diagnostics::{span_lint, span_lint_hir};
|
||||
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
|
||||
use clippy_utils::higher;
|
||||
use rustc_hir::{self as hir, AmbigArg, intravisit};
|
||||
use clippy_utils::source::snippet_with_applicability;
|
||||
use clippy_utils::sugg::Sugg;
|
||||
use rustc_data_structures::fx::FxHashSet;
|
||||
use rustc_errors::Applicability;
|
||||
use rustc_hir::{self as hir, AmbigArg, BorrowKind, Expr, ExprKind, HirId, Mutability, TyKind, intravisit};
|
||||
use rustc_lint::{LateContext, LateLintPass, LintContext};
|
||||
use rustc_middle::ty;
|
||||
use rustc_session::declare_lint_pass;
|
||||
use rustc_session::impl_lint_pass;
|
||||
|
||||
declare_clippy_lint! {
|
||||
/// ### What it does
|
||||
/// Checks for instances of `mut mut` references.
|
||||
///
|
||||
/// ### Why is this bad?
|
||||
/// Multiple `mut`s don't add anything meaningful to the
|
||||
/// source. This is either a copy'n'paste error, or it shows a fundamental
|
||||
/// misunderstanding of references.
|
||||
/// This is usually just a typo or a misunderstanding of how references work.
|
||||
///
|
||||
/// ### Example
|
||||
/// ```no_run
|
||||
/// # let mut y = 1;
|
||||
/// let x = &mut &mut y;
|
||||
/// let x = &mut &mut 1;
|
||||
///
|
||||
/// let mut x = &mut 1;
|
||||
/// let y = &mut x;
|
||||
///
|
||||
/// fn foo(x: &mut &mut u32) {}
|
||||
/// ```
|
||||
/// Use instead
|
||||
/// ```no_run
|
||||
/// let x = &mut 1;
|
||||
///
|
||||
/// let mut x = &mut 1;
|
||||
/// let y = &mut *x; // reborrow
|
||||
///
|
||||
/// fn foo(x: &mut u32) {}
|
||||
/// ```
|
||||
#[clippy::version = "pre 1.29.0"]
|
||||
pub MUT_MUT,
|
||||
pedantic,
|
||||
"usage of double-mut refs, e.g., `&mut &mut ...`"
|
||||
"usage of double mut-refs, e.g., `&mut &mut ...`"
|
||||
}
|
||||
|
||||
declare_lint_pass!(MutMut => [MUT_MUT]);
|
||||
impl_lint_pass!(MutMut => [MUT_MUT]);
|
||||
|
||||
#[derive(Default)]
|
||||
pub(crate) struct MutMut {
|
||||
seen_tys: FxHashSet<HirId>,
|
||||
}
|
||||
|
||||
impl<'tcx> LateLintPass<'tcx> for MutMut {
|
||||
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
|
||||
@@ -33,17 +53,48 @@ fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
|
||||
}
|
||||
|
||||
fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx hir::Ty<'_, AmbigArg>) {
|
||||
if let hir::TyKind::Ref(_, mty) = ty.kind
|
||||
&& mty.mutbl == hir::Mutability::Mut
|
||||
&& let hir::TyKind::Ref(_, mty) = mty.ty.kind
|
||||
&& mty.mutbl == hir::Mutability::Mut
|
||||
if let TyKind::Ref(_, mty) = ty.kind
|
||||
&& mty.mutbl == Mutability::Mut
|
||||
&& let TyKind::Ref(_, mty2) = mty.ty.kind
|
||||
&& mty2.mutbl == Mutability::Mut
|
||||
&& !ty.span.in_external_macro(cx.sess().source_map())
|
||||
{
|
||||
span_lint(
|
||||
if self.seen_tys.contains(&ty.hir_id) {
|
||||
// we have 2+ `&mut`s, e.g., `&mut &mut &mut x`
|
||||
// and we have already flagged on the outermost `&mut &mut (&mut x)`,
|
||||
// so don't flag the inner `&mut &mut (x)`
|
||||
return;
|
||||
}
|
||||
|
||||
// if there is an even longer chain, like `&mut &mut &mut x`, suggest peeling off
|
||||
// all extra ones at once
|
||||
let (mut t, mut t2) = (mty.ty, mty2.ty);
|
||||
let mut many_muts = false;
|
||||
loop {
|
||||
// this should allow us to remember all the nested types, so that the `contains`
|
||||
// above fails faster
|
||||
self.seen_tys.insert(t.hir_id);
|
||||
if let TyKind::Ref(_, next) = t2.kind
|
||||
&& next.mutbl == Mutability::Mut
|
||||
{
|
||||
(t, t2) = (t2, next.ty);
|
||||
many_muts = true;
|
||||
} else {
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
let mut applicability = Applicability::MaybeIncorrect;
|
||||
let sugg = snippet_with_applicability(cx.sess(), t.span, "..", &mut applicability);
|
||||
let suffix = if many_muts { "s" } else { "" };
|
||||
span_lint_and_sugg(
|
||||
cx,
|
||||
MUT_MUT,
|
||||
ty.span,
|
||||
"generally you want to avoid `&mut &mut _` if possible",
|
||||
"a type of form `&mut &mut _`",
|
||||
format!("remove the extra `&mut`{suffix}"),
|
||||
sugg.to_string(),
|
||||
applicability,
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -54,7 +105,7 @@ pub struct MutVisitor<'a, 'tcx> {
|
||||
}
|
||||
|
||||
impl<'tcx> intravisit::Visitor<'tcx> for MutVisitor<'_, 'tcx> {
|
||||
fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) {
|
||||
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
|
||||
if expr.span.in_external_macro(self.cx.sess().source_map()) {
|
||||
return;
|
||||
}
|
||||
@@ -68,24 +119,60 @@ fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) {
|
||||
// Let's ignore the generated code.
|
||||
intravisit::walk_expr(self, arg);
|
||||
intravisit::walk_expr(self, body);
|
||||
} else if let hir::ExprKind::AddrOf(hir::BorrowKind::Ref, hir::Mutability::Mut, e) = expr.kind {
|
||||
if let hir::ExprKind::AddrOf(hir::BorrowKind::Ref, hir::Mutability::Mut, _) = e.kind {
|
||||
span_lint_hir(
|
||||
} else if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, e) = expr.kind {
|
||||
if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, e2) = e.kind {
|
||||
if !expr.span.eq_ctxt(e.span) {
|
||||
return;
|
||||
}
|
||||
|
||||
// if there is an even longer chain, like `&mut &mut &mut x`, suggest peeling off
|
||||
// all extra ones at once
|
||||
let (mut e, mut e2) = (e, e2);
|
||||
let mut many_muts = false;
|
||||
loop {
|
||||
if !e.span.eq_ctxt(e2.span) {
|
||||
return;
|
||||
}
|
||||
if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, next) = e2.kind {
|
||||
(e, e2) = (e2, next);
|
||||
many_muts = true;
|
||||
} else {
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
let mut applicability = Applicability::MaybeIncorrect;
|
||||
let sugg = Sugg::hir_with_applicability(self.cx, e, "..", &mut applicability);
|
||||
let suffix = if many_muts { "s" } else { "" };
|
||||
span_lint_hir_and_then(
|
||||
self.cx,
|
||||
MUT_MUT,
|
||||
expr.hir_id,
|
||||
expr.span,
|
||||
"generally you want to avoid `&mut &mut _` if possible",
|
||||
"an expression of form `&mut &mut _`",
|
||||
|diag| {
|
||||
diag.span_suggestion(
|
||||
expr.span,
|
||||
format!("remove the extra `&mut`{suffix}"),
|
||||
sugg,
|
||||
applicability,
|
||||
);
|
||||
},
|
||||
);
|
||||
} else if let ty::Ref(_, ty, hir::Mutability::Mut) = self.cx.typeck_results().expr_ty(e).kind()
|
||||
} else if let ty::Ref(_, ty, Mutability::Mut) = self.cx.typeck_results().expr_ty(e).kind()
|
||||
&& ty.peel_refs().is_sized(self.cx.tcx, self.cx.typing_env())
|
||||
{
|
||||
span_lint_hir(
|
||||
let mut applicability = Applicability::MaybeIncorrect;
|
||||
let sugg = Sugg::hir_with_applicability(self.cx, e, "..", &mut applicability).mut_addr_deref();
|
||||
span_lint_hir_and_then(
|
||||
self.cx,
|
||||
MUT_MUT,
|
||||
expr.hir_id,
|
||||
expr.span,
|
||||
"this expression mutably borrows a mutable reference. Consider reborrowing",
|
||||
"this expression mutably borrows a mutable reference",
|
||||
|diag| {
|
||||
diag.span_suggestion(expr.span, "reborrow instead", sugg, applicability);
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,92 @@
|
||||
//@aux-build:proc_macros.rs
|
||||
|
||||
#![warn(clippy::mut_mut)]
|
||||
#![allow(unused)]
|
||||
#![allow(
|
||||
clippy::no_effect,
|
||||
clippy::uninlined_format_args,
|
||||
clippy::unnecessary_operation,
|
||||
clippy::needless_pass_by_ref_mut
|
||||
)]
|
||||
|
||||
extern crate proc_macros;
|
||||
use proc_macros::{external, inline_macros};
|
||||
|
||||
fn fun(x: &mut u32) {
|
||||
//~^ mut_mut
|
||||
}
|
||||
|
||||
fn less_fun(x: *mut *mut u32) {
|
||||
let y = x;
|
||||
}
|
||||
|
||||
macro_rules! mut_ptr {
|
||||
($p:expr) => {
|
||||
&mut $p
|
||||
};
|
||||
}
|
||||
|
||||
#[allow(unused_mut, unused_variables)]
|
||||
#[inline_macros]
|
||||
fn main() {
|
||||
let mut x = &mut 1u32;
|
||||
//~^ mut_mut
|
||||
{
|
||||
let mut y = &mut *x;
|
||||
//~^ mut_mut
|
||||
}
|
||||
|
||||
{
|
||||
let y: &mut u32 = &mut 2;
|
||||
//~^ mut_mut
|
||||
//~| mut_mut
|
||||
}
|
||||
|
||||
{
|
||||
let y: &mut u32 = &mut 2;
|
||||
//~^ mut_mut
|
||||
//~| mut_mut
|
||||
}
|
||||
|
||||
let mut z = inline!(&mut $(&mut 3u32));
|
||||
}
|
||||
|
||||
fn issue939() {
|
||||
let array = [5, 6, 7, 8, 9];
|
||||
let mut args = array.iter().skip(2);
|
||||
for &arg in &mut args {
|
||||
println!("{}", arg);
|
||||
}
|
||||
|
||||
let args = &mut args;
|
||||
for arg in args {
|
||||
println!(":{}", arg);
|
||||
}
|
||||
}
|
||||
|
||||
fn issue6922() {
|
||||
// do not lint from an external macro
|
||||
external!(let mut_mut_ty: &mut &mut u32 = &mut &mut 1u32;);
|
||||
}
|
||||
|
||||
mod issue9035 {
|
||||
use std::fmt::Display;
|
||||
|
||||
struct Foo<'a> {
|
||||
inner: &'a mut dyn Display,
|
||||
}
|
||||
|
||||
impl Foo<'_> {
|
||||
fn foo(&mut self) {
|
||||
let hlp = &mut self.inner;
|
||||
bar(hlp);
|
||||
}
|
||||
}
|
||||
|
||||
fn bar(_: &mut impl Display) {}
|
||||
}
|
||||
|
||||
fn allow_works() {
|
||||
#[allow(clippy::mut_mut)]
|
||||
let _ = &mut &mut 1;
|
||||
}
|
||||
+3
-8
@@ -12,9 +12,8 @@
|
||||
extern crate proc_macros;
|
||||
use proc_macros::{external, inline_macros};
|
||||
|
||||
fn fun(x: &mut &mut u32) -> bool {
|
||||
fn fun(x: &mut &mut u32) {
|
||||
//~^ mut_mut
|
||||
**x > 0
|
||||
}
|
||||
|
||||
fn less_fun(x: *mut *mut u32) {
|
||||
@@ -37,23 +36,19 @@ fn main() {
|
||||
//~^ mut_mut
|
||||
}
|
||||
|
||||
if fun(x) {
|
||||
{
|
||||
let y: &mut &mut u32 = &mut &mut 2;
|
||||
//~^ mut_mut
|
||||
//~| mut_mut
|
||||
**y + **x;
|
||||
}
|
||||
|
||||
if fun(x) {
|
||||
{
|
||||
let y: &mut &mut &mut u32 = &mut &mut &mut 2;
|
||||
//~^ mut_mut
|
||||
//~| mut_mut
|
||||
//~| mut_mut
|
||||
***y + **x;
|
||||
}
|
||||
|
||||
let mut z = inline!(&mut $(&mut 3u32));
|
||||
//~^ mut_mut
|
||||
}
|
||||
|
||||
fn issue939() {
|
||||
|
||||
+22
-36
@@ -1,61 +1,47 @@
|
||||
error: generally you want to avoid `&mut &mut _` if possible
|
||||
error: a type of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut.rs:15:11
|
||||
|
|
||||
LL | fn fun(x: &mut &mut u32) -> bool {
|
||||
| ^^^^^^^^^^^^^
|
||||
LL | fn fun(x: &mut &mut u32) {
|
||||
| ^^^^^^^^^^^^^ help: remove the extra `&mut`: `&mut u32`
|
||||
|
|
||||
= note: `-D clippy::mut-mut` implied by `-D warnings`
|
||||
= help: to override `-D warnings` add `#[allow(clippy::mut_mut)]`
|
||||
|
||||
error: generally you want to avoid `&mut &mut _` if possible
|
||||
--> tests/ui/mut_mut.rs:33:17
|
||||
error: an expression of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut.rs:32:17
|
||||
|
|
||||
LL | let mut x = &mut &mut 1u32;
|
||||
| ^^^^^^^^^^^^^^
|
||||
| ^^^^^^^^^^^^^^ help: remove the extra `&mut`: `&mut 1u32`
|
||||
|
||||
error: generally you want to avoid `&mut &mut _` if possible
|
||||
--> tests/ui/mut_mut.rs:55:25
|
||||
|
|
||||
LL | let mut z = inline!(&mut $(&mut 3u32));
|
||||
| ^
|
||||
|
|
||||
= note: this error originates in the macro `__inline_mac_fn_main` (in Nightly builds, run with -Z macro-backtrace for more info)
|
||||
|
||||
error: this expression mutably borrows a mutable reference. Consider reborrowing
|
||||
--> tests/ui/mut_mut.rs:36:21
|
||||
error: this expression mutably borrows a mutable reference
|
||||
--> tests/ui/mut_mut.rs:35:21
|
||||
|
|
||||
LL | let mut y = &mut x;
|
||||
| ^^^^^^
|
||||
| ^^^^^^ help: reborrow instead: `&mut *x`
|
||||
|
||||
error: generally you want to avoid `&mut &mut _` if possible
|
||||
--> tests/ui/mut_mut.rs:41:32
|
||||
error: an expression of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut.rs:40:32
|
||||
|
|
||||
LL | let y: &mut &mut u32 = &mut &mut 2;
|
||||
| ^^^^^^^^^^^
|
||||
| ^^^^^^^^^^^ help: remove the extra `&mut`: `&mut 2`
|
||||
|
||||
error: generally you want to avoid `&mut &mut _` if possible
|
||||
--> tests/ui/mut_mut.rs:41:16
|
||||
error: a type of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut.rs:40:16
|
||||
|
|
||||
LL | let y: &mut &mut u32 = &mut &mut 2;
|
||||
| ^^^^^^^^^^^^^
|
||||
| ^^^^^^^^^^^^^ help: remove the extra `&mut`: `&mut u32`
|
||||
|
||||
error: generally you want to avoid `&mut &mut _` if possible
|
||||
--> tests/ui/mut_mut.rs:48:37
|
||||
error: an expression of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut.rs:46:37
|
||||
|
|
||||
LL | let y: &mut &mut &mut u32 = &mut &mut &mut 2;
|
||||
| ^^^^^^^^^^^^^^^^
|
||||
| ^^^^^^^^^^^^^^^^ help: remove the extra `&mut`s: `&mut 2`
|
||||
|
||||
error: generally you want to avoid `&mut &mut _` if possible
|
||||
--> tests/ui/mut_mut.rs:48:16
|
||||
error: a type of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut.rs:46:16
|
||||
|
|
||||
LL | let y: &mut &mut &mut u32 = &mut &mut &mut 2;
|
||||
| ^^^^^^^^^^^^^^^^^^
|
||||
| ^^^^^^^^^^^^^^^^^^ help: remove the extra `&mut`s: `&mut u32`
|
||||
|
||||
error: generally you want to avoid `&mut &mut _` if possible
|
||||
--> tests/ui/mut_mut.rs:48:21
|
||||
|
|
||||
LL | let y: &mut &mut &mut u32 = &mut &mut &mut 2;
|
||||
| ^^^^^^^^^^^^^
|
||||
|
||||
error: aborting due to 9 previous errors
|
||||
error: aborting due to 7 previous errors
|
||||
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
//@no-rustfix
|
||||
|
||||
#![warn(clippy::mut_mut)]
|
||||
#![allow(unused)]
|
||||
#![expect(clippy::no_effect)]
|
||||
|
||||
//! removing the extra `&mut`s will break the derefs
|
||||
|
||||
fn fun(x: &mut &mut u32) -> bool {
|
||||
//~^ mut_mut
|
||||
**x > 0
|
||||
}
|
||||
|
||||
fn main() {
|
||||
let mut x = &mut &mut 1u32;
|
||||
//~^ mut_mut
|
||||
{
|
||||
let mut y = &mut x;
|
||||
//~^ mut_mut
|
||||
***y + **x;
|
||||
}
|
||||
|
||||
if fun(x) {
|
||||
let y = &mut &mut 2;
|
||||
//~^ mut_mut
|
||||
**y + **x;
|
||||
}
|
||||
|
||||
if fun(x) {
|
||||
let y = &mut &mut &mut 2;
|
||||
//~^ mut_mut
|
||||
***y + **x;
|
||||
}
|
||||
|
||||
if fun(x) {
|
||||
// The lint will remove the extra `&mut`, but the result will still be a `&mut` of an expr
|
||||
// of type `&mut _` (x), so the lint will fire again. That's because we've decided that
|
||||
// doing both fixes in one run is not worth it, given how improbable code like this is.
|
||||
let y = &mut &mut x;
|
||||
//~^ mut_mut
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,41 @@
|
||||
error: a type of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut_unfixable.rs:9:11
|
||||
|
|
||||
LL | fn fun(x: &mut &mut u32) -> bool {
|
||||
| ^^^^^^^^^^^^^ help: remove the extra `&mut`: `&mut u32`
|
||||
|
|
||||
= note: `-D clippy::mut-mut` implied by `-D warnings`
|
||||
= help: to override `-D warnings` add `#[allow(clippy::mut_mut)]`
|
||||
|
||||
error: an expression of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut_unfixable.rs:15:17
|
||||
|
|
||||
LL | let mut x = &mut &mut 1u32;
|
||||
| ^^^^^^^^^^^^^^ help: remove the extra `&mut`: `&mut 1u32`
|
||||
|
||||
error: this expression mutably borrows a mutable reference
|
||||
--> tests/ui/mut_mut_unfixable.rs:18:21
|
||||
|
|
||||
LL | let mut y = &mut x;
|
||||
| ^^^^^^ help: reborrow instead: `&mut *x`
|
||||
|
||||
error: an expression of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut_unfixable.rs:24:17
|
||||
|
|
||||
LL | let y = &mut &mut 2;
|
||||
| ^^^^^^^^^^^ help: remove the extra `&mut`: `&mut 2`
|
||||
|
||||
error: an expression of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut_unfixable.rs:30:17
|
||||
|
|
||||
LL | let y = &mut &mut &mut 2;
|
||||
| ^^^^^^^^^^^^^^^^ help: remove the extra `&mut`s: `&mut 2`
|
||||
|
||||
error: an expression of form `&mut &mut _`
|
||||
--> tests/ui/mut_mut_unfixable.rs:39:17
|
||||
|
|
||||
LL | let y = &mut &mut x;
|
||||
| ^^^^^^^^^^^ help: remove the extra `&mut`: `&mut x`
|
||||
|
||||
error: aborting due to 6 previous errors
|
||||
|
||||
Reference in New Issue
Block a user