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.

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

fix: resolve extern prelude for local mods in block modules #17251

Merged
merged 3 commits into from
May 22, 2024

Conversation

roife
Copy link
Contributor

@roife roife commented May 17, 2024

fix #17057, #17032.

We should use ModuleOrigin to check if the current module is a pseudo-module introduced by blocks (where names might be shadowed), rather than checking block_def_map.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2024
@roife
Copy link
Contributor Author

roife commented May 18, 2024

I'm not sure how to add unittests for it. 🤔

if self.block.is_some() {
// Don't resolve extern prelude in block `DefMap`s, defer it to the crate def map so
// that blocks can properly shadow them
if matches!(self[module].origin, ModuleOrigin::BlockExpr { .. }) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if matches!(self[module].origin, ModuleOrigin::BlockExpr { .. }) {
if self.block.is_some() && module == DefMap::ROOT {

If I understand the code here correctly, likjewise below. Though I am unsure whether this is the correct fix here

Copy link
Member

Choose a reason for hiding this comment

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

Though it should be more correct than the status quo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are equivalent. If a module is a BlockExpr, its local_id should be equivalent to ROOT, and it should be within a block; conversely, this is also true. Should I change it to the latter?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sorry I wasn't clear. With unsure whether this is the correct fix I meant the general approach here. My code suggestion is the more correct way to do your check here (they are equivalent, it's just that the origin shouldn't be used to check this property).

Comment on lines 521 to 522
if matches!(self[module].origin, ModuleOrigin::BlockExpr { .. }) {
// Don't resolve extern prelude in pseudo-module of a block.
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here

@Veykril
Copy link
Member

Veykril commented May 22, 2024

Thanks!
@bors delegate+
r=me with the review addressed

@bors
Copy link
Collaborator

bors commented May 22, 2024

✌️ @roife, you can now approve this pull request!

If @Veykril told you to "r=me" after making some further change, please make that change, then do @bors r=@Veykril

@Veykril
Copy link
Member

Veykril commented May 22, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented May 22, 2024

📌 Commit d9cc159 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 22, 2024

⌛ Testing commit d9cc159 with merge d4da3f9...

@bors
Copy link
Collaborator

bors commented May 22, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing d4da3f9 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve external preludes for modules inside blocks
4 participants