Fix nr2 exhaustiveness #3465
Fix nr2 exhaustiveness #3465
Conversation
P-E-P
force-pushed
the
fix-nr2-exhaustiveness
branch
from
aad256b to
7f085e6
Compare
P-E-P
requested review from
powerboat9 and
CohenArthur
and removed request for
powerboat9
P-E-P
force-pushed
the
fix-nr2-exhaustiveness
branch
2 times, most recently
from
6fbfe0a to
764a20c
Compare
| @@ -22,7 +22,8 @@ | |||
| namespace Rust { | |||
| namespace Resolver2_0 { | |||
|
|
|||
| Rib::Definition::Definition (NodeId id, Mode mode) | |||
| Rib::Definition::Definition (NodeId id, Mode mode, bool enum_variant) | |||
There was a problem hiding this comment.
Definitions shouldn't need an enum_variant property, since all items in an enum-kind rib (and only those items) should be enum variants
There was a problem hiding this comment.
I think we do need them because of generic types:
enum Test<G> {
// G should be available for any items inside the variant definition
Other(G),
Another(Struct<G>),
}There was a problem hiding this comment.
Dang, forgot about that. Maybe two nodes/ribs, like this?
Node (Kind::Enum)
| - Node (Kind::EnumInner)
| - G
| - Other
| - Another
There was a problem hiding this comment.
I'm not thrilled about this because it seems to change quite fundamentally the behavior of the foreverstack.
There was a problem hiding this comment.
I should probably adjust the diagram a bit for clarity. The node structure would be like this
Node (enum item definitions are in scope)
| - Node (generics are in scope) <--- current node
Would also block paths to generics, like Test::G, from working
There was a problem hiding this comment.
Unless I'm mistaken this diagram does not change how I interpret your proposal:
Enum variants have their own scope and a new anonymous rib dedicated to generics should be pushed within the enum among the variants (Correct me if I'm wrong).
I think there are two problems with this:
- We kinda have to "jump" over a scope in enums, this means we need to change the behavior of many things for what I consider an edge case. Once again correct me if I'm wrong but that behavior can only be found with enum variants ?
- Generics do not appear naturally at the same level as variants, this means that variant childs declaration won't be separated by their parent but should be put in the generic scope to gain access to those generics ?
EDIT: Just found out that generics cannot be used inside enum discriminant values.
|
|
||
| return candidate.map_or ( | ||
| [&resolved_definition, skip_enum_variant] (Rib::Definition found) { | ||
| if (skip_enum_variant && found.is_variant ()) |
There was a problem hiding this comment.
This can be moved out of the map_or, and check the kind of the current node's rib instead
There was a problem hiding this comment.
This is related to your previous comment, right ? Correct me if I'm wrong but with my understanding of generics we can't do that.
|
A similar tweak as done to |
P-E-P
force-pushed
the
fix-nr2-exhaustiveness
branch
11 times, most recently
from
11bbbc5 to
0d86546
Compare
Rib kind had no string representation, and thus were not used in the debug string representation. gcc/rust/ChangeLog: * resolve/rust-forever-stack.hxx: Output rib kind. * resolve/rust-rib.h: Add function to get string representation from a rib kind. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
Enum variants shouldn't be accessed directly even from within an enum. This commit keeps the provenance for enum variants definition so we can skip them when resolving a value within an enum definition. gcc/rust/ChangeLog: * resolve/rust-forever-stack.h: Add new function to insert enum variants and add argument to resolver's get function to explicitely skip enum variants. * resolve/rust-forever-stack.hxx: Update function definitions. * resolve/rust-name-resolution-context.cc (NameResolutionContext::insert_variant): Add function to insert enum variants. * resolve/rust-name-resolution-context.h: Add function's prototype. * resolve/rust-rib.cc (Rib::Definition::Definition): Add new boolean to hint at enum variant provenance. (Rib::Definition::is_variant): New getter for variant status. (Rib::Definition::Shadowable): Update constructor to opt out of enum variants. (Rib::Definition::Globbed): Likewise. (Rib::Definition::NonShadowable): Change constructor to forward enum variant provenance status. * resolve/rust-rib.h: Update function prototypes. * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::insert_enum_variant_or_error_out): Add function to insert enum variants in the name resolver. (TopLevel::visit): Update several enum variant's visitor function with the new enum variant name resolving code. * resolve/rust-toplevel-name-resolver-2.0.h: Update function prototypes. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
New enum variant status now appears in the string representation of the resolver's definition. gcc/rust/ChangeLog: * resolve/rust-rib.cc (Rib::Definition::to_string): Add enum variant status. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
gcc/testsuite/ChangeLog: * rust/compile/nr2/exclude: Remove test. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
Highlight the fact that a value inside an enum definition refers to a struct outside of the enum and not to the enum variant's name directly. gcc/testsuite/ChangeLog: * rust/compile/enum_variant_name.rs: New test. Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
P-E-P
force-pushed
the
fix-nr2-exhaustiveness
branch
from
0d86546 to
266c6c3
Compare
There was a problem hiding this comment.
as I said to PE privately, while this may not be a perfect solution it gets us further and makes us move forward so I'm happy merging it. we can think about how to improve it by looking at what rustc does in this situation and trying to figure out an even better way to do this, but the priority is about reducing the gap between nr1.0 and nr2.0 which this PR achieves. so LGTM
No description provided.