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: let glob imports override other globs' visibility #14930

Closed
wants to merge 1 commit into from
Closed

fix: let glob imports override other globs' visibility #14930

wants to merge 1 commit into from

Conversation

lowr
Copy link
Contributor

@lowr lowr commented May 30, 2023 •
edited
Loading

Fixes #11858
Fixes #14902

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

lowr commented Jun 11, 2023

While I think I did implement the right semantics, it complicates code (especially in that there are now two methods to push resolution to ItemScope) and I don't really like it implementation-wise. I'd like to reconsider the implementation a bit.

@rustbot author

@rustbot rustbot 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 Jun 11, 2023
@Veykril
Copy link
Member

Veykril commented Aug 15, 2023

Having looked myself at the import resolution code for some other stuff, I wonder if we can rewrite the entire thing in a different way (not speaking of your changes here, haven't looked at them). I don't like how it currently looks, and it also seems to place some stuff into item scope that's only relevant for the import resolution.

@bors
Copy link
Contributor

bors commented Aug 17, 2023

☔ The latest upstream changes (presumably #15472) made this pull request unmergeable. Please resolve the merge conflicts.

@Veykril
Copy link
Member

Veykril commented Dec 8, 2023

Closing due to inactivity

@Veykril Veykril closed this Dec 8, 2023
This was referenced Jul 26, 2024
Closed
Merged
bors added a commit that referenced this pull request Jul 29, 2024
@bors
Auto merge of #17715 - Throne3d:fix/glob-may-override-vis-2, r=Veykril
0cbcbb0 
fix: let glob imports override other globs' visibility

Follow up to #14930

Fixes #11858
Fixes #14902
Fixes #17704

I haven't reworked the code here at all - I don't feel confident in the codebase to do so - just rebased it onto the current main branch and fixed conflicts.

I'm not _entirely_ sure I understand the structure of the `check` function in `crates/hir-def/src/nameres` tests. I think the change to the test expectation from #14930 is correct, marking the `crate::reexport::inner` imports with `i`, and I understand it to mean there's a specific token in the import that we can match it to (in this case, `Trait`, `function` and `makro` of `pub use crate::defs::{Trait, function, makro};` respectively), but I had some trouble understanding the meaning of the different parts of `PerNs` to be sure.
Does this make sense?

I tested building and using RA locally with `cargo xtask install` and after this change the documentation for `arrow_array::ArrowPrimitiveType` seems to be picked up correctly!
@Throne3d
Copy link
Contributor

Thanks for doing this code implementation! I've just rebased it onto the latest version of rust-analyzer, and it solved an issue I was having with arrow_array. Looks like this issue will be fixed in an upcoming version of RA. =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
5 participants
@lowr @Veykril @bors @Throne3d @rustbot