Rust: Tracking issue for RFC 2008: Future-proofing enums/structs with #[non_exhaustive] attribute

Created on 27 Aug 2017 · 139Comments · Source: rust-lang/rust

This is a tracking issue for the RFC "Future-proofing enums/structs with #[non_exhaustive] attribute" (rust-lang/rfcs#2008).

Steps:

[x] Implement the RFC (mentoring instructions below) -- done in PR https://github.com/rust-lang/rust/pull/45394
[ ] Adjust documentation (see instructions on forge)
[ ] Stabilization PR (see instructions on forge)

Issues to be resolved prior to stabilization

[x] #[non_exhaustive] is not yet implemented for enum variants.
[x] Current behavior on unions. The current implementation (#44109) sometimes ignores non_exhaustive on unions, sometimes reports an error.
[x] In addition, the attribute is not currently validated (neither syntactic form nor location), so it can be written in weird forms (#[non_exhaustive(foo)]) and in arbitrary locations, including suspicious ones like traits (#[non_exhaustive] trait Trait {}).
[x] Fix #53549

B-RFC-implemented B-unstable C-tracking-issue E-mentor F-non_exhaustive T-lang WG-compiler-middle disposition-merge finished-final-comment-period

Source

scottmcm

🎉23 👍2

Most helpful comment

I don't have an opinion on syntax (please keep discussing) but I'd like to weigh in on one other aspect of #[non_exhaustive] that I find makes the current design inappropriate for my use case.

@Centril and I had a chat about #[non_exhaustive] because Syn is such an obvious use case for it -- we need to be able to add variants over time as new syntax is added to Rust. Our requirements are:

It needs to be easy to match a syntax tree node nonexhaustively, the most common case.
It needs to be easy for code that uses Syn to opt in to a test failure when Syn adds a variant.
It needs to be hard for code that uses code that uses Syn (2 levels removed) to get a build failure when Syn adds a variant.

Item 2 is important for the small fraction of use cases that want to update their code to take into account new syntax, such as a prettyprinter. Item 3 is important so that code downstream of the code that wants to update is never broken in the interim.

I have a solution that meets these requirements in https://github.com/dtolnay/syn/issues/694, not using #[non_exhaustive]. We add a hidden variant TestExhaustive and document the recommended pattern for exhaustive matches as being:

match expr {
    Expr::Array(e) => {...}
    Expr::Assign(e) => {...}
    ...
    Expr::Yield(e) => {...}

    #[cfg(test)]
    Expr::TestExhaustive => unimplemented!(),
    #[cfg(not(test))]
    _ => { /* some sane fallback */ }
}

I would love to use #[non_exhaustive] as a less hacky way of doing this but the current design isn't able to meet requirement 2.

To brainstorm a small extension that would make #[non_exhaustive] usable for Syn, it would be great if we could do:

match expr {
    Expr::Array(e) => {...}
    Expr::Assign(e) => {...}
    ...
    Expr::Yield(e) => {...}

    #[cfg_attr(test, deny(reachable))]
    _ => { /* some sane fallback */ }
}

where reachable is an allow-by-default lint on catch-all arms of a nonexhaustive match.

This meets requirement 1 because the lint is allow by default so you just match nonexhaustively and that's it.
This meets requirement 2 because you add this deny attribute to opt in to having your tests notify you of new variants. Note that this works even in the absence of any tests; we don't assume existence of a test case that actually exercises the new syntax at runtime.
This meets requirement 3 because it can't break crates downstream of the match, which cap their dependencies' lints to allow.

dtolnay on 15 Aug 2019

❤15

All 139 comments

I'd cc @rust-lang/compiler instead.

eddyb on 27 Aug 2017

im working on a pr for this. how should tests be structured?

tinaun on 27 Aug 2017

also: should there be a lint for #[non_exhaustive] on non public items, since it doesn't really do anything crate-locally?

tinaun on 27 Aug 2017

@tinaun shouldn't they all go under the useless_attribute lint? That includes both on non-pub items and structs with private fields.

clarfonthey on 27 Aug 2017

@tinaun I would expect tests for this to be scattered about in the run-pass and compile-fail and ui directories:

run-pass would be used to test things that should compile
ui is usually used to test for things that don't compile (compile-fail can also be used, but I prefer ui personally)
- if you want to test that a lint is issued, you can use #[deny] to convert the warning into an error

Just to keep things organized, I would make a subdirectory like src/test/run-pass/nonexhaustive-rfc2008 (and same for ui) to contain the tests.

So I would imagine using ui tests to check for:

Errors when exhaustively matching (without _) something from another crate that is tagged #[nonexhaustive]

The run-pass might be used to test that an exhaustive match within the same crate compiles just fine.

nikomatsakis on 28 Aug 2017

@tinaun any progress? need any tips?

nikomatsakis on 15 Sep 2017

I'm currently away for the weekend but I would love to have some help with understanding the implementation of privacy in the complier. sorry for forgetting about this!

tinaun on 15 Sep 2017

So, this feature shouldn't require too many changes to implement.

First, we need to require .. in patterns, this needs to be done in fn check_struct_pat_fields in librustc_typeck/check/_match.rs
Then we need to prohibit any struct expressions (with .. or not, doesn't matter), this needs to be done in fn check_expr_struct in librustc_typeck/check/mod.rs
After that we need to tweak visibilities for struct constructors. This currently requires changes to three places - 1) fn build_reduced_graph_for_item/ItemKind::Struct + fn build_reduced_graph_for_variant in librustc_resolve 2) fn encode_struct_ctor and fn encode_enum_variant_info in librustc_metadata and 3) fn def_id_visibility in librustc_privacy.
I all cases visibility of a struct/variant with non_exhaustive attribute should be lowered to pub(crate) if it was pub.
EDIT: Separating visibilities for enum variants and their constructors may potentially require some larger infrastructural changes because they share DefId currently, so non_exhaustive on tuple/unit variants can be omitted from initial implementation.
Test everything! In this case it's probably better to write failing tests before doing anything else and then make them pass gradually.
EDIT: I completely forgot about non_exhaustive on enums and match exhaustiveness checks.

petrochenkov on 16 Sep 2017

@tinaun just checking in -- did those instructions help? Still got questions? =)

UPDATE: @tinaun responded on Gitter.

nikomatsakis on 25 Sep 2017

Hoping to take a stab at this after speaking with @nikomatsakis in Gitter. @tinaun are you still working on this?

Update: @tinaun replied on Gitter.

davidtwco on 13 Oct 2017

@nikomatsakis @eddyb @arielb1 @pnkfelix (I'm not sure who I should be pinging about this, apologies)

So far I've implemented most of the mentoring instructions but there are a few things I've run into issues with.

In this section where I require .. (as per the first mentoring instruction) - this seems to work, except that it doesn't take into account whether the struct/enum is within the crate (and therefore should be able to match without a wildcard) - this causes issues with the run-pass/rfc-2008-non-exhaustive/struct_within_crate.rs test (and the similar enum test). I'm not sure what I should be checking to do that.

I've also not made changes to the visibilities in encode_enum_variant_info and build_reduced_graph_for_variant as when I did so, I found that I was no longer able to access the variants of a enum in order to match them from outside of a crate - similarly to the previous question, I wasn't sure how to have the change apply only outside of a crate.

I hope this is on the right track and I'd appreciate any feedback.

davidtwco on 15 Oct 2017

@davidtwco

To check whether an ADT is within the same crate as the crate you are type-checking, find the def-id of the ADT and check whether it's local - e.g. if you have an adt: &AdtDef, you can check adt.did.is_local().

arielb1 on 16 Oct 2017

For the visibility, you can ask @petrochenkov but I think you want to set the visibility to pub(crate) aka ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX))

arielb1 on 16 Oct 2017

If you can find me, feel free to ping me on IRC.

arielb1 on 16 Oct 2017

Thanks, that's super helpful.

With regards to the visibility, that's what I was setting the visibility to, but I'll take another stab at it and see what I can come up with.

davidtwco on 16 Oct 2017

@davidtwco glad to see that @arielb1 got you unblocked! Looking forward to seeing the PR. =) Let us know if you hit any other problems (are here or on gitter)

nikomatsakis on 16 Oct 2017

@nikomatsakis @arielb1

Apologies for how long this is taking and for a further query.

Since my previous comment here I think I've got it working with structs and after speaking with @nikomatsakis in Gitter earlier today, I've added a bunch more tests for cases that I hadn't considered. There are only five tests that still don't pass:

The first of which is related to instantiating a tuple struct, I think I've went wrong with the visibility somewhere for this so I'm going to look into that. The second of which is similar but for instantiating a unit struct.

The rest of which are all related to a lack of errors for things that should fail on enums with the attribute. @nikomatsakis mentioned that there are two cases with enums, handling new variants and new fields to existing variants - I do believe that the new fields case is handled by the existing structs code but I'm not 100% sure. However, I don't think the new variants case is handled. ~~The function mentioned in the first bullet point of the mentoring instructions doesn't seem to apply for this case and I've not been able to figure out where the equivalent is.~~ As the edited mentoring instructions now mention, there aren't any instructions for handling enums so I've been struggling to pinpoint where I need to make changes. I'd appreciate some pointers as to where I should be working for that.

All the help so far as been fantastic so I appreciate that.

davidtwco on 18 Oct 2017

OK @davidtwco -- so I'm no expert in the exhaustiveness checking code, but I can give you some tips. The main function of interest is is_useful -- there is actually a link to the paper describing the algorithm, if that's your sort of thing, but also a concise description of how it works.

For context, that function gets invoked from here, which is where we report the exhaustiveness errors.

The idea is that the Matrix basically represents the set of patterns that the user gave, and -- when doing exhaustiveness checking, at least -- the v argument will be a single _ pattern.

The algorithm proceeds as follows:

Figure out the constructors that are covered by the patterns the user gave.
- A "constructor" in this context is basically an enum variant.
Figure out all the constructors that a given enum has.
Figure out which constructors are not covered.
As it happens, we have some existing logic that is targeting the ! type (the "never" type), which is meant to allow for "privately empty" enums. i.e., it's meant to cover cases where we can see that the match is exhaustive, but the user shouldn't be allowed to assume that. Seems relevant.
It might therefore be the case that just setting is_privately_empty to true will do the job? I'm having a bit of trouble parsing that code that comes after, so this is not entirely obvious to me, but it seems plausible. =)

nikomatsakis on 19 Oct 2017

UPDATE: To check if things are exhaustive, I suggested adding data to adt-def here on gitter

nikomatsakis on 19 Oct 2017

The RFC doesn't tell anything about unions.
The current implementation (https://github.com/rust-lang/rust/issues/44109) sometimes ignores non_exhaustive on unions, sometimes reports an error.
In addition, the attribute is not currently validated (neither syntactic form nor location), so it can be written in weird forms (#[non_exhaustive(foo)]) and in arbitrary locations, including suspicious ones like traits (#[non_exhaustive] trait Trait {}).
These issues need to be resolved before stabilization.

petrochenkov on 3 Nov 2017

Huh, I'm surprised that never came up during FCP. IMHO the rules for unions should be identical for structs.

clarfonthey on 3 Nov 2017

@petrochenkov I copied your notes on what is left to do into the issue header, btw

nikomatsakis on 7 Nov 2017

Currently the unit structs created using this attribute will still put an item into value namespace, which, when used, results in a very non-obvious error message:

// foo.rs
#![feature(non_exhaustive)]
#[non_exhaustive]
pub struct Foo;

// bar.rs
extern crate foo;
fn main() {
    let foo = foo::Foo; //~ error[E0603]: unit struct `Foo` is private
}

I feel that the item should not be created in the value namespace at all, or at least a nicer error should be presented.

nagisa on 2 Jan 2018

👍2

@nagisa

Currently the unit structs created using this attribute will still put an item into value namespace, which, when used, results in a very non-obvious error message:

Well, the point of the attribute is to allow us to add more fields in the future, at which point the struct would appear in the value namespace, right? So it seems like we do want it to be present in that namespace.

(I can of course not object to a better error message.)

nikomatsakis on 23 Feb 2018

I would like to see this feature stabilized. Looking over the "to do" items:

#[non_exhaustive] is not yet implemented for enum variants.

This would be nice but doesn't seem necessary. It could be added later.

Current behavior on unions. The current implementation (#44109) sometimes ignores non_exhaustive on unions, sometimes reports an error.

This seems bad.

In addition, the attribute is not currently validated (neither syntactic form nor location), so it can be written in weird forms (#[non_exhaustive(foo)]) and in arbitrary locations, including suspicious ones like traits (#[non_exhaustive] trait Trait {}).

This is definitely a problem, since we don't want to accept things and then be locked in.

Would anybody be up to putting the finishing touches on this feature? Or writing out some mentoring instructions for what to do?

nikomatsakis on 23 Feb 2018

👍2

I've submitted a PR, #49345, that aims to address the finishing touches.

davidtwco on 24 Mar 2018

AFAIK nothing has been added to show #[non_exhaustive] in rustdoc, which also should be a blocker for stabilisation.

clarfonthey on 30 Mar 2018

👍2

I'd still like to reconsider using the .. syntax for this.

struct S {
    field: Type,
    ..
}

The motivation for #[non_exhaustive] being an attribute is to support unit structs

#[non_exhaustive]
pub struct Foo;

but so far I've seen some confusion about behavior of such structs (e.g. https://github.com/rust-lang/rust/issues/44109#issuecomment-354760974) and it's certainly not obvious why #[non_exhaustive] makes the constructor of Foo crate-private (despite this being the 100% intended and desirable behavior).

So maybe unit structs shouldn't be supported?
Then we can return to the .. syntax that makes definition and use symmetric and looks more "first class".

Non-exhaustive unit structs could be written as

pub struct Foo { .. } // The non-exhaustive struct
const Foo: Foo = Foo {}; // Optional convenience: a private constructor to avoid writing `{}`s in the current crate if it needs to be written often

in this case, so it's obvious why the "constructor" is private.

petrochenkov on 30 Mar 2018

👍4 ❤3

@petrochenkov Are you imagining a ..-like syntax for enums as well?

scottmcm on 30 Mar 2018

@scottmcm
Yes, same syntax.

enum E {
    V1,
    V2,
    ..
}

petrochenkov on 30 Mar 2018

By the way, there's a possible solution for the FRU problem (non-exhaustive structs cannot be used like this Foo { a, b, ..rest }) - attaching visibilities to ..:

pub struct S {
    pub a: A,
    pub b: B,
    pub .. // We are going to add fields, but we promise that they will be public
}


// In another crate
let s = S { a, b, ..rest }; // OK, won't break if new public fields are added

In theory visibilities can be placed into attributes as well (#[non_exhaustive(pub(in path))]), but

it's not obvious what the visibility refers to
attributes are purely syntactic constructs (token streams) we don't currently resolve any paths in them (except for the attribute name), but path in the visibility needs to be resolved

petrochenkov on 30 Mar 2018

@petrochenkov I myself have noticed the awkwardness of the arbitrary pub(crate) visibility and think that this change may be better than privacy in the attribute.

The only problem I see is that the visibility is actually the opposite of what's stated, logically. pub .. means "no one needs to know that fields are omitted" and .. by itself means "everyone but me needs to know that fields are omitted."

Also, the visibility specifier for FRU makes it unclear if it should apply to matching as well. If it does, then pub .. is basically useless. And if it doesn't, then that still has the problem of the arbitrary pub(crate) visibility on matching.

Technically using priv as a keyword instead of pub may solve this, but it feels weird.

clarfonthey on 30 Mar 2018

@clarcharr
To avoid pub .. being useless, visibility in vis .. should supposedly mean "we may add new fields, but they will be at least as visible as vis", so only FRU is affected.

I.e. pub .. means "public future fields".

(I certainly don't suggest stabilize this right now, my point is only that this extension fits to .. better than to an attribute.)

petrochenkov on 30 Mar 2018

I mean, we could do: #[non_exhaustive(fru= "vis", match = "vis")] with fru = "crate", match = "crate" the default.

I agree that this is ugly and your solution seems nicer for the case of FRU, although I'd like to solve the visibility for the exhaustiveness as well.

clarfonthey on 30 Mar 2018

I certainly like the .. (https://github.com/rust-lang/rfcs/pull/2008#issuecomment-305711585) as did others (https://github.com/rust-lang/rfcs/pull/2008#issuecomment-304195934).

The justification from the FCP proposal (https://github.com/rust-lang/rfcs/pull/2008#issuecomment-308109012):

The major question that came up in the discussion was whether to use a dedicated bit of syntax. The conclusion was that an attribute is a fine choice -- this doesn't change the semantics of the existing type in any particular way, it just forces end-users to write "defensively". But all the code that they are able to write still works the same way as it would if the attribute were removed.

But I don't see (in a quick scan) anything saying that syntax for it is clearly bad (though there were some searchability concern), so changing is probably fine. And it could be considered a case of "we tried it out with the attribute, and now that we're confident in it are adding the dedicated syntax" pattern.

scottmcm on 31 Mar 2018

Also, keep in mind that adding visibility to the syntax was something that really wasn't discussed at all in the RFC.

clarfonthey on 31 Mar 2018

I strongly prefer the attribute to the ... Although the special syntax is nicer in a way, I feel like this is not a core feature of enums more a direction for the compiler about how to lint them and as such an attribute is more appropriate. Furthermore, it's easier to search for information and clearer what it means. Finally, by using an existing, flexible syntax rather than adding a new one we avoid overusing sigils.

nrc on 4 Apr 2018

👍7

Side-note:

There's been discussion about adding a #[sealed] attribute to prevent traits from being implemented outside the crate that they're defined in. It hit me that #[non_exhaustive], though the wrong semantic meaning, accomplishes this goal (as you can't implement all members thus the trait), and thus could potentially share a code path.

Actually, a #[non_exhaustive] trait probably makes sense separately as well, for something like Itertools or {Type}Ext: it represents that functions are expected to be added to the trait later. If #[non_exhaustive] traits are allowed, #[sealed] traits could just be #[non_exhaustive] but documented differently.

TL;DR: I think #[non_exhaustive] trait is useful and not suspicious as stated in the summary comment.

CAD97 on 9 May 2018

@CAD97 In the original RFC I actually mentioned non_exhaustive on traits as a less-hacky way of making sealed traits. A lot of people disagreed that non_exhaustive for that purpose didn't seem best, though.

A #[sealed] trait would be nice, but I wouldn't put that in favour of non_exhaustive being an attribute, considering how .. makes sense for structs and enums but not for traits.

clarfonthey on 9 May 2018

👍1

I've just submitted #51854 to have this attribute show up in rustdoc as that's the only remaining blocker for stabilisation that I've seen mentioned. I'm happy to work on anything else that's holding this up from being stabilised and to work on the making the attribute apply to variants (though I'd probably need some pointers as it's been previously noted that this would require a larger infrastructural change).

davidtwco on 27 Jun 2018

❤4

@rust-lang/compiler @rust-lang/lang As far as I'm aware, with #51854 having landed, the only thing remaining for this RFC is implementation of the attribute for variants (they currently share a DefId with the enum which meant it was skipped on the initial implementation) - I'm happy to work on this if someone can provide some pointers. Is there anything else blocking stabilization?

davidtwco on 19 Jul 2018

❤1

@davidtwco I see a few things listed in the issue header:

#[non_exhaustive] is not yet implemented for enum variants.
Current behavior on unions. The current implementation (#44109) sometimes ignores non_exhaustive on unions, sometimes reports an error.
In addition, the attribute is not currently validated (neither syntactic form nor location), so it can be written in weird forms (#[non_exhaustive(foo)]) and in arbitrary locations, including suspicious ones like traits (#[non_exhaustive] trait Trait {}).

Have the other two been resolved?

nikomatsakis on 21 Jul 2018

@nikomatsakis #49345 resolved the latter two issues. The first issue is still pending.

davidtwco on 21 Jul 2018

So, to implement #[non_exhaustive] for enum variants, we might want to extend the existing check that works for structs:

https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/librustc_typeck/check/mod.rs#L3575-L3582

In that code, we have this variable variant which is a VariantDef.

Currently, we have the is_non_exhaustive flag being set on the AdtDef itself. I think that for structs, this is actually wrong. What I would do is to have two "non-exhaustive" flags (perhaps with distinct names):

On the AdtDef, we would have can_extend_variant_list
On each VariantDef, we would have can_extend_field_list.

When we build the adt-def for a struct, if it is tagged with #[non_exhaustive], we would set the can_extend_field_list flag. For an enum, we would set one or both flags, depending on where the #[non_exhaustive] attributes appear.

Once this is done, the existing code is almost correct, but it must be changed from saying adt.is_non_exhaustive to variant.is_non_exhaustive. This should be correct in either case.

The AdtDef for a struct is created by the adt_def provider:

https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/librustc_typeck/collect.rs#L558-L560

You can see that structs have one variant, built here:

https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/librustc_typeck/collect.rs#L594-L597

The existing flag is set in AdtDef::new, invoked from alloc_adt_def. This logic will have to change to take into account the kind of adt we have:

https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/librustc/ty/mod.rs#L1912-L1914

Anyway, I'm not -- I confess -- 100% sure where to set the flag best, but that's the idea. =)

nikomatsakis on 26 Jul 2018

👍2

Submitted #52775 with an implementation of the attribute on variants.

davidtwco on 27 Jul 2018

🎉1

In the currently implementation, E0639 fires when the attribute is applied to enums, while it should only be relevant when the attribute is on a specific variant: #53549

durka on 21 Aug 2018

@rust-lang/lang Can we stabilize what’s implemented (attribute on enum types to allow adding variants later) without waiting on what’s not (yet)? (attribute on struct types or enum variants to allow adding fields later)

SimonSapin on 22 Jan 2019

👍2

Seems reasonable to me -- @petrochenkov are you aware of any "gotchas" there?

nikomatsakis on 22 Jan 2019

No, not aware.
My initial concerns were fixed in https://github.com/rust-lang/rust/pull/49345 and I haven't looked at the feature after that.

petrochenkov on 22 Jan 2019

Was the non-exhaustiveness added to rustdoc or is that still a to-do? I'm fine with just stabilising enums and leaving the rest unstable.

clarfonthey on 22 Jan 2019

@clarcharr It has been added to rustdoc.

davidtwco on 22 Jan 2019

I'm very much in favor of stabilizing this for enums and leaving the rest unstable.

npmccallum on 14 Feb 2019

As far as I'm aware, the only thing that needs to be done is actually separating out the feature flag and an FCP. The FCP could probably be started before that's merged though, as it can be done as part of stabilisation.

clarfonthey on 14 Feb 2019

With #59376 having landed, #[non_exhaustive] is now implemented for enum variants.

davidtwco on 30 Mar 2019

❤10

Given the above, is there anything blocking stabilisation now?

auscompgeek on 30 Apr 2019

Given that this is almost ready to stabilize, I've worked on finding places for documentation, per the forge instructions.

Documentation exists for this in the Unstable Book. It needs to be updated to include documentation for non-exhaustive enum variants, and then added to the mainline documentation sources - here's what I've found:

Reference: Should be added to a section in the Attributes page (probably Miscellaneous Attributes)
Book: Not entirely sure; maybe in one of the pages about enums/pattern matching, or in advanced features
Standard LIbrary: There isn't any precedent for documenting attributes here, but it could be included in the struct and enum keyword documentation as those already include documentation about the types themselves.

agausmann on 8 May 2019

@rust-lang/docs, where should this feature be documented?

There is precedent for documenting under /std/ some parts of the language that are not from standard library: https://doc.rust-lang.org/std/#keywords

SimonSapin on 8 May 2019

@SimonSapin Yes, but I don't see any common attribute names. inline and derive, for example, are two of the most popular, but they currently don't have any top-level documentation item in std.

agausmann on 8 May 2019

Indeed, they are currently not documented there. That’s not saying they can never be, especially with the keyword precedent.

SimonSapin on 8 May 2019

👍1

How are proc-macro attributes currently displayed in rustdoc? Perhaps there could be an attributes section which includes these for std?

clarfonthey on 9 May 2019

How are proc-macro attributes currently displayed in rustdoc?

From a runtime crate? Nothing.
From a proc_macro crate? The exported fn.

CAD97 on 9 May 2019

From a runtime crate? Nothing.

That's just because

#[doc(hidden)]
pub use typetag_impl::*;

If the documentation is not suppressed then it appears in the re-exports section (unfortunately I don't know of any public documentation that includes re-exported proc-macro attributes that aren't using some of the workarounds that make them look mostly like normal macros).

Nemo157 on 9 May 2019

My 2c: I think it would be worth taking the chance now to add docs about relevant attributes to each of the keywords in the std docs.

auscompgeek on 9 May 2019

👍1

I've submitted documentation PRs rust-lang/book#1955 and rust-lang-nursery/reference#609 for this.

I've also written most of a stabilization report after feedback on those PRs have been addressed and #60529 has had time to bake in nightly.

davidtwco on 12 May 2019

👍4

@davidtwco is this blocked on https://github.com/rust-lang/book/pull/1955? That one seemed to fail because the feature hasn't been stabilized yet.

lnicola on 5 Jun 2019

@davidtwco is this blocked on rust-lang/book#1955? That one seemed to fail because the feature hasn't been stabilized yet.

I'm not sure what the exact procedure is. I think the documentation pull requests should probably be reviewed before a stabilization report is posted, though the various pull requests that implement this feature have probably had sufficient time to bake in nightly now, IMO.

davidtwco on 9 Jun 2019

The doc pull requests do not have to land before stabilization PRs.

cramertj on 10 Jun 2019

@cramertj Normally they should land alongside the stabilization. iirc, the stabilization has to provide the doc as well.

GuillaumeGomez on 11 Jun 2019

It looks like the only thing left is the stabilization PR? I can make one if so.

seanmonstar on 10 Jul 2019

@seanmonstar I'm not sure about the design yet (e.g. I'm not convinced #[non_exhaustive] is the better design as compared to struct A { field: u8, .. }) so it's not time for a stabilization report and at any rate some language team member should review the tests and write a report before we stabilize.

Centril on 10 Jul 2019

@Centril a stabilization report is mostly done above.

seanmonstar on 10 Jul 2019

❤2

That seems like a good start aside from possible changes that need to be made. I'll (or someone else will) talk to @davidtwco to make necessary changes and elaborations once the team has given initial clearance for stabilization.

Centril on 10 Jul 2019

@Centril It was originally decided in the RFC to not add extra syntax for this, and so the RFC process then decided that an attribute was better. The reasoning is that, if you want to add extra syntax for it, the attribute can simply be deprecated; syntax changes cannot.

I feel like stabilising now as-is is much better of a solution than delaying because the syntax isn't as preferred.

clarfonthey on 11 Jul 2019

👍5

It was originally decided in the RFC to not add extra syntax for this, and so the RFC process then decided that an attribute was better.

I'm aware of what the RFC decided. They are however not final and subject to change.

While reading the discussion on the RFC I found the arguments in favor of an attribute to be unpersuasive whereas I find .. to be natural from a consistency POV in that it is unsurprising given how pattern matching syntax already works.

I also agree in particular with @glaebhoerl:

I disagree that changing the things you are allowed to do with a type "doesn't change the semantics of [the type] in any particular way". The ways you are allowed to create a given type together with the ways you are allowed to use it is essentially its whole definition!

I also agree with @petrochenkov's notes in https://github.com/rust-lang/rust/issues/44109#issuecomment-377478367 and with @scottmcm in https://github.com/rust-lang/rust/issues/44109#issuecomment-377659828.

The reasoning is that, if you want to add extra syntax for it, the attribute can simply be deprecated; syntax changes cannot.

There are several cases in which syntax has been deprecated or even removed:

warning: `...` range patterns are deprecated
 --> src/lib.rs:3:10
  |
3 |         0...3 => 0,
  |          ^^^ help: use `..=` for an inclusive range
  |
  = note: #[warn(ellipsis_inclusive_range_patterns)] on by default

error: expected one of `:` or `@`, found `)`
 --> src/lib.rs:2:14
  |
2 |     fn bar(u8) {}
  |              ^ expected one of `:` or `@` here
  |
  = note: anonymous parameters are removed in the 2018 edition (see RFC 1685)

I feel like stabilising now as-is is _much better_ of a solution than delaying because the syntax isn't as preferred.

A similar argument was made for why we should not delay and instead stabilize await!(...) which we didn't.

Centril on 11 Jul 2019

👍5 👎2

Is there anything that can be done to move this forward? It would be very useful.

Ralith on 8 Aug 2019

@Ralith what this needs is a good full stabilization report along with a summary of the issues raised by and after the RFC, along with responses to said concerns. Then it's just a lang member "sponsorship" away from FCP.

CAD97 on 8 Aug 2019

It seems like a stabilization report was already mostly written, but perhaps abandoned after @Centril's comments?

Ralith on 8 Aug 2019

@Centril would you be willing to write up an RFC for the syntax change? If not, do you know someone who would?

If we're going to be going down that direction, I feel like progress should be started on it. Otherwise, stabilisation will probably happen instead.

clarfonthey on 8 Aug 2019

@clarfon is an RFC needed? The ..= vs ... decision was done by simple FCP as well, as the .await syntax decision.

est31 on 13 Aug 2019

If we changed the syntax at this point, we'd need to cancel stabilization, and instead make the change in nightly and evaluate it further in nightly.

I think many people are...not enthusiastic about changing the syntax at what feels like the last minute. And in any case we can't just change it and immediately stabilize.

joshtriplett on 14 Aug 2019

Personally I am all for changing the syntax as I do agree it looks better. All I care about is that this isn't just sitting in limbo for another few months.

That all said, my investment in this change is rather small compared to some others.

yodaldevoid on 14 Aug 2019

👍8

If people are in favour of the syntax change, I agree with not doing a PR now-- perhaps the syntax version could be added and the attribute given a lint to push people to comment here?

clarfonthey on 15 Aug 2019

👍1

I don't mind slowing things down to get the syntax right, so long as it's making forwards progress rather than languishing in indecision.

Ralith on 15 Aug 2019

👍2

I don't really care either way. If we want to stabilize a syntax instead of an attribute, I would personally be in favor of a trailing ... It seems to fit with its existing usage in constructors and patterns, and it is easy to notice in declarations where variants/fields are typically line-delimited.

Something else that we need to consider is displaying the "non-exhaustiveness" of items in API documentation. This is the main use of this feature, preventing public APIs from breaking under certain circumstances, so I think the reader should be made aware of it before they try to exhaustively match it in Rust and hit a compile error. This seems to be a matter of whether we rely on the documentation writers to provide a # Non-exhaustive section similar to # Panics and whatnot, or if we want to modify rustdoc to emit a note for all non-exhaustive items, and I'm not sure where I stand on that.

agausmann on 15 Aug 2019

❤1

Right now there is a header for enums, structs, and variants that clarifies non-exhaustiveness and what it means. I agree that the syntax would be very consistent with a trailing .., and that a syntax would be nicer than an attribute.

Right now I'm wondering if anyone has the time to make a PR to add this syntax so people can try it out. The attribute would be deprecated and point others here to comment. I feel that both of these should be done soonish to continue progressing this.

clarfonthey on 15 Aug 2019

I don't have an opinion on syntax (please keep discussing) but I'd like to weigh in on one other aspect of #[non_exhaustive] that I find makes the current design inappropriate for my use case.

It needs to be easy to match a syntax tree node nonexhaustively, the most common case.
It needs to be easy for code that uses Syn to opt in to a test failure when Syn adds a variant.
It needs to be hard for code that uses code that uses Syn (2 levels removed) to get a build failure when Syn adds a variant.

match expr {
    Expr::Array(e) => {...}
    Expr::Assign(e) => {...}
    ...
    Expr::Yield(e) => {...}

    #[cfg(test)]
    Expr::TestExhaustive => unimplemented!(),
    #[cfg(not(test))]
    _ => { /* some sane fallback */ }
}

I would love to use #[non_exhaustive] as a less hacky way of doing this but the current design isn't able to meet requirement 2.

To brainstorm a small extension that would make #[non_exhaustive] usable for Syn, it would be great if we could do:

match expr {
    Expr::Array(e) => {...}
    Expr::Assign(e) => {...}
    ...
    Expr::Yield(e) => {...}

    #[cfg_attr(test, deny(reachable))]
    _ => { /* some sane fallback */ }
}

where reachable is an allow-by-default lint on catch-all arms of a nonexhaustive match.

This meets requirement 1 because the lint is allow by default so you just match nonexhaustively and that's it.
This meets requirement 2 because you add this deny attribute to opt in to having your tests notify you of new variants. Note that this works even in the absence of any tests; we don't assume existence of a test case that actually exercises the new syntax at runtime.
This meets requirement 3 because it can't break crates downstream of the match, which cap their dependencies' lints to allow.

dtolnay on 15 Aug 2019

❤15

A solution on the syntax side:

If the syntax for a non-exhaustive enum is enum E { A, B, .. }, then it'd make sense for the consumption syntax to match. Specifically, allow

match e {
    E::A => { },
    E::B => { },
    ..
}

a trailing .. would then act as _ => unimplemented!() but have a warn-by-default lint if it's reachable. If a use case wanted a fallback other than a panic, they could write

match e {
    E::A => { },
    E::B => { },
    #[cfg(not(test))]
    _ => { /* fallback */ },
    #[cfg(test)] #[deny(non_exhaustive_fallback)]
    ..
}

CAD97 on 15 Aug 2019

👎2

I'm not a huge fan of having such a small, easily missed piece of syntax introduce a panic. Users should be encouraged to handle new cases with a graceful fallback, not by killing the process. It also requires even more attributes than @dtolnay's approach, despite introducing new syntax.

Ralith on 15 Aug 2019

👍8

I am strongly against any syntax affecting match blocks.

One thing that would help solve dtolnay's use case is the ability to modify the visibility of the non-exhaustiveness. If the visibility were public, then it would act as if the non-exhaustiveness weren't there.

The main issue I have with this is that you can't easily put an attribute on the visibility on syntax, and it would be probably easier to do so with a parameter to the attribute instead. I feel like adding #[cfg_attr(not(exhaustive_optout), non_exhaustive)] is enough for @dtolnay's use case, and that adding a visibility argument to the attribute (to control who "knows" about the non-exhaustiveness) would make things also work.

Making it so a public visibility non_exhaustive allows exhaustive matching while also not warning on dead code wildcard arms would probably be exactly what you want.

clarfonthey on 16 Aug 2019

We discussed this again at the @rust-lang/lang team meeting today.

The discussion centred around the trade-offs between the existing attribute syntax an the potential alternative syntax of .., to parallel what this requires in patterns, as was also discussed in the RFC. The outcome, as I understood it, is that while some members would prefer one or the other, people were _satisfied_ with having the current attribute syntax even for the long term, and were more interested in having this stable than in debating and baking a change. As such,

@rfcbot fcp merge

Stabilization report

Thank you @davidtwco for putting this together! ❤️

What is stabilized

The non_exhaustive attribute is used to indicate that a type will have
more fields/variants added in the future. It can be applied to
structs, enums and enum variants.

Within the defining crate, types annotated with non_exhaustive behave exactly
the same as if the type were not annotated with non_exhaustive:

```rust,ignore

[non_exhaustive]

pub struct Config {
pub window_width: u16,
pub window_height: u16,
}

[non_exhaustive]

pub enum Error {
Message(String),
Other,
}
pub enum Message {
#[non_exhaustive] Send { from: u32, to: u32, contents: String },
#[non_exhaustive] Reaction(u32),
#[non_exhaustive] Quit,
}
// Non-exhaustive structs can be constructed as normal within the defining crate.
let config = Config { window_width: 640, window_height: 480 };
// Non-exhaustive structs can be matched on exhaustively within the defining crate.
if let Ok(Config { window_width, window_height }) = config {
// ...
}
// Non-exhaustive enums can be matched on exhaustively within the defining crate.
match error {
Error::Message(ref s) => { },
Error::Other => { },
}
match message {
// Non-exhaustive variants can be matched on exhaustively within the defining crate.
Message::Send { from, to, contents } => { },
Message::Reaction(type) => { },
Message::Quit => { },
}

In downstream crates, types annotated with `non_exhaustive` have limitations that
preserve backwards compatibility when new fields/variants are added.

Non-exhaustive types cannot be constructed in downstream crates:

```rust,ignore
// `Config`, `Error` and `Message` are types defined in an upstream crate that have been
// annotated as `#[non_exhaustive]`.
use upstream::{Config, Error, Message};
// Cannot construct an instance of `Config`, if new fields were added in
// a new version of `upstream` then this would fail to compile, so it is
// disallowed.
let config = Config { window_width: 640, window_height: 480 };
// Can construct an instance of `Error`, new variants being introduced would
// not result in this failing to compile.
let error = Error::Message("foo".to_string());
// Cannot construct an instance of `Message::Send` or `Message::Reaction`,
// if new fields were added in a new version of `upstream` then this would
// fail to compile, so it is disallowed.
let message = Message::Send { from: 0, to: 1, contents: "foo".to_string(), };
let message = Message::Reaction(0);
// Cannot construct an instance of `Message::Quit`, if this were converted to
// a tuple-variant `upstream` then this would fail to compile.
let message = Message::Quit;

Non-exhaustive types cannot be used in a match/if let expression without a
wildcard arm:

rust, ignore // `Config`, `Error` and `Message` are types defined in an upstream crate that have been // annotated as `#[non_exhaustive]`. use upstream::{Config, Error, Message}; // Cannot match on a non-exhaustive enum without including a wildcard arm. match error { Error::Message(ref s) => { }, Error::Other => { }, // would compile with: `_ => {},` } // Cannot match on a non-exhaustive struct without a wildcard. if let Ok(Config { window_width, window_height }) = config { // would compile with: `..` } match message { // Cannot match on a non-exhaustive struct enum variant without including a wildcard. Message::Send { from, to, contents } => { }, // Cannot match on a non-exhaustive tuple or unit enum variant. Message::Reaction(type) => { }, Message::Quit => { }, }

Non-exhaustive types are always considered inhabited in downstream crates.

What is not stabilized

N/A

Divergences from RFC

It is not specified in the RFC what the inhabitedness of non-exhaustive types used extern
crates should be. Since #60529, non-exhaustive types are always considered inhabited in
extern crates.

Tests

The tests are all in src/test/ui/rfc-2008-non-exhaustive. Here are some of them:

Motivation

See RFC 2008.

History

Was originally proposed in RFC rust-lang/rfcs#2008.
Implemented for enums/structs in #45394 by @davidtwco (reviewed by @nikomatsakis, @petrochenkov, @oli-obk, @arielb1).
Merged on Nov 4th 2017.
Attribute prohibited on unions, other incorrect locations in #49345 by @davidtwco (reviewed by @nikomatsakis)
Merged on Apr 5th 2018.
Support in rustdoc in #51854 by @davidtwco (reviewed by @QuietMisdreavus, @clarfon, @frewsxcv)
Merged on Jul 19th 2018.
Improved in #53721 by @arielb1 (reviewed by @nikomatsakis, @eddyb)
Merged on Sep 6th 2018.
Fixed #53549.
Implemented for enum variants in #59376 by @davidtwco (reviewed by @petrochenkov, @QuietMisdreavus, @GuillaumeGomez)
Merged on March 30th 2019.
Fixed and added tests for uninhabitedness in #60529 by @davidtwco (reviewed by @petrochenkov, @Centril)
Merged on May 10th 2019.
Update reference for #[non_exhaustive] in rust-lang-nursery/reference#609 by @davidtwco
Submitted on May 12th 2019. Not yet merged!
Update book for #[non_exhaustive] in rust-lang/book#1955 by @davidtwco
Submitted on May 12th 2019. Not yet merged!

scottmcm on 20 Sep 2019

🎉8 ❤6

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

[x] @Centril
[ ] @cramertj
[ ] @eddyb
[x] @joshtriplett
[x] @nikomatsakis
[x] @pnkfelix
[x] @scottmcm
[x] @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot on 20 Sep 2019

Possibly also tag as relnotes?

jonhoo on 20 Sep 2019

Possibly also tag as relnotes?

Will be done on the stabilization PR. Up for that, @davidtwco?

Centril on 20 Sep 2019

Possibly also tag as relnotes?

Will be done on the stabilization PR. Up for that, @davidtwco?

Of course, I'll get right on this.

davidtwco on 20 Sep 2019

I've opened #64639 to stabilize this feature.

davidtwco on 20 Sep 2019

Exciting!

Once (something along the lines of) https://github.com/rust-lang/rust/issues/44109#issuecomment-521781237 is implemented, I'll be able to leverage this in Syn. Would this require a lang RFC?

dtolnay on 20 Sep 2019

@dtolnay I've got an idea to make it work like you want - it would be quite a significant change to the implementation and understanding of this feature, but it seems reasonable to me.

What about making the non_exhaustiveness of matching entirely optional for users? Convert the hard error to a deny-by-default lint so to preserve the expected behavior, but allow the use of an attribute at or above the match statement like #[cfg_attr(test, allow(non_exhaustive_match))] (lint name is a placeholder). That would allow the user to match it like any normal enum - if all of the variants are specified, it compiles successfully, but once a variant is added it will fail to compile.

agausmann on 21 Sep 2019

👍1

What about making the non_exhaustiveness of matching entirely optional for users?

I think that loses the advantage of this attribute for the library author, as it means that adding a new variant again becomes a breaking change, which is exactly what the attribute is trying to prevent.

scottmcm on 21 Sep 2019

@AGausmann All lints are capped at non-path crates e.g. ones you got from crates.io. As usually more code is in the dependencies of a crate than the crate itself, for most code you compile, the lint would actually be turned off. Of course, you could rely on the build step that is run before uploading to crates.io, but that step only checks the specific platform of the uploader and the default features. Any code outside of that could easily be missed before upload and you'll only realize the problem when you update the upstream crate.

@dtolnay your suggestion of adding a lint sounds great. To bikeshed the name, it could be called reachable_patterns because it is the opposite of the unreachable_patterns lint. The two could maybe even share code.

est31 on 21 Sep 2019

@scottmcm The point is that it only becomes breaking if it is explicitly allowed. Maybe "optional" is the wrong word to use as it implies opt-in. What I'm describing is more of an explicit opt-out, only where the user wants it to break.

Regardless, @est31 has made a good point about the limitations of lints ...

agausmann on 21 Sep 2019

Would this require a lang RFC?

@dtolnay Probably just an FCP on the PR in question since it's a forward compat lint, assuming it doesn't cause major slowdown in the compiler or something.

Centril on 21 Sep 2019

I do believe a lint would have advantages, but not for the testing case. In some cases you'd want to have exhaustive matches even if the library author doesn't guarantee stability.

When I'm writing an application and not a library, sometimes I need to translate or categorize things like errors. If a library author introduces a new error, I'm forced to wait for a runtime panic or similar to know I have to adjust the categorization/translation code. I'd prefer to have some option to keep exhaustiveness here as a compile time error or warning instead of a runtime one.

With the lint I'd get a compile error and fix it before deployment. The only machinery I need would be the lint attribute.

Without a lint, I'd have to either live with runtime panics when I choose the simple way out, or I have to build a way for the match to report to the outside that a new condition has occured. I'd have to send and store that message somewhere and so on.

So personally I'd love a lint with a big "you're going past stability guarantees" warning.

phaylon on 21 Sep 2019

In downstream crates, types annotated with non_exhaustive have limitations that
preserve backwards compatibility when new fields/variants are added.

Do we have a list of potentially backward-incompatible things that downstream crates can technically do but maybe shouldn't? For example, can downstream crates call size_of::<NonExhaustive>(), align_of::<NonExhaustive>(), etc. ? If so, they probably shouldn't write code that relies on those values because they can change when new fields are added.

gnzlbg on 22 Sep 2019

For example, can downstream crates call size_of::(), align_of::(), etc ?

I think this is similar to the situation with types that transitively contain private fields, i.e. just something that people need to be careful of.

Ralith on 22 Sep 2019

You shouldn't write code that relies on those values being _a particular number_, but using those values in general is entirely correct.

Lokathor on 22 Sep 2019

👍5

For example, this code compiles today without warnings (playground):

#![feature(non_exhaustive)]
mod foo {
  #[repr(C)]
  #[non_exhaustive]
  pub enum OptionLike<T> {
      Some(T),
      None
  }
}

extern "C" {
    pub fn bar(x: foo::OptionLike<&'static i32>);
}

FFI safety requires layout compatibility but #[non_exhaustive] is saying "don't rely on the layout of this type" so... I don't understand why things like this are allowed because I don't see how they could work correctly under the constraint that fields can be added to #[non_exhaustive] types any time. The improper_ctypes lint should at least warn about this (they might happen to work, but that's why improper_ctypes is a warning and not an error).

EDIT: the same would apply when using a #[non_exhaustive] type in a #[repr(C)] union for type punning, etc.

gnzlbg on 23 Sep 2019

👍4

@gnzlbg Seems like a reasonable concern. It's strictly back-compat to start emitting the lint later, but we might as well fix it from the outset.

@rust-lang/lang does anyone object to having improper_ctypes account for #[non_exhaustive], and if not, could you extend the lint, @davidtwco?

Centril on 23 Sep 2019

FWIW my concern is that it's unclear to me how #[non_exhaustive] constraints the assumptions that downstream crates can make about layout. extern "C" declarations is one place where that can matter, another one is repr(C) types. For example, for a #[non_exhaustive] #[repr(Rust)] enum/struct it doesn't matter where a new field is added, but for #[non_exhaustive] #[repr(C)] struct where a new field is added would matter, because it would change the offsets of all subsequent fields, and field offsets is something that one can in general rely on for repr(C) types (probably not for #[non_exhaustive] #[repr(C)] types).

gnzlbg on 23 Sep 2019

Wait, I just knew about that feature, which is, I have to say, very interesting and appealing as it would solve several issues I’ve been facing.

However, I haven’t found in the RFC an alternative that would be way easier to implement and would solve that problem (and probably others too): pub on variant.

mod foo {
  pub enum Foo {
    pub A,
    pub B,
    NonExhaustive
  }
}

fn main() {
  use foo::Foo;

  let a = Foo::A; // okay
  let b = Foo::B; // okay too
  let c = Foo::NonExhaustive; // nope

  match some_foo {
    Foo::A => …,
    Foo::B => …,
    _ => …, // needed here too
  }
}

I apologize in advance if that has already been pointed out (I haven’t read the whole conversation first, I just wanted to know whether you’ve considered that and/or if such a feature is available maybe elsewhere.

phaazon on 24 Sep 2019

@gnzlbg I imagine the full constraint there would be that you could only add new enum variants at discriminants that aren't yet used and that you cannot add a new variant that has a bigger maximum size than the current largest enum variant. But it wouldn't be strictly wrong to use them together AFAIK.

For structs, you'd basically have to also make sure on the Rust side to only add fields at the end and on the C side to treat it as an unsized type.

Though this is all a bit esoteric and I'm not sure how many people actually want something like this. So I'd be in favor of having the improper C type lint trigger until we know somebody actually wants this combination...

Havvy on 24 Sep 2019

However, I haven’t found in the RFC an alternative that would be way easier to implement and would solve that problem (and probably others too): pub on variant.

I think we should allow pub on variants as well as it is a more generally useful feature. The main reason this approach isn't used is that for enums, it necessitates adding an extra variant. That makes it non-zero-cost.

Centril on 24 Sep 2019

👍1

@Havvy I think I would also be fine with the answer that #[non_exhaustive] is not intended to help with future-proofing layout in any way, and that if you rely on any aspect of layout (call ABI, field offsets, ..), then you are on your own. From the RFC and associated discussion it wasn't clear to me whether this is an explicit goal of the feature or not - I did not see this being discussed anywhere. If the goal is for #[non_exhaustive] to not provide any kind of layout guarantees, then I think the docs should be more explicit about this and mention what the pitfalls are.

gnzlbg on 24 Sep 2019

@Centril what about a marker, like a PhantomData?

phaazon on 24 Sep 2019

@phaazon the problem is that adding a new variant means that the enum discriminant now has to support it because it could be constructed - even if it is private, some unsafe code somewhere could activate it - a marker wouldn't change that. One way to change that would be to make the variant uninhabited (e.g. enum Foo { pub PublicField, PrivateFieldZeroCost(!) } ), but without a more precise model for the Never type is hard to tell whether that would have this particular effect.

gnzlbg on 24 Sep 2019

Ah right, good point. 👍

phaazon on 24 Sep 2019

:bell: This is now entering its final comment period, as per the review above. :bell:

rfcbot on 24 Sep 2019

🎉3 ❤1

I am mildly opposed to linting if #[repr(C)] is combined with #[non_exhaustive], but I do think @gnzlbg raises a good point that we should document how #[repr(C)] interacts with semver.

The question comes down to "what sorts of changes are breaking changes" -- and in particular when is adding a variant or field a breaking change (or changing the name/type of a field). I don't believe #[repr(C)] has much to say on this matter. The current rules are: if all fields/variants are public, then adding a field/variant is a semver breaking change, and changing the name/type of a public field is a breaking change. With #[non_exhaustive], this changes: even if all fields/variants are public, adding a new field/variant is not a breaking change.

#[repr(C)], on the other hand, tells you what you can assume as a consequence of those other decisions. In particular, for an ordinary struct, if all its fields are public, then we know that the set of fields (and their types) cannot change. We further know that the struct is #[repr(C)], which tells us that its layout is fixed and observable.

But if the struct is non-exhaustive, we don't know that the set of fields cannot change, so we cannot assume that the layout is fixed and observable, even though it is tagged with #[repr(C)].

(In contrast, code within the library itself can of course make those assumptions, just as it can match exhaustively on the enum etc; this is because it shares a semver boundary.)

nikomatsakis on 24 Sep 2019

👍1

@nikomatsakis That's a good rationale...

(In contrast, code _within the library itself_ can of course make those assumptions, just as it can match exhaustively on the enum etc; this is because it shares a semver boundary.)

...but I wonder about this aspect. Would it make sense to make the improper_ctypes lint trigger outside of the current crate but not inside? That seems congruent with the intent of #[non_exhaustive]?

Centril on 24 Sep 2019

@Centril I see, maybe I misunderstood what was being proposed. I agree that this may make sense, but if it does trigger, it should probably also trigger for structs with private fields? At least I think the situation is quite analogous.

I suppose one difference is that it is ambiguous whether a struct with private fields may change size, but a #[non_exhaustive] attribute explicitly declares a desire to grow. Perhaps then it does make sense to warn only if the attribute is present (which may also imply a use for the attribute even for structs with private fields).

nikomatsakis on 24 Sep 2019

👍2

@nikomatsakis I think you understood right, but I took your feedback into account and changed the suggestion. ;) Good point regarding usage on structs with private fields.

Centril on 24 Sep 2019

@nikomatsakis that makes sense and sounds good to me.

The docs of #[non_exhaustive] could probably be expanded to mention this, and maybe the API Guidelines section about the semver policy w.r.t. private fields can be updated to mention #[non_exhaustive] as well, but I don't think doing this should block stabilization.

EDIT: opened an issue in the api-guidelines about this: https://github.com/rust-lang-nursery/api-guidelines/issues/203 - don't know if there is some better place for documenting this.

@Centril

Would it make sense to make the improper_ctypes lint trigger outside of the current crate but not inside?

This problem already exists without #[non_exhaustive], and maybe putting this lint along with the other API guidelines lints might make more sense than bolting it on top of improper_ctypes. Either way, we could maybe open a different issue to discuss that? semver compatibility lints are a topic on its own.

gnzlbg on 24 Sep 2019

I haven't read all of the discussion, but I searched for "Swift" in the RFC text, the RFC discussion and this issue and didn't find it referenced. So I'd mention that Swift has an (accepted and implemented) RFC for this feature, although without exposing the capability to user-defined enums. Of particular mention is the @unknown default construct, which is somewhat related to @dtolnay's concern.

bluetech on 25 Sep 2019

👍1

On Thursday's language team meeting (2019-09-26), we discussed the subject of interactions between improper_ctypes and #[repr(C)] #[non_exhaustive] along the lines discussed here. In particular in light of https://github.com/rust-lang/rust/issues/44109#issuecomment-534601202 and https://github.com/rust-lang/rust/issues/44109#issuecomment-534617778.

The consensus of the meeting was that improper_ctypes should take into account the existence of #[non_exhaustive] on a #[repr(C)] type (let's call it Foo). When Foo is used inside it's defining crate for FFI, the usage is regarded as proper but when used outside Foo's defining crate for FFI, it is regarded as improper.

@davidtwco Do you think you could roll that into the stabilization PR or some other PR?

Centril on 2 Oct 2019

@davidtwco Do you think you could roll that into the stabilization PR or some other PR?

Sure thing, will take a look at doing this.

davidtwco on 2 Oct 2019

❤1

@Centril Is there a recording of said meeting? I would be interested to hear the discussion that lead to this decision.

yodaldevoid on 3 Oct 2019

@nikomatsakis ^--- iirc you recorded said meeting but it's not uploaded at https://www.youtube.com/playlist?list=PL85XCvVPmGQg-gYy7R6a_Y91oQLdsbSpa yet.

Centril on 3 Oct 2019

I know that I didn't participate in the RFC discussion (only learned about it later) but would adding an attribute on the wildcard pattern to warn if there are any unimplemented variants be within scope?

Nokel81 on 3 Oct 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

rfcbot on 4 Oct 2019

❤1

Submitted #65130 to address the interaction with #[non_exhaustive] and improper_ctypes.

davidtwco on 5 Oct 2019

@yodaldevoid I think the recording (and minutes) are available here

nikomatsakis on 9 Oct 2019

Would it be possible to also have something like #[non_exhaustive(pub)] that would make enum non-exhaustive for dependencies but allow exhaustive matches in the crate defining the enum? This way, the crate author wouldn't forget to handle some cases (hard error if he does) and the dependencies would have to add wildcard pattern.