Fix ICE when applying test macro to crate root #147841

jdonszelmann · 2025-10-18T10:28:10Z

This PR does a couple of things. First of all, I found an ICE that happens when applying #![core::prelude::v1::test] to the crate root. This is caused by the test macro not expanding to an item when --test isn't applied. For the crate root, that means it deletes the crate....

The fix now first does target checking, and only if the target is valid discards the item when --test isn't applied. The discarding is, I think, important for perf.

The problem with this PR is that it means that #[test] applied to structs previously would give no errors unless --test is applied! That sounds like a bug to me, but maybe we should crater run it just in case, since technically that's a breaking change. Errors in such items wouldn't be reported previously.

Also fixed a smol diagnostics bug with #[bench]'s error messages refering to #[test] accidentally.

r? noratrieb (since I already explained you a bunch, feel free to re-assign)

Fixes #114920

rustbot · 2025-10-18T10:28:14Z

noratrieb is not on the review rotation at the moment.
They may take a while to respond.

jdonszelmann · 2025-10-18T10:29:07Z

compiler/rustc_builtin_macros/src/test.rs

+
+    if !is_bench {
+        err.with_span_suggestion(attr_sp,
            "replace with conditional compilation to make the item only exist when tests are being run",


only give the suggestion of cfg(test) when we're dealing with test attrs

jdonszelmann · 2025-10-18T10:29:39Z

tests/ui/feature-gates/gating-of-test-attrs.rs

@@ -0,0 +1,48 @@
+#![feature(test)]
+
+// test is a built-in macro, not a built-in attribute, but it kind of acts like both.


New test for test attributes. The previous tests simply asserted no errors would be given but that's not true anymore, and I think they should error!

jdonszelmann · 2025-10-18T10:30:01Z

tests/ui/feature-gates/issue-43106-gating-of-builtin-attrs.rs

-// non-crate-level #[test] attributes seem to be ignored.
-
-#[test]
-mod test { mod inner { #![test] }


these were weird here anyway, test isn't a built-in attribute but a built-in macro!

jdonszelmann · 2025-10-18T10:30:39Z

tests/ui/macros/issue-111749.rs

@@ -5,8 +5,8 @@ macro_rules! cbor_map {
 }

 fn main() {
-    cbor_map! { #[test(test)] 4};
-    //~^ ERROR removing an expression is not supported in this position
+    cbor_map! { #[test(test)] 4i32};


changed to an i32 cause signum wouldn't be valid on just 4

jdonszelmann · 2025-10-18T10:31:26Z

tests/ui/macros/issue-111749.rs

-    cbor_map! { #[test(test)] 4};
-    //~^ ERROR removing an expression is not supported in this position
+    cbor_map! { #[test(test)] 4i32};
+    //~^ ERROR the `#[test]` attribute may only be used on a non-associated function


I think this is the correct error here, not something about removing expressions. I think noting that the target is wrong is more important.

jdonszelmann · 2025-10-18T10:31:45Z

tests/ui/macros/test-on-crate-root.rs

@@ -0,0 +1,8 @@
+// ICE when applying `#![test]` to the crate root,
+// though only when specified with a full path. `#![test]` is not enough.
+#![core::prelude::v1::test]


tests the specific ICE from the PR description

jdonszelmann · 2025-10-18T10:33:12Z

tests/ui/macros/attr-empty-expr.stderr

-LL |     let _ = #[test] 0;
-   |             ^^^^^^^
-
-error: removing an expression is not supported in this position


this test was testing for "empty expressions". this is super niche, can only happen with ast-based attributes (only like 10 in the compiler) and none of the examples in this test should actually remove the expression, they should instead report that the target is wrong. I can't find any other examples of node-removing attributes so I think this test is simply superfluous now.

jdonszelmann · 2025-10-18T10:34:22Z

@bors try
@craterbot check

craterbot · 2025-10-18T10:34:24Z

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ask in t-infra on Zulip
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Fix ICE when applying test macro to crate root

matthiaskrgr · 2025-10-18T10:58:11Z

does this fix #114920 ?

jdonszelmann · 2025-10-18T11:10:10Z

yea, that's actually exactly what it fixes. there was a crashtest for it I just notice but the fix is rather trivial in the end

jdonszelmann · 2025-10-18T11:12:41Z

@bors try cancel

rust-bors · 2025-10-18T11:12:44Z

Try build cancelled. Cancelled workflows:

https://github.com/rust-lang/rust/actions/runs/18614510844

rustbot · 2025-10-18T11:14:19Z

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

jdonszelmann · 2025-10-18T11:14:25Z

@bors try

Fix ICE when applying test macro to crate root

rust-bors · 2025-10-18T13:25:55Z

☀️ Try build successful (CI)
Build commit: 73ffebf (73ffebff6f34db7eb167bf9edcf2afa19c3073e9, parent: ab1d2444533d829e2d5cff6634cd3c70de6d7103)

jdonszelmann · 2025-10-18T14:11:58Z

@craterbot check

craterbot · 2025-10-18T14:12:30Z

👌 Experiment pr-147841 created and queued.
🤖 Automatically detected try build 73ffebf
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-10-18T14:12:40Z

🚧 Experiment pr-147841 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-10-20T05:15:45Z

🎉 Experiment pr-147841 is completed!
📊 7 regressed and 3 fixed (719426 total)
📊 1761 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-147841/retry-regressed-list.txt ↩

jdonszelmann · 2025-10-20T08:57:54Z

seems to be one real failure: https://github.com/avandenbussche/nfq-rs/blob/717992f243541ef5242192f09c5eb27314a740a9/src/lib.rs#L1049, though its master already doesn't build hehe

jdonszelmann · 2025-10-20T09:00:55Z

I'd say this isn't very significant

jdonszelmann · 2025-10-20T09:02:03Z

@rustbot review (@Noratrieb?)

Noratrieb · 2025-10-28T18:26:14Z

do you think it would make sense to get quick lang team signoff here? given that it already errors in test mode I'm not sure whether we should treat it as breakage or not

jdonszelmann · 2025-10-28T21:55:32Z

sure, let's do it. cc @rust-lang/lang

Noratrieb · 2025-10-28T22:41:52Z

relevant part of the description

The fix now first does target checking, and only if the target is valid discards the item when --test isn't applied. The discarding is, I think, important for perf.

The problem with this PR is that it means that #[test] applied to structs previously would give no errors unless --test is applied! That sounds like a bug to me, but maybe we should crater run it just in case, since technically that's a breaking change. Errors in such items wouldn't be reported previously.

From the description. Should either be an easy FCP or a decision to not even FCP, given that test mode already errored.

rustbot assigned Noratrieb Oct 18, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 18, 2025

jdonszelmann commented Oct 18, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Oct 18, 2025

Auto merge of #147841 - jdonszelmann:test-macro-ice, r=<try>

013fb92

Fix ICE when applying test macro to crate root

This comment has been minimized.

Sign in to view

jdonszelmann added 2 commits October 18, 2025 13:11

ICE when applying test to crate root

810288e

only discard items with #[test] on it when target is valid

0885bd6

fixup name in diagnostics

a9e4630

jdonszelmann force-pushed the test-macro-ice branch from 3e897fe to a9e4630 Compare October 18, 2025 11:14

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Oct 18, 2025

Auto merge of #147841 - jdonszelmann:test-macro-ice, r=<try>

73ffebf

Fix ICE when applying test macro to crate root

jdonszelmann mentioned this pull request Oct 18, 2025

Fix ICE from invalid delimeter in attrs parsing #147778

Closed

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2025

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 20, 2025

traviscross added T-lang Relevant to the language team I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang labels Oct 28, 2025

		@@ -0,0 +1,48 @@
		#![feature(test)]

		// test is a built-in macro, not a built-in attribute, but it kind of acts like both.

Uh oh!

Fix ICE when applying test macro to crate root #147841

Are you sure you want to change the base?

Fix ICE when applying test macro to crate root #147841

Conversation

jdonszelmann commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 18, 2025

Uh oh!

jdonszelmann Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

jdonszelmann Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

jdonszelmann Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

jdonszelmann Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

jdonszelmann Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

jdonszelmann Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

jdonszelmann Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

jdonszelmann commented Oct 18, 2025

Uh oh!

craterbot commented Oct 18, 2025

Uh oh!

This comment has been minimized.

matthiaskrgr commented Oct 18, 2025

Uh oh!

This comment has been minimized.

jdonszelmann commented Oct 18, 2025

Uh oh!

jdonszelmann commented Oct 18, 2025

Uh oh!

rust-bors bot commented Oct 18, 2025

Uh oh!

rustbot commented Oct 18, 2025

Uh oh!

jdonszelmann commented Oct 18, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Oct 18, 2025

Uh oh!

jdonszelmann commented Oct 18, 2025

Uh oh!

craterbot commented Oct 18, 2025

Uh oh!

craterbot commented Oct 18, 2025

Uh oh!

craterbot commented Oct 20, 2025

Footnotes

Uh oh!

jdonszelmann commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdonszelmann commented Oct 20, 2025

Uh oh!

jdonszelmann commented Oct 20, 2025

Uh oh!

Noratrieb commented Oct 28, 2025

Uh oh!

jdonszelmann commented Oct 28, 2025

Uh oh!

Noratrieb commented Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jdonszelmann commented Oct 18, 2025 •

edited

Loading

jdonszelmann commented Oct 20, 2025 •

edited

Loading