Fix name resolution for enums with generics

[liamnaddell]

Fixes #3304
Here is a checklist to help you with your PR.

• GCC development requires copyright assignment or the Developer's Certificate of Origin sign-off, see https://gcc.gnu.org/contribute.html or https://gcc.gnu.org/dco.html
• Read contributing guidlines
• make check-rust passes locally
• Run clang-format
• Added any relevant test cases to gcc/testsuite/rust/

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

*Please write a comment explaining your change. This is the message
that will be part of the merge commit.

[liamnaddell]

Not quite sure if it's appropriate to add a test case for this. It seems like we already have testcases involving enum+Generic, they just weren't run with NR2.0. See, for example issue-850.rs, issue-2105.rs, etc

[CohenArthur]

thanks! this looks great

[CohenArthur]

Not quite sure if it's appropriate to add a test case for this. It seems like we already have testcases involving enum+Generic, they just weren't run with NR2.0. See, for example issue-850.rs, issue-2105.rs, etc

if these tests are passing now with nr2.0, can we take them out of the exclude list for NR2? or are there other issues with them?

if there are other issues with them I'd like to see a small testcase for this specifically for NR2.0

[liamnaddell]

Not quite sure if it's appropriate to add a test case for this. It seems like we already have testcases involving enum+Generic, they just weren't run with NR2.0. See, for example issue-850.rs, issue-2105.rs, etc

if these tests are passing now with nr2.0, can we take them out of the exclude list for NR2? or are there other issues with them?

if there are other issues with them I'd like to see a small testcase for this specifically for NR2.0

I did a run without excludes, and I didn't get any previous tests to start passing. I looked into why issue-850.rs fails, and it seems to be due to lang items paths not working under NR2.0. A lot of other tests involving enums+generics seem to be more complex, and I'm guessing, fail for other unrelated reasons.

[liamnaddell]

I added a test for this: gcc/testsuite/rust/compile/issue-3307.rs

[powerboat9]

Looks good -- should probably add the visitor to DefaultResolver instead of Late and modify TopLevel::visit(Enum &) to use DefaultResolver::visit(Enum &)

[liamnaddell]

Looks good -- should probably add the visitor to DefaultResolver instead of Late and modify TopLevel::visit(Enum &) to use DefaultResolver::visit(Enum &)

Are you referring to this? I can see how it would make sense to add this to DefaultResolver

  auto generic_vis = [this, &enum_item] () {
    for (auto &g : enum_item.get_generic_params ())
      {
	g->accept_vis (*this);
      }
  };

  ctx.scoped (Rib::Kind::Item, enum_item.get_node_id (), generic_vis);

[powerboat9]

Yes, I think DefaultResolver already takes care of that

[liamnaddell]

Yes, I think DefaultResolver already takes care of that

It didn't seem to from my look. I checked lines 101-110 of DefaultResover, and saw:

101 void
102 DefaultResolver::visit (AST::Enum &type)
103 {
104   // FIXME: Do we need to scope anything by default?
105
106   auto variant_fn = [this, &type] () { AST::DefaultASTVisitor::visit (type); };
107
108   ctx.scoped (Rib::Kind::Item /* FIXME: Correct? */, type.get_node_id (),
109         variant_fn, type.get_identifier ());
110 }

[liamnaddell]

Yes, I think DefaultResolver already takes care of that

It didn't seem to from my look. I checked lines 101-110 of DefaultResover, and saw:

101 void
102 DefaultResolver::visit (AST::Enum &type)
103 {
104   // FIXME: Do we need to scope anything by default?
105
106   auto variant_fn = [this, &type] () { AST::DefaultASTVisitor::visit (type); };
107
108   ctx.scoped (Rib::Kind::Item /* FIXME: Correct? */, type.get_node_id (),
109         variant_fn, type.get_identifier ());
110 }

Granted I am new to this part of the codebase, so it's possible I'm missing something.

[powerboat9]

Ah, yeah, it delegates that out to DefaultASTVisitor::visit

[CohenArthur]

I would rename this file issue-3304.rs instead, as 3307 is this PR and not the issue itself

[liamnaddell]

🤦 I fixed this

[CohenArthur]

no worries it's all good :D

[CohenArthur]

Suggested change

	#[lang = "sized"]
	trait Sized {}
	// { dg-additional-options "-frust-name-resolution-2.0" }
	#[lang = "sized"]
	trait Sized {}

[liamnaddell]

I added this patch. Wouldn't this cause issue-3304.rs to be run with NR2.0 twice?

[powerboat9]

Yes, but it shouldn't matter -- that line should be alright either present or not present

[liamnaddell]

I agree, I'm happy to keep it there since it clarifies that this is specifically a NR2.0 test