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

check_match: don't treat privately uninhabited types as uninhabited #39980

Merged
merged 2 commits into from
Feb 25, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Feb 20, 2017

Fixes #38972, which is a regression in 1.16 from @canndrew's patchset.

r? @nikomatsakis

beta-nominating because regression.

@arielb1 arielb1 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 20, 2017

if missing_ctors.is_empty() {
//
// However, if our scrutinee is *privately* an empty enum, we
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth defining what 'privately empty' means -- I presume it means that the uninhabited values are not visible from this module? I feel like there was logic to this effect before.

@@ -754,7 +774,19 @@ fn constructor_sub_pattern_tys<'a, 'tcx: 'a>(cx: &MatchCheckCtxt<'a, 'tcx>,
ty::TyRef(_, ref ty_and_mut) => vec![ty_and_mut.ty],
ty::TyAdt(adt, substs) => {
adt.variants[ctor.variant_index_for_adt(adt)].fields.iter().map(|field| {
field.ty(cx.tcx, substs)
let is_visible = adt.is_enum()
|| field.vis.is_accessible_from(cx.module, cx.tcx);
Copy link
Contributor

Choose a reason for hiding this comment

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

do enum fields not have appropriate visibility settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't.

let pat_ty = self.tables.node_id_to_type(scrut.id);
let module = self.tcx.hir.local_def_id(self.tcx.hir.get_module_parent(scrut.id));
if inlined_arms.is_empty() {
if !pat_ty.is_uninhabited_from(module, self.tcx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think eventually this wants to be a stricter definition of uninhabited (i.e., one that says "you could only write dead-code arms"), but I guess that can come later. Probably so long as never-type feature flag is not in use this one will match what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this what we agreed on at the sprint.

@nikomatsakis
Copy link
Contributor

@arielb1 how far have the inhabitedness changes from @canndrew gone in the release pipeline? are they still in beta only, or have they made it to release?

If the latter, do we have to worry about breaking things with this PR? I am imagining code where we accepted matches with fewer arms.

@nikomatsakis
Copy link
Contributor

(The code seems good, in any case.)

@arielb1
Copy link
Contributor Author

arielb1 commented Feb 23, 2017

In beta.

This is a [breaking-change] from 1.15, because this used to compile:

```Rust
enum Void {}
fn foo(x: &Void) {
    match x {}
}
```
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 24, 2017

📌 Commit 87e544b has been approved by nikomatsakis

@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 24, 2017
eddyb added a commit to eddyb/rust that referenced this pull request Feb 25, 2017
…komatsakis

check_match: don't treat privately uninhabited types as uninhabited

Fixes rust-lang#38972, which is a regression in 1.16 from @canndrew's patchset.

r? @nikomatsakis

beta-nominating because regression.
eddyb added a commit to eddyb/rust that referenced this pull request Feb 25, 2017
…komatsakis

check_match: don't treat privately uninhabited types as uninhabited

Fixes rust-lang#38972, which is a regression in 1.16 from @canndrew's patchset.

r? @nikomatsakis

beta-nominating because regression.
bors added a commit that referenced this pull request Feb 25, 2017
@bors bors merged commit 87e544b into rust-lang:master Feb 25, 2017
@brson brson mentioned this pull request Feb 25, 2017
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 25, 2017
bors added a commit that referenced this pull request Mar 2, 2017
[beta] next

- #39913
- #39730
- #39674
- #39602
- #39586
- #39471
- #39980
- #40020
- #40135

@nikomatsakis [this commit](3787d33) did not pick cleanly. You might peek at it.

I took the liberty of accepting all the nominations myself, but the [packed struct alignment](#39586) PR is quite large. It did pick fine though and there's a comment there suggesting it works on beta cc @rust-lang/compiler.

cc @alexcrichton
arielb1 added a commit to arielb1/rust that referenced this pull request Jan 13, 2018
the match-checking code used to use TyErr for signaling "unknown,
inhabited" types for a long time. It had been switched to using the
exact type in rust-lang#38069, to handle uninhabited types.

However, in rust-lang#39980, we discovered that we still needed the "unknown
inhabited" logic, but I used `()` instead of `TyErr` to handle that.
Revert to using `TyErr` to fix that problem.
bors added a commit that referenced this pull request Jan 21, 2018
check_match: fix handling of privately uninhabited types

the match-checking code used to use TyErr for signaling "unknown,
inhabited" types for a long time. It had been switched to using the
exact type in #38069, to handle uninhabited types.

However, in #39980, we discovered that we still needed the "unknown
inhabited" logic, but I used `()` instead of `TyErr` to handle that.
Revert to using `TyErr` to fix that problem.

Fixes #46964.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants