Provide more context and suggestions in borrowck errors involving closures #124136

estebank · 2024-04-18T20:51:01Z

Start pointing to where bindings where declared when they are captured in closures:

error[E0597]: `x` does not live long enough
  --> $DIR/suggest-return-closure.rs:23:9
   |
LL |     let x = String::new();
   |         - binding `x` declared here
...
LL |     |c| {
   |     --- value captured here
LL |         x.push(c);
   |         ^ borrowed value does not live long enough
...
LL | }
   | -- borrow later used here
   | |
   | `x` dropped here while still borrowed

Suggest cloning in more cases involving closures:

error[E0507]: cannot move out of `foo` in pattern guard
  --> $DIR/issue-27282-move-ref-mut-into-guard.rs:11:19
   |
LL |             if { (|| { let mut bar = foo; bar.take() })(); false } => {},
   |                   ^^                 --- move occurs because `foo` has type `&mut Option<&i32>`, which does not implement the `Copy` trait
   |                   |
   |                   `foo` is moved here
   |
   = note: variables bound in patterns cannot be moved from until after the end of the pattern guard
help: consider cloning the value if the performance cost is acceptable
   |
LL |             if { (|| { let mut bar = foo.clone(); bar.take() })(); false } => {},
   |                                         ++++++++

Mention when type parameter could be Clone

error[E0382]: use of moved value: `t`
  --> $DIR/use_of_moved_value_copy_suggestions.rs:7:9
   |
LL | fn duplicate_t<T>(t: T) -> (T, T) {
   |                   - move occurs because `t` has type `T`, which does not implement the `Copy` trait
...
LL |     (t, t)
   |      -  ^ value used here after move
   |      |
   |      value moved here
   |
help: if `T` implemented `Clone`, you could clone the value
  --> $DIR/use_of_moved_value_copy_suggestions.rs:4:16
   |
LL | fn duplicate_t<T>(t: T) -> (T, T) {
   |                ^ consider constraining this type parameter with `Clone`
...
LL |     (t, t)
   |      - you could clone this value
help: consider restricting type parameter `T`
   |
LL | fn duplicate_t<T: Copy>(t: T) -> (T, T) {
   |                 ++++++

The help is new. On ADTs, we also extend the output with span labels:

error[E0507]: cannot move out of static item `FOO`
  --> $DIR/issue-17718-static-move.rs:6:14
   |
LL |     let _a = FOO;
   |              ^^^ move occurs because `FOO` has type `Foo`, which does not implement the `Copy` trait
   |
note: if `Foo` implemented `Clone`, you could clone the value
  --> $DIR/issue-17718-static-move.rs:1:1
   |
LL | struct Foo;
   | ^^^^^^^^^^ consider implementing `Clone` for this type
...
LL |     let _a = FOO;
   |              --- you could clone this value
help: consider borrowing here
   |
LL |     let _a = &FOO;
   |              +

Suggest cloning captured binding in move closure

error[E0507]: cannot move out of `bar`, a captured variable in an `FnMut` closure
  --> $DIR/borrowck-move-by-capture.rs:9:29
   |
LL |     let bar: Box<_> = Box::new(3);
   |         --- captured outer variable
LL |     let _g = to_fn_mut(|| {
   |                        -- captured by this `FnMut` closure
LL |         let _h = to_fn_once(move || -> isize { *bar });
   |                             ^^^^^^^^^^^^^^^^   ----
   |                             |                  |
   |                             |                  variable moved due to use in closure
   |                             |                  move occurs because `bar` has type `Box<isize>`, which does not implement the `Copy` trait
   |                             `bar` is moved here
   |
help: clone the value before moving it into the closure 1
   |
LL ~         let value = bar.clone();
LL ~         let _h = to_fn_once(move || -> isize { value });
   |

rustbot · 2024-04-18T20:51:09Z

r? @nnethercote

rustbot has assigned @nnethercote.
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

nnethercote

In the first commit message, "where declared" should be "were declared".

compiler/rustc_borrowck/src/diagnostics/move_errors.rs

nnethercote · 2024-04-22T22:21:48Z

tests/ui/suggestions/option-content-move3.stderr

+LL |             let x = var;
+   |                     --- you could clone this value
+
+error[E0507]: cannot move out of `var`, a captured variable in an `FnMut` closure


Looks like the compiler is totally inconsistent about "a FnMut" vs "an FnMut" :/

Which one should we use?

The current split suggests that people say "fun mut" and "eff en mut" equally, so I don't think there's a clear answer.

I'll move towards a FnMut (in a separate PR) because making every place where we talk about trait names to be specialized based on the language rules of the actual name of the trait would be a headache, whereas moving in the other direction is just a handful of static strings.

I just saw this in compiler/rustc_ast/src/ast.rs, which agrees with the use of "a":

impl ItemKind { pub fn article(&self) -> &'static str { use ItemKind::*; match self { Use(..) | Static(..) | Const(..) | Fn(..) | Mod(..) | GlobalAsm(..) | TyAlias(..) | Struct(..) | Union(..) | Trait(..) | TraitAlias(..) | MacroDef(..) | Delegation(..) => "a", ExternCrate(..) | ForeignMod(..) | MacCall(..) | Enum(..) | Impl { .. } => "an", } }

nnethercote · 2024-04-22T22:32:02Z

I am ambivalent about the middle commit. There are a lot of extra suggestions to implement Clone, I wonder how often that is the right fix? If it's often not the right fix, then these just become annoying noise that bloat the messages.

estebank · 2024-04-22T23:20:00Z

There are a lot of extra suggestions to implement Clone, I wonder how often that is the right fix? If it's often not the right fix, then these just become annoying noise that bloat the messages.

We already suggest making the type param Copy, which I feel is less appropriate than suggesting constraining type params to Clone (for wider applicability), so I do feel for type params we should mention this somehow (even if we reduce the output for it). For ADTs, we already mention "if Type implemented Clone, you could clone the value", this PR only adds some span labels to provide more context, going from

note: if `S<()>` implemented `Clone`, you could clone the value
  --> $DIR/issue-25700.rs:1:1
   |
LL | struct S<T: 'static>(#[allow(dead_code)] Option<&'static T>);
   | ^^^^^^^^^^^^^^^^^^^^

to

note: if `S<()>` implemented `Clone`, you could clone the value
  --> $DIR/issue-25700.rs:1:1
   |
LL | struct S<T: 'static>(#[allow(dead_code)] Option<&'static T>);
   | ^^^^^^^^^^^^^^^^^^^^ consider implementing `Clone` for this type
...
LL |     drop(t);
   |          - you could clone this value

Whether this is the right fix is highly dependent on what the user is trying to accomplish. Sadly we can't provide a good suggestion for every case (which is why it's a note and not a suggestion), but I tend to lean towards "too much information" over "too little information". Too much text can be overwhelming, but not enough text can leave people to fend for themselves with no guidance at all.

What do you think?

nnethercote · 2024-04-23T01:53:23Z

Ok, you are way deeper in this stuff than I am so I can live with that explanation. r=me once the commit message typo is fixed and the extra comment added.

estebank · 2024-04-23T17:30:33Z

Ok, you are way deeper in this stuff than I am so I can live with that explanation.

I don't intend to push through my changes without agreement, I can be wrong or miss something! :)

once the commit message typo is fixed and the extra comment added.

done and done

@bors r+

bors · 2024-04-23T17:30:36Z

📌 Commit 17a27e8 has been approved by estebank

It is now in the queue for this repository.

Provide more context and suggestions in borrowck errors involving closures Start pointing to where bindings where declared when they are captured in closures: ``` error[E0597]: `x` does not live long enough --> $DIR/suggest-return-closure.rs:23:9 | LL | let x = String::new(); | - binding `x` declared here ... LL | |c| { | --- value captured here LL | x.push(c); | ^ borrowed value does not live long enough ... LL | } | -- borrow later used here | | | `x` dropped here while still borrowed ``` Suggest cloning in more cases involving closures: ``` error[E0507]: cannot move out of `foo` in pattern guard --> $DIR/issue-27282-move-ref-mut-into-guard.rs:11:19 | LL | if { (|| { let mut bar = foo; bar.take() })(); false } => {}, | ^^ --- move occurs because `foo` has type `&mut Option<&i32>`, which does not implement the `Copy` trait | | | `foo` is moved here | = note: variables bound in patterns cannot be moved from until after the end of the pattern guard help: consider cloning the value if the performance cost is acceptable | LL | if { (|| { let mut bar = foo.clone(); bar.take() })(); false } => {}, | ++++++++ ``` Mention when type parameter could be Clone ``` error[E0382]: use of moved value: `t` --> $DIR/use_of_moved_value_copy_suggestions.rs:7:9 | LL | fn duplicate_t<T>(t: T) -> (T, T) { | - move occurs because `t` has type `T`, which does not implement the `Copy` trait ... LL | (t, t) | - ^ value used here after move | | | value moved here | help: if `T` implemented `Clone`, you could clone the value --> $DIR/use_of_moved_value_copy_suggestions.rs:4:16 | LL | fn duplicate_t<T>(t: T) -> (T, T) { | ^ consider constraining this type parameter with `Clone` ... LL | (t, t) | - you could clone this value help: consider restricting type parameter `T` | LL | fn duplicate_t<T: Copy>(t: T) -> (T, T) { | ++++++ ``` The `help` is new. On ADTs, we also extend the output with span labels: ``` error[E0507]: cannot move out of static item `FOO` --> $DIR/issue-17718-static-move.rs:6:14 | LL | let _a = FOO; | ^^^ move occurs because `FOO` has type `Foo`, which does not implement the `Copy` trait | note: if `Foo` implemented `Clone`, you could clone the value --> $DIR/issue-17718-static-move.rs:1:1 | LL | struct Foo; | ^^^^^^^^^^ consider implementing `Clone` for this type ... LL | let _a = FOO; | --- you could clone this value help: consider borrowing here | LL | let _a = &FOO; | + ``` Suggest cloning captured binding in move closure ``` error[E0507]: cannot move out of `bar`, a captured variable in an `FnMut` closure --> $DIR/borrowck-move-by-capture.rs:9:29 | LL | let bar: Box<_> = Box::new(3); | --- captured outer variable LL | let _g = to_fn_mut(|| { | -- captured by this `FnMut` closure LL | let _h = to_fn_once(move || -> isize { *bar }); | ^^^^^^^^^^^^^^^^ ---- | | | | | variable moved due to use in closure | | move occurs because `bar` has type `Box<isize>`, which does not implement the `Copy` trait | `bar` is moved here | help: clone the value before moving it into the closure 1 | LL ~ let value = bar.clone(); LL ~ let _h = to_fn_once(move || -> isize { value }); | ```

matthiaskrgr · 2024-04-23T18:18:57Z

@bors r-
may need rebase :(
#124301 (comment)

estebank · 2024-04-23T20:00:26Z

rebased and fixed

@bors r=nnethercote

bors · 2024-04-23T20:00:28Z

📌 Commit 2f29f78 has been approved by nnethercote

It is now in the queue for this repository.

…cote Provide more context and suggestions in borrowck errors involving closures Start pointing to where bindings where declared when they are captured in closures: ``` error[E0597]: `x` does not live long enough --> $DIR/suggest-return-closure.rs:23:9 | LL | let x = String::new(); | - binding `x` declared here ... LL | |c| { | --- value captured here LL | x.push(c); | ^ borrowed value does not live long enough ... LL | } | -- borrow later used here | | | `x` dropped here while still borrowed ``` Suggest cloning in more cases involving closures: ``` error[E0507]: cannot move out of `foo` in pattern guard --> $DIR/issue-27282-move-ref-mut-into-guard.rs:11:19 | LL | if { (|| { let mut bar = foo; bar.take() })(); false } => {}, | ^^ --- move occurs because `foo` has type `&mut Option<&i32>`, which does not implement the `Copy` trait | | | `foo` is moved here | = note: variables bound in patterns cannot be moved from until after the end of the pattern guard help: consider cloning the value if the performance cost is acceptable | LL | if { (|| { let mut bar = foo.clone(); bar.take() })(); false } => {}, | ++++++++ ``` Mention when type parameter could be Clone ``` error[E0382]: use of moved value: `t` --> $DIR/use_of_moved_value_copy_suggestions.rs:7:9 | LL | fn duplicate_t<T>(t: T) -> (T, T) { | - move occurs because `t` has type `T`, which does not implement the `Copy` trait ... LL | (t, t) | - ^ value used here after move | | | value moved here | help: if `T` implemented `Clone`, you could clone the value --> $DIR/use_of_moved_value_copy_suggestions.rs:4:16 | LL | fn duplicate_t<T>(t: T) -> (T, T) { | ^ consider constraining this type parameter with `Clone` ... LL | (t, t) | - you could clone this value help: consider restricting type parameter `T` | LL | fn duplicate_t<T: Copy>(t: T) -> (T, T) { | ++++++ ``` The `help` is new. On ADTs, we also extend the output with span labels: ``` error[E0507]: cannot move out of static item `FOO` --> $DIR/issue-17718-static-move.rs:6:14 | LL | let _a = FOO; | ^^^ move occurs because `FOO` has type `Foo`, which does not implement the `Copy` trait | note: if `Foo` implemented `Clone`, you could clone the value --> $DIR/issue-17718-static-move.rs:1:1 | LL | struct Foo; | ^^^^^^^^^^ consider implementing `Clone` for this type ... LL | let _a = FOO; | --- you could clone this value help: consider borrowing here | LL | let _a = &FOO; | + ``` Suggest cloning captured binding in move closure ``` error[E0507]: cannot move out of `bar`, a captured variable in an `FnMut` closure --> $DIR/borrowck-move-by-capture.rs:9:29 | LL | let bar: Box<_> = Box::new(3); | --- captured outer variable LL | let _g = to_fn_mut(|| { | -- captured by this `FnMut` closure LL | let _h = to_fn_once(move || -> isize { *bar }); | ^^^^^^^^^^^^^^^^ ---- | | | | | variable moved due to use in closure | | move occurs because `bar` has type `Box<isize>`, which does not implement the `Copy` trait | `bar` is moved here | help: clone the value before moving it into the closure 1 | LL ~ let value = bar.clone(); LL ~ let _h = to_fn_once(move || -> isize { value }); | ```

Rollup of 6 pull requests Successful merges: - rust-lang#122500 (delegation: Support renaming, and async, const, extern "ABI" and C-variadic functions) - rust-lang#123316 (Test `#[unix_sigpipe = "inherit"]` with both `SIG_DFL` and `SIG_IGN`) - rust-lang#124136 (Provide more context and suggestions in borrowck errors involving closures) - rust-lang#124280 (Port repr128-dwarf run-make test to rmake) - rust-lang#124282 (windows fill_utf16_buf: explain the expected return value) - rust-lang#124308 (Add diagnostic item for `std::iter::Enumerate`) r? `@ghost` `@rustbot` modify labels: rollup

fmease · 2024-04-24T11:34:27Z

Needs another rebase.
@bors r-

Rollup of 5 pull requests Successful merges: - rust-lang#122500 (delegation: Support renaming, and async, const, extern "ABI" and C-variadic functions) - rust-lang#123316 (Test `#[unix_sigpipe = "inherit"]` with both `SIG_DFL` and `SIG_IGN`) - rust-lang#123794 (More DefineOpaqueTypes::Yes) - rust-lang#124282 (windows fill_utf16_buf: explain the expected return value) - rust-lang#124308 (Add diagnostic item for `std::iter::Enumerate`) Failed merges: - rust-lang#124136 (Provide more context and suggestions in borrowck errors involving closures) r? `@ghost` `@rustbot` modify labels: rollup

Start pointing to where bindings were declared when they are captured in closures: ``` error[E0597]: `x` does not live long enough --> $DIR/suggest-return-closure.rs:23:9 | LL | let x = String::new(); | - binding `x` declared here ... LL | |c| { | --- value captured here LL | x.push(c); | ^ borrowed value does not live long enough ... LL | } | -- borrow later used here | | | `x` dropped here while still borrowed ``` Suggest cloning in more cases involving closures: ``` error[E0507]: cannot move out of `foo` in pattern guard --> $DIR/issue-27282-move-ref-mut-into-guard.rs:11:19 | LL | if { (|| { let mut bar = foo; bar.take() })(); false } => {}, | ^^ --- move occurs because `foo` has type `&mut Option<&i32>`, which does not implement the `Copy` trait | | | `foo` is moved here | = note: variables bound in patterns cannot be moved from until after the end of the pattern guard help: consider cloning the value if the performance cost is acceptable | LL | if { (|| { let mut bar = foo.clone(); bar.take() })(); false } => {}, | ++++++++ ```

``` error[E0382]: use of moved value: `t` --> $DIR/use_of_moved_value_copy_suggestions.rs:7:9 | LL | fn duplicate_t<T>(t: T) -> (T, T) { | - move occurs because `t` has type `T`, which does not implement the `Copy` trait ... LL | (t, t) | - ^ value used here after move | | | value moved here | help: if `T` implemented `Clone`, you could clone the value --> $DIR/use_of_moved_value_copy_suggestions.rs:4:16 | LL | fn duplicate_t<T>(t: T) -> (T, T) { | ^ consider constraining this type parameter with `Clone` ... LL | (t, t) | - you could clone this value help: consider restricting type parameter `T` | LL | fn duplicate_t<T: Copy>(t: T) -> (T, T) { | ++++++ ``` The `help` is new. On ADTs, we also extend the output with span labels: ``` error[E0507]: cannot move out of static item `FOO` --> $DIR/issue-17718-static-move.rs:6:14 | LL | let _a = FOO; | ^^^ move occurs because `FOO` has type `Foo`, which does not implement the `Copy` trait | note: if `Foo` implemented `Clone`, you could clone the value --> $DIR/issue-17718-static-move.rs:1:1 | LL | struct Foo; | ^^^^^^^^^^ consider implementing `Clone` for this type ... LL | let _a = FOO; | --- you could clone this value help: consider borrowing here | LL | let _a = &FOO; | + ```

``` error[E0507]: cannot move out of `bar`, a captured variable in an `FnMut` closure --> $DIR/borrowck-move-by-capture.rs:9:29 | LL | let bar: Box<_> = Box::new(3); | --- captured outer variable LL | let _g = to_fn_mut(|| { | -- captured by this `FnMut` closure LL | let _h = to_fn_once(move || -> isize { *bar }); | ^^^^^^^^^^^^^^^^ ---- | | | | | variable moved due to use in closure | | move occurs because `bar` has type `Box<isize>`, which does not implement the `Copy` trait | `bar` is moved here | help: clone the value before moving it into the closure | LL ~ let value = bar.clone(); LL ~ let _h = to_fn_once(move || -> isize { value }); | ```

estebank · 2024-04-24T22:28:01Z

@bors r=nnethercote

bors · 2024-04-24T22:28:03Z

📌 Commit ad9a5a5 has been approved by nnethercote

It is now in the queue for this repository.

bors · 2024-04-25T01:00:44Z

⌛ Testing commit ad9a5a5 with merge cb3752d...

bors · 2024-04-25T03:03:00Z

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing cb3752d to master...

rust-timer · 2024-04-25T04:19:42Z

Finished benchmarking commit (cb3752d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.592s -> 673.228s (0.09%)
Artifact size: 315.45 MiB -> 315.45 MiB (-0.00%)

rustbot assigned nnethercote Apr 18, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 18, 2024

estebank changed the title ~~Modify find_expr from Span to better account for closures~~ Provide more context and suggestions in borrowck errors involving closures Apr 19, 2024

estebank force-pushed the clone-o-rama-2 branch from ce141f8 to a8c7820 Compare April 22, 2024 21:41

nnethercote reviewed Apr 22, 2024

View reviewed changes

compiler/rustc_borrowck/src/diagnostics/move_errors.rs Show resolved Hide resolved

nnethercote reviewed Apr 22, 2024

View reviewed changes

estebank force-pushed the clone-o-rama-2 branch from a8c7820 to 17a27e8 Compare April 22, 2024 23:36

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2024

matthiaskrgr mentioned this pull request Apr 23, 2024

Rollup of 4 pull requests #124301

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 23, 2024

estebank force-pushed the clone-o-rama-2 branch from 17a27e8 to 2f29f78 Compare April 23, 2024 19:59

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2024

fmease mentioned this pull request Apr 24, 2024

Rollup of 6 pull requests #124316

Closed

fmease mentioned this pull request Apr 24, 2024

Rollup of 5 pull requests #124329

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 24, 2024

estebank added 3 commits April 24, 2024 22:21

estebank force-pushed the clone-o-rama-2 branch from 2f29f78 to ad9a5a5 Compare April 24, 2024 22:24

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 25, 2024

bors merged commit cb3752d into rust-lang:master Apr 25, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Apr 25, 2024

bors mentioned this pull request Apr 25, 2024

Detect borrow error involving sub-slices and suggest split_at_mut #124313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide more context and suggestions in borrowck errors involving closures #124136

Provide more context and suggestions in borrowck errors involving closures #124136

estebank commented Apr 18, 2024 •

edited

Loading

rustbot commented Apr 18, 2024

nnethercote left a comment

nnethercote Apr 22, 2024

estebank Apr 22, 2024

nnethercote Apr 23, 2024

estebank Apr 23, 2024

nnethercote Apr 24, 2024

nnethercote commented Apr 22, 2024

estebank commented Apr 22, 2024

nnethercote commented Apr 23, 2024

estebank commented Apr 23, 2024

bors commented Apr 23, 2024

matthiaskrgr commented Apr 23, 2024

estebank commented Apr 23, 2024

bors commented Apr 23, 2024

fmease commented Apr 24, 2024

estebank commented Apr 24, 2024

bors commented Apr 24, 2024

bors commented Apr 25, 2024

bors commented Apr 25, 2024

rust-timer commented Apr 25, 2024

Provide more context and suggestions in borrowck errors involving closures #124136

Provide more context and suggestions in borrowck errors involving closures #124136

Conversation

estebank commented Apr 18, 2024 • edited Loading

rustbot commented Apr 18, 2024

nnethercote 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

nnethercote commented Apr 22, 2024

estebank commented Apr 22, 2024

nnethercote commented Apr 23, 2024

estebank commented Apr 23, 2024

bors commented Apr 23, 2024

matthiaskrgr commented Apr 23, 2024

estebank commented Apr 23, 2024

bors commented Apr 23, 2024

fmease commented Apr 24, 2024

estebank commented Apr 24, 2024

bors commented Apr 24, 2024

bors commented Apr 25, 2024

bors commented Apr 25, 2024

rust-timer commented Apr 25, 2024

Overall result: no relevant changes - no action needed

estebank commented Apr 18, 2024 •

edited

Loading