Skip to content

Commit

Permalink
Fix manual_inspect to consider mutability
Browse files Browse the repository at this point in the history
  • Loading branch information
SpriteOvO committed Jan 15, 2025
1 parent 19e305b commit 9615e76
Show file tree
Hide file tree
Showing 6 changed files with 477 additions and 32 deletions.
8 changes: 4 additions & 4 deletions clippy_lints/src/dereference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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<_>`
Expand All @@ -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.
Expand Down Expand Up @@ -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)
Expand Down
65 changes: 55 additions & 10 deletions clippy_lints/src/methods/manual_inspect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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(());
Expand Down Expand Up @@ -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()));
Expand Down Expand Up @@ -207,36 +211,77 @@ 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
},
ExprUseNode::Callee | ExprUseNode::MethodArg(_, _, 0) => UseKind::WillAutoDeref,
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
}
5 changes: 3 additions & 2 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -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>),
Expand All @@ -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,
Expand Down
149 changes: 148 additions & 1 deletion tests/ui/manual_inspect.fixed
Original file line number Diff line number Diff line change
@@ -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| {
Expand Down Expand Up @@ -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
});
}
Loading

0 comments on commit 9615e76

Please sign in to comment.