-
Notifications
You must be signed in to change notification settings - Fork 14k
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 |
|
@bors try
|
This comment has been minimized.
This comment has been minimized.
…ros, r=<try> Explicitly export core and std macros
|
@craterbot run mode=check-only p=1 crates=https://crater-reports.s3.amazonaws.com/pr-139493-1/retry-regressed-list.txt |
|
👌 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
Footnotes
|
|
Significant regressions:
Insignificant regressions:
|
|
Looks good enough to me, if you send fixes to the crates from the "significant" category. Could you also write a more detailed proposal / change description for this change, so I could send it to language and library teams for formal approval? |
|
☔ The latest upstream changes (presumably #148208) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Fixing the relevant crates sounds easy enough. I'll see if I can make progress. |
|
@PoignardAzur I have some time Friday and had planned on tackling it then, how about we split the work? |
These uses of assert_eq will be broken by changes to how the prelude handles macros: rust-lang/rust#139493.
These uses of assert_eq will be broken by changes to how the prelude handles macros: rust-lang/rust#139493.
|
Well, too late, I've done most of it. Some notes:
Overall I don't think this PR needs to wait on any of the crates listed in the crater report except maybe threescalers. There is virtually no chance that a crate sees one of its dependencies stop working because of this PR (aside from maybe playdate dependents, but those are on nightly). |
|
|
@PoignardAzur wonderful, thanks for the help. |
|
@petrochenkov A more detailed change description as discussion basis for the lang team: This change if merged would change the code injected into user crates to no longer include #![no_std]
extern crate std;
use std::prelude::v1::*;
fn xx() {
panic!(); // resolves to core::panic
}This resolution is surprising or at least not obvious. It's possible for user code to invoke this ambiguity by defining their own macros with standard library names and glob importing them, e.g. There are some side-effect changes like no longer exporting |
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