Skip to content

resolve: Fix ICE on ambiguous glob re-exports#4451

Open
Harishankar14 wants to merge 1 commit intoRust-GCC:masterfrom
Harishankar14:glob
Open

resolve: Fix ICE on ambiguous glob re-exports#4451
Harishankar14 wants to merge 1 commit intoRust-GCC:masterfrom
Harishankar14:glob

Conversation

@Harishankar14
Copy link
Contributor

This patch adds an ambiguity check in 'finalize_rebind_import'. If a Definition is ambiguous, it emits a proper error diagnostic instead of crashing, consistent with rustc's behavior(verified)

Fixes #4411

gcc/rust/ChangeLog:

* resolve/rust-early-name-resolver-2.0.cc (Early::finalize_rebind_import): Add ambiguity check before calling get_node_id() on glob import definitions.

gcc/testsuite/ChangeLog:

* rust/compile/issue-4411.rs: New test.

Thank you for making Rust GCC better!

If your PR fixes an issue, you can add "Fixes #issue_number" into this
PR description and the git commit message. This way the issue will be
automatically closed when your PR is merged. If your change addresses
an issue but does not fully fix it please mark it as "Addresses #issue_number"
in the git commit message.

Here is a checklist to help you with your PR.

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

*Please write a comment explaining your change. This is the message
that will be part of the merge commit.

@Harishankar14
Copy link
Contributor Author

Harishankar14 commented Feb 27, 2026

@P-E-P
Copy link
Member

P-E-P commented Feb 27, 2026

I think this directly collides with some work from @CohenArthur that refactors the whole re-export mechanisms, I'll let him decide whether we should accept those changes or wait for the use declaration rework.

@P-E-P P-E-P requested a review from CohenArthur February 27, 2026 17:40
@Harishankar14
Copy link
Contributor Author

Harishankar14 commented Feb 27, 2026

@P-E-P Alright.

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.

Overall I like the changes as they avoid an ICE and they show the proper behavior, but I'm not sure if this is the right place or not. I think maybe those changes should be done more in resolve_rebind_import? I think you are on the right track and I definitely want to get this PR merged

@Harishankar14 Harishankar14 force-pushed the glob branch 2 times, most recently from 750abce to d9a99a2 Compare March 1, 2026 17:32
@Harishankar14
Copy link
Contributor Author

@CohenArthur Thank you. I have updated the changes !!

@Harishankar14
Copy link
Contributor Author

Harishankar14 commented Mar 5, 2026

@P-E-P Thank you for addressing the small details that i had missed. I have fixed it !!

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.

Looks great, I'll merge this once the remaining blank line will be removed. Great work!

}

for (auto &&definition : data.definitions ())

Copy link
Member

Choose a reason for hiding this comment

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

This blank line isn't required and is confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, me being silly again :)

This patch adds an ambiguity check in 'finalize_rebind_import'.
If a Definition is ambiguous, it emits a proper error diagnostic
instead of crashing, consistent with rustc's behavior(verified)

Fixes Rust-GCC#4411

gcc/rust/ChangeLog:

	* resolve/rust-early-name-resolver-2.0.cc (Early::finalize_rebind_import): Add ambiguity
	check before calling get_node_id() on glob import definitions.

gcc/testsuite/ChangeLog:

	* rust/compile/issue-4411.rs: New test.

Signed-off-by: Harishankar <[email protected]>
@Harishankar14
Copy link
Contributor Author

@P-E-P should be okay now ?

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.

ICE resolve_path

3 participants