Re-land early syntax feature gating (was: Some features can no longer be controlled by conditional compilation)

[RalfJung]

#66004 fixed the original issue; now this tracks re-landing those checks (possibly after some transition period).

Original issue

At https://github.com/RalfJung/miri-test-libstd/, I have set things up such that one can run the libcore and liballoc test suite against Miri. The basic idea is to have a Cargo.toml file like this (very close to the normal one for libcore):

[package]
authors = ["The Rust Project Developers"]
name = "core_miri_test"
version = "0.0.0"
autotests = false
autobenches = false
edition = "2018"

[lib]
name = "core_miri_test"
path = "../libcore/lib.rs"
test = false
bench = false

[[test]]
name = "coretests"
path = "../libcore/tests/lib.rs"

[dev-dependencies]
rand = "0.7"

Then I make ../libcore a symlink to the actual libcore sources, cd into the dir with the above Cargo.toml, and run cargo miri test. This worked fine until recently (until nightly-2019-10-17, to be precise).

But since then something broke, and now I get lots of build failures:

error[E0658]: associated type bounds are unstable
   --> core_miri_test/../libcore/pin.rs:445:15
    |
445 | impl<P: Deref<Target: Unpin>> Pin<P> {
    |               ^^^^^^^^^^^^^
    |
    = note: for more information, see https://github.com/rust-lang/rust/issues/52662
    = help: add `#![feature(associated_type_bounds)]` to the crate attributes to enable

error[E0658]: associated type bounds are unstable
   --> core_miri_test/../libcore/pin.rs:754:18
    |
754 | impl<P: DerefMut<Target: Unpin>> DerefMut for Pin<P> {
    |                  ^^^^^^^^^^^^^
    |
    = note: for more information, see https://github.com/rust-lang/rust/issues/52662
    = help: add `#![feature(associated_type_bounds)]` to the crate attributes to enable

I tried adding these feature gates to every crate root I could think of (the lib.rs of both libcore and its test crate), but the errors are sticking. Unfortunately the error doesn't tell me which file it thinks is the crate root.

Any idea what this could be caused by? Until this is fixed, we won't have Miri coverage of the libcore and liballoc test suite.

[RalfJung]

The failing command seems to be

/home/r/.cargo/bin/cargo-miri rustc --edition=2018 --crate-name core_miri_test core_miri_test/../libcore/lib.rs --error-format=json --json=diagnostic-rendered-ansi --emit=dep-info,link -C debuginfo=2 cargo-miri-marker-begin cargo-miri-marker-end --test -C metadata=c5be0bd0faaf3ad2 -C extra-filename=-c5be0bd0faaf3ad2 --out-dir /home/r/src/rust/miri-test-libstd/target/debug/deps -C incremental=/home/r/src/rust/miri-test-libstd/target/debug/incremental -L dependency=/home/r/src/rust/miri-test-libstd/target/debug/deps --extern rand=/home/r/src/rust/miri-test-libstd/target/debug/deps/librand-bd1cec56ecfccb49.rlib

So this is where it builds libcore itself, not the test crate.

[RalfJung]

I can get the same errors without Miri being involved by running rustc +nightly lib.rs --test --edition 2018 in src/libcore.

Cc @alexcrichton and @eddyb who helped me to set this up at the all-hands early this year

[eddyb]

cc @Centril maybe there is something weird going on with the feature-gating here.

[Centril]

The only relevant information I can contribute here is that feature gating for this is done pre-expansion today.

[RalfJung]

libcore has a #![cfg(not(test))], so the crate really should be empty. I guess "pre-expansion" means stuff happens before the cfg is applied?

But then, why does it see the uses of this feature, but does not see the feature flag?

[RalfJung]

@Centril here's a minimal example:

#![cfg(foobar)]

#![feature(decl_macro)]
pub macro Default($item:item) { }

This gets rejected, which IMO is a bug.

[eddyb]

cc @petrochenkov

[RalfJung]

Here's another variant:

#![cfg_attr(nightly, feature(decl_macro))]

#[cfg(nightly)]
pub macro Default($item:item) { }

This shows that only conditionally using this feature is broken right now.

[RalfJung]

Regression likely introduced by #65742.

[RalfJung]

Nominating for compiler team discussion. IMO this regression should be fixed or the PR reverted.

[Centril]

That's not a bug.

Pre-expansion gating happens by collecting a set of Spans before parsing. cfg-stripping happens after parsing and does not remove those spans, which then later all get feature gate errors if the feature gate is not enabled (and if the feature gate was stripped away during cfg-stripping then it isn't). The point of pre-expansion gating is that we should be able to remove syntax from the parser and the AST.

If you want to use unstable syntax behind cfg(flag) you'll need to put the unstable syntax behind a module and #[cfg(flag)] mod the_module_with_unstable_syntax;.

[eddyb]

The original example @RalfJung had involved #![cfg(not(test))] at the crate level, and somehow that didn't prevent non-inline modules from being loaded. I wonder if that's a deficiency of cfg-stripping, or maybe it's just inside-out?

So this would always parse bar.rs and only afterwards remove all of foo:

#[cfg(whatever)]
mod foo {
    mod bar;
}

@RalfJung This would mean you need #[cfg(not(test))] on every mod xyz; in libcore for which you get an error in the corresponding xyz.rs file.

[Centril]

@eddyb I'm being literal here; you need #[cfg(...)] directly on the mod xyz;.

[RalfJung]

@Centril I think it is a bug when cfg does not behave as expected. I understand it's not a bug in the code but a conceptual bug and you consider this intended behavior -- but your justification for why this is intended is the one of an implementer, ignoring the user perspective. Is there a reasonable way (not referring to compiler implementation details) to describe the end-to-end behavior that we are seeing now?

Taking the perspective of an end-user, I think this is quite clearly a bug: cfg removes the feature flag but fails to remove the use of the unstable feature. Behavior of cfg should be consistent, but it is not. Either you should honor cfg for both feature flag detection and stability checking, or you should ignore cfg for both feature flag detection and stability detection; the current behavior is applying different rules and that makes no sense.

If you want to use unstable syntax behind cfg(flag) you'll need to put the unstable syntax behind a module and #[cfg(flag)] mod the_module_with_unstable_syntax;.

So there are some obscure rules for cases where the cfg flag is applied early enough for this to work? This is getting even weirder.^^

[eddyb]

One subtle point here is that you kind of have to imagine mod xyz; expanding to mod xyz { include!("xyz.rs"); } and if it's cfg-stripped the expansion will never happen, resulting in xyz.rs never being read or parsed.

I guess that "expansion" could be on the fly, with cfg-stripping happening very early, but regardless, cfg-stripping appears to apply between parsing the semicolon form and reading the associated file.

[Centril]

@RalfJung Sometimes there are architectural reasons for why things are the way they are that may seem weird to an end user. I'm sure you can appreciate that wrt. other parts of the compiler. :)

Is there a reasonable way (not referring to compiler implementation details) to describe the end-to-end behavior that we are seeing now?

Here's the best I can do for now: When #[cfg(stuff)] is written on a non-inline mod foo; and stuff is determined not to be in the configuration, then the stuff in mod foo; is not parsed. In all other circumstances, mod foo; will parse foo within the parser. Feature gating of syntax (for a certain set of syntactic feature gates) happens in the parser. Cfg-stripping happens, except in the case of #[cfg(stuff)] mod foo; after parsing.

So there are some obscure rules for cases where the cfg flag is applied early enough for this to work? This is getting even weirder.^^

Yes, it's weird, but the conceptual bug is not in #65742 which merely extended the current pre-expansion infra to a new set of feature gates (as it should!) and you were unlucky to hit a specific feature gate.

I guess that "expansion" could be on the fly, with cfg-stripping happening very early, but regardless, cfg-stripping appears to apply between parsing the semicolon form and reading the associated file.

@eddyb Yes, that's right; the weird behavior comes from

rust/src/libsyntax/parse/parser/module.rs

Lines 40 to 90 in 0b7e28a

	pub(super) fn parse_item_mod(&mut self, outer_attrs: &[Attribute]) -> PResult<'a, ItemInfo> {
	let (in_cfg, outer_attrs) = {
	// FIXME(Centril): This results in a cycle between config and parsing.
	// Consider using dynamic dispatch via `self.sess` to disentangle the knot.
	let mut strip_unconfigured = crate::config::StripUnconfigured {
	sess: self.sess,
	features: None, // Don't perform gated feature checking.
	};
	let mut outer_attrs = outer_attrs.to_owned();
	strip_unconfigured.process_cfg_attrs(&mut outer_attrs);
	(!self.cfg_mods || strip_unconfigured.in_cfg(&outer_attrs), outer_attrs)
	};
	let id_span = self.token.span;
	let id = self.parse_ident()?;
	if self.eat(&token::Semi) {
	if in_cfg && self.recurse_into_file_modules {
	// This mod is in an external file. Let's go get it!
	let ModulePathSuccess { path, directory_ownership, warn } =
	self.submod_path(id, &outer_attrs, id_span)?;
	let (module, mut attrs) =
	self.eval_src_mod(path, directory_ownership, id.to_string(), id_span)?;
	// Record that we fetched the mod from an external file.
	if warn {
	let attr = attr::mk_attr_outer(
	attr::mk_word_item(Ident::with_dummy_span(sym::warn_directory_ownership)));
	attr::mark_known(&attr);
	attrs.push(attr);
	}
	Ok((id, ItemKind::Mod(module), Some(attrs)))
	} else {
	let placeholder = ast::Mod {
	inner: DUMMY_SP,
	items: Vec::new(),
	inline: false
	};
	Ok((id, ItemKind::Mod(placeholder), None))
	}
	} else {
	let old_directory = self.directory.clone();
	self.push_directory(id, &outer_attrs);
	self.expect(&token::OpenDelim(token::Brace))?;
	let mod_inner_lo = self.token.span;
	let attrs = self.parse_inner_attributes()?;
	let module = self.parse_mod_items(&token::CloseDelim(token::Brace), mod_inner_lo)?;
	self.directory = old_directory;
	Ok((id, ItemKind::Mod(module), Some(attrs)))
	}
	}

.

The proper fix is to address #64197 but that will require more work.

[Mark-Simulacrum]

I will raise my voice here again (similarly to #65846, which I'll merge into this shortly, it has some good discussion IMO) in my belief that we're being too aggressive here; we're breaking people in practice and without any warning. We should work to phase this in, not just break folks without warning. In practice the breakage this introduces does not seem entirely worth it -- indeed, I'm not sure that it's correct to do so in general.

I would expect us to gate against e.g. macros matching against such input, but I continue to be skeptical about failing to compile something like if --cfg foo is not passed. I get that box syntax is unstable, but I feel like #[cfg(foo)] is specifically intended for this sort of "this code does not exist" type gating. We might want to be careful and decide on a case-by-case basis, e.g., maybe box syntax we're super uncertain will ever be stabilized, so we gate against it. But if we think type ascription is generally plausible to stabilize, then maybe we don't syntax-level gate it.

#[cfg(foo)]
fn foo() {
      let f: Box<u32> = box 3;
}

As it stands, if I interpret this change correctly, there's essentially no good way for a crate to have nightly-only usage of syntax-dependent features, right? Or they need to put the cfg's strictly on module-level, which is also painful and unhelpful -- it seems like this is what @RalfJung here is hitting.

We'll have a more accurate picture of who/what we've broken once this change hits beta in a week and we get a crater run going, I think, as on nightly lots of crates are plausibly just enabling the feature gate(s) so we're not seeing the true scope of regression I suspect.

[eddyb]

@Centril Ah, I see, I was initially wondering if what #64197 proposes already happened, but in this case cfg-stripping during parsing also explains the "inside-out" expansion-like behavior.

[Centril]

I will raise my voice here again (similarly to #65846, which I'll merge into this shortly, it has some good discussion IMO) in my belief that we're being too aggressive here; we're breaking people in practice and without any warning. We should work to phase this in, not just break folks without warning.

As I've noted before, we did a crater run, with sufficiently few regressions per gate to do the pre-expansion gating. It may be that the crater runs on beta will turn up more regressions, it may be that people will catch these problems in their nightly CI and fix them early. At any rate, I don't think we should revert anything until we have specific numbers from runs and when we do they may be partial reverts for specific feature gates.

Also, soft-gating pre-expansion but hard-gating post-expansion won't nice to do; it will most likely require some sort of hacks of the form "are we in expansion?" when adding spans. We then need to avoid post-expansion gating those spans in the AST visitor.

In practice the breakage this introduces does not seem entirely worth it -- indeed, I'm not sure that it's correct to do so in general.

The purpose of feature gating is to avoid leaking experimental things into the stable language. This doesn't just apply to semantics, it also applies to syntax. Like any unstable aspect of the language, we may wish to remove it, or change how the syntax looks. The idea that we would be forced to keep around garbage in the grammar due to a lack of pre-expansion gating is not acceptable to me. Therefore, moving forward, all new syntax will be pre-expansion gated unless it is for some reason exceptionally difficult to do so and the language team has approved of an exception in the specific case.

But if we think type ascription is generally plausible to stabilize, then maybe we don't syntax-level gate it.

I'm not going to speculate how likely it is that we stabilize X or Y feature. Once upon a time we presumably thought that box syntax was likely to be stabilized but then we changed our minds. Also, while I would like to see type ascription on stable, others feel differently. Instead, we ungate features when they are ready.

As it stands, if I interpret this change correctly, there's essentially no good way for a crate to have nightly-only usage of syntax-dependent features, right? Or they need to put the cfg's strictly on module-level, which is also painful and unhelpful -- it seems like this is what @RalfJung here is hitting.

There is a good way: #[cfg($flag)] mod stuff_with_unstable_syntax;. Once #64197 happens it would become easier, but ergonomic difficulties today is not a good reason to leak syntax into stable.

We'll have a more accurate picture of who/what we've broken once this change hits beta in a week and we get a crater run going, I think, as on nightly lots of crates are plausibly just enabling the feature gate(s) so we're not seeing the true scope of regression I suspect.

I think judgement should be withheld until that happens.

[RalfJung]

Sometimes there are architectural reasons for why things are the way they are that may seem weird to an end user. I'm sure you can appreciate that wrt. other parts of the compiler. :)

I can appreciate that. But that also always requires some consideration of whether the effect is really acceptable, in particular when considering regressions. This change will make some code that compiles on stable today no longer work on stable.

What is happening here is that an important property of cfg is destroyed: so far, one could reliably use cfg to hide things from the compiler. This is used by some crates to automatically enable features on nightly (and hide them from stable compilers), and by others to automatically enable the use of new features on recent compiler versions (and hide them from older compilers). That is an extremely useful property to have, and it is a property we have had since 1.0. You are removing this feature without offering a good alternative. (Having to put the "hidden" code into a separate file is not a good solution.)

My personal opinion is we should keep this property even if it makes the compiler a bit more complicated -- but more fundamentally I feel this is something that should be discussed with stakeholders first before we break the world, e.g. through an RFC.

I get that box syntax is unstable, but I feel like #[cfg(foo)] is specifically intended for this sort of "this code does not exist" type gating.

Exactly! That has been another point on the list of things that "Rust got right", but it seems now we decided we'd rather get it wrong. :/

As I've noted before, we did a crater run, with sufficiently few regressions per gate to do the pre-expansion gating. It may be that the crater runs on beta will turn up more regressions, it may be that people will catch these problems in their nightly CI and fix them early.

This is unsurprising. Crater would not have found crates that used either of the two idioms I described above: a crater run with a nightly compiler will enable the unstable features and their flags, so no trouble, and it will also be the latest compiler version so "hiding code from old compilers" does not apply either.

To properly test the impact at this, you need to at least do a crater run with a rustc that does not consider itself a nightly compiler; then you'll test the case of hiding things from stable compilers. The part about hiding things from older versions is much harder to test.

The idea that we would be forced to keep around garbage in the grammar due to a lack of pre-expansion gating is not acceptable to me. Therefore, moving forward, all new syntax will be pre-expansion gated unless it is for some reason exceptionally difficult to do so and the language team has approved of an exception in the specific case.

The idea of making it impossible to use cfg to hide code from the compiler is also unacceptable to some people.
I understand your opinion, but you finding things unacceptable is not how we decide about trade-offs in Rust. Can you point at where consensus was formed that this is indeed the right way forward for the language, given its cost?

There is a good way: #[cfg($flag)] mod stuff_with_unstable_syntax;. Once #64197 happens it would become easier, but ergonomic difficulties today is not a good reason to leak syntax into stable.

That's not a good way, it forces users to put every use of the experimental feature that they want to hide into a separate file. It is a horrible hack, not a solution.

[petrochenkov]

(Didn't read the thread.)

See #46480 for a motivating example for gating unstable syntaxes early.
Long story short: we wanted to remove entirely unstable impl Trait for .. {} but couldn't, it suddenly became a part of the stable language due to cfg(FALSE).

So all new features changing the language grammar are now gated in the parser.
Some older features were implemented without following this policy, so @Centril recently changed them to follow it and here is the fallout through which we need to work on case by case basis depending on the amount and importance of the broken code.

[Centril]

But that also always requires some consideration of whether the effect is really acceptable, in particular when considering regressions. This change will make some code that compiles on stable today no longer work on stable.

Consideration was, and is being given.

My personal opinion is we should keep this property even if it makes the compiler a bit more complicated

Why is syntax special here? One could also argue that cfg-gating alone is too unergonomic and that semantic aspects should also not be gated.

You are removing this feature without offering a good alternative.

You are suggesting that somehow pre-expansion gating started with #65742, but in fact that is not the case. You will note that it was @petrochenkov who (rightly) requested this in the first cases. Another example is in https://github.com/rust-lang/rust/pull/64588/files?file-filters%5B%5D=.rs#diff-54f2c35b9f39ddec06693253b803e65eR824.

(Having to put the "hidden" code into a separate file is not a good solution.)

That's not a good way, it forces users to put every use of the experimental feature that they want to hide into a separate file. It is a horrible hack, not a solution.

I disagree. It incentivizes segregating stable and unstable things such that it becomes clearer which is which. It may become more ergonomic in the future, but don't think that should be the first order priority.

What is happening here is that an important property of cfg is destroyed: so far, one could reliably use cfg to hide things from the compiler.

I think you can still do it "reliably" (because you'll get errors otherwise), but I think you dispute that a separate file is reliable.

[petrochenkov]

Generic workaround:

macro_rules! delay_parsing { ($($tt:tt)*) => ($($tt)*) }

#[cfg(nightly)]
delay_parsing! {
    macro foo() {}
}

If macro syntax is changed (likely) or removed (unlikely) then only the nighthly mode will be broken.

[Ixrec]

One thought I've had while thinking about this is that syntax is for the most part, not something you put behind cfg or so, as you don't want to write two code paths. However, I would be interested in seeing if we can draw upon the JS community's success with Babel and similer "transpilers" that lower modern syntax into older syntax to enable backwards-compatible experimentation in any library with e.g. try and other "pure" syntaxes, that would lower via proc macro expansion into forms that work on stable today. That's a bit of a tangent though :)

My impression is that in Rust this niche is already covered by the various nanocrates that offer macros for faking not-yet-stable features, like async-trait, because the nature of proc macros and an AOT-compiled language means those are already guaranteed to do all their work at "build time" just like Babel is for JS. Perhaps someone could write a canonical list of links to all such crates, but other than that I don't think there's anything left for a general transpilation tool to solve.

(on topic, I'm also convinced by the crater results that this change is fine as long as we do the usual work of sending PRs for the crater regressions and documenting it clearly in release notes)

[joshtriplett]

For the record, I don't think we should land this even if crater passes, and especially if crater doesn't pass (whether or not we submit patches to crates it breaks).

[pnkfelix]

removing nomination label; since the most problematic parts of this issue have been reverted (PR #66004), the T-compiler team does not need to discuss this, at least not until follow-up work has been done.

[RalfJung]

What is the status of this and what are the plans? Does the "regression" label still apply?

[nikomatsakis]

As far as I know, we never had the follow-up discussion around this. I would still like to have it, though!

I'm not totally sure on what parts were reverted and what remains.

As far as I know, the de facto standard is that one must still isolate "nightly syntax" into separate files in order to avoid feature gates. This is certainly true of new syntax that is being added.

It's certainly the easiest implementation strategy, but I would still like to talk out whether we could do something at a different granularity (e.g., ignoring the contents of function bodies that are cfg'd out), which I think would match user's expectations.

That said, I could be persuaded that the implementation effort of supporting that is not worth it at this time.

[RalfJung]

@Centril says reverting the revert hasn't happened yet.

[CAD97]

(apparently didn't record it in this thread)

The gates for unstable syntax which were reverted by #66004 were turned into future-compatibility warnings in

• Re-add most early unstable syntax gates as future-compat lints compiler-team#535
• Reenable disabled early syntax gates as future-incompatibility lints #99935

[bjorn3]

I tried to make pre_configure_attrs change krate.attrs in place, but it seems to cause the future compat error here to turn into a hard error. I'm leaving my patch here in case someone wants to make the same change after this gets turned into a hard error.

diff --git a/compiler/rustc_builtin_macros/src/standard_library_imports.rs b/compiler/rustc_builtin_macros/src/standard_library_imports.rs
index 07e6288ed8c..5db8fa34843 100644
--- a/compiler/rustc_builtin_macros/src/standard_library_imports.rs
+++ b/compiler/rustc_builtin_macros/src/standard_library_imports.rs
@@ -8,20 +8,15 @@
 use rustc_span::DUMMY_SP;
 use thin_vec::thin_vec;
 
-pub fn inject(
-    krate: &mut ast::Crate,
-    pre_configured_attrs: &[ast::Attribute],
-    resolver: &mut dyn ResolverExpand,
-    sess: &Session,
-) -> usize {
+pub fn inject(krate: &mut ast::Crate, resolver: &mut dyn ResolverExpand, sess: &Session) -> usize {
     let orig_num_items = krate.items.len();
     let edition = sess.parse_sess.edition;
 
     // the first name in this list is the crate name of the crate with the prelude
-    let names: &[Symbol] = if attr::contains_name(pre_configured_attrs, sym::no_core) {
+    let names: &[Symbol] = if attr::contains_name(&krate.attrs, sym::no_core) {
         return 0;
-    } else if attr::contains_name(pre_configured_attrs, sym::no_std) {
-        if attr::contains_name(pre_configured_attrs, sym::compiler_builtins) {
+    } else if attr::contains_name(&krate.attrs, sym::no_std) {
+        if attr::contains_name(&krate.attrs, sym::compiler_builtins) {
             &[sym::core]
         } else {
             &[sym::core, sym::compiler_builtins]
diff --git a/compiler/rustc_expand/src/config.rs b/compiler/rustc_expand/src/config.rs
index 8658cea137a..d649fb56ccf 100644
--- a/compiler/rustc_expand/src/config.rs
+++ b/compiler/rustc_expand/src/config.rs
@@ -190,18 +190,18 @@ fn active_features_up_to(edition: Edition) -> impl Iterator<Item = &'static Feat
     features
 }
 
-pub fn pre_configure_attrs(sess: &Session, attrs: &[Attribute]) -> ast::AttrVec {
+pub fn pre_configure_attrs(sess: &Session, attrs: &mut ast::AttrVec) {
     let strip_unconfigured = StripUnconfigured {
         sess,
         features: None,
         config_tokens: false,
         lint_node_id: ast::CRATE_NODE_ID,
     };
-    attrs
+    *attrs = attrs
         .iter()
         .flat_map(|attr| strip_unconfigured.process_cfg_attr(attr))
         .take_while(|attr| !is_cfg(attr) || strip_unconfigured.cfg_true(attr).0)
-        .collect()
+        .collect();
 }
 
 #[macro_export]
diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs
index def9fdcd3c7..893f95c9771 100644
--- a/compiler/rustc_interface/src/passes.rs
+++ b/compiler/rustc_interface/src/passes.rs
@@ -140,29 +140,19 @@ fn pre_expansion_lint(
 /// harness if one is to be provided, injection of a dependency on the
 /// standard library and prelude, and name resolution.
 #[instrument(level = "trace", skip(krate, resolver))]
-fn configure_and_expand(
-    mut krate: ast::Crate,
-    pre_configured_attrs: &[ast::Attribute],
-    resolver: &mut Resolver<'_, '_>,
-) -> ast::Crate {
+fn configure_and_expand(mut krate: ast::Crate, resolver: &mut Resolver<'_, '_>) -> ast::Crate {
     let tcx = resolver.tcx();
     let sess = tcx.sess;
     let lint_store = unerased_lint_store(tcx);
     let crate_name = tcx.crate_name(LOCAL_CRATE);
-    let lint_check_node = (&krate, pre_configured_attrs);
-    pre_expansion_lint(sess, lint_store, tcx.registered_tools(()), lint_check_node, crate_name);
+    pre_expansion_lint(sess, lint_store, tcx.registered_tools(()), &krate, crate_name);
     rustc_builtin_macros::register_builtin_macros(resolver);
 
     let num_standard_library_imports = sess.time("crate_injection", || {
-        rustc_builtin_macros::standard_library_imports::inject(
-            &mut krate,
-            pre_configured_attrs,
-            resolver,
-            sess,
-        )
+        rustc_builtin_macros::standard_library_imports::inject(&mut krate, resolver, sess)
     });
 
-    util::check_attr_crate_type(sess, pre_configured_attrs, &mut resolver.lint_buffer());
+    util::check_attr_crate_type(sess, &krate.attrs, &mut resolver.lint_buffer());
 
     // Expand all macros
     krate = sess.time("macro_expand_crate", || {
@@ -199,7 +189,7 @@ fn configure_and_expand(
 
         // Create the config for macro expansion
         let features = sess.features_untracked();
-        let recursion_limit = get_recursion_limit(pre_configured_attrs, sess);
+        let recursion_limit = get_recursion_limit(&krate.attrs, sess);
         let cfg = rustc_expand::expand::ExpansionConfig {
             features: Some(features),
             recursion_limit,
@@ -334,7 +324,7 @@ fn early_lint_checks(tcx: TyCtxt<'_>, (): ()) {
         tcx.registered_tools(()),
         Some(lint_buffer),
         rustc_lint::BuiltinCombinedEarlyLintPass::new(),
-        (&**krate, &*krate.attrs),
+        &**krate,
     )
 }
 
@@ -560,9 +550,9 @@ fn resolver_for_lowering<'tcx>(
 ) -> &'tcx Steal<(ty::ResolverAstLowering, Lrc<ast::Crate>)> {
     let arenas = Resolver::arenas();
     let _ = tcx.registered_tools(()); // Uses `crate_for_resolver`.
-    let (krate, pre_configured_attrs) = tcx.crate_for_resolver(()).steal();
-    let mut resolver = Resolver::new(tcx, &pre_configured_attrs, krate.spans.inner_span, &arenas);
-    let krate = configure_and_expand(krate, &pre_configured_attrs, &mut resolver);
+    let krate = tcx.crate_for_resolver(()).steal();
+    let mut resolver = Resolver::new(tcx, &krate.attrs, krate.spans.inner_span, &arenas);
+    let krate = configure_and_expand(krate, &mut resolver);
 
     // Make sure we don't mutate the cstore from here on.
     tcx.untracked().cstore.leak();
diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs
index f833a374b3d..28382b395a8 100644
--- a/compiler/rustc_interface/src/queries.rs
+++ b/compiler/rustc_interface/src/queries.rs
@@ -85,7 +85,7 @@ pub struct Queries<'tcx> {
     hir_arena: WorkerLocal<rustc_hir::Arena<'tcx>>,
 
     parse: Query<ast::Crate>,
-    pre_configure: Query<(ast::Crate, ast::AttrVec)>,
+    pre_configure: Query<ast::Crate>,
     // This just points to what's in `gcx_cell`.
     gcx: Query<&'tcx GlobalCtxt<'tcx>>,
 }
@@ -115,7 +115,7 @@ pub fn parse(&self) -> Result<QueryResult<'_, ast::Crate>> {
             .compute(|| passes::parse(self.session()).map_err(|mut parse_error| parse_error.emit()))
     }
 
-    pub fn pre_configure(&self) -> Result<QueryResult<'_, (ast::Crate, ast::AttrVec)>> {
+    pub fn pre_configure(&self) -> Result<QueryResult<'_, ast::Crate>> {
         self.pre_configure.compute(|| {
             let mut krate = self.parse()?.steal();
 
@@ -125,10 +125,9 @@ pub fn pre_configure(&self) -> Result<QueryResult<'_, (ast::Crate, ast::AttrVec)
                 &sess.parse_sess,
                 &sess.opts.unstable_opts.crate_attr,
             );
+            rustc_expand::config::pre_configure_attrs(sess, &mut krate.attrs);
 
-            let pre_configured_attrs =
-                rustc_expand::config::pre_configure_attrs(sess, &krate.attrs);
-            Ok((krate, pre_configured_attrs))
+            Ok(krate)
         })
     }
 
@@ -179,11 +178,12 @@ fn dep_graph(&self, dep_graph_future: Option<DepGraphFuture>) -> DepGraph {
     pub fn global_ctxt(&'tcx self) -> Result<QueryResult<'_, &'tcx GlobalCtxt<'tcx>>> {
         self.gcx.compute(|| {
             let sess = self.session();
-            let (krate, pre_configured_attrs) = self.pre_configure()?.steal();
+            let krate = self.pre_configure()?.steal();
 
             // parse `#[crate_name]` even if `--crate-name` was passed, to make sure it matches.
-            let crate_name = find_crate_name(self.session(), &pre_configured_attrs);
-            let crate_types = util::collect_crate_types(sess, &pre_configured_attrs);
+            let crate_name = find_crate_name(self.session(), &krate.attrs);
+            let crate_types = util::collect_crate_types(sess, &krate.attrs);
+
             let stable_crate_id = StableCrateId::new(
                 crate_name,
                 crate_types.contains(&CrateType::Executable),
@@ -199,7 +199,7 @@ pub fn global_ctxt(&'tcx self) -> Result<QueryResult<'_, &'tcx GlobalCtxt<'tcx>>
                 sess,
                 &*self.codegen_backend().metadata_loader(),
                 self.compiler.register_lints.as_deref(),
-                &pre_configured_attrs,
+                &krate.attrs,
             ));
             let cstore = RwLock::new(Box::new(CStore::new(stable_crate_id)) as _);
             let definitions = RwLock::new(Definitions::new(stable_crate_id));
@@ -209,7 +209,7 @@ pub fn global_ctxt(&'tcx self) -> Result<QueryResult<'_, &'tcx GlobalCtxt<'tcx>>
             let untracked = Untracked { cstore, source_span, definitions };
 
             // FIXME: Move features from session to tcx and make them immutable.
-            sess.init_features(rustc_expand::config::features(sess, &pre_configured_attrs));
+            sess.init_features(rustc_expand::config::features(sess, &krate.attrs));
 
             let qcx = passes::create_global_ctxt(
                 self.compiler,
@@ -228,7 +228,7 @@ pub fn global_ctxt(&'tcx self) -> Result<QueryResult<'_, &'tcx GlobalCtxt<'tcx>>
                 feed.crate_name(crate_name);
 
                 let feed = tcx.feed_unit_query();
-                feed.crate_for_resolver(tcx.arena.alloc(Steal::new((krate, pre_configured_attrs))));
+                feed.crate_for_resolver(tcx.arena.alloc(Steal::new(krate)));
                 feed.metadata_loader(
                     tcx.arena.alloc(Steal::new(self.codegen_backend().metadata_loader())),
                 );
diff --git a/compiler/rustc_lint/src/early.rs b/compiler/rustc_lint/src/early.rs
index 9f1f5a26ee5..fef93d44f2d 100644
--- a/compiler/rustc_lint/src/early.rs
+++ b/compiler/rustc_lint/src/early.rs
@@ -340,7 +340,7 @@ fn check<'b, T: EarlyLintPass>(self, cx: &mut EarlyContextAndPass<'b, T>)
         'a: 'b;
 }
 
-impl<'a> EarlyCheckNode<'a> for (&'a ast::Crate, &'a [ast::Attribute]) {
+impl<'a> EarlyCheckNode<'a> for &'a ast::Crate {
     fn id(self) -> ast::NodeId {
         ast::CRATE_NODE_ID
     }
@@ -348,15 +348,15 @@ fn attrs<'b>(self) -> &'b [ast::Attribute]
     where
         'a: 'b,
     {
-        &self.1
+        &self.attrs
     }
     fn check<'b, T: EarlyLintPass>(self, cx: &mut EarlyContextAndPass<'b, T>)
     where
         'a: 'b,
     {
-        lint_callback!(cx, check_crate, self.0);
-        ast_visit::walk_crate(cx, self.0);
-        lint_callback!(cx, check_crate_post, self.0);
+        lint_callback!(cx, check_crate, self);
+        ast_visit::walk_crate(cx, self);
+        lint_callback!(cx, check_crate_post, self);
     }
 }
 
diff --git a/compiler/rustc_middle/src/arena.rs b/compiler/rustc_middle/src/arena.rs
index 5a320865c95..620fe1fe0b4 100644
--- a/compiler/rustc_middle/src/arena.rs
+++ b/compiler/rustc_middle/src/arena.rs
@@ -41,7 +41,7 @@ macro_rules! arena_types {
             )>,
             [] output_filenames: std::sync::Arc<rustc_session::config::OutputFilenames>,
             [] metadata_loader: rustc_data_structures::steal::Steal<Box<rustc_session::cstore::MetadataLoaderDyn>>,
-            [] crate_for_resolver: rustc_data_structures::steal::Steal<(rustc_ast::Crate, rustc_ast::AttrVec)>,
+            [] crate_for_resolver: rustc_data_structures::steal::Steal<rustc_ast::Crate>,
             [] resolutions: rustc_middle::ty::ResolverGlobalCtxt,
             [decode] unsafety_check_result: rustc_middle::mir::UnsafetyCheckResult,
             [decode] code_region: rustc_middle::mir::coverage::CodeRegion,
diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs
index 52a18c99edb..3dac565275c 100644
--- a/compiler/rustc_middle/src/query/mod.rs
+++ b/compiler/rustc_middle/src/query/mod.rs
@@ -2102,7 +2102,7 @@
         desc { "raw operations for metadata file access" }
     }
 
-    query crate_for_resolver((): ()) -> &'tcx Steal<(rustc_ast::Crate, rustc_ast::AttrVec)> {
+    query crate_for_resolver((): ()) -> &'tcx Steal<rustc_ast::Crate> {
         feedable
         no_hash
         desc { "the ast before macro expansion and name resolution" }
diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs
index 614a29e7578..e0cb76117c3 100644
--- a/compiler/rustc_resolve/src/macros.rs
+++ b/compiler/rustc_resolve/src/macros.rs
@@ -118,8 +118,8 @@ fn fast_print_path(path: &ast::Path) -> Symbol {
 
 pub(crate) fn registered_tools(tcx: TyCtxt<'_>, (): ()) -> RegisteredTools {
     let mut registered_tools = RegisteredTools::default();
-    let (_, pre_configured_attrs) = &*tcx.crate_for_resolver(()).borrow();
-    for attr in attr::filter_by_name(pre_configured_attrs, sym::register_tool) {
+    let krate = &*tcx.crate_for_resolver(()).borrow();
+    for attr in attr::filter_by_name(&krate.attrs, sym::register_tool) {
         for nested_meta in attr.meta_item_list().unwrap_or_default() {
             match nested_meta.ident() {
                 Some(ident) => {
diff --git a/tests/ui/cfg/future-compat-crate-attributes-using-cfg_attr.rs b/tests/ui/cfg/future-compat-crate-attributes-using-cfg_attr.rs
index 1f23dadc432..b23e0e932ef 100644
--- a/tests/ui/cfg/future-compat-crate-attributes-using-cfg_attr.rs
+++ b/tests/ui/cfg/future-compat-crate-attributes-using-cfg_attr.rs
@@ -1,15 +1,11 @@
 // check-fail
 // compile-flags:--cfg foo
 
-#![cfg_attr(foo, crate_type="bin")]
+#![cfg_attr(foo, crate_type = "bin")]
 //~^ERROR `crate_type` within
 //~| WARN this was previously accepted
-//~|ERROR `crate_type` within
-//~| WARN this was previously accepted
-#![cfg_attr(foo, crate_name="bar")]
+#![cfg_attr(foo, crate_name = "bar")]
 //~^ERROR `crate_name` within
 //~| WARN this was previously accepted
-//~|ERROR `crate_name` within
-//~| WARN this was previously accepted
 
 fn main() {}
diff --git a/tests/ui/cfg/future-compat-crate-attributes-using-cfg_attr.stderr b/tests/ui/cfg/future-compat-crate-attributes-using-cfg_attr.stderr
index 9ce4710d69b..3d0501ae942 100644
--- a/tests/ui/cfg/future-compat-crate-attributes-using-cfg_attr.stderr
+++ b/tests/ui/cfg/future-compat-crate-attributes-using-cfg_attr.stderr
@@ -1,39 +1,21 @@
 error: `crate_type` within an `#![cfg_attr] attribute is deprecated`
   --> $DIR/future-compat-crate-attributes-using-cfg_attr.rs:4:18
    |
-LL | #![cfg_attr(foo, crate_type="bin")]
-   |                  ^^^^^^^^^^^^^^^^
+LL | #![cfg_attr(foo, crate_type = "bin")]
+   |                  ^^^^^^^^^^^^^^^^^^
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #91632 <https://github.com/rust-lang/rust/issues/91632>
    = note: `#[deny(deprecated_cfg_attr_crate_type_name)]` on by default
 
 error: `crate_name` within an `#![cfg_attr] attribute is deprecated`
-  --> $DIR/future-compat-crate-attributes-using-cfg_attr.rs:9:18
+  --> $DIR/future-compat-crate-attributes-using-cfg_attr.rs:7:18
    |
-LL | #![cfg_attr(foo, crate_name="bar")]
-   |                  ^^^^^^^^^^^^^^^^
+LL | #![cfg_attr(foo, crate_name = "bar")]
+   |                  ^^^^^^^^^^^^^^^^^^
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #91632 <https://github.com/rust-lang/rust/issues/91632>
 
-error: `crate_type` within an `#![cfg_attr] attribute is deprecated`
-  --> $DIR/future-compat-crate-attributes-using-cfg_attr.rs:4:18
-   |
-LL | #![cfg_attr(foo, crate_type="bin")]
-   |                  ^^^^^^^^^^^^^^^^
-   |
-   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
-   = note: for more information, see issue #91632 <https://github.com/rust-lang/rust/issues/91632>
-
-error: `crate_name` within an `#![cfg_attr] attribute is deprecated`
-  --> $DIR/future-compat-crate-attributes-using-cfg_attr.rs:9:18
-   |
-LL | #![cfg_attr(foo, crate_name="bar")]
-   |                  ^^^^^^^^^^^^^^^^
-   |
-   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
-   = note: for more information, see issue #91632 <https://github.com/rust-lang/rust/issues/91632>
-
-error: aborting due to 4 previous errors
+error: aborting due to 2 previous errors