-
Notifications
You must be signed in to change notification settings - Fork 155
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.
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
nr2.0: Add base for Early name resolution #2470
nr2.0: Add base for Early name resolution #2470
Conversation
will drop annoying commits from this PR and #2469 tomorrow, just running builds and testsuite on all commits here and can't really modify the branch while doing a |
df43d22
to
1f9735b
Compare
{} | ||
|
||
void | ||
DefaultResolver::visit (AST::IdentifierPattern &) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Identifier pattern contains name binding value (@
syntax)
afe484e
to
4904d3b
Compare
@@ -0,0 +1,42 @@ | |||
// { dg-options "-frust-name-resolution-2.0" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a couple of simpler tests that are easier to undetstand here. Maybe also have test(s) that compile successfully to check things expand correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expansion should not change after this new name resolution algorithm, so I plan to just reuse the existing expansion testsuite when we enable name resolution 2.0 by default. I will split up the test in simpler ones though, you are completely right
|
||
void | ||
DefaultResolver::visit (AST::CallExpr &expr) | ||
{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should everything be visiting children here? The classes derived from this are presumably also going to be handling uses,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this should visit the called expression as well as each given argument and generic arguments if any - good catch
if (!definition.has_value ()) | ||
{ | ||
rust_error_at (invoc.get_locus (), ErrorCode ("E0433"), | ||
"could not resolve macro invocation"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be changed for macros that expand to macro definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I plan on working on this soon - basically keep a list of errors and run both the expander and resolver in a fixed point, and if the fixed point is done and we still have errors, emit them, otherwise nope
} | ||
|
||
void | ||
Early::visit (AST::BlockExpr &block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing where modules are handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modules are handled directly in the TopLevel
visitor - I don't think we have anything to do in the Early
visitor other than resolve use
statements to the proper node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They need their own textual scopes, for cases like this
mod a { // No #[macro_use]
macro_rules! mac {
() => {}
}
mac!(); // OK
}
mac!(); // ERROR - `mac` not in scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, right! I missed them. they'll get the same treatment as block expressions:
void
Early::visit (AST::BlockExpr &block)
{
textual_scope.push ();
DefaultResolver::visit (block);
textual_scope.pop ();
}
thank you for noticing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just realized that's the line you were pointing to lol. sorry I'm super tired haha
TopLevel::visit (AST::MacroRulesDefinition ¯o) | ||
{ | ||
// we do not insert macros in the current rib as that needs to be done in the | ||
// textual scope of the Early pass. we only insert them in the root of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
macro
definitions (which libcore uses for some built in macros) are inserted in module scopes,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grrrrr you're right. I hate macro name resolution rules lol. thank you! I'll take care of this
*/ | ||
|
||
// Now our resolver, which keeps track of all the `ForeverStack`s we could want | ||
class Resolver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a class called Resolver
that does not inherit from DefaultResolver
is a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I'll rename it to ResolverContext
, which is different from what we had but makes more sense IMO
969e6d9
to
afc1ca4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (once CI succeed)
d58408c
to
011356c
Compare
0e3cb01
to
ef60369
Compare
gcc/rust/ChangeLog: * resolve/rust-rib.h: Add shadowing parameter. Make kind field public. * resolve/rust-rib.cc (Rib::insert): Likewise.
gcc/rust/ChangeLog: * resolve/rust-forever-stack.h (insert_at_root): New method. (resolve_path): New method. (reverse_iter): Iterate on `Node`s instead of `Rib`s * resolve/rust-forever-stack.hxx: Add path resolution.
gcc/rust/ChangeLog: * resolve/rust-name-resolution-context.cc (Resolver::insert): Do not call into `rust_unreachable` when resolving macros anymore.
gcc/rust/ChangeLog: * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::insert_or_error_out): Fix format string. (is_macro_export): New method. (TopLevel::visit): Handle macro definitions.
gcc/rust/ChangeLog: * resolve/rust-default-resolver.cc (DefaultResolver::visit): Visit CallExpr and MethodCallExpr properly. * resolve/rust-default-resolver.h: Switch "node" to plural in documentation.
This visitor takes care of resolving macro invocations, procedural macros and imports - it is used in conjunction with the `TopLevel` pass and the macro expander. gcc/rust/ChangeLog: * Make-lang.in: Add new object file. * resolve/rust-early-name-resolver-2.0.cc: New file. * resolve/rust-early-name-resolver-2.0.h: New file.
gcc/rust/ChangeLog: * rust-session-manager.cc (Session::expansion): Use new `Early` name resolution 2.0 pass
gcc/testsuite/ChangeLog: * rust/compile/name_resolution6.rs: New test. * rust/compile/name_resolution7.rs: New test. * rust/compile/name_resolution8.rs: New test. * rust/compile/name_resolution9.rs: New test.
gcc/rust/ChangeLog: * ast/rust-macro.h: Add new method to `MacroRulesDefinition` to allow getting the `MacroKind` contained.
gcc/rust/ChangeLog: * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::visit): Declare macros in the current rib if they are macros 2.0.
gcc/testsuite/ChangeLog: * rust/compile/name_resolution10.rs: New test.
ef60369
to
fe2dae1
Compare
This adds a base visitor for resolving macro invocations in the new name resolution 2.0 algorithm. Needs #2469 and #2456