From 9615e7667ec31af62029bb7d6de1ec5451777386 Mon Sep 17 00:00:00 2001 From: Asuna Date: Sun, 27 Oct 2024 02:35:03 +0100 Subject: [PATCH] Fix `manual_inspect` to consider mutability --- clippy_lints/src/dereference.rs | 8 +- clippy_lints/src/methods/manual_inspect.rs | 65 +++++++-- clippy_utils/src/lib.rs | 5 +- tests/ui/manual_inspect.fixed | 149 ++++++++++++++++++- tests/ui/manual_inspect.rs | 157 ++++++++++++++++++++- tests/ui/manual_inspect.stderr | 125 ++++++++++++++-- 6 files changed, 477 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 653726872c64..67e2d3af3c2f 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -270,7 +270,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { RefOp::Deref if use_cx.same_ctxt => { let use_node = use_cx.use_node(cx); let sub_ty = typeck.expr_ty(sub_expr); - if let ExprUseNode::FieldAccess(name) = use_node + if let ExprUseNode::FieldAccess(_, name) = use_node && !use_cx.moved_before_use && !ty_contains_field(sub_ty, name.name) { @@ -339,7 +339,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { TyCoercionStability::for_defined_ty(cx, ty, use_node.is_return()) }); let can_auto_borrow = match use_node { - ExprUseNode::FieldAccess(_) + ExprUseNode::FieldAccess(_, _) if !use_cx.moved_before_use && matches!(sub_expr.kind, ExprKind::Field(..)) => { // `DerefMut` will not be automatically applied to `ManuallyDrop<_>` @@ -350,7 +350,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { // deref through `ManuallyDrop<_>` will not compile. !adjust_derefs_manually_drop(use_cx.adjustments, expr_ty) }, - ExprUseNode::Callee | ExprUseNode::FieldAccess(_) if !use_cx.moved_before_use => true, + ExprUseNode::Callee | ExprUseNode::FieldAccess(_, _) if !use_cx.moved_before_use => true, ExprUseNode::MethodArg(hir_id, _, 0) if !use_cx.moved_before_use => { // Check for calls to trait methods where the trait is implemented // on a reference. @@ -438,7 +438,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { count: deref_count - required_refs, msg, stability, - for_field_access: if let ExprUseNode::FieldAccess(name) = use_node + for_field_access: if let ExprUseNode::FieldAccess(_, name) = use_node && !use_cx.moved_before_use { Some(name.name) diff --git a/clippy_lints/src/methods/manual_inspect.rs b/clippy_lints/src/methods/manual_inspect.rs index 20e4d233525a..7ec2b1b672fb 100644 --- a/clippy_lints/src/methods/manual_inspect.rs +++ b/clippy_lints/src/methods/manual_inspect.rs @@ -6,7 +6,7 @@ use clippy_utils::visitors::{for_each_expr, for_each_expr_without_closures}; use clippy_utils::{ExprUseNode, expr_use_ctxt, is_diag_item_method, is_diag_trait_item, path_to_local_id}; use core::ops::ControlFlow; use rustc_errors::Applicability; -use rustc_hir::{BindingMode, BorrowKind, ByRef, ClosureKind, Expr, ExprKind, Mutability, Node, PatKind}; +use rustc_hir::{BindingMode, BorrowKind, ByRef, ClosureKind, Expr, ExprKind, HirId, Mutability, Node, PatKind}; use rustc_lint::LateContext; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; use rustc_span::{DUMMY_SP, Span, Symbol, sym}; @@ -34,6 +34,7 @@ pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name: { let mut requires_copy = false; let mut requires_deref = false; + let mut has_mut_use = false; // The number of unprocessed return expressions. let mut ret_count = 0u32; @@ -47,7 +48,8 @@ pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name: // Nested closures don't need to treat returns specially. let _: Option = for_each_expr(cx, cx.tcx.hir().body(c.body).value, |e| { if path_to_local_id(e, arg_id) { - let (kind, same_ctxt) = check_use(cx, e); + let (kind, mutbl, same_ctxt) = check_use(cx, e); + has_mut_use |= mutbl.is_mut(); match (kind, same_ctxt && e.span.ctxt() == ctxt) { (_, false) | (UseKind::Deref | UseKind::Return(..), true) => { requires_copy = true; @@ -65,7 +67,8 @@ pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name: } else if matches!(e.kind, ExprKind::Ret(_)) { ret_count += 1; } else if path_to_local_id(e, arg_id) { - let (kind, same_ctxt) = check_use(cx, e); + let (kind, mutbl, same_ctxt) = check_use(cx, e); + has_mut_use |= mutbl.is_mut(); match (kind, same_ctxt && e.span.ctxt() == ctxt) { (UseKind::Return(..), false) => { return ControlFlow::Break(()); @@ -161,6 +164,7 @@ pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name: && (!requires_copy || cx.type_is_copy_modulo_regions(arg_ty)) // This case could be handled, but a fair bit of care would need to be taken. && (!requires_deref || arg_ty.is_freeze(cx.tcx, cx.typing_env())) + && !has_mut_use { if requires_deref { edits.push((param.span.shrink_to_lo(), "&".into())); @@ -207,30 +211,31 @@ enum UseKind<'tcx> { FieldAccess(Symbol, &'tcx Expr<'tcx>), } -/// Checks how the value is used, and whether it was used in the same `SyntaxContext`. -fn check_use<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (UseKind<'tcx>, bool) { +/// Checks how the value is used, mutability, and whether it was used in the same `SyntaxContext`. +fn check_use<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (UseKind<'tcx>, Mutability, bool) { let use_cx = expr_use_ctxt(cx, e); + let mutbl = use_mutability(cx, e.hir_id); if use_cx .adjustments .first() .is_some_and(|a| matches!(a.kind, Adjust::Deref(_))) { - return (UseKind::AutoBorrowed, use_cx.same_ctxt); + return (UseKind::AutoBorrowed, mutbl, use_cx.same_ctxt); } let res = match use_cx.use_node(cx) { ExprUseNode::Return(_) => { if let ExprKind::Ret(Some(e)) = use_cx.node.expect_expr().kind { UseKind::Return(e.span) } else { - return (UseKind::Return(DUMMY_SP), false); + return (UseKind::Return(DUMMY_SP), mutbl, false); } }, - ExprUseNode::FieldAccess(name) => UseKind::FieldAccess(name.name, use_cx.node.expect_expr()), + ExprUseNode::FieldAccess(_, name) => UseKind::FieldAccess(name.name, use_cx.node.expect_expr()), ExprUseNode::Callee | ExprUseNode::MethodArg(_, _, 0) if use_cx .adjustments .first() - .is_some_and(|a| matches!(a.kind, Adjust::Borrow(AutoBorrow::Ref(AutoBorrowMutability::Not)))) => + .is_some_and(|a| matches!(a.kind, Adjust::Borrow(AutoBorrow::Ref(_)))) => { UseKind::AutoBorrowed }, @@ -238,5 +243,45 @@ fn check_use<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (UseKind<'tcx>, ExprUseNode::AddrOf(BorrowKind::Ref, _) => UseKind::Borrowed(use_cx.node.expect_expr().span), _ => UseKind::Deref, }; - (res, use_cx.same_ctxt) + (res, mutbl, use_cx.same_ctxt) +} + +fn use_mutability<'tcx>(cx: &LateContext<'tcx>, expr_id: HirId) -> Mutability { + let adjusted = |expr: &Expr<'_>| -> Mutability { + let adj = cx.typeck_results().expr_adjustments(expr); + if let Some(Adjust::Borrow(AutoBorrow::Ref(mutbl))) = adj.last().map(|adj| &adj.kind) { + (*mutbl).into() + } else { + Mutability::Not + } + }; + + let mut last_child = None; + + for (_, node) in cx.tcx.hir().parent_iter(expr_id) { + match node { + Node::Expr(expr) => match expr.kind { + ExprKind::AddrOf(_, mutbl, _) => return mutbl, + ExprKind::MethodCall(_, self_arg, _, _) => return adjusted(self_arg), + ExprKind::Call(f, args) => return adjusted(args.iter().find(|arg| arg.hir_id == expr_id).unwrap_or(f)), + ExprKind::Field(field, _) | ExprKind::Index(field, _, _) => last_child = Some(field.hir_id), + ExprKind::Assign(lhs, _, _) | ExprKind::AssignOp(_, lhs, _) => { + let is_lhs = match lhs.kind { + ExprKind::Field(child, _) | ExprKind::Index(child, _, _) => { + last_child.is_some_and(|field_id| field_id == child.hir_id) + }, + _ => false, + }; + if is_lhs { + return Mutability::Mut; + } else { + return Mutability::Not; + } + }, + _ => {}, + }, + _ => {}, + } + } + Mutability::Not } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index a0de9a76a0ef..f038221c3d9b 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -2753,7 +2753,7 @@ impl<'tcx> ExprUseCtxt<'tcx> { .position(|arg| arg.hir_id == self.child_id) .map_or(0, |i| i + 1), ), - ExprKind::Field(_, name) => ExprUseNode::FieldAccess(name), + ExprKind::Field(_, name) => ExprUseNode::FieldAccess(use_expr.hir_id, name), ExprKind::AddrOf(kind, mutbl, _) => ExprUseNode::AddrOf(kind, mutbl), _ => ExprUseNode::Other, }, @@ -2763,6 +2763,7 @@ impl<'tcx> ExprUseCtxt<'tcx> { } /// The node which consumes a value. +#[derive(Debug)] pub enum ExprUseNode<'tcx> { /// Assignment to, or initializer for, a local LetStmt(&'tcx LetStmt<'tcx>), @@ -2779,7 +2780,7 @@ pub enum ExprUseNode<'tcx> { /// The callee of a function call. Callee, /// Access of a field. - FieldAccess(Ident), + FieldAccess(HirId, Ident), /// Borrow expression. AddrOf(ast::BorrowKind, Mutability), Other, diff --git a/tests/ui/manual_inspect.fixed b/tests/ui/manual_inspect.fixed index 0e1b8fe3edb5..88d0d9bd048b 100644 --- a/tests/ui/manual_inspect.fixed +++ b/tests/ui/manual_inspect.fixed @@ -1,5 +1,10 @@ #![warn(clippy::manual_inspect)] -#![allow(clippy::no_effect, clippy::op_ref)] +#![allow( + clippy::no_effect, + clippy::op_ref, + clippy::explicit_auto_deref, + clippy::needless_borrow +)] fn main() { let _ = Some(0).inspect(|&x| { @@ -172,3 +177,145 @@ fn main() { }); } } + +fn issue_13185() { + struct T(u32); + + impl T { + fn do_immut(&self) { + println!("meow~"); + } + + fn do_immut2(&self, other: &T) { + println!("meow~"); + } + + fn do_mut(&mut self) { + self.0 += 514; + } + + fn do_mut2(&mut self, other: &mut T) { + self.0 += 114; + other.0 += 514; + } + } + + _ = Some(T(114)).as_mut().inspect(|t| { + t.0 + 514; + }); + + _ = Some(T(114)).as_mut().map(|t| { + t.0 = 514; + t + }); + + _ = Some(T(114)).as_mut().map(|t| { + t.0 += 514; + t + }); + + // FIXME: It's better to lint this case + _ = Some(T(114)).as_mut().map(|t| { + let indirect = t; + indirect.0 + 514; + indirect + }); + + _ = Some(T(114)).as_mut().map(|t| { + let indirect = t; + indirect.0 += 514; + indirect + }); + + _ = Some(T(114)).as_mut().map(|t| { + t.do_mut(); + t + }); + + _ = Some(T(114)).as_mut().map(|t| { + T(514).do_mut2(t); + t + }); + + _ = Some(T(114)).as_mut().inspect(|t| { + t.do_immut(); + }); + + _ = Some(T(114)).as_mut().inspect(|t| { + t.do_immut2(t); + }); + + _ = Some(T(114)).as_mut().map(|t| { + T::do_mut(t); + t + }); + + _ = Some(T(114)).as_mut().inspect(|t| { + T::do_immut(t); + }); + + // FIXME: It's better to lint this case + _ = Some(T(114)).as_mut().map(|t| { + let indirect = t; + indirect.do_immut(); + indirect + }); + + // FIXME: It's better to lint this case + _ = Some(T(114)).as_mut().map(|t| { + (&*t).do_immut(); + t + }); + + // Array element access + + let mut sample = Some([1919, 810]); + _ = sample.as_mut().map(|t| { + t[1] += 1; + t + }); + + _ = sample.as_mut().inspect(|t| { + let mut a = 1; + a += t[1]; // immut + }); + + // Nested fields access + struct N((T, T), [T; 2]); + + let mut sample = Some(N((T(114), T(514)), [T(1919), T(810)])); + + _ = sample.as_mut().map(|n| { + n.0.0.do_mut(); + n + }); + + _ = sample.as_mut().inspect(|t| { + t.0.0.do_immut(); + }); + + _ = sample.as_mut().map(|t| { + T::do_mut(&mut t.0.0); + t + }); + + _ = sample.as_mut().inspect(|t| { + T::do_immut(&t.0.0); + }); + + // FnMut + let mut state = 1; + let immut_fn = Some(|| { + println!("meow"); + }); + let mut mut_fn = Some(|| { + state += 1; + }); + immut_fn.inspect(|f| { + f(); + }); + mut_fn.as_mut().map(|f| { + f(); + f + }); +} diff --git a/tests/ui/manual_inspect.rs b/tests/ui/manual_inspect.rs index 94cdfe391400..d2307b3db6da 100644 --- a/tests/ui/manual_inspect.rs +++ b/tests/ui/manual_inspect.rs @@ -1,5 +1,10 @@ #![warn(clippy::manual_inspect)] -#![allow(clippy::no_effect, clippy::op_ref)] +#![allow( + clippy::no_effect, + clippy::op_ref, + clippy::explicit_auto_deref, + clippy::needless_borrow +)] fn main() { let _ = Some(0).map(|x| { @@ -184,3 +189,153 @@ fn main() { }); } } + +fn issue_13185() { + struct T(u32); + + impl T { + fn do_immut(&self) { + println!("meow~"); + } + + fn do_immut2(&self, other: &T) { + println!("meow~"); + } + + fn do_mut(&mut self) { + self.0 += 514; + } + + fn do_mut2(&mut self, other: &mut T) { + self.0 += 114; + other.0 += 514; + } + } + + _ = Some(T(114)).as_mut().map(|t| { + t.0 + 514; + t + }); + + _ = Some(T(114)).as_mut().map(|t| { + t.0 = 514; + t + }); + + _ = Some(T(114)).as_mut().map(|t| { + t.0 += 514; + t + }); + + // FIXME: It's better to lint this case + _ = Some(T(114)).as_mut().map(|t| { + let indirect = t; + indirect.0 + 514; + indirect + }); + + _ = Some(T(114)).as_mut().map(|t| { + let indirect = t; + indirect.0 += 514; + indirect + }); + + _ = Some(T(114)).as_mut().map(|t| { + t.do_mut(); + t + }); + + _ = Some(T(114)).as_mut().map(|t| { + T(514).do_mut2(t); + t + }); + + _ = Some(T(114)).as_mut().map(|t| { + t.do_immut(); + t + }); + + _ = Some(T(114)).as_mut().map(|t| { + t.do_immut2(t); + t + }); + + _ = Some(T(114)).as_mut().map(|t| { + T::do_mut(t); + t + }); + + _ = Some(T(114)).as_mut().map(|t| { + T::do_immut(t); + t + }); + + // FIXME: It's better to lint this case + _ = Some(T(114)).as_mut().map(|t| { + let indirect = t; + indirect.do_immut(); + indirect + }); + + // FIXME: It's better to lint this case + _ = Some(T(114)).as_mut().map(|t| { + (&*t).do_immut(); + t + }); + + // Array element access + + let mut sample = Some([1919, 810]); + _ = sample.as_mut().map(|t| { + t[1] += 1; + t + }); + + _ = sample.as_mut().map(|t| { + let mut a = 1; + a += t[1]; // immut + t + }); + + // Nested fields access + struct N((T, T), [T; 2]); + + let mut sample = Some(N((T(114), T(514)), [T(1919), T(810)])); + + _ = sample.as_mut().map(|n| { + n.0.0.do_mut(); + n + }); + + _ = sample.as_mut().map(|t| { + t.0.0.do_immut(); + t + }); + + _ = sample.as_mut().map(|t| { + T::do_mut(&mut t.0.0); + t + }); + + _ = sample.as_mut().map(|t| { + T::do_immut(&t.0.0); + t + }); + + // FnMut + let mut state = 1; + let immut_fn = Some(|| { + println!("meow"); + }); + let mut mut_fn = Some(|| { + state += 1; + }); + immut_fn.map(|f| { + f(); + f + }); + mut_fn.as_mut().map(|f| { + f(); + f + }); +} diff --git a/tests/ui/manual_inspect.stderr b/tests/ui/manual_inspect.stderr index 0559b3bd6611..4b2edcdf5319 100644 --- a/tests/ui/manual_inspect.stderr +++ b/tests/ui/manual_inspect.stderr @@ -1,5 +1,5 @@ error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:5:21 + --> tests/ui/manual_inspect.rs:10:21 | LL | let _ = Some(0).map(|x| { | ^^^ @@ -13,7 +13,7 @@ LL ~ println!("{}", x); | error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:10:21 + --> tests/ui/manual_inspect.rs:15:21 | LL | let _ = Some(0).map(|x| { | ^^^ @@ -25,7 +25,7 @@ LL ~ println!("{x}"); | error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:15:21 + --> tests/ui/manual_inspect.rs:20:21 | LL | let _ = Some(0).map(|x| { | ^^^ @@ -37,7 +37,7 @@ LL ~ println!("{}", x * 5 + 1); | error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:20:21 + --> tests/ui/manual_inspect.rs:25:21 | LL | let _ = Some(0).map(|x| { | ^^^ @@ -51,7 +51,7 @@ LL ~ } | error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:27:21 + --> tests/ui/manual_inspect.rs:32:21 | LL | let _ = Some(0).map(|x| { | ^^^ @@ -66,7 +66,7 @@ LL ~ } | error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:78:41 + --> tests/ui/manual_inspect.rs:83:41 | LL | let _ = Some((String::new(), 0u32)).map(|x| { | ^^^ @@ -81,7 +81,7 @@ LL ~ } | error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:104:33 + --> tests/ui/manual_inspect.rs:109:33 | LL | let _ = Some(String::new()).map(|x| { | ^^^ @@ -99,7 +99,7 @@ LL ~ println!("test"); | error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:115:21 + --> tests/ui/manual_inspect.rs:120:21 | LL | let _ = Some(0).map(|x| { | ^^^ @@ -114,7 +114,7 @@ LL ~ } | error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:130:46 + --> tests/ui/manual_inspect.rs:135:46 | LL | let _ = Some(Cell2(Cell::new(0u32))).map(|x| { | ^^^ @@ -126,7 +126,7 @@ LL ~ x.0.set(1); | error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:146:34 + --> tests/ui/manual_inspect.rs:151:34 | LL | let _: Result<_, ()> = Ok(0).map(|x| { | ^^^ @@ -138,7 +138,7 @@ LL ~ println!("{}", x); | error: using `map_err` over `inspect_err` - --> tests/ui/manual_inspect.rs:151:35 + --> tests/ui/manual_inspect.rs:156:35 | LL | let _: Result<(), _> = Err(0).map_err(|x| { | ^^^^^^^ @@ -150,7 +150,7 @@ LL ~ println!("{}", x); | error: this call to `map()` won't have an effect on the call to `count()` - --> tests/ui/manual_inspect.rs:156:13 + --> tests/ui/manual_inspect.rs:161:13 | LL | let _ = [0] | _____________^ @@ -167,7 +167,7 @@ LL | | .count(); = help: to override `-D warnings` add `#[allow(clippy::suspicious_map)]` error: using `map` over `inspect` - --> tests/ui/manual_inspect.rs:158:10 + --> tests/ui/manual_inspect.rs:163:10 | LL | .map(|x| { | ^^^ @@ -178,5 +178,102 @@ LL ~ .inspect(|&x| { LL ~ println!("{}", x); | -error: aborting due to 13 previous errors +error: using `map` over `inspect` + --> tests/ui/manual_inspect.rs:215:31 + | +LL | _ = Some(T(114)).as_mut().map(|t| { + | ^^^ + | +help: try + | +LL ~ _ = Some(T(114)).as_mut().inspect(|t| { +LL ~ t.0 + 514; + | + +error: using `map` over `inspect` + --> tests/ui/manual_inspect.rs:253:31 + | +LL | _ = Some(T(114)).as_mut().map(|t| { + | ^^^ + | +help: try + | +LL ~ _ = Some(T(114)).as_mut().inspect(|t| { +LL ~ t.do_immut(); + | + +error: using `map` over `inspect` + --> tests/ui/manual_inspect.rs:258:31 + | +LL | _ = Some(T(114)).as_mut().map(|t| { + | ^^^ + | +help: try + | +LL ~ _ = Some(T(114)).as_mut().inspect(|t| { +LL ~ t.do_immut2(t); + | + +error: using `map` over `inspect` + --> tests/ui/manual_inspect.rs:268:31 + | +LL | _ = Some(T(114)).as_mut().map(|t| { + | ^^^ + | +help: try + | +LL ~ _ = Some(T(114)).as_mut().inspect(|t| { +LL ~ T::do_immut(t); + | + +error: using `map` over `inspect` + --> tests/ui/manual_inspect.rs:294:25 + | +LL | _ = sample.as_mut().map(|t| { + | ^^^ + | +help: try + | +LL ~ _ = sample.as_mut().inspect(|t| { +LL | let mut a = 1; +LL ~ a += t[1]; // immut + | + +error: using `map` over `inspect` + --> tests/ui/manual_inspect.rs:310:25 + | +LL | _ = sample.as_mut().map(|t| { + | ^^^ + | +help: try + | +LL ~ _ = sample.as_mut().inspect(|t| { +LL ~ t.0.0.do_immut(); + | + +error: using `map` over `inspect` + --> tests/ui/manual_inspect.rs:320:25 + | +LL | _ = sample.as_mut().map(|t| { + | ^^^ + | +help: try + | +LL ~ _ = sample.as_mut().inspect(|t| { +LL ~ T::do_immut(&t.0.0); + | + +error: using `map` over `inspect` + --> tests/ui/manual_inspect.rs:333:14 + | +LL | immut_fn.map(|f| { + | ^^^ + | +help: try + | +LL ~ immut_fn.inspect(|f| { +LL ~ f(); + | + +error: aborting due to 21 previous errors