-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Handle conflicting import of items declared in the same module #19522
Conversation
Could you add a test for this? |
// check_for_conflicts_between_imports_and_items call below handle | ||
// the conflict | ||
match (module_.def_id.get(), containing_module.def_id.get()) { | ||
(Some(id1), Some(id2)) if id1 == id2 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this missing some cases - sorry to be vague, but it seems like you are only catching reflexive loops, but any loop could cause this issue (is that right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nick29581 : I assume you are talking about cases like this (the below one is a simple loop, while we can have arbitrary length cycles)
mod A {
pub use B::submodule; // error: unresolved import B::submodule;
}
mod B {
pub use A::submodule; // error: unresolved import A::submodule;
}
fn main() {
}
In such cases, the fixed point algorithm will terminate because no progress will be made in resolving any of the import directive, and an error will be printed for each import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nick29581 : I think now I understand your concern. The existing logic takes care of case like this.
mod A {
use self::B::C::B; // error: import `B` conflicts with existing submodule
mod B { // note: note conflicting module here
mod C {
mod B {}
}
}
}
fn main() {
}
@cmr: I mainly submitted this PR for feedback on the approach I have taken. This PR fixes the issue for the Single Import case, while Globs still give the same "unresolved import" error, which I haven't yet looked into. Do you think my current approach is fine (i.e globs will be handled within resolve_glob_import) or should I think about doing the check at a higher level (perhaps here). FWIW, although I can't confirm it yet, I believe even if we moved the check to a higher level, we need to perform the same kinds of checks performed in the individual functions if the error message needs to be something like "type/value/module x conflicts y". However if the error message is generic like "can't import from the same module", without listing specific conflicts, we can just do the check and bailout at the higher level. EDIT: cc'ing @nick29581 |
643898b
to
4b75a5d
Compare
This doesn't add a test for the main problem in rust-lang#8640 since it seems that was already fixed (including a test) in PR rust-lang#19522. This just adds a test for a program mentioned in the comments that used to erroneously compile. Closes rust-lang#8640.
Fixes #19498