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

Future-proofing enums/structs with #[non_exhaustive] attribute #2008

Merged
merged 11 commits into from Aug 26, 2017

Conversation

@clarcharr
Contributor

clarcharr commented May 25, 2017

This is a post-1.0 version of #757, changed to use an attribute instead of a dedicated syntax. This would allow crates to more easily mark enums as "non-exhaustive," requiring downstream crates to add a wildcard arm to matches, so that adding new variants is not a breaking change.

Additionally, this syntax is extended to structs as well.

Rendered

Edit: Updated rendered link to rfcs repo version.

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm May 25, 2017

Member

Another thing in favour of #[non_exhaustive] on structs: the non-pub field prevents construction, whereas the attribute could allow it through FRU. That would allow extensible builders, with struct Builder { a: b, c: d, ..Default::default() }. And it has the same ergonomic advantages as with enum, allowing in-crate matching on the exact set of fields without the bonus non_exhaustive: (), and not needing it on in-crate construction either.

Member

scottmcm commented May 25, 2017

Another thing in favour of #[non_exhaustive] on structs: the non-pub field prevents construction, whereas the attribute could allow it through FRU. That would allow extensible builders, with struct Builder { a: b, c: d, ..Default::default() }. And it has the same ergonomic advantages as with enum, allowing in-crate matching on the exact set of fields without the bonus non_exhaustive: (), and not needing it on in-crate construction either.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 25, 2017

Contributor

Because you mentioned it: if there's enough support for it, I'd be more than happy to promote the struct/trait parts into the main RFC. I've mostly kept what's there small so that we can at least get the enum part in.

Part of the reason why I did that is also ensuring that we choose a name that would work for structs/traits if/when they get added too.

Contributor

clarcharr commented May 25, 2017

Because you mentioned it: if there's enough support for it, I'd be more than happy to promote the struct/trait parts into the main RFC. I've mostly kept what's there small so that we can at least get the enum part in.

Part of the reason why I did that is also ensuring that we choose a name that would work for structs/traits if/when they get added too.

@KodrAus

This comment has been minimized.

Show comment
Hide comment
@KodrAus

KodrAus May 26, 2017

Contributor

I like it 👍

This RFC cements the current state of the world with respect to exhaustive matching: where a producer's enum/struct declares itself unexhaustively matchable, and a consumer must respect that with a catch-all pattern. That sounds reasonable to me, because it's the only way for a producer to be sure additions are non-breaking.

If I understand the RFC correctly, the #[non_exhaustive] attribute isn't adding hidden fields or anything. ie it's not making observable changes beyond the match semantics in the consumer, so I think an attribute is a fine way to do this.

Contributor

KodrAus commented May 26, 2017

I like it 👍

This RFC cements the current state of the world with respect to exhaustive matching: where a producer's enum/struct declares itself unexhaustively matchable, and a consumer must respect that with a catch-all pattern. That sounds reasonable to me, because it's the only way for a producer to be sure additions are non-breaking.

If I understand the RFC correctly, the #[non_exhaustive] attribute isn't adding hidden fields or anything. ie it's not making observable changes beyond the match semantics in the consumer, so I think an attribute is a fine way to do this.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler May 26, 2017

Member

I feel a bit uneasy about using an attribute to control this. I'd prefer .. I think. Attributes don't normally change the semantics of a type that significantly.

Member

sfackler commented May 26, 2017

I feel a bit uneasy about using an attribute to control this. I'd prefer .. I think. Attributes don't normally change the semantics of a type that significantly.

@dlight

This comment has been minimized.

Show comment
Hide comment
@dlight

dlight May 26, 2017

I feel a bit uneasy about using an attribute to control this. I'd prefer .. I think. Attributes don't normally change the semantics of a type that significantly.

A tangential comment: #[async] is exactly an attribute that will change the semantics (of a function) significantly. So I think the naming scheme of attributes #[non_exhaustive] and #[async] (and the possibility of other attributes like this) should be discussed together, for consistency.

dlight commented May 26, 2017

I feel a bit uneasy about using an attribute to control this. I'd prefer .. I think. Attributes don't normally change the semantics of a type that significantly.

A tangential comment: #[async] is exactly an attribute that will change the semantics (of a function) significantly. So I think the naming scheme of attributes #[non_exhaustive] and #[async] (and the possibility of other attributes like this) should be discussed together, for consistency.

@seanmonstar

This comment has been minimized.

Show comment
Hide comment
@seanmonstar

seanmonstar May 26, 2017

Contributor

A tangential comment: #[async] is exactly an attribute that will change the semantics (of a function) significantly.

I'd kind of expect to eventually have keywords async and await, I must assume they are currently an attribute and macro is because that's the easiest to implement and explore. But probably not the place to have this discussion.

Contributor

seanmonstar commented May 26, 2017

A tangential comment: #[async] is exactly an attribute that will change the semantics (of a function) significantly.

I'd kind of expect to eventually have keywords async and await, I must assume they are currently an attribute and macro is because that's the easiest to implement and explore. But probably not the place to have this discussion.

@kevincox

Excellent writeup. This sounds like a great change to me.

Show outdated Hide outdated text/0000-non-exhaustive.md
Show outdated Hide outdated text/0000-non-exhaustive.md
Show outdated Hide outdated text/0000-non-exhaustive.md
@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 26, 2017

Contributor

I feel a bit uneasy about using an attribute to control this. I'd prefer .. I think. Attributes don't normally change the semantics of a type that significantly.

That is fair! I mostly changed it to an attribute because at the time of the original RFC (#757), it was decided that adding extra syntax was not worth the benefits, and someone suggested that an attribute might be better.

That said, a lot has happened since the initial 1.0 release, and I think that people are more willing to accept a syntax addition for this now than when the original RFC was written.

Contributor

clarcharr commented May 26, 2017

I feel a bit uneasy about using an attribute to control this. I'd prefer .. I think. Attributes don't normally change the semantics of a type that significantly.

That is fair! I mostly changed it to an attribute because at the time of the original RFC (#757), it was decided that adding extra syntax was not worth the benefits, and someone suggested that an attribute might be better.

That said, a lot has happened since the initial 1.0 release, and I think that people are more willing to accept a syntax addition for this now than when the original RFC was written.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm May 26, 2017

Member

👍 extending this to structs, 👎 extending this to traits. The semantic change applied to traits is different enough that should take a different attribute or syntax (e.g. #[sealed]); I was expecting #[non_exhaustive] applied to traits means "this trait may gain additional methods in the future".

Member

kennytm commented May 26, 2017

👍 extending this to structs, 👎 extending this to traits. The semantic change applied to traits is different enough that should take a different attribute or syntax (e.g. #[sealed]); I was expecting #[non_exhaustive] applied to traits means "this trait may gain additional methods in the future".

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm May 26, 2017

Member

If this RFC is accepted and stabilized, there should be a (clippy?) lint that suggests any C-like enum returned from extern function (-> E or &mut E) be annotated #[non_exhaustive], to prevent rust-lang/rust#36927 ("Match on repr(C) enum returned from C library with unknown value leads to UB").

Member

kennytm commented May 26, 2017

If this RFC is accepted and stabilized, there should be a (clippy?) lint that suggests any C-like enum returned from extern function (-> E or &mut E) be annotated #[non_exhaustive], to prevent rust-lang/rust#36927 ("Match on repr(C) enum returned from C library with unknown value leads to UB").

@clarcharr clarcharr changed the title from Future-proofing enums with #[non_exhaustive] attribute to Future-proofing enums/structs with #[non_exhaustive] attribute May 26, 2017

@leodasvacas

This comment has been minimized.

Show comment
Hide comment
@leodasvacas

leodasvacas May 28, 2017

Another thing in favour of #[non_exhaustive] on structs: the non-pub field prevents construction, whereas the attribute could allow it through FRU.

@scottmcm I don't see how the attribute would allow FRU, to the contrary it should forbid FRU outside the module that defines the struct. This should be in the RFC.

leodasvacas commented May 28, 2017

Another thing in favour of #[non_exhaustive] on structs: the non-pub field prevents construction, whereas the attribute could allow it through FRU.

@scottmcm I don't see how the attribute would allow FRU, to the contrary it should forbid FRU outside the module that defines the struct. This should be in the RFC.

@ubsan

This comment has been minimized.

Show comment
Hide comment
@ubsan

ubsan May 28, 2017

Contributor

@kennytm that doesn't really make much sense. A match on a non-exhaustive enum outside the bounds would still be UB, and it's not uncommon for C enums to be treated like Rust enums - i.e., only within the bounds is okay.

Contributor

ubsan commented May 28, 2017

@kennytm that doesn't really make much sense. A match on a non-exhaustive enum outside the bounds would still be UB, and it's not uncommon for C enums to be treated like Rust enums - i.e., only within the bounds is okay.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm May 28, 2017

Member

@ubsan a match on a non-exhaustive C-like enum, not arbitrary enum. This particular case should be definable.

Member

kennytm commented May 28, 2017

@ubsan a match on a non-exhaustive C-like enum, not arbitrary enum. This particular case should be definable.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 28, 2017

Contributor

I'm pretty sure that casting an arbitrary integer to an enum, C-like or not, is undefined behaviour unless it corresponds to a valid variant. I don't think that annotating the enum differently is going to change this problem.

Contributor

clarcharr commented May 28, 2017

I'm pretty sure that casting an arbitrary integer to an enum, C-like or not, is undefined behaviour unless it corresponds to a valid variant. I don't think that annotating the enum differently is going to change this problem.

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm May 29, 2017

Member

@leodasvacas Oops, you're absolutely correct. Allowing FRU would need it to use the alternative desugar. ("jFransham's Super-FRUs", I remember it being called...)

Member

scottmcm commented May 29, 2017

@leodasvacas Oops, you're absolutely correct. Allowing FRU would need it to use the alternative desugar. ("jFransham's Super-FRUs", I remember it being called...)

@le-jzr

This comment has been minimized.

Show comment
Hide comment
@le-jzr

le-jzr May 29, 2017

@clarcharr What makes it UB for C-like enums is that it fails the assumption that an exhaustive match will always execute one of its branches. If exhaustive match is impossible, missing labels pose no problem.

le-jzr commented May 29, 2017

@clarcharr What makes it UB for C-like enums is that it fails the assumption that an exhaustive match will always execute one of its branches. If exhaustive match is impossible, missing labels pose no problem.

@le-jzr

This comment has been minimized.

Show comment
Hide comment
@le-jzr

le-jzr May 29, 2017

Or to put it simply, putting #[non_exhaustive] on C-like enum could be a statement that every value of the underlying integer type is valid, even if those values don't have labels. Insofar as that statement applies to all code, even within the same crate, which of course is not what's being proposed in this RFC.

le-jzr commented May 29, 2017

Or to put it simply, putting #[non_exhaustive] on C-like enum could be a statement that every value of the underlying integer type is valid, even if those values don't have labels. Insofar as that statement applies to all code, even within the same crate, which of course is not what's being proposed in this RFC.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr May 29, 2017

Contributor

Note that the attribute does not say "all bit patterns of this enum are valid," only "more variants may be added in the future."

I don't think that this is a good solution to the problem you're suggesting.

Contributor

clarcharr commented May 29, 2017

Note that the attribute does not say "all bit patterns of this enum are valid," only "more variants may be added in the future."

I don't think that this is a good solution to the problem you're suggesting.

@le-jzr

This comment has been minimized.

Show comment
Hide comment
@le-jzr

le-jzr May 29, 2017

The attribute says what the RFC says it says, and RFC is not merged yet. ;)

But yes, it's somewhat different, even if intimately related. The solution to C enum problem requires #[non_exhaustive], but it's a somewhat stronger guarantee. In particular, match code generation needs to ensure that all unnamed values fall into the wildcard branch, which is not necessarily true right now (as far as I know).

le-jzr commented May 29, 2017

The attribute says what the RFC says it says, and RFC is not merged yet. ;)

But yes, it's somewhat different, even if intimately related. The solution to C enum problem requires #[non_exhaustive], but it's a somewhat stronger guarantee. In particular, match code generation needs to ensure that all unnamed values fall into the wildcard branch, which is not necessarily true right now (as far as I know).

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 May 29, 2017

Contributor

I do want some "real" solution for this. I want to be able to machine-check semver comparability, and the "doc trick" will be ugly to implement for any tool which does that.

Contributor

Ericson2314 commented May 29, 2017

I do want some "real" solution for this. I want to be able to machine-check semver comparability, and the "doc trick" will be ugly to implement for any tool which does that.

@Virtlink

This comment has been minimized.

Show comment
Hide comment
@Virtlink

Virtlink May 31, 2017

This solution would be welcome, as it's a non-breaking change for Rust 1.x. But I think it's the wrong way around.

Non-exhaustive should be the default. This allows a crate author to add new enum variants and add new public and private fields to a struct, without breaking user's code. Of course, this forces users to handle the case where enums are extended, prevents users from using the default constructor to construct a struct, and also prevent users from destructuring the struct.

If the author really wants to enable the user to construct and destruct structs and exhaustively match enums, they have to promise that they won't add members to the struct or enum by finalizing it through some keyword or attribute (or otherwise risk breaking user's code).

Maybe for Rust 2.0?

Virtlink commented May 31, 2017

This solution would be welcome, as it's a non-breaking change for Rust 1.x. But I think it's the wrong way around.

Non-exhaustive should be the default. This allows a crate author to add new enum variants and add new public and private fields to a struct, without breaking user's code. Of course, this forces users to handle the case where enums are extended, prevents users from using the default constructor to construct a struct, and also prevent users from destructuring the struct.

If the author really wants to enable the user to construct and destruct structs and exhaustively match enums, they have to promise that they won't add members to the struct or enum by finalizing it through some keyword or attribute (or otherwise risk breaking user's code).

Maybe for Rust 2.0?

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Jul 30, 2017

The final comment period is now complete.

rfcbot commented Jul 30, 2017

The final comment period is now complete.

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Aug 21, 2017

Member

@clarcharr Are there any updates from the FCP you plan to make to this?

Member

scottmcm commented Aug 21, 2017

@clarcharr Are there any updates from the FCP you plan to make to this?

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Aug 22, 2017

Contributor

Sorry for the delays @petrochenkov and @scottmcm. I will try and update this tomorrow.

Contributor

clarcharr commented Aug 22, 2017

Sorry for the delays @petrochenkov and @scottmcm. I will try and update this tomorrow.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Aug 26, 2017

Member

@clarcharr Ping on the updates?

Member

aturon commented Aug 26, 2017

@clarcharr Ping on the updates?

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Aug 26, 2017

Contributor

@aturon @scottmcm @petrochenkov Sorry for the delays; I've updated with the notes on FRU and the tuple struct constructor visibility. Is this good to merge?

Contributor

clarcharr commented Aug 26, 2017

@aturon @scottmcm @petrochenkov Sorry for the delays; I've updated with the notes on FRU and the tuple struct constructor visibility. Is this good to merge?

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Aug 26, 2017

Contributor

LGTM now 👍

Contributor

petrochenkov commented Aug 26, 2017

LGTM now 👍

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Aug 26, 2017

Member

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issue here: rust-lang/rust#44109

Thanks for the RFC, @clarcharr!

Member

scottmcm commented Aug 26, 2017

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issue here: rust-lang/rust#44109

Thanks for the RFC, @clarcharr!

@scottmcm scottmcm merged commit edbb821 into rust-lang:master Aug 26, 2017

@clarcharr clarcharr deleted the clarcharr:non_exhaustive branch Aug 27, 2017

bors added a commit to rust-lang/rust that referenced this pull request Nov 4, 2017

Auto merge of #45394 - davidtwco:rfc-2008, r=petrochenkov
RFC 2008: Future-proofing enums/structs with #[non_exhaustive] attribute

This work-in-progress pull request contains my changes to implement [RFC 2008](rust-lang/rfcs#2008). The related tracking issue is #44109.

As of writing, enum-related functionality is not included and there are some issues related to tuple/unit structs. Enum related tests are currently ignored.

WIP PR requested by @nikomatsakis [in Gitter](https://gitter.im/rust-impl-period/WG-compiler-middle?at=59e90e6297cedeb0482ade3e).

bors added a commit to rust-lang/rust that referenced this pull request Nov 4, 2017

Auto merge of #45394 - davidtwco:rfc-2008, r=petrochenkov
RFC 2008: Future-proofing enums/structs with #[non_exhaustive] attribute

This work-in-progress pull request contains my changes to implement [RFC 2008](rust-lang/rfcs#2008). The related tracking issue is #44109.

As of writing, enum-related functionality is not included and there are some issues related to tuple/unit structs. Enum related tests are currently ignored.

WIP PR requested by @nikomatsakis [in Gitter](https://gitter.im/rust-impl-period/WG-compiler-middle?at=59e90e6297cedeb0482ade3e).

@tobz1000 tobz1000 referenced this pull request Jul 10, 2018

Open

Error type redesign #1131

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment