For move errors, suggest match ergonomics instead of `ref` #53147

ashtneoi · 2018-08-07T08:20:11Z

Partially fixes issue #52423. Also makes errors and suggestions more consistent between move-from-place and move-from-value errors.

Limitations:

Only the first pattern in a match arm can have a "consider removing this borrow operator" suggestion.
Suggestions don't always compile as-is (see the TODOs in the test for details).

Sorry for the really long test. I wanted to make sure I handled every case I could think of, and it turned out there were a lot of them.

Questions:

Is there any particular applicability I should set on those suggestions?
Are the notes about the Copy trait excessive?

rust-highfive · 2018-08-07T08:20:13Z

r? @cramertj

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

ashtneoi · 2018-08-07T08:24:22Z

There are still tests to fix and bless, but I figured I'd see if this is the right idea before I start on that.

cramertj · 2018-08-07T16:47:13Z

src/librustc_mir/borrow_check/move_errors.rs

                    }
+                    err.span_suggestion(
+                        pat_span,


Can you change this span to point at the span of the &/&mut rather than the whole pattern? I think it would make it more clear exactly what you're suggesting to remove.

Although that would mean that the "suggestion" now points at the wrong place, meaning that the applicability would need to indicate that it was intended to be read and performed by a human. Perhaps leaving as-is but making the change to the error message I suggested below would be better?

Yeah, changing the help message looks like it would be clear enough. Good call.

We could synthesize a new span from pat_span by looking at it's string constents and remove the leading &, whitespace, mut (when appropriate) and any further whitespace. Once we've done that, we can create a new span that points at sp.lo()~sp.hi()-snippet.len(). (I don't think that this is something urgent to do in this PR.)

Ooh, thanks for reminding me about the whitespace between & and mut.

cramertj · 2018-08-07T16:55:13Z

src/librustc_mir/borrow_check/move_errors.rs

                    }
+                    err.span_suggestion(
+                        pat_span,
+                        "consider removing this borrow operator",


Nit: perhaps "consider removing the &" / "consider removing the &mut" would be clearer than "consider removing this borrow operator"?

With the suggestion next to it it works as an introduction to the jargon that is sadly used in other places. The suggestion code should be enough to make it clear what we're telling the user to do, despite the jargon. That being said, the text could be "consider not borrowing here".

IMO "consider removing this borrow operator: Foo::Bar(..)" looks confusing, since "this" makes it sound like it's referring to Foo::Bar(..) as a "borrow operator", which isn't what's going on. I personally find "consider removing the &: Foo::Bar(...)" easier to read.

In the (probably rare) case where the pattern is a double reference (&&Foo::Bar(..)), "consider not borrowing" would be sorta imprecise. But that might not be worth worrying about.

Although, now that I think about it, I guess we could strip all the leading &/&mut instead of just the first one.

That is if we don't change the span either. If we do, this could look like this:

error[E0507]: cannot move out of borrowed content --> $DIR/dont-suggest-ref.rs:103:18 | LL | let &X(_t) = s; | - -- ^ cannot move out of borrowed content | | | | | data moved here | | move occurs because `_t` has type `Y`, which does not implement the `Copy` trait | help: consider removing this borrow

cramertj · 2018-08-07T17:06:18Z

This looks great to me so far! Thanks so much for doing this-- I'm really excited about these new errors.

Is there any particular applicability I should set on those suggestions?

It seems like they're MachineApplicable to me, but @estebank would know better what are guidelines are for when something is rigorous enough to be considered MachineApplicable.

Are the notes about the Copy trait excessive?

I think it seems reasonable to make a note about Copy (although the current "cannot move out of borrowed content" error does not), but IMO re-showing the span results in too much vertical space usage. Perhaps the span_note in move_error_labels could be made into a plain note instead?

cramertj · 2018-08-07T17:06:29Z

src/librustc_mir/borrow_check/move_errors.rs

-                _ => false,
-            },
+
+            err.span_note(


estebank

Is there any particular applicability I should set on those suggestions?

The examples seem to be machine applicable. That being said, I am slightly worried about knock on effects on code after this error that is expecting a mutable borrow, for example. After applying the changes you will see new errors. This is fine most of the time, but it is less than ideal. I would go for MachineApplicable for now.

estebank · 2018-08-07T17:43:01Z

src/librustc_mir/borrow_check/move_errors.rs

+            err.span_note(
+                binding_span,
+                &format!(
+                    "move occurs because {} has type `{}`, \


move occurs because `{}`

estebank · 2018-08-07T17:44:32Z

src/librustc_mir/borrow_check/move_errors.rs

                    }
+                    err.span_suggestion(
+                        pat_span,
+                        "consider removing this borrow operator",


With the suggestion next to it it works as an introduction to the jargon that is sadly used in other places. The suggestion code should be enough to make it clear what we're telling the user to do, despite the jargon. That being said, the text could be "consider not borrowing here".

estebank · 2018-08-07T17:59:44Z

src/librustc_mir/borrow_check/move_errors.rs

                    }
+                    err.span_suggestion(
+                        pat_span,


We could synthesize a new span from pat_span by looking at it's string constents and remove the leading &, whitespace, mut (when appropriate) and any further whitespace. Once we've done that, we can create a new span that points at sp.lo()~sp.hi()-snippet.len(). (I don't think that this is something urgent to do in this PR.)

ashtneoi · 2018-08-07T18:40:07Z

Just realized that when destructuring a tuple, array, or struct, you end up with duplicate suggestions. Here's an example:

#![feature(nll)]

struct X {
    a: String,
    b: String,
}

fn main() {
    let x = X {
        a: String::new(),
        b: String::new(),
    };
    let &X { a, b } = &x;
}

error[E0507]: cannot move out of borrowed content
  --> a.rs:13:23
   |
13 |     let &X { a, b } = &x;
   |              -  -     ^^ cannot move out of borrowed content
   |              |  |
   |              |  ... and here
   |              data moved here
   |
note: move occurs because a has type `std::string::String`, which does not implement the `Copy` trait
  --> a.rs:13:14
   |
13 |     let &X { a, b } = &x;
   |              ^
note: move occurs because b has type `std::string::String`, which does not implement the `Copy` trait
  --> a.rs:13:17
   |
13 |     let &X { a, b } = &x;
   |                 ^
help: consider removing this borrow operator
   |
13 |     let X { a, b } = &x;
   |         ^^^^^^^^^^
help: consider removing this borrow operator
   |
13 |     let X { a, b } = &x;
   |         ^^^^^^^^^^

I'm guessing I should probably collect them together and dedup them based on their span?

estebank · 2018-08-07T18:48:14Z

I'm guessing I should probably collect them together and dedup them based on their span?

That would be a great idea, and also present:

note: move occurs because `a` has type `std::string::String` and `b` has type `std::string::String`, which do not implement the `Copy` trait
  --> a.rs:13:14
   |
13 |     let &X { a, b } = &x;
   |              ^  ^

ashtneoi · 2018-08-07T18:56:21Z

Also it turns out I messed up the logic for derefs that don't start with * (as with index expressions)---it should fall back to adding &, not just give up.

Edit: fixed.

cramertj · 2018-08-07T19:07:42Z

r? @estebank -- they know diagnostics better than I, and I've given what feedback I had to offer.

ashtneoi · 2018-08-07T23:01:47Z

@cramertj Thanks for your help!

ashtneoi · 2018-08-12T21:54:02Z

So I'm a little worried about combining the Copy notes into one, since if there's more than two or three vars then the note will end up being really long ("move occurs because a has type std::string::String, b has type std::string::String, c has type std::string::String, d has type std::string::String, ..."). Maybe something more like

note: move occurs because these variables have types which do not implement the `Copy` trait
  --> a.rs:13:14
   |
13 |     let &X { a, b } = &x;
   |              ^  ^

It might also be nice to add a label on each variable showing its type, but I guess that would end up being verbose again.

estebank · 2018-08-16T01:50:07Z

@bors r+

bors · 2018-08-16T01:50:07Z

📌 Commit f4229b8 has been approved by estebank

bors · 2018-08-16T03:15:23Z

⌛ Testing commit f4229b8 with merge 894c01f23b458da53696ae5bef2213d44ac208f3...

bors · 2018-08-16T04:16:07Z

💔 Test failed - status-travis

ashtneoi · 2018-08-16T05:29:09Z

Looks like this is just #53332 on Travis' end. Hopefully splitting the test into more pieces will help, and I'll take a look at fixing #53332 when I'm able.

estebank · 2018-08-16T07:38:34Z

@bors r+

bors · 2018-08-16T07:38:34Z

📌 Commit 0023dd9 has been approved by estebank

bors · 2018-08-16T08:45:39Z

⌛ Testing commit 0023dd9 with merge 142bb27...

For move errors, suggest match ergonomics instead of `ref` Partially fixes issue #52423. Also makes errors and suggestions more consistent between move-from-place and move-from-value errors. Limitations: - Only the first pattern in a match arm can have a "consider removing this borrow operator" suggestion. - Suggestions don't always compile as-is (see the TODOs in the test for details). Sorry for the really long test. I wanted to make sure I handled every case I could think of, and it turned out there were a lot of them. Questions: - Is there any particular applicability I should set on those suggestions? - Are the notes about the `Copy` trait excessive?

bors · 2018-08-16T11:18:07Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 142bb27 to master...

rust-highfive assigned cramertj Aug 7, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 7, 2018

ashtneoi force-pushed the dont-suggest-ref branch from 9c4ca2d to 0ff2dea Compare August 7, 2018 08:33

This comment has been minimized.

Sign in to view

cramertj reviewed Aug 7, 2018

View reviewed changes

src/librustc_mir/borrow_check/move_errors.rs Outdated

_ => false,

},

err.span_note(

Copy link

Member

cramertj Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one

estebank reviewed Aug 7, 2018

View reviewed changes

rust-highfive assigned estebank and unassigned cramertj Aug 7, 2018

This comment has been minimized.

Sign in to view

ashtneoi force-pushed the dont-suggest-ref branch from 8ddc3d6 to 2562312 Compare August 8, 2018 07:09

This comment has been minimized.

Sign in to view

estebank 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2018

ashtneoi force-pushed the dont-suggest-ref branch 2 times, most recently from 60b3e5e to 76d40fc Compare August 12, 2018 20:34

This comment has been minimized.

Sign in to view

ashtneoi added 8 commits August 15, 2018 15:14

Add another FIXME about suggestions

81c27c6

Allocate fewer Strings at a time

3c3a7ba

Fix compiletest JSON error message

6a24abb

Test move-into-Fn/FnMut errors too

a5b008c

Bless tests

bd2b54c

Change a method name and clarify a comment

671d7e0

Add files I forgot to commit earlier

4c4e1ef

Re-bless test

f4229b8

ashtneoi force-pushed the dont-suggest-ref branch from e1e7064 to f4229b8 Compare August 15, 2018 22:26

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 Aug 16, 2018

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

ashtneoi added 2 commits August 15, 2018 22:35

Move tests into their own directory

f335fb0

Split tests more and bless them again

0023dd9

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 Aug 16, 2018

bors merged commit 0023dd9 into rust-lang:master Aug 16, 2018

ashtneoi deleted the dont-suggest-ref branch August 8, 2020 20:12

For move errors, suggest match ergonomics instead of ref #53147

For move errors, suggest match ergonomics instead of ref #53147

Conversation

ashtneoi commented Aug 7, 2018 • edited Loading

rust-highfive commented Aug 7, 2018

ashtneoi commented Aug 7, 2018

This comment has been minimized.

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

cramertj Aug 7, 2018 • 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

cramertj commented Aug 7, 2018

Choose a reason for hiding this comment

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

ashtneoi commented Aug 7, 2018

estebank commented Aug 7, 2018

ashtneoi commented Aug 7, 2018 • edited Loading

cramertj commented Aug 7, 2018

ashtneoi commented Aug 7, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ashtneoi commented Aug 12, 2018

estebank commented Aug 16, 2018

bors commented Aug 16, 2018

bors commented Aug 16, 2018

bors commented Aug 16, 2018

ashtneoi commented Aug 16, 2018

estebank commented Aug 16, 2018

bors commented Aug 16, 2018

bors commented Aug 16, 2018

bors commented Aug 16, 2018

For move errors, suggest match ergonomics instead of `ref` #53147

For move errors, suggest match ergonomics instead of `ref` #53147

ashtneoi commented Aug 7, 2018 •

edited

Loading

cramertj Aug 7, 2018 •

edited

Loading

ashtneoi commented Aug 7, 2018 •

edited

Loading