regression: rustc suggests .as_ref() at incorrect location and other spans have regressed

[camelid]

Code

Playground (regressed rustc version)

fn foo(opt: &Option<Box<i32>>) -> String {
    opt.map(|x| x.to_string()).unwrap_or_else(String::new)
}

Output (stable 1.56.0, good)

error[E0507]: cannot move out of `*opt` which is behind a shared reference
 --> src/lib.rs:2:5
  |
2 |     opt.map(|x| x.to_string()).unwrap_or_else(String::new)
  |     ^^^ move occurs because `*opt` has type `Option<Box<i32>>`, which does not implement the `Copy` trait
  |
help: consider borrowing the `Option`'s content
  |
2 |     opt.as_ref().map(|x| x.to_string()).unwrap_or_else(String::new)
  |        +++++++++

For more information about this error, try `rustc --explain E0507`.

Output (beta 1.57.0-beta.2, incorrect)

error[E0507]: cannot move out of `*opt` which is behind a shared reference
 --> src/lib.rs:2:5
  |
2 |     opt.map(|x| x.to_string()).unwrap_or_else(String::new)
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ move occurs because `*opt` has type `Option<Box<i32>>`, which does not implement the `Copy` trait
  |
help: consider borrowing the `Option`'s content
  |
2 |     opt.map(|x| x.to_string()).as_ref().unwrap_or_else(String::new)
  |                               +++++++++

For more information about this error, try `rustc --explain E0507`.

[camelid]

My hunch is that this regression is caused by the change in error spans between stable and beta, which also feels like a regression to me since it makes the span less specific:

stable:

2 |     opt.map(|x| x.to_string()).unwrap_or_else(String::new)
  |     ^^^ move occurs because `*opt` has type `Option<Box<i32>>`, which does not implement the `Copy` trait

beta:

2 |     opt.map(|x| x.to_string()).unwrap_or_else(String::new)
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ move occurs because `*opt` has type `Option<Box<i32>>`, which does not implement the `Copy` trait

[WaffleLapkin]

searched nightlies: from nightly-2021-09-29 to nightly-2021-10-21
regressed nightly: nightly-2021-10-01
searched commit range: 1149193...aa7aca3
regressed commit: 4aa7879

bisected with cargo-bisect-rustc v0.6.1

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --preserve --test-dir=tstdr --end=2021-10-21 --script=./test.sh

test.sh:

#!/bin/sh

cargo check 2>&1 | grep " ^^^ move occurs because"

[camelid]

cc @Aaron1011 @estebank

The new spans caused this suggestion regression, and I feel like they make the errors harder to read in some cases.

[Aaron1011]

@camelid: Setting aside the incorrect suggestion for the moment, I think the new span is better. By itself, pointing to opt does not explain why a move is occuring, as not every usage of a variable name is a move. Pointing to the the method call helps emphasize that the method call is responsible for the move.

However, I think adding a note could help emphasize that the method is causing the move (we already do this for other borrowcheck errors).

The incorrect suggestion is tricky - I'll see if I can do something about that.

[camelid]

Setting aside the incorrect suggestion for the moment, I think the new span is better. By itself, pointing to opt does not explain why a move is occuring, as not every usage of a variable name is a move. Pointing to the the method call helps emphasize that the method call is responsible for the move.

I understand, and I can see how highlighting the whole call is more correct in some ways, but then a lot of code is highlighted, which can be visually distracting. I wonder if it could make sense to highlight just the method name?

opt.map(|x| x.to_string())
^^^^^^^

Although that looks a bit weird. Another option is something like this:

opt.map(|x| x.to_string())
^^^ --- `opt` moved by `map` here

Additionally, a large part of why I feel the new span is visually distracting is that move occurs because `*opt` has type `Option<Box<i32>>`, which does not implement the `Copy` trait takes up a lot of space as a span_label with the new span. Perhaps the new span could be made easier to read if the label were moved to a note?

[WaffleLapkin]

When a function call is multiline a lot of unnecessary context is also included:

error[E0507]: cannot move out of `*v` which is behind a shared reference
  --> src/lib.rs:26:13
   |
26 |       let r = v.long_function_with_many_params(
   |  _____________^
27 | |         &[1, 2, 3, 4],
28 | |         12..42,
29 | |         b"ababababbaabab",
...  |
32 | |         Flag::Yes,
33 | |     )?;
   | |_____^ move occurs because `*v` has type `Type`, which does not implement the `Copy` trait

I personally agree with @camelid that a diagnostic like this would be better:

error[E0507]: cannot move out of `*v` which is behind a shared reference
  --> src/lib.rs:26:13
   |
26 |       let r = v.long_function_with_many_params(
   |               ^ ------------------------------ `v` moved because of this function call
   |
   | note: move occurs because `*v` has type `Type`, which does not implement the `Copy` trait

While it's nice to highlight the function call that caused the move, I don't think that it's necessary to highlight all arguments. And the function probably shouldn't be highlighted with the receiver, they are in a sense separate entities that contributed to the error, so their highlighting should be split/different.

[camelid]

I'll see if I can experiment with some different solutions and see how well they work in practice.

[camelid]

Hmm, I'm not sure if there's a way to separately point to the variable and the function name without either making Expr.span a MultiSpan (which doesn't seem good) or adding a global lookup table that borrowck can use. The first change would probably be a huge change, while the second seems hacky and bug-prone.

For now, I think I could split the label into a label and a note like discussed above without it being too big of a change. But that also wouldn't fix this regression.

[camelid]

Some of the diagnostic changes in #89110 IMO feel like they make the errors much more confusing:

 error[E0597]: `z` does not live long enough
   --> $DIR/regions-escape-loop-via-vec.rs:7:17
    |
 LL |         _y.push(&mut z);
-   |         --      ^^^^^^ borrowed value does not live long enough
-   |         |
+   |         --------^^^^^^-
+   |         |       |
+   |         |       borrowed value does not live long enough
    |         borrow later used here

Does it make sense to revert that PR for now until we can figure out a way to improve the errors that doesn't cause regressions and make some errors more confusing?

[camelid]

I'm also surprised that there wasn't a test for the as_ref() suggestion that would have caught this regression; once this is fixed, it seems like something that should be tested.

[camelid]

Another thing I wanted to note is that changing the spans of expressions may reduce debuginfo precision, as the .mir test changes show. For example, based on the diff, it looks like the span of the temporary variable representing the receiver is now the span of the whole call, which could be extremely confusing:

-    let mut _3: &str;                    // in scope 0 at $DIR/no-spurious-drop-after-call.rs:9:20: 9:22
+    let mut _3: &str;                    // in scope 0 at $DIR/no-spurious-drop-after-call.rs:9:20: 9:34

[apiraino]

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-high

[pnkfelix]

@rustbot assign @camelid

[pnkfelix]

I spoke to @camelid ; they will not have a lot of time to investigate this in the near term future. So, assigning self to determine if a revert of PR #89110 is warranted (and maybe to identify alternative solutions).

[pnkfelix]

@rustbot claim

[Mark-Simulacrum]

@pnkfelix we're ~1 week out from release, will you be able to prepare the revert in time, or do we need to find someone else to do that?

[estebank]

I'd looked at this a few weeks back: the problem is that reverting this causes another bug to resurface. We need two different spans for both these suggestions (and the one that were "fixed" by the PR with the regression) to work correctly :-/

If needed I can try and fix it next week before the release, but I'll need someone to be responsive for the review.

[fogti]

Could, as a temporary somewhat-partial mitigation, the as_ref suggestion be downgraded from {points directly at a (currently wrong) location in the code} to {it is suggested in a note that an as_ref should be inserted near the highlighted code, but without a direct pointer with +++}?. That would reduce the confusion a bit, I suppose.

[estebank]

Triage: fixed in current beta:

error[E0507]: cannot move out of `*opt` which is behind a shared reference
 --> src/lib.rs:2:5
  |
2 |     opt.map(|x| x.to_string()).unwrap_or_else(String::new)
  |     ^^^^----------------------
  |     |   |
  |     |   `*opt` moved due to this method call
  |     move occurs because `*opt` has type `Option<Box<i32>>`, which does not implement the `Copy` trait
  |
note: this function takes ownership of the receiver `self`, which moves `*opt`
help: consider calling `.as_ref()` to borrow the type's contents
  |
2 |     opt.as_ref().map(|x| x.to_string()).unwrap_or_else(String::new)
  |         +++++++++

[WaffleLapkin]

Hm, it seems like there are already tests for as_ref suggestion, namely option-content-move.rs, as-ref.rs and as-ref-2.rs, should this be closed then?

[fogti]

should we wait for the commits which seem to fix this to hit stable, or can we close this already?

[estebank]

We can close now.