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

Matching on a place in borrowed context should no longer suggest `ref` #52423

Open
scottmcm opened this Issue Jul 16, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@scottmcm
Copy link
Member

scottmcm commented Jul 16, 2018

https://play.rust-lang.org/?gist=a88b996b2d1c86d1f6d32eee66c70cd4&version=stable&mode=debug&edition=2015

struct MyStruct<T> {
    x: Option<T>,
}

fn foo<T>(s: &MyStruct<T>) {
    match s.x {
        Some(_a) => {}
        None => {}
    }
}

This suggests using ref:

error[E0507]: cannot move out of borrowed content
 --> src/lib.rs:8:11
  |
8 |     match s.x {
  |           ^ cannot move out of borrowed content
9 |         Some(_a) => {}
  |              -- hint: to prevent move, use `ref _a` or `ref mut _a`

With match ergonomics now stable, it would be better to instead suggest borrowing the field:

error[E0507]: cannot move out of borrowed content
 --> src/lib.rs:8:11
  |
8 |     match s.x {
  |           ^ cannot move out of borrowed content
  |           - hint: to prevent move, use `&s.x` or `&mut s.x`

To help people move away from needing to know that ref exists.

(Context that lead me to open this: https://discordapp.com/channels/273534239310479360/274215136414400513/468303728986816512)

@csmoe

This comment has been minimized.

Copy link
Member

csmoe commented Jul 16, 2018

@ashtneoi

This comment has been minimized.

Copy link
Contributor

ashtneoi commented Jul 16, 2018

Yeah, I can probably tackle this.

@ashtneoi

This comment has been minimized.

Copy link
Contributor

ashtneoi commented Jul 17, 2018

For something like this...

error[E0507]: cannot move out of borrowed content
  --> src/main.rs:12:11
   |
12 | fn bar<T>(&MyStruct { x: _a }: &MyStruct<T>) { }
   |           ^^^^^^^^^^^^^^^--^^
   |           |              |
   |           |              hint: to prevent move, use `ref _a` or `ref mut _a`
   |           cannot move out of borrowed content

...should it suggest changing the type, the pattern (which might be tricky, idk), or neither?

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Jul 17, 2018

@ashtneoi My personal goal is to have the compiler never mention ref, so I'd hope for it to suggest

fn bar<T>(MyStruct { x: _a }: &MyStruct<T>)

...but I have no idea how hard that might be.

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Jul 17, 2018

...Can we lint against top-level type declarations like that? It's really confusing trying to figure out the type of _a.

@ashtneoi

This comment has been minimized.

Copy link
Contributor

ashtneoi commented Jul 18, 2018

Oh, one more question (sorry): In a lot of cases, NLL already has better suggestions. Should I bother fixing the non-NLL suggestions or just focus on NLL assuming it'll land sometime soon?

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Jul 18, 2018

Oh, good point; I didn't try NLL. Only fixing the NLL ones sounds like a smart plan.

@ashtneoi

This comment has been minimized.

Copy link
Contributor

ashtneoi commented Jul 30, 2018

It took a ton of reading (which is why I'm only now figuring it out), but I managed to put each binding's top-level pattern span into the LocalDecl's VarBindingForm. That means it's definitely feasible to suggest a change to the entire pattern (rather than just the binding site or the RHS value). Things should be easier from here on and I'll hopefully have a PR in a few days.

bors added a commit that referenced this issue Aug 16, 2018

Auto merge of #53147 - ashtneoi:dont-suggest-ref, r=estebank
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 added a commit that referenced this issue Aug 16, 2018

Auto merge of #53147 - ashtneoi:dont-suggest-ref, r=estebank
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?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment