fix: let glob imports override other globs' visibility #17715
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
S-waiting-on-review
Throne3d
commented
Jul 27, 2024
Comment on lines
+1129
to
+1130
| if let Some((def, def_vis, _)) = defs.types { | ||
| if let Some((prev_def, prev_vis, _)) = prev_defs.types { |
There was a problem hiding this comment.
I believe the third parameter here is the location/token the import occurred at, if the type is named directly (rather than a glob or such) - it seems it wasn't present at the time the code was written, but I don't think there's a condition necessary on it here, so hopefully it's correct to leave it ignored here.
If you can think of any test cases to add to validate if there are other necessary conditions here, I'd appreciate any suggestions!
|
@bors r+ |
|
☀️ Test successful - checks-actions |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
checkfunction incrates/hir-def/src/namerestests. I think the change to the test expectation from #14930 is correct, marking thecrate::reexport::innerimports withi, and I understand it to mean there's a specific token in the import that we can match it to (in this case,Trait,functionandmakroofpub use crate::defs::{Trait, function, makro};respectively), but I had some trouble understanding the meaning of the different parts ofPerNsto be sure.Does this make sense?
I tested building and using RA locally with
cargo xtask installand after this change the documentation forarrow_array::ArrowPrimitiveTypeseems to be picked up correctly!