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

RFC 2008: Uninhabitedness fixes for enum variants and tests #60529

Merged
merged 5 commits into from
May 10, 2019

Conversation

davidtwco
Copy link
Member

Part of #44109.

At the request of @Centril, this PR adds tests asserting that uninhabited non-exhaustive types are considered inhabited in extern crates. In adding these tests, I fixed an oversight in the implementation of RFC 2008 on enum variants that resulted in non-exhaustive enum variants being considered uninhabited in extern crates.

Before this PR, these lines would error:

// extern crate
pub enum UninhabitedVariants {
    #[non_exhaustive] Tuple(!),
    #[non_exhaustive] Struct { x: ! }
}

pub enum PartiallyInhabitedVariants {
    Tuple(u8),
    #[non_exhaustive] Struct { x: ! }
}

// current crate
match uninhabited_variant() /* fn() -> Option<UninhabitedVariants> */ {
    Some(_x) => (), //~ ERROR unreachable pattern
    None => (),
}

while let PartiallyInhabitedVariants::Struct { x, .. } = partially_inhabited_variant() /* fn() -> PartiallyInhabitedVariants */ {
    //~^ ERROR unreachable pattern
}

cc @Centril
r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2019
src/librustc/ty/inhabitedness/mod.rs Outdated Show resolved Hide resolved
src/test/ui/rfc-2008-non-exhaustive/uninhabited.rs Outdated Show resolved Hide resolved
@davidtwco davidtwco force-pushed the rfc-2008-uninhabited branch 2 times, most recently from 805a891 to 2121cc7 Compare May 4, 2019 10:22
@rust-highfive

This comment has been minimized.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Love the barrage of tests! :)

@petrochenkov
Copy link
Contributor

I don't think I understand the examples.

pub enum UninhabitedVariants {
    #[non_exhaustive] Struct { x: ! }
}

The type already has a public uninhabited field, so no amount of added fields will make it inhabited, so it's publicly uninhabited for this crate or for other crates, doesn't matter.

#[non_exhaustive] on enums can affect inhabitedness, because new variants can make the enum inhabited, but for structs/variants that's not the case.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2019
@Centril
Copy link
Contributor

Centril commented May 5, 2019

but for structs/variants that's not the case.

If you have exhaustive_patterns, then the type system would let you match x: UninhabitedStruct {} on an otherwise uninhabited struct or variant (at least with rust-lang/rfcs#2593 for the latter). However, until we have collectively decided otherwise, I think #[non_exhaustive] should prevent that in other crates. It might be the case that we use a different mechanism in the end, but let's be conservative for now and allow exhaustive pattern matching deliberately in these cases. The use case for this is when you have something like a struct TcpStream(!); on some platforms where it cannot be constructed and where it can be constructed on other platforms.

@petrochenkov petrochenkov 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 (such as code changes or more information) from the author. labels May 5, 2019
@petrochenkov
Copy link
Contributor

#[non_exhaustive] implies that new fields can be added to a struct, so here are the possible cases of new fields affecting the struct inhabitedness as it's seen from other crates:

// The struct is externally inhabited, the field addition doesn't change that
struct S { a: PrivDoesntmatter } -> struct S { a: PrivDoesntmatter, b: PrivDoesntmatter }

// The struct is externally inhabited, the field addition doesn't change that
struct S { a: PrivDoesntmatter } -> struct S { a: PrivDoesntmatter, pub b: PubInhabited }

// The struct is externally inhabited, the field addition changes that
struct S { a: PrivDoesntmatter } -> struct S { a: PrivDoesntmatter, pub b: PubUninhabited }

---

// The struct is externally inhabited, the field addition doesn't change that
struct S { pub a: PubInhabited } -> struct S { pub a: PubInhabited, b: PrivDoesntmatter }

// The struct is externally inhabited, the field addition doesn't change that
struct S { pub a: PubInhabited } -> struct S { pub a: PubInhabited, pub b: PubInhabited }

// The struct is externally inhabited, the field addition changes that
struct S { pub a: PubInhabited } -> struct S { pub a: PubInhabited, pub b: PubUninhabited }

---

// The struct is externally uninhabited, the field addition doesn't change that
struct S { pub a: PubUninhabited } -> struct S { pub a: PubInhabited, b: PrivDoesntmatter }

// The struct is externally uninhabited, the field addition doesn't change that
struct S { pub a: PubUninhabited } -> struct S { pub a: PubInhabited, pub b: PubInhabited }

// The struct is externally uninhabited, the field addition doesn't change that
struct S { pub a: PubUninhabited } -> struct S { pub a: PubInhabited, pub b: PubUninhabited }

The situation with variants is a bit simpler since they don't have private fields:

// The variant is externally inhabited, the field addition doesn't change that
variant S { pub a: PubInhabited } -> variant S { pub a: PubInhabited, pub b: PubInhabited }

// The variant is externally inhabited, the field addition changes that
variant S { pub a: PubInhabited } -> variant S { pub a: PubInhabited, pub b: PubUninhabited }

---

// The variant is externally uninhabited, the field addition doesn't change that
variant S { pub a: PubUninhabited } -> variant S { pub a: PubInhabited, pub b: PubInhabited }

// The variant is externally uninhabited, the field addition doesn't change that
variant S { pub a: PubUninhabited } -> variant S { pub a: PubInhabited, pub b: PubUninhabited }

So, if #[non_exhaustive] does nothing with inhabitedness, then adding a new public uninhabited field to a previously inhabited struct or variant is a breaking change (kind of, unreachable_patterns is only a lint).

Is adding a new public uninhabited field to something previously inhabited a realistic scenario?

(The struct TcpStream(!); is already covered by privacy, doesn't need #[non_exhaustive].)

@petrochenkov
Copy link
Contributor

Any other properties that can change if a new field is added?
Something Drop-related perhaps?

@petrochenkov
Copy link
Contributor

Ok, I had some time to digest this and I agree that if addition of a new public uninhabited field can break code, then #[non_exhaustive] should prevent that.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2019
@davidtwco
Copy link
Member Author

@petrochenkov I've fixed all of your suggestions. I can't relabel from S-waiting-on-author to S-waiting-on-review.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 10, 2019

📌 Commit f9e4da04abc8e97cd1ae9468b6b4eff67e1c9c64 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 10, 2019
@bors
Copy link
Contributor

bors commented May 10, 2019

⌛ Testing commit f9e4da04abc8e97cd1ae9468b6b4eff67e1c9c64 with merge 1865e9b9959474dd84d33416372a3e6aadc7224e...

@bors
Copy link
Contributor

bors commented May 10, 2019

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 10, 2019
This commit adds tests checking that uninhabited non-exhaustive types
are considered inhabited when used in another crate.
This commit ensures that non-exhaustive variants are considered
inhabited when used in extern crates.
This commit ensures that non-exhaustive enums are considered inhabited
when used in extern crates.
This commit just tries to tidy up a little.
@davidtwco
Copy link
Member Author

I've rebased this and updated the tests with longer names to use the same fix as #60648 to avoid spurious failures on Windows.

@petrochenkov
Copy link
Contributor

Legitimate error - file paths are too long (ui\rfc-2008-non-exhaustive\uninhabited\indirect_match_with_exhaustive_patterns_same_crate.rs).

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 10, 2019

📌 Commit 1f0fb03 has been approved by petrochenkov

@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 May 10, 2019
Centril added a commit to Centril/rust that referenced this pull request May 10, 2019
…etrochenkov

RFC 2008: Uninhabitedness fixes for enum variants and tests

Part of rust-lang#44109.

At the request of @Centril, this PR adds tests asserting that uninhabited non-exhaustive types are considered inhabited in extern crates. In adding these tests, I fixed an oversight in the implementation of RFC 2008 on enum variants that resulted in non-exhaustive enum variants being considered uninhabited in extern crates.

Before this PR, these lines would error:

```rust
// extern crate
pub enum UninhabitedVariants {
    #[non_exhaustive] Tuple(!),
    #[non_exhaustive] Struct { x: ! }
}

pub enum PartiallyInhabitedVariants {
    Tuple(u8),
    #[non_exhaustive] Struct { x: ! }
}

// current crate
match uninhabited_variant() /* fn() -> Option<UninhabitedVariants> */ {
    Some(_x) => (), //~ ERROR unreachable pattern
    None => (),
}

while let PartiallyInhabitedVariants::Struct { x, .. } = partially_inhabited_variant() /* fn() -> PartiallyInhabitedVariants */ {
    //~^ ERROR unreachable pattern
}
```

cc @Centril
r? @petrochenkov
bors added a commit that referenced this pull request May 10, 2019
Rollup of 6 pull requests

Successful merges:

 - #60529 (RFC 2008: Uninhabitedness fixes for enum variants and tests)
 - #60620 (Fix a couple of FIXMEs in ext::tt::transcribe)
 - #60659 (Tweak `Symbol` and `InternedString`)
 - #60692 (Extend #60676 test for nested mut patterns.)
 - #60697 (add regression test for #60629)
 - #60701 (Update mailmap for mati865)

Failed merges:

r? @ghost
@bors bors merged commit 1f0fb03 into rust-lang:master May 10, 2019
@davidtwco davidtwco deleted the rfc-2008-uninhabited branch May 12, 2019 10:16
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