-
Notifications
You must be signed in to change notification settings - Fork 13k
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
resolve: Always update globs importing from a module when bindings in that module change #113242
Conversation
… that module change
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
@bors try |
⌛ Trying commit 4e7e62a with merge 45b14c26f76a39a9945ef8518d3276f1bc880008... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@bvanjoi Now I'm sufficiently sure that the new logic is correct (because it's very simple - if any bindings in a module are updated, then we update globs as well), and the main remaining problem is avoiding breaking code when possible. We should probably try landing this subset first, as a more impactful fix, and leave the question of missing glob-vs-glob ambiguities (#113240) for later. |
☀️ Try build successful - checks-actions |
👌 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 |
🎉 Experiment
|
So, proper updates and not new glob-vs-glob ambiguities cause most of the breakage. |
@@ -9,7 +8,7 @@ use zeroable::*; | |||
|
|||
mod pod { | |||
use super::*; | |||
pub trait Pod: Zeroable {} | |||
pub trait Pod: Zeroable {} //~ ERROR expected trait, found derive macro `Zeroable` |
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.
It seems like a correct fix. Do we need to add a lint for this?
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 need to fix #113099 (comment) first, then run crater again, then think about the lint.
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 fix is not correct because we need to locate (Mod(root::pod), Key(Zeroable, TypeNS))
instead of (Mod(root::pod), Key(Zeroable, Macro))
. Therefore, this case should pass the check.
reduced: mod a {
pub trait P {}
}
pub use a::*;
mod c {
use crate::*; // `use super::*` in `issue-56593-2.rs`
pub struct S(Vec<P>);
}
#[derive(Clone)]
pub enum P {
A
}
fn main() {} similar with |
Second independent subset of #112743 for a mini crater run.
cc @bvanjoi
TODO:
@craterbot check p=1 crates=https://crater-reports.s3.amazonaws.com/pr-112743/retry-regressed-list.txt