fix is_non_exhaustive confusion between structs and enums
#53721
Conversation
rust-highfive
added
the
S-waiting-on-review
arielb1
force-pushed
the
exhaustively-unpun
branch
from
c16028f to
37b9455
Compare
This comment has been minimized.
This comment has been minimized.
Structs and enums can both be non-exhaustive, with a very different meaning. This PR splits `is_non_exhaustive` to 2 separate functions - 1 for structs, and another for enums, and fixes the places that got the usage confused. Fixes rust-lang#53549.
arielb1
force-pushed
the
exhaustively-unpun
branch
from
37b9455 to
993e7e2
Compare
src/librustc/ty/mod.rs
Outdated
| @@ -1981,10 +1981,30 @@ impl<'a, 'gcx, 'tcx> AdtDef { | |||
| } | |||
|
|
|||
| #[inline] | |||
| pub fn is_non_exhaustive(&self) -> bool { | |||
| fn is_non_exhaustive(&self) -> bool { | |||
There was a problem hiding this comment.
I would rename this to has_nonexhaustive_flag and add a comment about how the meaning of this flag depends a lot on the kind of the ADT. Personally, I find this setup pretty unfortunate: I think we should be "desugaring" the flag so that it appears on the variant for structs and on the adt only for enums (as I described here). But I guess we can separate that out as a distinct sort of refactoring.
There was a problem hiding this comment.
sure, that looks like something that should be done when we implement per-variant exhaustive.
src/librustc/ty/mod.rs
Outdated
| self.flags.intersects(AdtFlags::IS_NON_EXHAUSTIVE) | ||
| } | ||
|
|
||
| #[inline] | ||
| pub fn is_enum_non_exhaustive(&self) -> bool { |
There was a problem hiding this comment.
I think we should call this method:
can_extend_variant_listand add a suitable comment explaining how it is derived from a #[non_exhaustive] that appears on the enum (but not a variant).
src/librustc/ty/mod.rs
Outdated
| } | ||
|
|
||
| #[inline] | ||
| pub fn is_univariant_non_exhaustive(&self) -> bool { |
There was a problem hiding this comment.
I think we should call this method:
can_extend_field_listand -- really -- I think it should be on the variant. But if we want to keep it here, perhaps a name like can_extend_univariant_field_list (again with a comment linking to how it maps to Rust source).
There was a problem hiding this comment.
Normally you are suppose to call is_variant_non_exhaustive, which takes both the AdtDef and the VariantDef.
This is just s a shortcut method because someone removed the old .struct_variant() method.
There was a problem hiding this comment.
Wait it just got renamed to non_enum_variant. I'll just handle that.
There was a problem hiding this comment.
I like to use the name nonexhaustive here, because that's how the feature is called.
This completely splits the IS_NON_EXHAUSTIVE flag. No functional changes intended.
|
pushed.. |
|
@bors r+ |
|
📌 Commit ae2ad30 has been approved by |
bors
added
S-waiting-on-bors
fix `is_non_exhaustive` confusion between structs and enums Structs and enums can both be non-exhaustive, with a very different meaning. This PR splits `is_non_exhaustive` to 2 separate functions - 1 for structs, and another for enums, and fixes the places that got the usage confused. Fixes #53549. r? @eddyb
|
☀️ Test successful - status-appveyor, status-travis |
|
This hurt compile times by up to 3%, mostly for shorter-running benchmarks: @arielb1: any thoughts? |
|
Some bug? This commit should not change performance. |
|
Looks like |
|
Ah decoding of item attrs is duplicated between a struct and its constructor: The code I have written uses the constructor's attrs in |
During metadata loading, the AdtDefs for every ADT in the universe need to be loaded (for example, for coherence of builtin traits). For that, the attributes of the AdtDef need to be loaded too. The attributes of a struct are duplicated between 2 def ids - the constructor def-id, and the "type" def id. Loading attributes for both def-ids, which was done in rust-lang#53721, slowed the compilation of small crates by 2-3%. This PR makes sure we only load the attributes for the "type" def-id, avoiding the slowdown.
avoid loading constructor attributes in AdtDef decoding During metadata loading, the AdtDefs for every ADT in the universe need to be loaded (for example, for coherence of builtin traits). For that, the attributes of the AdtDef need to be loaded too. The attributes of a struct are duplicated between 2 def ids - the constructor def-id, and the "type" def id. Loading attributes for both def-ids, which was done in #53721, slowed the compilation of small crates by 2-3%. This PR makes sure we only load the attributes for the "type" def-id, avoiding the slowdown. r? @eddyb & cc @nnethercote
Structs and enums can both be non-exhaustive, with a very different
meaning. This PR splits
is_non_exhaustiveto 2 separate functions - 1for structs, and another for enums, and fixes the places that got the
usage confused.
Fixes #53549.
r? @eddyb