Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
scottmcm opened this issue Aug 26, 2017 · 133 comments · Fixed by #64639

Comments

@scottmcm
Copy link
Member

@scottmcm scottmcm commented Aug 26, 2017

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

Steps:

Issues to be resolved prior to stabilization

  • #[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 {}).
  • Fix #53549
@eddyb

This comment has been minimized.

Copy link
Member

@eddyb eddyb commented Aug 26, 2017

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

@tinaun

This comment has been minimized.

Copy link
Contributor

@tinaun tinaun commented Aug 27, 2017

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

@tinaun

This comment has been minimized.

Copy link
Contributor

@tinaun tinaun commented Aug 27, 2017

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

@clarfon

This comment has been minimized.

Copy link
Contributor

@clarfon clarfon commented Aug 27, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 28, 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

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 15, 2017

@tinaun any progress? need any tips?

@tinaun

This comment has been minimized.

Copy link
Contributor

@tinaun tinaun commented Sep 15, 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!

@petrochenkov

This comment has been minimized.

Copy link
Contributor

@petrochenkov petrochenkov commented Sep 16, 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.
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 25, 2017

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

UPDATE: @tinaun responded on Gitter.

@davidtwco

This comment has been minimized.

Copy link
Member

@davidtwco davidtwco commented Oct 13, 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

This comment has been minimized.

Copy link
Member

@davidtwco davidtwco commented Oct 15, 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.

@arielb1

This comment has been minimized.

Copy link
Contributor

@arielb1 arielb1 commented Oct 16, 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

This comment has been minimized.

Copy link
Contributor

@arielb1 arielb1 commented Oct 16, 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

This comment has been minimized.

Copy link
Contributor

@arielb1 arielb1 commented Oct 16, 2017

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

@davidtwco

This comment has been minimized.

Copy link
Member

@davidtwco davidtwco commented Oct 16, 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.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 16, 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)

@davidtwco

This comment has been minimized.

Copy link
Member

@davidtwco davidtwco commented Oct 17, 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 added a commit to davidtwco/rust that referenced this issue Oct 1, 2019
This commit stabilizes RFC 2008 (rust-lang#44109) by removing the feature gate.

Signed-off-by: David Wood <david@davidtw.co>
@Centril

This comment has been minimized.

Copy link
Member

@Centril Centril commented Oct 2, 2019

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 #44109 (comment) and #44109 (comment).

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?

@davidtwco

This comment has been minimized.

Copy link
Member

@davidtwco davidtwco commented Oct 2, 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.

@yodaldevoid

This comment has been minimized.

Copy link
Contributor

@yodaldevoid yodaldevoid commented Oct 3, 2019

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

@Centril

This comment has been minimized.

Copy link
Member

@Centril Centril commented Oct 3, 2019

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

@Nokel81

This comment has been minimized.

Copy link
Contributor

@Nokel81 Nokel81 commented Oct 3, 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?

@rfcbot

This comment has been minimized.

Copy link

@rfcbot rfcbot commented Oct 4, 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.

@davidtwco

This comment has been minimized.

Copy link
Member

@davidtwco davidtwco commented Oct 5, 2019

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

tmandry added a commit to tmandry/rust that referenced this issue Oct 5, 2019
… r=petrochenkov

lint: extern non-exhaustive types are improper

This PR makes the `improper_ctype` lint trigger for non-exhaustive types when those types aren't defined in the current crate, as per [this comment](rust-lang#44109 (comment)).

cc @Centril
tmandry added a commit to tmandry/rust that referenced this issue Oct 6, 2019
… r=petrochenkov

lint: extern non-exhaustive types are improper

This PR makes the `improper_ctype` lint trigger for non-exhaustive types when those types aren't defined in the current crate, as per [this comment](rust-lang#44109 (comment)).

cc @Centril
davidtwco added a commit to davidtwco/rust that referenced this issue Oct 6, 2019
This commit stabilizes RFC 2008 (rust-lang#44109) by removing the feature gate.

Signed-off-by: David Wood <david@davidtw.co>
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 9, 2019

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

davidtwco added a commit to davidtwco/rust that referenced this issue Oct 14, 2019
This commit stabilizes RFC 2008 (rust-lang#44109) by removing the feature gate.

Signed-off-by: David Wood <david@davidtw.co>
@Kixunil

This comment was marked as resolved.

Copy link
Contributor

@Kixunil Kixunil commented Oct 17, 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.

@Nemo157

This comment has been minimized.

Copy link
Contributor

@Nemo157 Nemo157 commented Oct 17, 2019

@Kixunil that is how the feature works already.

@Kixunil

This comment has been minimized.

Copy link
Contributor

@Kixunil Kixunil commented Oct 17, 2019

@Nemo157 ah I missed that, awesome! Thank you!

davidtwco added a commit to davidtwco/rust that referenced this issue Oct 25, 2019
This commit stabilizes RFC 2008 (rust-lang#44109) by removing the feature gate.

Signed-off-by: David Wood <david@davidtw.co>
@bors bors closed this in 959b6e3 Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.