From d45f4ef763e90b45c3c1d13930c8216ff2f8b431 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 | 23 +++-- clippy_utils/src/lib.rs | 48 +++++++++- tests/ui/manual_inspect.fixed | 100 +++++++++++++++++++- tests/ui/manual_inspect.rs | 105 ++++++++++++++++++++- tests/ui/manual_inspect.stderr | 88 ++++++++++++++--- 6 files changed, 339 insertions(+), 33 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index f34f5e056064..f27bf7820478 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -269,7 +269,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) { @@ -338,7 +338,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<_>` @@ -349,7 +349,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. @@ -437,7 +437,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 64c19c327b25..b0f6b956df22 100644 --- a/clippy_lints/src/methods/manual_inspect.rs +++ b/clippy_lints/src/methods/manual_inspect.rs @@ -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 || arg_ty.is_copy_modulo_regions(cx.tcx, cx.param_env)) // 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.param_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_cx.use_mutability(cx); 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,5 @@ 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) } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 65e6f8847bca..7789e774a9e9 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -91,7 +91,7 @@ use std::sync::{Mutex, MutexGuard, OnceLock}; use clippy_config::types::DisallowedPath; use itertools::Itertools; -use rustc_ast::ast::{self, LitKind, RangeLimits}; +use rustc_ast::ast::{self, BinOpKind, LitKind, RangeLimits}; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::packed::Pu128; use rustc_data_structures::unhash::UnhashMap; @@ -2791,16 +2791,46 @@ 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), + ExprKind::Assign(lhs, rhs, _) => { + debug_assert!(lhs.hir_id == self.child_id || rhs.hir_id == self.child_id); + #[allow(clippy::bool_to_int_with_if)] + let idx = if lhs.hir_id == self.child_id { 0 } else { 1 }; + ExprUseNode::Assign(idx) + }, + ExprKind::AssignOp(op, lhs, rhs) => { + debug_assert!(lhs.hir_id == self.child_id || rhs.hir_id == self.child_id); + #[allow(clippy::bool_to_int_with_if)] + let idx = if lhs.hir_id == self.child_id { 0 } else { 1 }; + ExprUseNode::AssignOp(op.node, idx) + }, _ => ExprUseNode::Other, }, _ => ExprUseNode::Other, } } + + pub fn use_mutability(&self, cx: &LateContext<'tcx>) -> Mutability { + match self.use_node(cx) { + ExprUseNode::FieldAccess(parent, _) => { + let parent = cx.tcx.hir().expect_expr(parent); + expr_use_ctxt(cx, parent).use_mutability(cx) + }, + ExprUseNode::AssignOp(_, 0) | ExprUseNode::Assign(0) => Mutability::Mut, + ExprUseNode::AddrOf(_, mutbl) => mutbl, + ExprUseNode::FnArg(_, _) | ExprUseNode::MethodArg(_, _, _) => { + let child_expr = cx.tcx.hir().expect_expr(self.child_id); + let ty = cx.typeck_results().expr_ty_adjusted(child_expr); + ty.ref_mutability().unwrap_or(Mutability::Not) + }, + _ => Mutability::Not, + } + } } /// The node which consumes a value. +#[derive(Debug)] pub enum ExprUseNode<'tcx> { /// Assignment to, or initializer for, a local LetStmt(&'tcx LetStmt<'tcx>), @@ -2817,9 +2847,13 @@ 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), + /// Assignment. + Assign(usize), + /// Assignment with an operator. + AssignOp(BinOpKind, usize), Other, } impl<'tcx> ExprUseNode<'tcx> { @@ -2896,7 +2930,13 @@ impl<'tcx> ExprUseNode<'tcx> { let sig = cx.tcx.fn_sig(id).skip_binder(); Some(DefinedTy::Mir(cx.tcx.param_env(id).and(sig.input(i)))) }, - Self::LetStmt(_) | Self::FieldAccess(..) | Self::Callee | Self::Other | Self::AddrOf(..) => None, + Self::LetStmt(_) + | Self::FieldAccess(..) + | Self::Callee + | Self::Other + | Self::AddrOf(..) + | Self::Assign(_) + | Self::AssignOp(..) => None, } } } diff --git a/tests/ui/manual_inspect.fixed b/tests/ui/manual_inspect.fixed index 0e1b8fe3edb5..0609eb6e9d94 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,96 @@ fn main() { }); } } + +fn issue_13185() { + struct T(u32); + + impl T { + fn do_immut(&self) { + println!("meow~"); + } + + fn do_mut(&mut self) { + self.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().inspect(|t| { + t.do_immut(); + }); + + _ = 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 + }); + + // Nested fields access + struct N(T); + + _ = Some(N(T(114))).as_mut().map(|t| { + t.0.do_mut(); + t + }); + + _ = Some(N(T(114))).as_mut().inspect(|t| { + t.0.do_immut(); + }); + + _ = Some(N(T(114))).as_mut().map(|t| { + T::do_mut(&mut t.0); + t + }); + + _ = Some(N(T(114))).as_mut().inspect(|t| { + T::do_immut(&t.0); + }); +} diff --git a/tests/ui/manual_inspect.rs b/tests/ui/manual_inspect.rs index 94cdfe391400..c41e5f10d71a 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,101 @@ fn main() { }); } } + +fn issue_13185() { + struct T(u32); + + impl T { + fn do_immut(&self) { + println!("meow~"); + } + + fn do_mut(&mut self) { + self.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.do_immut(); + 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 + }); + + // Nested fields access + struct N(T); + + _ = Some(N(T(114))).as_mut().map(|t| { + t.0.do_mut(); + t + }); + + _ = Some(N(T(114))).as_mut().map(|t| { + t.0.do_immut(); + t + }); + + _ = Some(N(T(114))).as_mut().map(|t| { + T::do_mut(&mut t.0); + t + }); + + _ = Some(N(T(114))).as_mut().map(|t| { + T::do_immut(&t.0); + t + }); +} diff --git a/tests/ui/manual_inspect.stderr b/tests/ui/manual_inspect.stderr index 0559b3bd6611..36cf317a6c8d 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,65 @@ LL ~ .inspect(|&x| { LL ~ println!("{}", x); | -error: aborting due to 13 previous errors +error: using `map` over `inspect` + --> tests/ui/manual_inspect.rs:206: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:239: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:249: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:275:34 + | +LL | _ = Some(N(T(114))).as_mut().map(|t| { + | ^^^ + | +help: try + | +LL ~ _ = Some(N(T(114))).as_mut().inspect(|t| { +LL ~ t.0.do_immut(); + | + +error: using `map` over `inspect` + --> tests/ui/manual_inspect.rs:285:34 + | +LL | _ = Some(N(T(114))).as_mut().map(|t| { + | ^^^ + | +help: try + | +LL ~ _ = Some(N(T(114))).as_mut().inspect(|t| { +LL ~ T::do_immut(&t.0); + | + +error: aborting due to 18 previous errors