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

Allow explicit matches on ! without warning #55119

Merged
merged 3 commits into from
Oct 20, 2018

Conversation

varkor
Copy link
Member

@varkor varkor commented Oct 16, 2018

It's now possible to explicitly match on ! without an unreachable code warning. This seems desirable as promoting explicitness.

Fixes #55116.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(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 Oct 16, 2018
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Oct 16, 2018

What about this? I think it should get the warning.

fn foo(x: !) {
    return;
    match x {}
}

Somehow it "manages" to not warn about fn foo(x: !) { x } even though the x here is just as unreachable, can't match x get a similar exception?

@varkor
Copy link
Member Author

varkor commented Oct 16, 2018

I've spotted that this the root cause here is actually different than I originally thought; the fix should be simpler and address those other cases.

@rust-highfive

This comment has been minimized.

@varkor
Copy link
Member Author

varkor commented Oct 16, 2018

Turns out match was being special-cased before...

@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Oct 16, 2018

LGTM :) Except of course that the test is failing...?

@rust-highfive

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me with new test


fn main() {
return;
match () { //~ ERROR: unreachable expression
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to test

fn main() {
  match (return) {
    () => ()
  }
}

too (this should warn, I think, on the arm?)

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment, this won't warn at all. I'll try to get the arms warning.

@varkor
Copy link
Member Author

varkor commented Oct 18, 2018

The arm warning message could be improved in the future, but this is definitely an uncommon case, so the new warning should suffice for now.

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Oct 18, 2018

📌 Commit 2634646 has been approved by nikomatsakis

@bors 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 Oct 18, 2018
@bors
Copy link
Contributor

bors commented Oct 20, 2018

⌛ Testing commit 2634646 with merge 22cc2ae...

bors added a commit that referenced this pull request Oct 20, 2018
Allow explicit matches on ! without warning

It's now possible to explicitly match on `!` without an unreachable code warning. This seems desirable as promoting explicitness.

Fixes #55116.
@bors
Copy link
Contributor

bors commented Oct 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 22cc2ae to master...

@bors bors merged commit 2634646 into rust-lang:master Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants