-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Explicitly export core and std macros #139493
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?
Explicitly export core and std macros #139493
Conversation
|
r? @ChrisDenton rustbot has assigned @ChrisDenton. Use |
|
r? @Amanieu |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@Amanieu the tidy issue highlights an annoying and unforeseen side-effect of this change. The fn xx(i: vec::IntoIter<i32>) {
let _ = i.as_slice();
}
fn main() {}that currently doesn't compile on stable would now compile. Initially I thought this would cause name collisions if users define their own |
|
There's an issue for this change - #53977. |
|
@Voultapher, avoiding the vec module re-export can be done like this: #[macro_export]
macro_rules! myvec {
() => {};
}
pub mod myvec {
pub struct Vec;
}
pub mod prelude {
// Bad: re-exports both macro and type namespace
// pub use crate::myvec;
mod vec_macro_only {
#[allow(hidden_glob_reexports)]
mod myvec {}
pub use crate::*;
}
pub use self::vec_macro_only::myvec;
}
fn main() {
prelude::myvec!();
let _: prelude::myvec::Vec; // error
} |
|
|
This comment has been minimized.
This comment has been minimized.
|
@Voultapher Based on the CI failure I think that a try build would fail now. |
|
Ok, I'll try to get the CI passing first. |
|
@petrochenkov I went through all macros and searched the docs and |
This comment has been minimized.
This comment has been minimized.
|
@Amanieu this program previously worked: use std::*;
fn main() {
panic!("panic works")
}and now runs into: I don't see how we can resolve that without changing language import rules and or special casing the prelude import. |
|
@petrochenkov Do you have any ideas about that? |
|
Could you add a test making sure that the modules |
The ambiguity wouldn't happen if it was the same panic in std root and in the stdlib prelude. Previously |
An incorrect merge conflict resolution had deleted a relevant new line.
While this is undesired, blocking explicit macro export and assert_matches for this bug that already exists in a smaller fashion was deemed not worth it. See rust-lang#145577 and rust-lang#139493 (comment)
This turns the ambiguous panic import error into a warning to avoid breaking large parts of the Rust crate ecosystem.
bdc4c56 to
54262e7
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Purely from the language perspective, some ambiguities between preludes that previously produced hard errors now produce lints. |
The work is already done (I don't see any significantly better fixes to the panic problem than what this PR does), it just needs to be split into two parts landed ~1 release apart, that can be reverted individually, have separate dedicated crater runs and team approvals. The prelude move and the panic hack are two different changes. |
Possible combinations of panic locations:
If I'm not mistaken, in libcore |
|
My worries are that splitting up the change won't be trivial, this change has proven to involve lots of unexpected side-effects.
Arguably it should be easier to explain this to our users as one change "the prelude macros are now explicitly exported". Saying they are exported but [...] could be more confusing. My suggestion would be to run another crater run on this. Only a couple crates should fail for shadowing imports other than |
|
@bors try |
This comment has been minimized.
This comment has been minimized.
…ros, r=<try> Explicitly export core and std macros
|
@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 |
Currently all core and std macros are automatically added to the prelude via #[macro_use]. However a situation arose where we want to add a new macro
assert_matchesbut don't want to pull it into the standard prelude for compatibility reasons. By explicitly exporting the macros found in the core and std crates we get to decide on a per macro basis and can later add them via the rust_20xx preludes.Closes #53977
Unlocks #137487