-
Notifications
You must be signed in to change notification settings - Fork 13.9k
resolve: Do not consider traits from ambiguous imports to be in scope #147995
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
base: master
Are you sure you want to change the base?
Conversation
|
@bors try |
This comment has been minimized.
This comment has been minimized.
resolve: Do not consider traits from ambiguous imports to be in scope
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
| impl Trait for u8 {} | ||
| } | ||
|
|
||
| fn test1() { |
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.
Nit: Could you add #[rustfmt::skip] to this and test2, I'd be worried about the test no longer testing the right thing if someone decided to reformat these.
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.
This makes sense to me, and I agree that it needs a crater run.
| let mut collected_traits = Vec::new(); | ||
| self.for_each_child(resolver, |r, name, ns, binding| { | ||
| if ns != TypeNS { | ||
| if ns != TypeNS || binding.is_ambiguity_recursive() { |
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.
I'm not too familiar with rustc_resolve - are there be other instances of for_each_child that would benefit from a similar is_ambiguity_recursive check?
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.
Not at the moment, #57199 describes the logic behind these checks, this was the one place missing.
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.
we do have for_each_child_mut?
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.
we do have
for_each_child_mut?
Its only use in process_macro_use_imports doesn't need this check either.
One of unblocking steps for #145108.
Fixes #147992.