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.

Sign up for GitHub

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

Fix nr2 exhaustiveness #3465

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+156 −30
Open

Fix nr2 exhaustiveness #3465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bf3bc03
Add rib kind debug representation
P-E-P Feb 27, 2025
62f597c
Keep definition provenance to skip enum variants
P-E-P Mar 4, 2025
2a94a9b
Add enum variant string information to definition
P-E-P Mar 4, 2025
550608b
Remove nr2 exhaustiveness test from exclusion list
P-E-P Mar 4, 2025
266c6c3
Add a test for enum variant name resolution
P-E-P Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions gcc/rust/resolve/rust-forever-stack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,9 @@ template <Namespace N> class ForeverStack
*/
tl::expected<NodeId, DuplicateNameError> insert (Identifier name, NodeId id);

tl::expected<NodeId, DuplicateNameError> insert_variant (Identifier name,
NodeId id);

/**
* Insert a new shadowable definition in the innermost `Rib` in this stack
*
Expand Down
28 changes: 21 additions & 7 deletions gcc/rust/resolve/rust-forever-stack.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,14 @@ ForeverStack<Namespace::Labels>::insert (Identifier name, NodeId node)
Rib::Definition::Shadowable (node));
}

template <>
inline tl::expected<NodeId, DuplicateNameError>
ForeverStack<Namespace::Types>::insert_variant (Identifier name, NodeId node)
{
return insert_inner (peek (), name.as_string (),
Rib::Definition::NonShadowable (node, true));
}

template <Namespace N>
Rib &
ForeverStack<N>::peek ()
Expand Down Expand Up @@ -275,10 +283,12 @@ ForeverStack<N>::get (const Identifier &name)

return candidate.map_or (
[&resolved_definition] (Rib::Definition found) {
// for most namespaces, we do not need to care about various ribs - they
// are available from all contexts if defined in the current scope, or
// an outermore one. so if we do have a candidate, we can return it
// directly and stop iterating
if (found.is_variant ())
return KeepGoing::Yes;
// for most namespaces, we do not need to care about various ribs -
// they are available from all contexts if defined in the current
// scope, or an outermore one. so if we do have a candidate, we can
// return it directly and stop iterating
resolved_definition = found;

return KeepGoing::No;
Expand Down Expand Up @@ -786,13 +796,17 @@ ForeverStack<N>::stream_rib (std::stringstream &stream, const Rib &rib,
const std::string &next,
const std::string &next_next) const
{
std::string rib_kind = Rib::kind_to_string (rib.kind);
stream << next << "rib [" << rib_kind << "]: {";
if (rib.get_values ().empty ())
{
stream << next << "rib: {},\n";
stream << "}\n";
return;
}

stream << next << "rib: {\n";
else
{
stream << "\n";
}

for (const auto &kv : rib.get_values ())
stream << next_next << kv.first << ": " << kv.second.to_string () << "\n";
Expand Down
6 changes: 6 additions & 0 deletions gcc/rust/resolve/rust-name-resolution-context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ NameResolutionContext::insert (Identifier name, NodeId id, Namespace ns)
}
}

tl::expected<NodeId, DuplicateNameError>
NameResolutionContext::insert_variant (Identifier name, NodeId id)
{
return types.insert_variant (name, id);
}

tl::expected<NodeId, DuplicateNameError>
NameResolutionContext::insert_shadowable (Identifier name, NodeId id,
Namespace ns)
Expand Down
3 changes: 3 additions & 0 deletions gcc/rust/resolve/rust-name-resolution-context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,9 @@ class NameResolutionContext
tl::expected<NodeId, DuplicateNameError> insert (Identifier name, NodeId id,
Namespace ns);

tl::expected<NodeId, DuplicateNameError> insert_variant (Identifier name,
NodeId id);

tl::expected<NodeId, DuplicateNameError>
insert_shadowable (Identifier name, NodeId id, Namespace ns);

Expand Down
19 changes: 14 additions & 5 deletions gcc/rust/resolve/rust-rib.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitions shouldn't need an enum_variant property, since all items in an enum-kind rib (and only those items) should be enum variants

Copy link
Member Author

@P-E-P P-E-P Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>),
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang, forgot about that. Maybe two nodes/ribs, like this?

Node (Kind::Enum)
| - Node (Kind::EnumInner)
    | - G
| - Other
| - Another

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not thrilled about this because it seems to change quite fundamentally the behavior of the foreverstack.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@P-E-P P-E-P Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

: enum_variant (enum_variant)
{
switch (mode)
{
Expand Down Expand Up @@ -51,6 +52,12 @@ Rib::Definition::is_ambiguous () const
return ids_globbed.size () > 1;
}

bool
Rib::Definition::is_variant () const
{
return enum_variant;
}

std::string
Rib::Definition::to_string () const
{
Expand All @@ -69,25 +76,27 @@ Rib::Definition::to_string () const
}
}
out << "]";
if (enum_variant)
out << "(enum variant)";
return out.str ();
}

Rib::Definition
Rib::Definition::Shadowable (NodeId id)
{
return Definition (id, Mode::SHADOWABLE);
return Definition (id, Mode::SHADOWABLE, false);
}

Rib::Definition
Rib::Definition::NonShadowable (NodeId id)
Rib::Definition::NonShadowable (NodeId id, bool enum_variant)
{
return Definition (id, Mode::NON_SHADOWABLE);
return Definition (id, Mode::NON_SHADOWABLE, enum_variant);
}

Rib::Definition
Rib::Definition::Globbed (NodeId id)
{
return Definition (id, Mode::GLOBBED);
return Definition (id, Mode::GLOBBED, false);
}

DuplicateNameError::DuplicateNameError (std::string name, NodeId existing)
Expand Down
43 changes: 41 additions & 2 deletions gcc/rust/resolve/rust-rib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class Rib
class Definition
{
public:
static Definition NonShadowable (NodeId id);
static Definition NonShadowable (NodeId id, bool enum_variant = false);
static Definition Shadowable (NodeId id);
static Definition Globbed (NodeId id);

Expand All @@ -124,11 +124,21 @@ class Rib
std::vector<NodeId> ids_non_shadowable;
std::vector<NodeId> ids_globbed;

// Enum variant should be skipped when dealing with inner definition.
// struct E2;
//
// enum MyEnum<T> /* <-- Should be kept */{
// E2 /* <-- Should be skipped */ (E2);
// }
bool enum_variant;

Definition () = default;

Definition &operator= (const Definition &) = default;
Definition (Definition const &) = default;

bool is_variant () const;

bool is_ambiguous () const;

NodeId get_node_id () const
Expand All @@ -155,7 +165,7 @@ class Rib
GLOBBED
};

Definition (NodeId id, Mode mode);
Definition (NodeId id, Mode mode, bool enum_variant);
};

enum class Kind
Expand All @@ -175,6 +185,35 @@ class Rib
ConstParamType,
} kind;

static std::string kind_to_string (Rib::Kind kind)
{
switch (kind)
{
case Rib::Kind::Normal:
return "Normal";
case Rib::Kind::Module:
return "Module";
case Rib::Kind::Function:
return "Function";
case Rib::Kind::ConstantItem:
return "ConstantItem";
case Rib::Kind::TraitOrImpl:
return "TraitOrImpl";
case Rib::Kind::Item:
return "Item";
case Rib::Kind::Closure:
return "Closure";
case Rib::Kind::MacroDefinition:
return "Macro definition";
case Rib::Kind::ForwardTypeParamBan:
return "Forward type param ban";
case Rib::Kind::ConstParamType:
return "Const Param Type";
default:
rust_unreachable ();
}
}

Rib (Kind kind);
Rib (Kind kind, std::string identifier, NodeId id);
Rib (Kind kind, std::unordered_map<std::string, NodeId> values);
Expand Down
54 changes: 41 additions & 13 deletions gcc/rust/resolve/rust-toplevel-name-resolver-2.0.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,18 @@ TopLevel::TopLevel (NameResolutionContext &resolver)

template <typename T>
void
TopLevel::insert_or_error_out (const Identifier &identifier, const T &node,
Namespace ns)
TopLevel::insert_enum_variant_or_error_out (const Identifier &identifier,
const T &node)
{
insert_or_error_out (identifier, node.get_locus (), node.get_node_id (), ns);
insert_enum_variant_or_error_out (identifier, node.get_locus (),
node.get_node_id ());
}

void
TopLevel::insert_or_error_out (const Identifier &identifier,
const location_t &locus, const NodeId &node_id,
Namespace ns)
TopLevel::check_multiple_insertion_error (
tl::expected<NodeId, DuplicateNameError> result, const Identifier &identifier,
const location_t &locus, const NodeId node_id)
{
// keep track of each node's location to provide useful errors
node_locations.emplace (node_id, locus);

auto result = ctx.insert (identifier, node_id, ns);
if (result)
dirty = true;
else if (result.error ().existing != node_id)
Expand All @@ -58,6 +55,37 @@ TopLevel::insert_or_error_out (const Identifier &identifier,
identifier.as_string ().c_str ());
}
}
void
TopLevel::insert_enum_variant_or_error_out (const Identifier &identifier,
const location_t &locus,
const NodeId node_id)
{
// keep track of each node's location to provide useful errors
node_locations.emplace (node_id, locus);

auto result = ctx.insert_variant (identifier, node_id);
check_multiple_insertion_error (result, identifier, locus, node_id);
}

template <typename T>
void
TopLevel::insert_or_error_out (const Identifier &identifier, const T &node,
Namespace ns)
{
insert_or_error_out (identifier, node.get_locus (), node.get_node_id (), ns);
}

void
TopLevel::insert_or_error_out (const Identifier &identifier,
const location_t &locus, const NodeId &node_id,
Namespace ns)
{
// keep track of each node's location to provide useful errors
node_locations.emplace (node_id, locus);

auto result = ctx.insert (identifier, node_id, ns);
check_multiple_insertion_error (result, identifier, locus, node_id);
}

void
TopLevel::go (AST::Crate &crate)
Expand Down Expand Up @@ -336,19 +364,19 @@ TopLevel::visit (AST::TupleStruct &tuple_struct)
void
TopLevel::visit (AST::EnumItem &variant)
{
insert_or_error_out (variant.get_identifier (), variant, Namespace::Types);
insert_enum_variant_or_error_out (variant.get_identifier (), variant);
}

void
TopLevel::visit (AST::EnumItemTuple &variant)
{
insert_or_error_out (variant.get_identifier (), variant, Namespace::Types);
insert_enum_variant_or_error_out (variant.get_identifier (), variant);
}

void
TopLevel::visit (AST::EnumItemStruct &variant)
{
insert_or_error_out (variant.get_identifier (), variant, Namespace::Types);
insert_enum_variant_or_error_out (variant.get_identifier (), variant);
}

void
Expand Down
17 changes: 15 additions & 2 deletions gcc/rust/resolve/rust-toplevel-name-resolver-2.0.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,14 @@ class TopLevel : public DefaultResolver
return std::move (imports_to_resolve);
}

void check_multiple_insertion_error (
tl::expected<NodeId, DuplicateNameError> result,
const Identifier &identifier, const location_t &locus,
const NodeId node_id);

/**
* Insert a new definition or error out if a definition with the same name was
* already present in the same namespace in the same scope.
* Insert a new definition or error out if a definition with the same name
* was already present in the same namespace in the same scope.
*
* @param identifier The identifier of the definition to add.
* @param node A reference to the node, so we can get its `NodeId` and
Expand All @@ -130,6 +135,14 @@ class TopLevel : public DefaultResolver
const location_t &locus, const NodeId &id,
Namespace ns);

template <typename T>
void insert_enum_variant_or_error_out (const Identifier &identifier,
const T &node);

void insert_enum_variant_or_error_out (const Identifier &identifier,
const location_t &locus,
const NodeId node_id);

private:
// If a new export has been defined whilst visiting the visitor is considered
// dirty
Expand Down
12 changes: 12 additions & 0 deletions gcc/testsuite/rust/compile/enum_variant_name.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// { dg-additional-options "-w -frust-name-resolution-2.0" }
struct E1;

enum Test {
E1 = {
let x = E1;
{
let x = E1;
}
0
},
}
1 change: 0 additions & 1 deletion gcc/testsuite/rust/compile/nr2/exclude
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ use_1.rs
v0-mangle1.rs
v0-mangle2.rs
while_break_expr.rs
exhaustiveness2.rs
issue-3139-2.rs
issue-3033.rs
issue-3009.rs
Expand Down