Skip to content

fix: Fix associated item visibility in block-local impls #14176

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
Merged
merged 3 commits into from
Mar 1, 2023
Merged

fix: Fix associated item visibility in block-local impls #14176

merged 3 commits into from
Mar 1, 2023

Conversation

lowr
Copy link
Contributor

@lowr lowr commented Feb 19, 2023

Fixes #14046

When we're resolving visibility of block-local items...

self normally refers to the containing non-block module, and super to its parent (etc.). However, visibilities must only refer to a module in the DefMap they're written in, so we restrict them when that happens. (link)

...unless we're resolving visibility of associated items in block-local impls, because that impl is semantically "hoisted" to the nearest (non-block) module. With this PR, we skip the adjustment for such items.

Since visibility representation of those items is modified, this PR also adjusts visibility rendering in HirDisplay.

lowr added 2 commits February 19, 2023 23:55
@lowr
Refactor
4438017 
- Remove unnecessary references and derefs
- Manual formatting
@lowr
Fix associated item visibility in block-local impls
83e24fe
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2023
lowr
lowr commented Feb 19, 2023
View reviewed changes
crates/hir/src/lib.rs
Comment on lines +495 to +497
let def_map = id.def_map(db.upcast());
let origin = def_map[id.local_id].origin;
if matches!(origin, ModuleOrigin::BlockExpr { .. }) {
Copy link
Contributor Author

@lowr lowr Feb 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a simpler way to check whether a hir::Module is a block? I thought I could do better but failed badly...

Copy link
Member

Choose a reason for hiding this comment

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

the ModuleId contained in a hir::Module has a block field that's set when its a block

Copy link
Contributor Author

@lowr lowr Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

block is also set for a non-block module defined inside a block module, because it's derived from a DefMap for block module. It is rare but I'd like to support as much as possible unless it breaks our incremental model.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, then I think this is the appropriate way

@lowr lowr force-pushed the fix/assoc-func-vis-in-local-impl branch from 1c89ead to d416623 Compare February 19, 2023 15:45
@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2023
@Veykril
Copy link
Member

Veykril commented Feb 28, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Feb 28, 2023

📌 Commit d416623 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 28, 2023

⌛ Testing commit d416623 with merge ff4c201...

bors added a commit that referenced this pull request Feb 28, 2023
@bors
Auto merge of #14176 - lowr:fix/assoc-func-vis-in-local-impl, r=Veykril
ff4c201 
Fix associated item visibility in block-local impls

Fixes #14046

When we're resolving visibility of block-local items...

> `self` normally refers to the containing non-block module, and `super` to its parent (etc.). However, visibilities must only refer to a module in the DefMap they're written in, so we restrict them when that happens. ([link])

 ...unless we're resolving visibility of associated items in block-local impls, because that impl is semantically "hoisted" to the nearest (non-block) module. With this PR, we skip the adjustment for such items.

Since visibility representation of those items is modified, this PR also adjusts visibility rendering in `HirDisplay`.

[link]: https://github.com/rust-lang/rust-analyzer/blob/a6603fc21d50b3386a488c96225b2d1fd492e533/crates/hir-def/src/nameres/path_resolution.rs#L101-L103
@bors
Copy link
Contributor

bors commented Feb 28, 2023

💔 Test failed - checks-actions

@lowr
Copy link
Contributor Author

lowr commented Feb 28, 2023

The tests are failing because we fail to render constant tuples (consteval seems fine, only rendering is the problem).

@HKalbasi Can we add support for tuples in hir_ty::display::render_const_scalar()? If it's hard, I can temporarily replace () with some random integers in the tests.

@lowr lowr added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 28, 2023
@HKalbasi HKalbasi mentioned this pull request Mar 1, 2023
Merged
@HKalbasi
Copy link
Member

HKalbasi commented Mar 1, 2023

@bors r=Veykril

@bors
Copy link
Contributor

bors commented Mar 1, 2023

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Contributor

bors commented Mar 1, 2023

📌 Commit d416623 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 1, 2023

⌛ Testing commit d416623 with merge 32424d0...

@bors
Copy link
Contributor

bors commented Mar 1, 2023

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

@bors bors merged commit 32424d0 into rust-lang:master Mar 1, 2023
@lnicola lnicola changed the title Fix associated item visibility in block-local impls fix: Fix associated item visibility in block-local impls Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Veykril Veykril Veykril left review comments

Assignees
No one assigned
Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

False positive private-assoc-item error
5 participants
@lowr @Veykril @bors @HKalbasi @rustbot