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

davidtwco · Oct 19, 2017

This work-in-progress pull request contains my changes to implement RFC 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.

rust-highfive · Oct 19, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nikomatsakis · Oct 19, 2017

r? @nikomatsakis

nikomatsakis · Oct 20, 2017

nikomatsakis reviewed Oct 20, 2017

View changes

nikomatsakis · Oct 20, 2017

src/librustc/ty/mod.rs

@@ -1371,6 +1371,7 @@ pub struct AdtDef {
    pub variants: Vec<VariantDef>,
    flags: AdtFlags,
    pub repr: ReprOptions,
+    pub non_exhaustive: bool,


Can you add a comment briefly explaining what this is, perhaps linking to the RFC? I know none of the other fields have comments, but that's no reason not to do better... =)

I definitely will, I plan on doing another pass to make sure everything is well commented, just wanted to get that pushed up before I called it a night. Will do this ASAP.

Just pushed up a comment here.

oli-obk · Oct 20, 2017

oli-obk requested changes Oct 20, 2017

View changes

All the error-cases have a trivial solution. I think it would be cool if we also produced structured suggestions showing how to fix it

oli-obk · Oct 20, 2017

src/test/compile-fail/rfc-2008-non-exhaustive/enum_field_attempt_create.rs

+use enums::FieldEnum;
+
+// This errors as we cannot create a enum variant marked non_exhaustive
+// from an external crate as if the create added new fields it would be


typo: create -> crate

oli-obk · Oct 20, 2017

src/test/compile-fail/rfc-2008-non-exhaustive/enum_missing_wildcard.rs

+
+fn match_enum(ne: &NormalEnum) -> &str {
+    match *ne {
+        //~^ ERROR `..` required with variant marked as non-exhaustive


The error message is wrong, the enum is marked as non-exhaustive, not the variant

Fixed this part of the test.

oli-obk · Oct 20, 2017

src/librustc_typeck/check/_match.rs

+        if tcx.sess.features.borrow().non_exhaustive {
+            // Require `..` if structure or enum has non_exhaustive attribute.
+            if adt.non_exhaustive && !adt.did.is_local() && !etc {
+                tcx.sess.span_err(


I think this error should have its own error number.

I've added error numbers in a WIP commit that I couldn't get compiling locally - messages regarding the diagnostic code not being registered despite my not being able to spot a difference in how they are being added. Would appreciate any thoughts here.

oli-obk · Oct 20, 2017

src/librustc_typeck/check/mod.rs

+                let descr = self.tcx.adt_def(variant.did).variant_descr();
+
+                // Prohibit struct expressions when non exhaustive flag is set.
+                self.tcx.sess.span_err(expr.span, &format!("cannot create non-exhaustive {}",


Same here, but a different new error code

See other comment w/r/t error codes.

oli-obk · Oct 20, 2017

tidy failed:

[00:03:26] tidy error: /checkout/src/test/compile-fail/rfc-2008-non-exhaustive/functional_record.rs:24: line longer than 100 chars
[00:03:26] tidy error: /checkout/src/librustc_privacy/lib.rs:632: line longer than 100 chars
[00:03:26] tidy error: /checkout/src/librustc_privacy/lib.rs:635: line longer than 100 chars

davidtwco · Oct 20, 2017

@oli-obk Thanks for the comments, I'll get on these ASAP.

Update: pushed up changes that fixed most everything mentioned including the tidy.

oli-obk · Oct 21, 2017

oli-obk reviewed Oct 21, 2017

View changes

oli-obk · Oct 21, 2017

src/librustc/diagnostics.rs

@@ -1939,6 +1939,65 @@ fn foo<'a>(x: &'a i32, y: &i32) -> &'a i32 {
 ```
 "##,

+E0638: r##"


You need to add this to the diagnostics file of librustc_typeck

Thanks, I've pushed up a fix for this. I wasn't aware that there was a diagnostics file in the other crates, my bad.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/librustc/ty/mod.rs

+    /// fields/variants of this data type.
+    ///
+    /// See RFC 2008 (https://github.com/rust-lang/rfcs/pull/2008).
+    pub non_exhaustive: bool,


This is better merged into the flags: AdtFlags field.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/librustc_metadata/encoder.rs

@@ -648,6 +649,14 @@ impl<'a, 'b: 'a, 'tcx: 'b> IsolatedEncoder<'a, 'b, 'tcx> {
            }
        }

+        if tcx.sess.features.borrow().non_exhaustive {


This condition seems unnecessary.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/librustc_privacy/lib.rs

@@ -625,6 +625,18 @@ impl<'a, 'tcx> TypePrivacyVisitor<'a, 'tcx> {
                                ctor_vis = field_vis;
                            }
                        }
+
+                        if self.tcx.sess.features.borrow().non_exhaustive {


Unnecessary condition again.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/librustc_resolve/build_reduced_graph.rs

@@ -337,8 +337,20 @@ impl<'a> Resolver<'a> {

            // These items live in both the type and value namespaces.
            ItemKind::Struct(ref struct_def, _) => {
+                if self.session.features.borrow().non_exhaustive {


Unnecessary condition again, you don't have to check this all the time, one feature check in feature_gate.rs is enough.

Fixed. Wasn't aware that I didn't need it everywhere - I misinterpreted the instructions on the Rust forge.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/librustc_resolve/build_reduced_graph.rs

+                    // If the structure is marked as non_exhaustive then lower the visibility
+                    // to within the crate.
+                    if has_non_exhaustive && vis == ty::Visibility::Public {
+                        vis = ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX));


Hmm, this doesn't look correct - constructor's visibility below (ctor_vis) should be lowered to pub(crate), not visibility of the struct itself.

Adjusted this.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/librustc_typeck/check/_match.rs

@@ -867,6 +868,15 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
            self.check_pat_walk(&field.pat, field_ty, def_bm, true);
        }

+        if tcx.sess.features.borrow().non_exhaustive {


Unnecessary feature check.

After doing this, I noticed a regression in creating the structs - wasn't able to work out what was causing it.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/librustc_typeck/check/mod.rs

@@ -3408,6 +3408,19 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
            return self.tcx.types.err;
        };

+        if self.tcx.sess.features.borrow().non_exhaustive {


Unnecessary feature check.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/librustc_typeck/check/_match.rs

@@ -867,6 +868,15 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
            self.check_pat_walk(&field.pat, field_ty, def_bm, true);
        }

+        if tcx.sess.features.borrow().non_exhaustive {
+            // Require `..` if structure or enum has non_exhaustive attribute.


Require .. if structure or variant has the non_exhaustive attribute.
It looks like this patch doesn't implement non_exhaustive for variants yet, but non_exhaustive on enum should not restrict use of its variants.

Adjusted this comment.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/librustc_typeck/check/mod.rs

+        if self.tcx.sess.features.borrow().non_exhaustive {
+            let is_local = variant.did.is_local();
+
+            if !is_local {


Looks like the check for non_exhaustive is forgotten here.

Also see https://github.com/rust-lang/rust/pull/45394/files#r146130454, non_exhaustive on enum should not prohibit struct expressions using that enum's variants.

Added check.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/test/compile-fail/rfc-2008-non-exhaustive/auxiliary/enums.rs

+
+pub enum FieldEnum {
+    WithoutFields,
+    #[non_exhaustive] WithAnonStruct { field: u32 },


Since non_exhaustive on variants is not implemented, could you make it an error? (Currently it's silently ignored.)

I've not gotten around to doing this yet.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/test/compile-fail/rfc-2008-non-exhaustive/auxiliary/enums.rs

+pub enum FieldEnum {
+    WithoutFields,
+    #[non_exhaustive] WithAnonStruct { field: u32 },
+    #[non_exhaustive] WithTuple(u32)


I'd like to see all of these configurations tested:

#[non_exhaustive] enum NonExhaustiveEnum { Unit, Tuple(u8), Struct { field: u8 }, } enum NonExhaustiveVariants { #[non_exhaustive] Unit, #[non_exhaustive] Tuple(u8), #[non_exhaustive] Struct { field: u8 }, }

Now testing with these enums.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/test/compile-fail/rfc-2008-non-exhaustive/enum_field_attempt_create.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// FIXME issue #44109, PR #45394


Nit: This can probably be omitted (also FIXME seems out-of-place).

Same with other tests.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/test/compile-fail/rfc-2008-non-exhaustive/enum_field_attempt_create.rs

+// except according to those terms.
+
+// FIXME issue #44109, PR #45394
+// gate-test-non_exhaustive


This marker is used for feature gate tests, but this test is not a feature gate test.

Also, could you actually add a feature gate test? (Making sure a feature error is reported in #[non_exhaustive] is used without feature(non_exhaustive)).

Same with other tests.

I've added a proper feature gate test.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/test/compile-fail/rfc-2008-non-exhaustive/enum_field_attempt_create.rs

+
+// FIXME issue #44109, PR #45394
+// gate-test-non_exhaustive
+#![feature(non_exhaustive)]


Unnecessary feature, feature(non_exhaustive) is required only in crated defining something non_exhaustive, not using it.

Same with other tests.

Removed the unnecessary feature usages.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/librustc_typeck/check/mod.rs

+
+                // Prohibit struct expressions when non exhaustive flag is set.
+                span_err!(self.tcx.sess, expr.span, E0639,
+                          "cannot create non-exhaustive {}",


Nit: "cannot create non-exhaustive {} using struct expression"?
I mean you can obtain non-exhaustive structs using other means, e.g. function calls.

petrochenkov · Oct 22, 2017

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov · Oct 22, 2017

src/test/compile-fail/rfc-2008-non-exhaustive/unit_struct_match_exhaustively.rs

+
+fn match_struct(us: &UnitStruct) {
+    let &UnitStruct { } = us;
+    //~^ ERROR `..` required with struct marked as non-exhaustive


You can merge several test files testing similar things into one file with several //~ ERROR markers, it's usually more convenient that way (both for writing better more exhaustive tests, and for reviewing them).

Changed this. It's a lot easier to keep track of now.

davidtwco · Oct 22, 2017

@petrochenkov Thanks a bunch for the comments. Just fixed a bunch of them and I think there's one that I'm still working to resolve.

arielb1 · Oct 26, 2017

arielb1 reviewed Oct 26, 2017

View changes

arielb1 · Oct 26, 2017

src/librustc_const_eval/_match.rs

            all_ctors.is_empty() && !cx.is_uninhabited(pcx.ty);
        debug!("missing_ctors={:?} is_privately_empty={:?}", missing_ctors,
               is_privately_empty);
+
+        if cx.is_non_exhaustive(pcx.ty) && !cx.is_local(pcx.ty) {


It's confusing to mutate this variable, especially after the debug print

Maybe have

let is_privately_empty = all_ctors.is_empty() && !cx.is_uninhabited(pcx.ty); let is_declared_nonexhaustive = cx.is_non_exhaustive(pcx.ty) && !cx.is_local(pcx.ty); // For privately empty and nonexhaustive enums, we work as // if there were an "extra" `_` constructor for the type, so we // can never match over all constructors. let is_nonexhaustive = is_privately_empty || is_declared_nonexhaustive; if missing_ctors.is_empty() && !is_nonexhaustive {

Fixed this, thanks.

petrochenkov · Nov 3, 2017

petrochenkov reviewed Nov 3, 2017

View changes

petrochenkov · Nov 3, 2017

src/librustc_resolve/build_reduced_graph.rs

+            ctor_vis = ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX));
+        }
+
+        self.define(parent, ident, ValueNS, (ctor_def, ctor_vis, variant.span, expansion));


Since we don't support variants yet, this change can be removed.

petrochenkov · Nov 3, 2017

@davidtwco
Variants are not supported yet, but the attribute is still silently accepted on them.
Could you temporarily prohibit #[non_exhaustive] on variants in ast_validation.rs?

petrochenkov · Nov 3, 2017

Almost ready, r=me after addressing the last round of comments and squashing commits.

davidtwco · Nov 3, 2017

@petrochenkov That's all your comments resolved.

@bors r=petrochenkov

bors · Nov 3, 2017

@davidtwco: 🔑 Insufficient privileges: Not in reviewers

petrochenkov · Nov 3, 2017

@bors r+

bors · Nov 3, 2017

📋 Looks like this PR is still in progress, ignoring approval

petrochenkov · Nov 3, 2017

@bors r+

bors · Nov 3, 2017

📌 Commit d3babe5 has been approved by petrochenkov

bors · Nov 4, 2017

⌛️ Testing commit d3babe5 with merge 5fe8cd7...

bors · Nov 4, 2017

💔 Test failed - status-travis

kennytm · Nov 4, 2017

pretty test of run-pass/rfc-2008-non-exhaustive/enums.rs failed.

[01:16:34] ---- [pretty] run-pass/rfc-2008-non-exhaustive/enums.rs stdout ----
[01:16:34] 	
[01:16:34] error: pretty-printed source does not match expected source

Diff (a single empty line 😕):

--- expected
+++ actual
@@ -20,6 +20,7 @@
 [01:16:34]         NonExhaustiveEnum::Unit => 1,
 [01:16:34]         NonExhaustiveEnum::Tuple(_) => 2,
 [01:16:34] 
+[01:16:34] 
 [01:16:34]         // This particular arm tests that a enum marked as non-exhaustive
 [01:16:34]         // will not error if its variants are matched exhaustively.
 [01:16:34]         NonExhaustiveEnum::Struct { field } => field,

petrochenkov · Nov 4, 2017

This is a pretty-printer bug (#37199).
// ignore-pretty issue #37199 in the test will fix the issue.

petrochenkov · Nov 4, 2017

@bors r+

bors · Nov 4, 2017

📌 Commit 86c62d0 has been approved by petrochenkov

bors · Nov 4, 2017

⌛️ Testing commit 86c62d0 with merge d762b1d...

bors · Nov 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing d762b1d to master...

nikomatsakis · Nov 7, 2017

Just saw that this landed, nice work @davidtwco !

rust-highfive assigned steveklabnik Oct 19, 2017

rust-highfive assigned nikomatsakis and unassigned steveklabnik Oct 19, 2017

petrochenkov self-assigned this Oct 19, 2017

kennytm added the S-waiting-on-author label Oct 20, 2017

petrochenkov changed the title ~~[WIP] RFC 2008: Future-proofing enums/structs with #[non_exhaustive] attribute~~ RFC 2008: Future-proofing enums/structs with #[non_exhaustive] attribute Nov 3, 2017

Ignoring pretty print for test due to #37199

Verified

This commit was signed with a verified signature.

davidtwco David Wood

GPG key ID: 01760B4F9F53F154 Learn about signing commits

Loading status checks…

86c62d0

bors merged commit 86c62d0 into rust-lang:master Nov 4, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

davidtwco deleted the davidtwco:rfc-2008 branch Nov 4, 2017

scottmcm referenced this pull request Nov 7, 2017
Open
Tracking issue for RFC 2008: Future-proofing enums/structs with #[non_exhaustive] attribute #44109
5 of 7 tasks complete

petrochenkov referenced this pull request Jul 29, 2018
Closed
RFC 2008: Variants #52775

rust-lang/rust

Join GitHub today

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

Conversation

davidtwco commented Oct 19, 2017 • edited

rust-highfive assigned steveklabnik Oct 19, 2017

This comment has been minimized.

rust-highfive commented Oct 19, 2017

This comment has been minimized.

nikomatsakis commented Oct 19, 2017

rust-highfive assigned nikomatsakis and unassigned steveklabnik Oct 19, 2017

petrochenkov self-assigned this Oct 19, 2017

nikomatsakis reviewed Oct 20, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk requested changes Oct 20, 2017 View changes

oli-obk left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Oct 20, 2017

This comment has been minimized.

davidtwco commented Oct 20, 2017 • edited

kennytm added the S-waiting-on-author label Oct 20, 2017

oli-obk reviewed Oct 21, 2017 View changes

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov reviewed Oct 22, 2017 View changes

This comment has been minimized.

davidtwco commented Oct 19, 2017 •

edited

nikomatsakis reviewed Oct 20, 2017

View changes

oli-obk requested changes Oct 20, 2017

View changes

davidtwco commented Oct 20, 2017 •

edited

oli-obk reviewed Oct 21, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

petrochenkov reviewed Oct 22, 2017

View changes

arielb1 reviewed Oct 26, 2017

View changes

arielb1 Oct 26, 2017 •

edited

petrochenkov reviewed Nov 3, 2017

View changes