Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

powerboat9
Copy link
Collaborator

No description provided.

@@ -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)
Copy link
Member

@P-E-P P-E-P May 21, 2025

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.

Copy link
Collaborator Author

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.

@CohenArthur
Copy link
Member

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

@powerboat9
Copy link
Collaborator Author

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 recursion_limit tracking -- I've reworked the patch to put desugaring + the final TopLevel pass in the fixed point loop though, which seems a bit less hacky.

@powerboat9
Copy link
Collaborator Author

powerboat9 commented May 28, 2025

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 recursion_limit tracking -- I've reworked the patch to put desugaring + the final TopLevel pass in the fixed point loop though, which seems a bit less hacky.

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 TopLevel pass to the expansion function, though.

@powerboat9
Copy link
Collaborator Author

@CohenArthur @P-E-P

@CohenArthur
Copy link
Member

It looks like doing exactly that would screw up recursion_limit tracking

how do you mean? desugaring shouldn't count for the expansion limit, as we control the expansion (as opposed to a user-defined macro).

@powerboat9
Copy link
Collaborator Author

It looks like doing exactly that would screw up recursion_limit tracking

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.

@CohenArthur
Copy link
Member

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

@powerboat9
Copy link
Collaborator Author

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 cfg.recursion_limit. I've pushed what I have so far -- it looks correct, but I'm not sure its better than what I had before.

@powerboat9 powerboat9 force-pushed the fix-apit branch 2 times, most recently from a553768 to 5e57886 Compare June 15, 2025 00:00
Copy link
Member

@P-E-P P-E-P left a 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]>
@powerboat9
Copy link
Collaborator Author

@P-E-P this changeset?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants