-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect when move of !Copy value occurs within loop and should likely not be cloned #121652
Conversation
☔ The latest upstream changes (presumably #121489) made this pull request unmergeable. Please resolve the merge conflicts. |
14f5d78
to
910fc1f
Compare
I don't currently have the bandwidth to review this. |
Cool stuff! I don't completely follow the "look for breaks" heuristic but I'll trust you on that. A few comments but looks good overall. |
@rustbot author |
…ely not be cloned When encountering a move error on a value within a loop of any kind, identify if the moved value belongs to a call expression that should not be cloned and avoid the semantically incorrect suggestion. Also try to suggest moving the call expression outside of the loop instead. ``` error[E0382]: use of moved value: `vec` --> $DIR/recreating-value-in-loop-condition.rs:6:33 | LL | let vec = vec!["one", "two", "three"]; | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait LL | while let Some(item) = iter(vec).next() { | ----------------------------^^^-------- | | | | | value moved here, in previous iteration of loop | inside of this loop | note: consider changing this parameter type in function `iter` to borrow instead if owning the value isn't necessary --> $DIR/recreating-value-in-loop-condition.rs:1:17 | LL | fn iter<T>(vec: Vec<T>) -> impl Iterator<Item = T> { | ---- ^^^^^^ this parameter takes ownership of the value | | | in this function help: consider moving the expression out of the loop so it is only moved once | LL ~ let mut value = iter(vec); LL ~ while let Some(item) = value.next() { | ``` We use the presence of a `break` in the loop that would be affected by the moved value as a heuristic for "shouldn't be cloned". Fix rust-lang#121466.
Sometimes move errors are because of a misplaced `continue`, but we didn't surface that anywhere. Now when there are more than one set of nested loops we show them out and point at the `continue` and `break` expressions within that might need to go elsewhere. ``` error[E0382]: use of moved value: `foo` --> $DIR/nested-loop-moved-value-wrong-continue.rs:46:18 | LL | for foo in foos { | --- | | | this reinitialization might get skipped | move occurs because `foo` has type `String`, which does not implement the `Copy` trait ... LL | for bar in &bars { | ---------------- inside of this loop ... LL | baz.push(foo); | --- value moved here, in previous iteration of loop ... LL | qux.push(foo); | ^^^ value used here after move | note: verify that your loop breaking logic is correct --> $DIR/nested-loop-moved-value-wrong-continue.rs:41:17 | LL | for foo in foos { | --------------- ... LL | for bar in &bars { | ---------------- ... LL | continue; | ^^^^^^^^ this `continue` advances the loop at line 33 help: consider moving the expression out of the loop so it is only moved once | LL ~ let mut value = baz.push(foo); LL ~ for bar in &bars { LL | ... LL | if foo == *bar { LL ~ value; | help: consider cloning the value if the performance cost is acceptable | LL | baz.push(foo.clone()); | ++++++++ ``` Fix rust-lang#92531.
910fc1f
to
3b237d7
Compare
Nice work, ty @bors r+ |
…ion, r=Nadrieril Detect when move of !Copy value occurs within loop and should likely not be cloned When encountering a move error on a value within a loop of any kind, identify if the moved value belongs to a call expression that should not be cloned and avoid the semantically incorrect suggestion. Also try to suggest moving the call expression outside of the loop instead. ``` error[E0382]: use of moved value: `vec` --> $DIR/recreating-value-in-loop-condition.rs:6:33 | LL | let vec = vec!["one", "two", "three"]; | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait LL | while let Some(item) = iter(vec).next() { | ----------------------------^^^-------- | | | | | value moved here, in previous iteration of loop | inside of this loop | note: consider changing this parameter type in function `iter` to borrow instead if owning the value isn't necessary --> $DIR/recreating-value-in-loop-condition.rs:1:17 | LL | fn iter<T>(vec: Vec<T>) -> impl Iterator<Item = T> { | ---- ^^^^^^ this parameter takes ownership of the value | | | in this function help: consider moving the expression out of the loop so it is only moved once | LL ~ let mut value = iter(vec); LL ~ while let Some(item) = value.next() { | ``` We use the presence of a `break` in the loop that would be affected by the moved value as a heuristic for "shouldn't be cloned". Fix rust-lang#121466. --- *Point at continue and break that might be in the wrong place* Sometimes move errors are because of a misplaced `continue`, but we didn't surface that anywhere. Now when there are more than one set of nested loops we show them out and point at the `continue` and `break` expressions within that might need to go elsewhere. ``` error[E0382]: use of moved value: `foo` --> $DIR/nested-loop-moved-value-wrong-continue.rs:46:18 | LL | for foo in foos { | --- | | | this reinitialization might get skipped | move occurs because `foo` has type `String`, which does not implement the `Copy` trait ... LL | for bar in &bars { | ---------------- inside of this loop ... LL | baz.push(foo); | --- value moved here, in previous iteration of loop ... LL | qux.push(foo); | ^^^ value used here after move | note: verify that your loop breaking logic is correct --> $DIR/nested-loop-moved-value-wrong-continue.rs:41:17 | LL | for foo in foos { | --------------- ... LL | for bar in &bars { | ---------------- ... LL | continue; | ^^^^^^^^ this `continue` advances the loop at line 33 help: consider moving the expression out of the loop so it is only moved once | LL ~ let mut value = baz.push(foo); LL ~ for bar in &bars { LL | ... LL | if foo == *bar { LL ~ value; | help: consider cloning the value if the performance cost is acceptable | LL | baz.push(foo.clone()); | ++++++++ ``` Fix rust-lang#92531.
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#121652 (Detect when move of !Copy value occurs within loop and should likely not be cloned) - rust-lang#122639 (Fix typos) - rust-lang#122645 (Remove some only- clauses from mir-opt tests) - rust-lang#122654 (interpret/memory: explain why we use == on bool) - rust-lang#122656 (simplify_cfg: rename some passes so that they make more sense) r? `@ghost` `@rustbot` modify labels: rollup
☀️ Test successful - checks-actions |
Finished benchmarking commit (5608c7f): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 669.037s -> 668.349s (-0.10%) |
When encountering a move error on a value within a loop of any kind,
identify if the moved value belongs to a call expression that should not
be cloned and avoid the semantically incorrect suggestion. Also try to
suggest moving the call expression outside of the loop instead.
We use the presence of a
break
in the loop that would be affected bythe moved value as a heuristic for "shouldn't be cloned".
Fix #121466.
Point at continue and break that might be in the wrong place
Sometimes move errors are because of a misplaced
continue
, but we didn'tsurface that anywhere. Now when there are more than one set of nested loops
we show them out and point at the
continue
andbreak
expressions withinthat might need to go elsewhere.
Fix #92531.