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

Feature gate &Void's uninhabitedness. #39151

Merged
merged 1 commit into from
Jan 19, 2017

Conversation

canndrew
Copy link
Contributor

Here's a totally crazy PR which should never be merged.

References to empty types are only considered empty if feature(never_type) is
enabled.
@canndrew
Copy link
Contributor Author

Ping @nikomatsakis @arielb1

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@arielb1
Copy link
Contributor

arielb1 commented Jan 18, 2017

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 18, 2017

📌 Commit 2c6bc18 has been approved by arielb1

@arielb1 arielb1 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 18, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jan 18, 2017

The non-reference parts seem to be mostly uncontroversial, except for the issue that you can't have a match expression in a macro without risking unreachable code warnings. Not sure what is the best way to handle that. Macro reachability hygiene?

@Ericson2314
Copy link
Contributor

This will continue to work, right?

match void_ref_result {
    Err(e) => ... ,
    Ok(&r) => match r { },
}

@Ericson2314
Copy link
Contributor

Not sure what is the best way to handle that. Macro reachability hygiene?

allow unreachable annotation on arm?

@arielb1
Copy link
Contributor

arielb1 commented Jan 18, 2017

This will continue to work, right?

If it worked before the matchck changes, it should continue working.

allow unreachable annotation on arm?

The problem is that the change is an hygiene hazard - every match of a generic-containing enum in a macro has to be careful not to cause spurious unreachable arm errors. Maybe just disabling unreachable arm errors in cross-crate macros or something would work as a basic form of hygiene.

@brson
Copy link
Contributor

brson commented Jan 18, 2017

@bors r+ p=1 beta fix

@bors
Copy link
Contributor

bors commented Jan 18, 2017

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jan 18, 2017

📌 Commit 2c6bc18 has been approved by brson

@brson
Copy link
Contributor

brson commented Jan 18, 2017

@bors p=1

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jan 19, 2017

@arielb1 yeah, the current "unhygenic" approach is somewhat analogous to doing a post-monomorphization warning with generic functions. I feel like maybe you don't always want that to be ignored, but then a use-site "no---really----please warn/error" would suffice.

@brson
Copy link
Contributor

brson commented Jan 19, 2017

This patch does not apply to beta - the inhabitedness mod does not exist. Is this really a beta backport?

@brson brson mentioned this pull request Jan 19, 2017
@nikomatsakis
Copy link
Contributor

@canndrew I just wanted to say thanks for turning this around so quickly.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 19, 2017
…eferences, r=brson

Feature gate `&Void`'s uninhabitedness.

Here's a totally crazy PR which should never be merged.
bors added a commit that referenced this pull request Jan 19, 2017
Rollup of 11 pull requests

- Successful merges: #38457, #38922, #38970, #39039, #39091, #39115, #39121, #39149, #39150, #39151, #39165
- Failed merges:
@arielb1 arielb1 removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 19, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jan 19, 2017

@brson

No. it's a 1.16 issue only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants