Fix `manual_inspect` to consider mutability #13609

SpriteOvO · 2024-10-27T01:53:06Z

In this PR, we fix the false positives. I left some FIXME cases, it would be nice if they could be supported in the future.

changelog: [manual_inspect]: Fix to consider mutability

rustbot · 2024-10-27T01:53:10Z

r? @xFrednet

rustbot has assigned @xFrednet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Jarcho · 2024-10-27T12:32:38Z

clippy_utils/src/lib.rs

+    /// Assignment.
+    Assign(usize),
+    /// Assignment with an operator.
+    AssignOp(BinOpKind, usize),


No need to capture this. The lint already checks if the binding in the closure is mutable so x = something would fail to compile.

The lint already checks if the binding in the closure is mutable so x = something would fail to compile.

Do you mean this line?

&& let PatKind::Binding(BindingMode(ByRef::No, Mutability::Not), arg_id, _, None) = param.pat.kind

This checks the mut pattern, right. So Assign is indeed not needed for this lint, but given that it's a function in clippy_utils, I'd like to make it take all cases into account as much as possible. And AssignOp is still needed for something like x += y.

x += y also wouldn't compile. Once you fix use_mutability there will be no uses of these variants left (use_node and expr_use_ctxt shouldn't be used there).

Jarcho · 2024-10-27T12:35:58Z

clippy_utils/src/lib.rs

+    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,
+        }
+    }


Using expr_use_ctxt here is wrong since it allows moves to occur. Just use parent_iter.

You'll need to handle mutable borrow or deref adjustments, mutable borrows, mutable derefs, mutable indexing, and assignments. For indexing and derefs you can't tell at the expression so you have to keep walking up the tree. For adjustments, don't go by the resulting type, but walk the adjustments themselves. Mutable auto-deref doesn't always result in a reference after all adjustments take place.

Oh, thanks for pointing those out, I'll check it out.

Mutable auto-deref doesn't always result in a reference after all adjustments take place.

Would you mind explaining it further? Under what circumstances would adjustments not work? So that I can write a test case to make sure of that.

Field accesses through a pointer don't. e.g. x.y where x is a reference will have have a deref adjustment, but not a borrow adjustment.

xFrednet

Tiny NIT, besides the comments from @Jarcho. Otherwise, it looks good to me :D

xFrednet · 2024-11-02T18:12:15Z

clippy_utils/src/lib.rs

                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)]


NIT:

Suggested change

#[allow(clippy::bool_to_int_with_if)]

#[expect(clippy::bool_to_int_with_if)]

Is there a reason why you rather expect this lint over taking the suggestion?

bors · 2024-11-07T18:16:21Z

☔ The latest upstream changes (presumably #13639) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2024-12-13T13:24:36Z

Hey @SpriteOvO , this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

SpriteOvO · 2024-12-13T13:34:31Z

Sorry about that, I'm a bit busy recently, and I will probably continue this PR starting next week.

xFrednet

LGTM, thanks for getting back to this, I appreciate it!

Roses are red,
Roses are blue,
They have to be mut,
for this to be true

SpriteOvO · 2025-01-15T20:17:18Z

Oh, waittt.. I'm still working on it now, that force-push was just a rebasing.

xFrednet · 2025-01-15T20:26:19Z

xD, sorry, I only checked the changed file and that one looked good, so I got a bit too trigger happy. Thank you for catching this in time!

SpriteOvO · 2025-01-15T20:29:35Z

@xFrednet It's fine, I also very sorry for the super delay updates (just finished it), I have pushed the latest changes based on the reviews. Could you and @Jarcho review it again?

I have changed to use parent_iter, and I'm not sure I'm handling adjustments correctly. (at least, ui-tests looks good)

SpriteOvO · 2025-01-25T18:46:50Z

@rustbot ready

xFrednet · 2025-02-16T13:13:52Z

Could you take over the review? You previously commented and I'm busy with RL stuff 😅

r? @Jarcho

Jarcho · 2025-02-16T23:47:46Z

clippy_utils/src/lib.rs

@@ -2779,7 +2780,7 @@ pub enum ExprUseNode<'tcx> {
    /// The callee of a function call.
    Callee,
    /// Access of a field.
-    FieldAccess(Ident),
+    FieldAccess(HirId, Ident),


This no longer needs to be there.

Jarcho · 2025-02-17T00:00:51Z

clippy_lints/src/methods/manual_inspect.rs

    let use_cx = expr_use_ctxt(cx, e);
+    let mutbl = use_mutability(cx, e.hir_id);


This should be starting from the use node. You can start with a quick check on use_cx for moved_before_use (not mutable if true) and the adjustments (if borrowed that mutability can be used). child_id can then be passed to use_mutability

Jarcho · 2025-02-17T00:17:56Z

clippy_lints/src/methods/manual_inspect.rs

+    for (_, node) in cx.tcx.hir().parent_iter(expr_id) {
+        if let Node::Expr(expr) = node {
+            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;
+                    }
+                    return Mutability::Not;
+                },
+                _ => {},
+            }
+        }
+    }


This loop has a few errors.

The loop should stop at the first unmatched node and adjustments should be checked here.

The current child id should be initialized to the starting id and updated before looping so it can be used for checking adjustments.

Checking the lhs for assignment is then just child_id == lhs.hir_id.

MethodCall and Call don't need to be special cased.

Jarcho · 2025-02-17T00:19:40Z

clippy_lints/src/methods/manual_inspect.rs

+    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
+        }
+    };


If the first adjustment is a deref of a reference then this isn't a mutable use.

Outdated

rustbot · 2025-03-31T23:42:27Z

☔ The latest upstream changes (possibly d28d234) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned xFrednet Oct 27, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 27, 2024

Jarcho reviewed Oct 27, 2024

View reviewed changes

xFrednet reviewed Nov 2, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 13, 2024

SpriteOvO force-pushed the fix-manual_inspect branch from d45f4ef to ff22ee9 Compare January 9, 2025 20:18

xFrednet previously approved these changes Jan 15, 2025

View reviewed changes

xFrednet added this pull request to the merge queue Jan 15, 2025

SpriteOvO marked this pull request as draft January 15, 2025 20:19

xFrednet removed this pull request from the merge queue due to a manual request Jan 15, 2025

SpriteOvO force-pushed the fix-manual_inspect branch from ff22ee9 to 9615e76 Compare January 15, 2025 20:26

Fix manual_inspect to consider mutability

0595b19

SpriteOvO force-pushed the fix-manual_inspect branch from 9615e76 to 0595b19 Compare January 15, 2025 20:34

SpriteOvO marked this pull request as ready for review January 16, 2025 03:10

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 25, 2025

rustbot assigned Jarcho and unassigned xFrednet Feb 16, 2025

Jarcho requested changes Feb 17, 2025

View reviewed changes

Jarcho reviewed Feb 17, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `manual_inspect` to consider mutability #13609

Fix `manual_inspect` to consider mutability #13609

SpriteOvO commented Oct 27, 2024 •

edited by xFrednet

Loading

rustbot commented Oct 27, 2024

Jarcho Oct 27, 2024

SpriteOvO Oct 27, 2024 •

edited

Loading

Jarcho Oct 27, 2024

Jarcho Oct 27, 2024

SpriteOvO Oct 27, 2024 •

edited

Loading

Jarcho Oct 27, 2024

xFrednet left a comment

xFrednet Nov 2, 2024

bors commented Nov 7, 2024

xFrednet commented Dec 13, 2024

SpriteOvO commented Dec 13, 2024

xFrednet left a comment

SpriteOvO commented Jan 15, 2025 •

edited

Loading

xFrednet commented Jan 15, 2025

SpriteOvO commented Jan 15, 2025 •

edited

Loading

SpriteOvO commented Jan 25, 2025

xFrednet commented Feb 16, 2025

Jarcho Feb 16, 2025

Jarcho Feb 17, 2025 •

edited

Loading

Jarcho Feb 17, 2025

Jarcho Feb 17, 2025

rustbot commented Mar 31, 2025

	#[allow(clippy::bool_to_int_with_if)]
	#[expect(clippy::bool_to_int_with_if)]

		let use_cx = expr_use_ctxt(cx, e);
		let mutbl = use_mutability(cx, e.hir_id);

Fix manual_inspect to consider mutability #13609

Are you sure you want to change the base?

Fix manual_inspect to consider mutability #13609

Conversation

SpriteOvO commented Oct 27, 2024 • edited by xFrednet Loading

rustbot commented Oct 27, 2024

Choose a reason for hiding this comment

SpriteOvO Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpriteOvO Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 7, 2024

xFrednet commented Dec 13, 2024

SpriteOvO commented Dec 13, 2024

xFrednet left a comment

Choose a reason for hiding this comment

SpriteOvO commented Jan 15, 2025 • edited Loading

xFrednet commented Jan 15, 2025

SpriteOvO commented Jan 15, 2025 • edited Loading

SpriteOvO commented Jan 25, 2025

xFrednet commented Feb 16, 2025

Choose a reason for hiding this comment

Jarcho Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Mar 31, 2025

Fix `manual_inspect` to consider mutability #13609

Fix `manual_inspect` to consider mutability #13609

SpriteOvO commented Oct 27, 2024 •

edited by xFrednet

Loading

SpriteOvO Oct 27, 2024 •

edited

Loading

SpriteOvO Oct 27, 2024 •

edited

Loading

SpriteOvO commented Jan 15, 2025 •

edited

Loading

SpriteOvO commented Jan 15, 2025 •

edited

Loading

Jarcho Feb 17, 2025 •

edited

Loading