Add help on reinitialization between move and access #85686

ptrojahn · 2021-05-25T17:47:25Z

rust-highfive · 2021-05-25T17:47:28Z

(rust-highfive has picked a reviewer for you, use r? to override)

estebank · 2021-05-26T02:54:48Z

compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs

+        // Check if we can reach these reinits from a move location.
+        let mut maybe_reinitialized = false;
+        let mut visited = FxHashSet::default();
+        let mut stack = reinits;


Any reason for the rename of reinits?

At first we just collect the reinits we found and then use those to initialize the stack for the search. I thought this would be less confusing. Alternatively we could rename reinits to something like stack_reinits and remove the rename.

The rename makes sense to me.

estebank · 2021-05-26T02:55:53Z

compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs

@@ -1478,6 +1487,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
        }));

        let mut visited = FxHashSet::default();
+        let mut reinits = vec![];
+        let mut move_locations = vec![];


This should likely be an FxHashSet instead of a Vec.

estebank · 2021-05-26T03:00:25Z

src/test/ui/borrowck/issue-83760.stderr

+LL |     while let Some(foo) = val {
+   |                    ^^^ value moved here, in previous iteration of loop
+   |
+   = help: make sure that it is reinitialized on all paths


I'm not entirely happy with the help wording because it feels like it might lead to confusion. It's likely better than we have already, but I'd feel better with a more newcomer friendly wording and potentially some spans pointing at the move locations when stating this.

How about something like:

46 | while let Some(_foo) = val { //~ ERROR use of moved value | ^^^^ | | | the value might not be reinitialized at this point | value moved here, in previous iteration of loop

We could also print the spans of the reinits that might get skipped, though I think this would probably clutter the output.

We could also print the spans of the reinits that might get skipped, though I think this would probably clutter the output.

Could we see what that would look like in the tests we have available? Something we do in other cases is provide a full print out if few things are involved, and hide almost everything when they become too verbose. We could do something like that, but I'd like to see what it'd look like first.

ptrojahn · 2021-06-13T16:17:34Z

I actually think this is an improvement:

error[E0382]: use of moved value
   --> scratchpad.rs:116:20
    |
116 |     while let Some(_foo) = val { //~ ERROR use of moved value
    |                    ^^^^ value moved here, in previous iteration of loop
117 |         if true {
118 |             val = None;
    |             ---------- this reinitialization might get skipped
    |
    = note: move occurs because value has type `Struct`, which does not implement the `Copy` trait

error[E0382]: use of moved value: `foo`
   --> scratchpad.rs:132:14
    |
126 |     let mut foo = Some(Struct);
    |         ------- move occurs because `foo` has type `Option<Struct>`, which does not implement the `Copy` trait
127 |     let _x = foo.unwrap();
    |                  -------- `foo` moved due to this method call
128 |     if true {
129 |         foo = Some(Struct);
    |         ------------------ this reinitialization might get skipped
...
132 |     let _y = foo; //~ ERROR use of moved value: `foo`
    |              ^^^ value used here after move
    |
note: this function takes ownership of the receiver `self`, which moves `foo`
   --> /home/paul/projects/rust/library/core/src/option.rs:383:25
    |
383 |     pub const fn unwrap(self) -> T {
    |                         ^^^^

If we have multiple ones we could do something like:

error[E0382]: use of moved value: `foo`
   --> scratchpad.rs:148:14
    |
136 |     let mut foo = Some(Struct);
    |         ------- move occurs because `foo` has type `Option<Struct>`, which does not implement the `Copy` trait
137 |     let _x = foo.unwrap();
    |                  -------- `foo` moved due to this method call
...
148 |     let _y = foo; //~ ERROR use of moved value: `foo`
    |              ^^^ value used here after move
    |
note: these reinitializations might get skipped
   --> scratchpad.rs:141:9
    |
141 |         foo = Some(Struct);
    |         ^^^^^^^^^^^^^^^^^^
142 |     } else if 2 + 2 == 3 {
143 |         foo = Some(Struct);
    |         ^^^^^^^^^^^^^^^^^^
144 |     } else if 2 + 2 == 5 {
145 |         foo = Some(Struct);
    |         ^^^^^^^^^^^^^^^^^^
note: this function takes ownership of the receiver `self`, which moves `foo`
   --> /home/paul/projects/rust/library/core/src/option.rs:383:25
    |
383 |     pub const fn unwrap(self) -> T {
    |                         ^^^^

I capped the number of printed reinitializations at 3. What do you think?

estebank

I love it and am ok to merge as is, but there's a small tweak I'd like to address as well to avoid accidentally misleading when there are more than 3 reinits.

estebank · 2021-06-14T15:50:20Z

compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs

                if let UseSpans::PatUse(span) = move_spans {
-                    err.span_suggestion_verbose(
-                        span.shrink_to_lo(),
-                        &format!(
-                            "borrow this field in the pattern to avoid moving {}",
-                            self.describe_place(moved_place.as_ref())
-                                .map(|n| format!("`{}`", n))
-                                .unwrap_or_else(|| "the value".to_string())
-                        ),
-                        "ref ".to_string(),
-                        Applicability::MachineApplicable,
-                    );
-                    in_pattern = true;
+                    if maybe_reinitialized_locations.is_empty() {


You can merge these two lines with something like the following to avoid one level of indentation:

if let (UseSpans::PatUse(span), []) = (move_spans, &maybe_reinitialized_locations[..]) {

estebank · 2021-06-14T15:51:23Z

compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs

+        // Check if we can reach these reinits from a move location.
+        let reinits_reachable = reinits
+            .into_iter()
+            .filter(|reinit| {
+                let mut visited = FxHashSet::default();
+                let mut stack = vec![*reinit];
+                while let Some(location) = stack.pop() {
+                    if !visited.insert(location) {
+                        continue;
+                    }
+                    if move_locations.contains(&location) {
+                        return true;
+                    }
+                    stack.extend(predecessor_locations(self.body, location));
+                }
+                false
+            })
+            .collect::<Vec<Location>>();


Does this happen unconditionally, even in the happy path? We might want to do a perf run just to be sure we don't regress things too much.

The only place where get_moved_indexes is called is in report_use_of_moved_or_uninitialized. I am pretty sure that this function is only called if we know for sure that an error occurred. This shouldn't run in normal error free compilation.

estebank · 2021-06-14T15:53:09Z

compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs

+            let reinit_spans = maybe_reinitialized_locations
+                .iter()
+                .take(3)
+                .map(|loc| {
+                    self.move_spans(self.move_data.move_paths[mpi].place.as_ref(), *loc)
+                        .args_or_use()
+                })
+                .collect::<Vec<Span>>();
+            if reinit_spans.len() == 1 {
+                err.span_label(reinit_spans[0], "this reinitialization might get skipped");
+            } else if reinit_spans.len() > 1 {
+                err.span_note(
+                    MultiSpan::from_spans(reinit_spans),
+                    "these reinitializations might get skipped",
+                );
+            }


I love the new output, but I would like it if for the case where there are more than 3 we provided something like

note: these 3 reinitializations and N others might get skipped --> $DIR/issue-83760.rs:30:9 | LL | foo = Some(Struct); | ^^^^^^^^^^^^^^^^^^ LL | } else if true { LL | foo = Some(Struct); | ^^^^^^^^^^^^^^^^^^ LL | } else if true { LL | foo = Some(Struct); | ^^^^^^^^^^^^^^^^^^

bors · 2021-07-07T03:43:34Z

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

Fixes rust-lang#83760

estebank · 2021-07-18T00:40:16Z

@bors r+

bors · 2021-07-18T00:40:17Z

📌 Commit 34ff259 has been approved by estebank

bors · 2021-07-18T02:13:22Z

⌛ Testing commit 34ff259 with merge 77d1559...

bors · 2021-07-18T04:54:06Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 77d1559 to master...

rust-highfive assigned estebank May 25, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2021

estebank reviewed May 26, 2021

View reviewed changes

ptrojahn force-pushed the loop_reinitialize branch from 7fdcb77 to ab1a719 Compare June 13, 2021 16:36

estebank approved these changes Jun 14, 2021

View reviewed changes

ptrojahn force-pushed the loop_reinitialize branch from ab1a719 to 76f3613 Compare June 15, 2021 13:31

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2021

Add help on reinitialization between move and access

34ff259

Fixes rust-lang#83760

ptrojahn force-pushed the loop_reinitialize branch from 76f3613 to 34ff259 Compare July 7, 2021 16:46

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 Jul 18, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 18, 2021

bors merged commit 77d1559 into rust-lang:master Jul 18, 2021

rustbot added this to the 1.55.0 milestone Jul 18, 2021

bors mentioned this pull request Jul 18, 2021

Use multipart_suggestions more #86754

Merged

philjb mentioned this pull request Feb 22, 2024

Rustc identifies use of moved value correctly in while let loop match correctly, but the suggested fix results in infinite loop. #121466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add help on reinitialization between move and access #85686

Add help on reinitialization between move and access #85686

ptrojahn commented May 25, 2021

rust-highfive commented May 25, 2021

estebank May 26, 2021

ptrojahn May 28, 2021 •

edited

Loading

estebank Jun 2, 2021

estebank May 26, 2021

ptrojahn May 28, 2021

estebank May 26, 2021

ptrojahn May 28, 2021

estebank Jun 2, 2021

ptrojahn commented Jun 13, 2021

estebank left a comment

estebank Jun 14, 2021

ptrojahn Jun 15, 2021

estebank Jun 14, 2021

ptrojahn Jun 15, 2021

estebank Jun 14, 2021

ptrojahn Jun 15, 2021

bors commented Jul 7, 2021

estebank commented Jul 18, 2021

bors commented Jul 18, 2021

bors commented Jul 18, 2021

bors commented Jul 18, 2021

Add help on reinitialization between move and access #85686

Add help on reinitialization between move and access #85686

Conversation

ptrojahn commented May 25, 2021

rust-highfive commented May 25, 2021

Choose a reason for hiding this comment

ptrojahn May 28, 2021 • edited Loading

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

Choose a reason for hiding this comment

ptrojahn commented Jun 13, 2021

estebank 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

Choose a reason for hiding this comment

bors commented Jul 7, 2021

estebank commented Jul 18, 2021

bors commented Jul 18, 2021

bors commented Jul 18, 2021

bors commented Jul 18, 2021

ptrojahn May 28, 2021 •

edited

Loading