-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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: main
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 |
Retested. Yes, this is the behavior I see in testing. For this code: #![no_implicit_prelude]
fn f() {
panic!();
//[nightly]~^ error: cannot find macro `panic` in this scope
//[this-pr]~^ OK
}Run with: rustc +stage1 --edition=2024 --crate-type=rlib src/lib.rsIt compiles without error on my build of the branch of this PR. On nightly, it produces this error: |
ed65c75 to
af5665b
Compare
|
This PR was rebased onto a different main 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. |
This comment has been minimized.
This comment has been minimized.
|
I pushed up a test demonstrating the behavior. |
f3cd775 to
82371a1
Compare
|
Thanks for the test. Strange ... very strange I'll look into it. |
|
@Voultapher this behavior comes from this logic so it's working as expected: https://github.com/rust-lang/rust/blob/main/compiler/rustc_resolve/src/ident.rs#L160-L166 This is just a consequence of relocating these macros, not sure there's much that needs investigating here. |
|
I wish |
|
FWIW, as a random user of ¹ for testing that macros don't accidentally use any unqualified symbols |
|
So I finally got around looking at this again. I can confirm what was previously discoverd with one major caveat:
This is not true in practice. It only applies to panic. Examples: // Rust 2018 + nightly
#![no_implicit_prelude]
fn main() {
let _ = vec![3, 6];
// ^ error: cannot find macro `vec` in this scope
}// Rust 2018 + PR
#![no_implicit_prelude]
fn main() {
let _ = vec![3, 6];
// ^ error: cannot find macro `vec` in this scope
}// Rust 2015 + nightly
#![no_implicit_prelude]
fn main() {
let _ = vec![3, 6]; // NO error
}// Rust 2015 + PR
#![no_implicit_prelude]
fn main() {
let _ = vec![3, 6];
// ^ error: cannot find macro `vec` in this scope
}Error matrix, ✅ means compiler passes 🚫 means compiler error:
|
82371a1 to
d2f5ccd
Compare
|
I've updated this PR with additional tests and improvements to the hack by @yaahc. |
|
I've updated the Stabilization report to include all the new information. For reference: mod m {
pub use core::env as panic;
}
use m::*;
fn main() {
panic!("PATH");
//[nightly]~^ error: `panic` is ambiguous
//[this-pr]~^ error: `panic` is ambiguous
}Now continues producing an error. |
|
@traviscross it is my understanding that all open points have been investigated and either improved or documented in the stabilization report. Please discuss this change in an upcoming lang meeting. If you think it's not ready for that yet please provide actionable feedback on what specifically is missing. |
It'd be better, I think, all else being equal, to not do this. Is it feasible to adjust the behavior such that we do not begin to make |
|
Doing a more exhaustive check the picture is even more complex:
|
After looking at it for a couple hours, my attempts to limit the behavior resulted in new issues downstream. The information used to make the relevant decisions by the resolver is tied to multiple other uses, we can't simply strip At this point the lang team to has decide if they want to waste the work that has gone into this feature because |
|
Thanks for your work on this. @petrochenkov, any thoughts there? |
|
About |
|
I see this PR currently has a S-waiting-on-author label, what's missing? |
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
Reference PR:
Stabilization report lib
Everything N/A or already covered by lang report except, breaking changes: The unstable and never intended for public use
format_args_nlmacro is no longer publicly accessible as requested by @petrochenkov. Affects <10 crates including dependencies.Stabilization report lang
Summary
Explicitly export core and std macros.
This change if merged would change the code injected into user crates to no longer include #[macro_use] on extern crate core and extern crate std. This change is motivated by a near term goal and a longer term goal. The near term goal is to allow a macro to be defined at the std or core crate root but not have it be part of the implicit prelude. Such macros can then be separately promoted to the prelude in a new edition. Specifically this is blocking the stabilization of assert_matches #137487. The longer term goal is to gradually deprecate #[macro_use]. By no longer requiring it for standard library usage, this serves as a step towards that goal. For more information see #53977.
PR link: #139493
Tracking:
ambiguous_panic_imports#147319Reference PRs:
cc @rust-lang/lang @rust-lang/lang-advisors
What is stabilized
Stabilization:
#[macro_use]is no longer automatically included in the crate root module. This allows the explicit import of macros in thecoreandstdprelude e.g.pub use crate::dbg;.ambiguous_panic_importslint. Code that previously passed without warnings, but included the following or equivalent - only pertaining to core vs std panic - will now receive a warning:This lint is tied to a new exception to the name resolution logic in compiler/rustc_resolve/src/ident.rs similar to an exception added for Glob import causes ambiguity on nightly #145575. Specifically this only happens if the import of two builtin macros is ambiguous and they are named
sym::panic. I.e. this can only happen forcore::panicandstd::panic. While there are some tiny differences in what syntax is allowed instd::panicvscore::panicin editions 2015 and 2018, see. The behavior at runtime will always be the same if it compiles, implying minimal risk in what specific macro is resolved. At worst some closed source project not captured by crater will stop compiling because a different panic is resolved than previously and they were using obscure syntax likepanic!(&String::new()).Design
N/A
Reference
RFC history
N/A
Answers to unresolved questions
N/A
Post-RFC changes
N/A
Key points
Nightly extensions
N/A
Doors closed
No known doors are closed.
Feedback
Call for testing
No.
Nightly use
N/A
Implementation
Major parts
The key change is compiler/rustc_builtin_macros/src/standard_library_imports.rs removing the macro_use inject and the
v1.rspreludes now explicitlypub useing the macros https://github.com/rust-lang/rust/pull/139493/files#diff-a6f9f476d41575b19b399c6d236197355556958218fd035549db6d584dbdea1d + https://github.com/rust-lang/rust/pull/139493/files#diff-49849ff961ebc978f98448c8990cf7aae8e94cb03db44f016011aa8400170587.Coverage
A variety of UI tests including edge cases have been added.
Outstanding bugs
An old bug is made more noticeable by this change #145577 but it was recommended to not block on it #139493 (comment).
Outstanding FIXMEs
https://github.com/rust-lang/rust/pull/139493/files#diff-c046507afdba3b0705638f53fffa156cbad72ed17aa01d96d7bd1cc10b8d9bce
Tool changes
rustfmtrust-analyzerrustdoc (both JSON and HTML)cargoclippyrustupdocs.rsNo known changes needed or expected.
Breaking changes
Breaking changes:
It's possible for user code to invoke an ambiguity by defining their own macros with standard library names and glob importing them, e.g.
use nom::*importingnom::dbg. In practice this happens rarely based on crater data. The 3 public crates where this was an issue, have been fixed. The ambiguous panic import is more common and affects a non-trivial amount of the public - and likely private - crate ecosystem. To avoid a breaking change, a new future incompatible lint was added ambiguous_panic_imports see Tracking Issue for future-incompatibility lintambiguous_panic_imports#147319. This allows current code to continue compiling, albeit with a new warning. Future editions of Rust make this an error and future versions of Rust can choose to make this error. Technically this is a breaking change, but crater gives us the confidence that the impact will be at worst a new warning for 99+% of public and private crates.Code using
#![no_implicit_prelude]and Rust edition 2015 will no longer automatically have access to the prelude macros. The following works on nightly but would stop working with this change:Inversely with this change the
panicandunreachablemacro will always be in the prelude even if#![no_implicit_prelude]is specified.Error matrix when using
#![no_implicit_prelude], ✅ means compiler passes 🚫 means compiler error:Addressing this issue is deemed expensive.
Crater found no instance of this pattern in use. Affected code can fix the issue by directly importing the macros. The new behavior matches the behavior of
#![no_implicit_prelude]in Rust editions 2018 and beyond and it's intuitive meaning.Crater report:
Crater analysis:
PRs to affected crates:
Type system, opsem
Compile-time checks
N/A
Type system rules
N/A
Sound by default?
N/A
Breaks the AM?
N/A
Common interactions
Temporaries
N/A
Drop order
N/A
Pre-expansion / post-expansion
N/A
Edition hygiene
N/A
SemVer implications
No.
Exposing other features
No.
History
assert_matchesand move it tocore::macros#137487 (comment)Acknowledgments
More or less solo developed by @Voultapher with some help from @petrochenkov.
Open items
None.