Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix nr2 use declaration import #3499

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Fix nr2 use declaration import #3499

wants to merge 3 commits into from

Conversation

P-E-P
Copy link
Member

@P-E-P P-E-P commented Mar 13, 2025 •
edited
Loading

Fixes #3487

@P-E-P P-E-P added this to the Name Resolution 2.0 rework milestone Mar 13, 2025
@P-E-P P-E-P requested a review from CohenArthur March 13, 2025 14:20
P-E-P
P-E-P commented Mar 13, 2025
View reviewed changes
gcc/rust/resolve/rust-finalize-imports-2.0.h
@@ -49,62 +49,5 @@ class GlobbingVisitor : public AST::DefaultASTVisitor
NameResolutionContext &ctx;
};

// TODO: Fix documentation
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I rename the file ? This pass technically does not exist anymore.

P-E-P added 3 commits March 13, 2025 15:34
@P-E-P
Move import mapping resolution to in tree visit
4ab7513 
Import mapping was relying on resolve_path which in turn relies on
the cursor function. This means the mapping resolver should be called
from the correct scope instead of being called from the crate scope.

gcc/rust/ChangeLog:

	* resolve/rust-early-name-resolver-2.0.cc (Early::Early): Move the
	top level visitor from the function scope to attributes.
	(Early::go): Remove top level visitor creation and adapt calling code.
	Remove call to mapping resolution and import finalization.
	(Early::finalize_simple_import): Move the finalization from it's
	visitor.
	(Early::finalize_glob_import): Likewise.
	(Early::finalize_rebind_import): Likewise.
	(Early::visit): Add mapping resolution and finalization in
	UseDeclaration visitor function.
	* resolve/rust-finalize-imports-2.0.cc (finalize_simple_import): Move
	function.
	(finalize_glob_import): Likewise.
	(finalize_rebind_import): Likewise.
	(FinalizeImports::visit): Remove call to finalizers.
	* resolve/rust-early-name-resolver-2.0.h (class Early): Add top level
	attribute.
	* resolve/rust-finalize-imports-2.0.h: Add function prototypes.
	* resolve/rust-toplevel-name-resolver-2.0.h: Change getter return type
	to reference.

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
@P-E-P
Remove tests from exclusion list
5b49997 
gcc/testsuite/ChangeLog:

	* rust/compile/nr2/exclude: Remove issue-1786 and issue-3033 from
	exclusion list.

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
@P-E-P
Remove finalize import visitor
ebd2a33 
This visitor is not used anymore.

gcc/rust/ChangeLog:

	* resolve/rust-finalize-imports-2.0.cc (FinalizeImports::FinalizeImports):
	Remove constructor.
	(FinalizeImports::go): Remove function.
	(FinalizeImports::visit): Likewise.
	* resolve/rust-finalize-imports-2.0.h (class FinalizeImports): Remove
	FinalizeImports class.

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
@P-E-P P-E-P force-pushed the fix-nr2-imports branch from 3c851f6 to ebd2a33 Compare March 13, 2025 14:35
CohenArthur
CohenArthur approved these changes Mar 14, 2025
View reviewed changes
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM at first glance! I'm not sure I understand if there is still a fixed-point going on, but the fact that two new tests are passing is reassuring. good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CohenArthur CohenArthur CohenArthur approved these changes

Assignees
No one assigned
Labels
name resolution
Projects
name resolution 2.0
Status: In Progress
Milestone
Name Resolution 2.0 rework
Development

Successfully merging this pull request may close these issues.

NR2: Too many super keywords
2 participants
@P-E-P @CohenArthur