New lint: `needless_move` #11759

dnbln · 2023-11-04T21:57:15Z

A lint to check for uses of move on closures which aren't necessary. See #11721.

Fixes #11721.
~~Currently blocked on rust-lang/rust#117585, and an eventual sync.~~

changelog: [needless_move]: A lint for unnecessary moves on closures / async blocks.

rustbot · 2023-11-04T21:57:19Z

r? @Centri3

(rustbot has picked a reviewer for you, use r? to override)

y21

Some drive-by comments, I noticed the pr is marked ready now :)

y21 · 2023-11-07T14:41:22Z

clippy_lints/src/needless_move.rs

+        // Collect moved & borrowed variables from the closure, which the closure *actually* needs.
+        let MovedVariablesCtxt {
+            moved_vars,
+            mut captured_vars,
+        } = {


Can this use the closure_captures query, or is there a reason that it isn't used? Reusing rustc's code seems like it would make it simpler and less error prone than reimplementing the capture rules in clippy

I used that in the first commit (a66be9d), but it was kind of faulty because a move closure will always capture by value, e.g. have the captured_place.info.capture_kind == UpvarCapture::ByValue, regardless of whether that value is actually consumed in the body of the closure or not (unless the type is Copy?). What this would need in order to properly function is basically the closure_captures query applied on the same closure, but without the move keyword, because then we could use the inference results to tell whether a variable needs to be moved or if it is just referenced, but sadly getting that modified closure is a quest I failed.

oh right, that's unfortunate

Actually I've found the file that does most of that analysis: https://github.com/rust-lang/rust/blob/master/compiler/rustc_hir_typeck/src/upvar.rs

Maybe exposing a few of those functions would be helpful for this lint, so I'm gonna look into it soon.

clippy_lints/src/needless_move.rs

tests/ui/needless_move.rs

bors · 2023-11-14T16:12:27Z

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

bors · 2023-11-19T11:13:53Z

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

clippy_lints/src/needless_move.rs

Centri3 · 2023-11-29T17:42:25Z

clippy_lints/src/needless_move.rs

+    /// E.g. all the values are captured by value into the closure / `async` block.
+    ///
+    /// ### Why is this bad?
+    /// Pedantry


This should be expanded upon

Centri3 · 2023-11-29T17:43:53Z

clippy_lints/src/needless_move.rs

+
+        match lint_result {
+            LintResult::NothingCaptured => {
+                lint("there were no captured variables, so the `move` is unnecessary");


This could probably be a let note_msg = match lint_result ... instead, and early return on NeedMove.

Using a closure is a bit more complex, and imo harder to read

Centri3 · 2023-11-29T17:44:28Z

clippy_lints/src/needless_move.rs

+            return;
+        }
+
+        let ExprKind::Closure(closure) = &expr.kind else {


This could be a let-chain instead. However since this is only ever called once, maybe you can inline it?

clippy_lints/src/needless_move.rs

bors · 2023-12-01T19:02:27Z

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

rustbot · 2023-12-03T16:30:38Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

065aad2

bors · 2023-12-11T09:43:55Z

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

Centri3

Nya

clippy_lints/src/needless_move.rs

Centri3 · 2024-02-02T12:53:00Z

tests/ui/needless_move.stderr

+LL |       let closure = assert_static(move || {
+   |                                   ^----
+   |                                   |
+   |  _________________________________help: remove the `move`
+   | |
+LL | |         with_ref(&a);
+LL | |         with_owned(a);
+LL | |     });
+   | |_____^


I can see this getting out of hand quickly on larger closures. Can you make it so the span doesn't include the value (the block/expression)?

clippy_lints/src/needless_move.rs

Centri3 · 2024-02-02T13:03:37Z

tests/ui/needless_move.rs

+    // rust/tests/ui/async-await/deep-futures-are-freeze.rs
+
+    fn _deep_futures_are_freeze() {
+        // no-prefer-dynamic


I would like to ensure x test won't read these, somehow, I dunno, very cautious 😅 I doubt it does when running tests in subtrees or when they aren't at the top of the file

Or just remove them

clippy_lints/src/needless_move.rs

Centri3 · 2024-02-02T13:08:39Z

clippy_lints/src/needless_move.rs

+impl<'tcx> euv::Delegate<'tcx> for MovedVariablesCtxt<'tcx> {
+    fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, hir_id: HirId) {
+        if let euv::PlaceBase::Upvar(_) = cmt.place.base {
+            self.moved.push((cmt.place.clone(), hir_id));


Is there any reason we can't store &'tcx Place<'tcx> to prevent cloning? This is cheap if projections is empty, tho you're cloning a Vec otherwise

The cmt is not a &'tcx reference, so the lifetimes don't add up and the borrow checker is unhappy.

Centri3 · 2024-02-02T13:13:39Z

clippy_lints/src/needless_move.rs

+            },
+        };
+
+        span_lint_and_then(


We should call is_from_proc_macro here. You can implement WithSearchPat for Closure by reusing the code for ExprKind::Closure in expr_search_pat

clippy_lints/src/needless_move.rs

xFrednet · 2024-04-01T09:54:50Z

Hey @dnbln, 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

Also this PR needs a new reviewer since @Centri3 is currently busy.

r? @y21 would you mind taking this review? You can also ping @ARandomDev99 or @GuillaumeGomez for a pre-review if that helps :)

An implementation for the lint described in rust-lang#11721

see discussion in rust-lang#11721

dnbln · 2024-04-01T11:26:19Z

Hey @dnbln, 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.

Hi @xFrednet. I've honestly really began to doubt the usefulness of this lint, so I don't think I'll be investing any more time into it.

Centri3 · 2024-04-01T15:26:09Z

I do still quite like this as a restriction (very pedantic style) lint. I might get around to it eventually if nobody else picks it up :3

dnbln · 2024-04-01T17:04:02Z

I'll just leave the script I used to extract the tests from the rust repo here, if anyone else is interested in continuing this and might find it useful:

https://gist.github.com/dnbln/6b03d98bf189d76236a51ae9e2082433

rustbot assigned Centri3 Nov 4, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 4, 2023

dnbln marked this pull request as ready for review November 7, 2023 11:21

y21 reviewed Nov 7, 2023

View reviewed changes

dnbln force-pushed the feat/needless_move branch from e80c3ba to d1b3fdf Compare November 7, 2023 17:39

dnbln force-pushed the feat/needless_move branch 4 times, most recently from 7a949c3 to fea8899 Compare November 18, 2023 17:39

dnbln force-pushed the feat/needless_move branch 6 times, most recently from fb343f3 to 14da14a Compare November 19, 2023 15:12

y21 reviewed Nov 19, 2023

View reviewed changes

clippy_lints/src/needless_move.rs Outdated Show resolved Hide resolved

dnbln force-pushed the feat/needless_move branch 3 times, most recently from 20b12fc to f3fcd2a Compare November 19, 2023 19:51

Centri3 reviewed Nov 29, 2023

View reviewed changes

dnbln force-pushed the feat/needless_move branch from 015ce17 to e2dc5a1 Compare November 29, 2023 18:59

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 3, 2023

dnbln force-pushed the feat/needless_move branch from 065aad2 to 1e1efac Compare December 3, 2023 16:36

Centri3 reviewed Feb 2, 2024

View reviewed changes

Centri3 removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) has-merge-commits PR has merge commits, merge with caution. labels Feb 2, 2024

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 Apr 1, 2024

rustbot assigned y21 and unassigned Centri3 Apr 1, 2024

dnbln added 5 commits April 1, 2024 12:24

feat: needless_move lint

0e66693

An implementation for the lint described in rust-lang#11721

change category to restriction

d7f06a2

see discussion in rust-lang#11721

fix review comments

64ba370

fix stuff

e7976fe

fix: minor fixes

7a3337a

dnbln force-pushed the feat/needless_move branch from 1e1efac to 7a3337a Compare April 1, 2024 10:43

dnbln closed this Apr 1, 2024

xFrednet added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint: `needless_move` #11759

New lint: `needless_move` #11759

dnbln commented Nov 4, 2023 •

edited

Loading

rustbot commented Nov 4, 2023

y21 left a comment

y21 Nov 7, 2023

dnbln Nov 7, 2023

y21 Nov 7, 2023

dnbln Nov 8, 2023

bors commented Nov 14, 2023

bors commented Nov 19, 2023

Centri3 Nov 29, 2023

Centri3 Nov 29, 2023

Centri3 Nov 29, 2023

bors commented Dec 1, 2023

rustbot commented Dec 3, 2023

bors commented Dec 11, 2023

Centri3 left a comment

Centri3 Feb 2, 2024

Centri3 Feb 2, 2024

Centri3 Feb 2, 2024

dnbln Apr 1, 2024

Centri3 Feb 2, 2024

xFrednet commented Apr 1, 2024

dnbln commented Apr 1, 2024

Centri3 commented Apr 1, 2024

dnbln commented Apr 1, 2024

New lint: needless_move #11759

New lint: needless_move #11759

Conversation

dnbln commented Nov 4, 2023 • edited Loading

rustbot commented Nov 4, 2023

y21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 14, 2023

bors commented Nov 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 1, 2023

rustbot commented Dec 3, 2023

bors commented Dec 11, 2023

Centri3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xFrednet commented Apr 1, 2024

dnbln commented Apr 1, 2024

Centri3 commented Apr 1, 2024

dnbln commented Apr 1, 2024

New lint: `needless_move` #11759

New lint: `needless_move` #11759

dnbln commented Nov 4, 2023 •

edited

Loading