-
Notifications
You must be signed in to change notification settings - Fork 179
nr2.0: Run a final TopLevel
pass after desugaring
#3801
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
base: master
Are you sure you want to change the base?
Conversation
gcc/rust/rust-session-manager.cc
Outdated
@@ -622,6 +622,14 @@ Session::compile_crate (const char *filename) | |||
AST::DesugarQuestionMark ().go (parsed_crate); | |||
AST::DesugarApit ().go (parsed_crate); | |||
|
|||
// HACK: run a final toplevel pass | |||
// since desugaring may have added definitions | |||
if (!saw_errors () && flag_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.
Shouldn't we also run early nr again since some of those definitions could technically be matched to some invocations ?
If we could make macro fixed point more generic that would allow us to avoid this weird top level nr call.
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.
I was thinking we'd be done with early resolution before the desugaring and would only need to run TopLevel
for the benefit of Late
resolution.
I don't know if this is the best way to go about this, it's definitely hacky. maybe we could do all the desugars as part of the fixed point and mark the visitor as dirty when we find something to desugar |
It looks like doing exactly that would screw up |
It looks like this doesn't work, as the desugaring doesn't seem to handle partially complete early resolution well. I have moved the desugaring and last |
how do you mean? desugaring shouldn't count for the expansion limit, as we control the expansion (as opposed to a user-defined macro). |
The fixed point loop would either have to skip desugaring if we hit the expansion limit or run an extra iteration past the expansion limit. |
I think this is not really an issue, if we hit the expansion limit we'll have an error anyway and will stop the pipeline. so the desugaring does not matter. I think the resulting code would be more correct |
I'm having trouble writing code that both includes desugaring in the main loop and that I can verify doesn't introduce an off-by-one error to |
a553768
to
5e57886
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.
I don't think we'll get any better version soon, it looks fine so let's merge this.
This fixes some issues with name resolution 2.0. gcc/rust/ChangeLog: * rust-session-manager.cc (Session::compile_crate): Move AST desugaring to... (Session::expansion): ...here and add a final TopLevel pass afterwards. gcc/testsuite/ChangeLog: * rust/compile/nr2/exclude: Remove entries. Signed-off-by: Owen Avery <[email protected]>
@P-E-P this changeset? |
No description provided.