Conversation
Lets derived visitors override just the traversal of a binder, like visit_function_params already does for function params. No functional change. gcc/rust/ChangeLog: * ast/rust-ast-visitor.h (DefaultASTVisitor::visit_generic_params): Declare. (DefaultASTVisitor::visit_for_lifetimes): Likewise. * ast/rust-ast-visitor.cc (DefaultASTVisitor::visit_generic_params): Define. (DefaultASTVisitor::visit_for_lifetimes): Likewise. (DefaultASTVisitor::visit): Call them instead of walking the parameter lists inline. * resolve/rust-default-resolver.cc (DefaultResolver::visit): Call visit_generic_params for Trait, InherentImpl and TraitImpl. Signed-off-by: Utkarsh Bahuguna <utkarshbahuguna10@gmail.com>
720808d to
b335f24
Compare
CohenArthur
left a comment
There was a problem hiding this comment.
I think this is a good start! The changes to the visitor are great, just need to make sure everything aligns together properly :) good work!!
| if (param->get_kind () == AST::GenericParam::Kind::Lifetime) | ||
| lifetimes.push_back (static_cast<AST::LifetimeParam *> (param.get ())); | ||
|
|
||
| check_duplicate_lifetimes (lifetimes); |
There was a problem hiding this comment.
Do we not define lifetimes in the Labels namespace? Why can't we do something like insert_or_error_out(...) like in the rest of TopLevel?
| for (auto ¶m : for_lifetimes) | ||
| lifetimes.push_back (¶m); | ||
|
|
||
| check_duplicate_lifetimes (lifetimes); |
There was a problem hiding this comment.
Likewise here if this is a definition spot
|
@CohenArthur We don't currently insert lifetime params into Labels, and Labels is shadowable for loop labels, so insert_or_error_out wouldn't catch duplicate lifetime params. I'm happy to make lifetime params proper non-shadowable definitions instead. That would need a small change to the Labels insertion path and E0403 handling. Would you prefer that in this PR? |
struct Foo<'a, 'a> and for<'a, 'a> were both accepted. Lifetimes are definitions now, inserted into the label namespace, so a duplicate falls out of the rib as E0403. They go in non-shadowably - loop labels are allowed to shadow, lifetime params are not. Each for<...> binder gets its own rib, since reusing an outer lifetime name there is shadowing (E0496) rather than a duplicate. gcc/rust/ChangeLog: * resolve/rust-forever-stack.h (ForeverStack::insert_non_shadowable): Declare. * resolve/rust-forever-stack.hxx (ForeverStack::insert_non_shadowable): Define. * resolve/rust-name-resolution-context.h (NameResolutionContext::insert_non_shadowable): Declare. * resolve/rust-name-resolution-context.cc (NameResolutionContext::insert_non_shadowable): Define. * resolve/rust-toplevel-name-resolver-2.0.h (TopLevel::visit): Declare for lifetime parameters. (TopLevel::visit_for_lifetimes): Likewise. * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::visit): Insert lifetime parameters as definitions. (TopLevel::visit_for_lifetimes): Scope each binder. gcc/testsuite/ChangeLog: * rust/compile/dup_lifetime_params.rs: New test. Signed-off-by: Utkarsh Bahuguna <utkarshbahuguna10@gmail.com>
b335f24 to
77c5445
Compare
|
@CohenArthur took your suggestion lifetimes are definitions in the Two things made that work. The second is scoping: with every binder sharing one rib, Net effect is smaller than before |
gccrs currently accepts
struct Foo<'a, 'a>,for<'a, 'a>, etc. rustc rejects these with E0403, so this adds the check.Two commits: first adds
visit_generic_params/visit_for_lifetimestoDefaultASTVisitor(pure refactor, no behavior change), then overrides both inTopLevelto actually report E0403.Went with
TopLevelsince it's the pass that collects definitions and enforces uniqueness (per Arthur on Zulip), whileLateis for resolving uses.Check is scoped to one binder, so an inner
for<'a>reusing an outer'ais untouched, that's shadowing (E0496), different issue.TopLevelvisits each item 3x, so I track reported params inNameResolutionContextto avoid tripling the error. This 3x-visit thing is an existing bug on master too, unrelated to this PR:Output matches rustc:
Tests: 11646 pass / 0 fail (+7 new, covering struct/fn/trait/type alias/impl/HRTB plus the shadowing and distinct-lifetime cases that should stay silent).