Skip to content
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

match_like_matches_macro: strip refs in suggestion #6532

Merged
merged 1 commit into from
Jan 21, 2021

Conversation

matthiaskrgr
Copy link
Member

fixes #6503

changelog: match_like_matches_macro: strip refs in suggestion (#6503)

@rust-highfive
Copy link

r? @ebroto

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 1, 2021
@alex-700
Copy link
Contributor

alex-700 commented Jan 2, 2021

@matthiaskrgr thanks for PR! But this implementation is not accurate enough. It should also think about a using the variable after matches! macro. For example the following code does not compile and using reference here is meaningful.

enum A { X, Y }

fn foo(x: A) {}

fn main() {
    let z = A::X;
    let _ = matches!(z, A::Y);
    foo(z);
}

@matthiaskrgr matthiaskrgr added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 2, 2021
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

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

@alex-700 makes a good point, although I was a bit confused at the beginning by the example because A is a variable binding and not lowercase. Since the scrutinee is not a reference anymore and the pattern is not a reference pattern, the binding moves the value. The same case slightly modified:

struct S(i32);

fn fun(_val: Option<S>) {}

fn main() {
    let val = Some(S(42));
    
//    let _res = match &val {
//        Some(_a) => true,
//        _ => false,
//    };

    let _res = matches!(val, Some(_a));
    fun(val);
}

(playground)


The current implementation would also fail if the match uses a reference pattern, so match ergonomics does not apply. Consider the following case:

struct S(i32);

fn fun(_val: Option<S>) {}

fn main() {
    let val = Some(S(42));
    
//    let _res = match &val {
//        &Some(ref _a) => true,
//        _ => false,
//    };

    let _res = matches!(val, &Some(ref _a));
    fun(val);
}

(playground)

This would yield a mismatched types error

error[E0308]: mismatched types
  --> src/main.rs:11:30
   |
11 |     let _res = matches!(val, &Some(ref _a));
   |                         ---  ^^^^^^^^^^^^^ expected enum `Option`, found reference
   |                         |
   |                         this expression has type `Option<S>`
   |
   = note:   expected enum `Option<S>`
           found reference `&_`

@matthiaskrgr matthiaskrgr force-pushed the mlmm branch 3 times, most recently from 6a52586 to a34aca2 Compare January 3, 2021 18:41
fixes rust-lang#6503

changelog: match_like_matches_macro: strip refs in suggestion (rust-lang#6503)
@matthiaskrgr matthiaskrgr added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 3, 2021
@matthiaskrgr
Copy link
Member Author

I think I got all the options covered now..?
There's still some cases where we could remove a & but the suggestion will still compile.

@ebroto
Copy link
Member

ebroto commented Jan 16, 2021

r? @llogiq (I'm leaving the team, so I'm reassigning my PRs to other active members)

@rust-highfive rust-highfive assigned llogiq and unassigned ebroto Jan 16, 2021
@llogiq
Copy link
Contributor

llogiq commented Jan 21, 2021

From my point of view, this looks like a very solid improvement, also the tests seem to cover all cases. Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 21, 2021

📌 Commit 39f39d5 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Jan 21, 2021

⌛ Testing commit 39f39d5 with merge a982ab4...

@bors
Copy link
Contributor

bors commented Jan 21, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing a982ab4 to master...

@bors bors merged commit a982ab4 into rust-lang:master Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

match_like_matches_macro: strip refs in suggestion
6 participants