Fix invalid special casing of the unreachable! macro

[Urgau]

This pull-request fix an invalid special casing of the unreachable! macro in the same way the panic! macro was solved, by adding two new internal only macros unreachable_2015 and unreachable_2021 edition dependent and turn unreachable! into a built-in macro that do dispatching. This logic is stolen from the panic! macro.

This pull-request also adds an internal feature format_args_capture_non_literal that allows capturing arguments from formatted string that expanded from macros. The original RFC #2795 mentioned this as a future possibility. This feature is required because of concatenation that needs to be done inside the macro:

$crate::concat!("internal error: entered unreachable code: ", $fmt)

In summary the new behavior for the unreachable! macro with this pr is:

Edition 2021:

let x = 5;
unreachable!("x is {x}");

internal error: entered unreachable code: x is 5

Edition <= 2018:

let x = 5;
unreachable!("x is {x}");

internal error: entered unreachable code: x is {x}

Also note that the change in this PR are insta-stable and breaking changes but this a considered as being a bug.
If someone could start a perf run and then a crater run this would be appreciated.

Fixes #92137

[rust-highfive]

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

[camelid]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 7331ddeed4310ba606bae8479850985251a35dd3 with merge 07d94fb27d02adc112536d7c6439de6a373f2e03...

[camelid]

cc discussion about implementation strategy: #92137 (comment)

[camelid]

cc @m-ou-se

[bors]

☀️ Try build successful - checks-actions
Build commit: 07d94fb27d02adc112536d7c6439de6a373f2e03 (07d94fb27d02adc112536d7c6439de6a373f2e03)

[rust-timer]

Queued 07d94fb27d02adc112536d7c6439de6a373f2e03 with parent 17d29dc, future comparison URL.

[rust-timer]

Finished benchmarking commit (07d94fb27d02adc112536d7c6439de6a373f2e03): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

[m-ou-se]

Thanks for working on this! I'll try to review this soon.

I'll have to note that for any edition-specific changes, we also need to add a migration lint to automatically upgrade code from older editions. (Just like we already have for panic!().) Unfortuantely, because Rust 2021 is already stable, this will have to happen at the same time in this case.

This pull-request also adds an internal feature format_args_capture_non_literal that allows capturing arguments from formatted string that expanded from macros. The original RFC #2795 mentioned this as a future possibility. This feature is required because of concatenation that needs to be done inside the macro:

Another option is to make that work using a nested format_args:

-        $crate::panic!($crate::concat!("internal error: entered unreachable code: ", $fmt), $($arg)*)
+        $crate::panic!("internal error: entered unreachable code: {}", $crate::format_args!($fmt, $($arg)*))

(Edit: In general I try to avoid using concat!() for format strings, because concat automatically converts non-string literals to strings. E.g. unreachable!(1, 2) happily appends "1" to the format string, and only complains because the 2 argument is unused. Screenshot.)

(Edit 2: I've started a discussion on optimizing nested format_args on Zulip.)

[m-ou-se]

we also need to add a migration lint

I'd be happy to extend the non_fmt_panics lint to also cover unreachable. It's not trivial as panic!(123) and unreachable!(123) are a bit different: the first throws a i32 as a Box<dyn Any>, while the latter just formats the i32 with "{}". But it shouldn't be too much work.

[Urgau]

I've updated the implementation to use a nested format_args! and reverted the introduction of format_args_capture_non_literal. I've also updated and improved the test using revisions as camelid suggested.

we also need to add a migration lint

I'd be happy to extend the non_fmt_panics lint to also cover unreachable. It's not trivial as panic!(123) and unreachable!(123) are a bit different: the first throws a i32 as a Box<dyn Any>, while the latter just formats the i32 with "{}". But it shouldn't be too much work.

I tried to expand the current non_fmt_panics but I'm stuck on the fact that the argument passed to panic_str is the result of $crate::concat!("internal error: entered unreachable code: ", $fmt) and I don't know how (if even possible) to get the argument passed to concat or unreachable_2015. Any suggestions ?

[camelid]

Why are you ignoring emscripten in these tests?

[Urgau]

I did the same as this test unreachable-fmt-msg.rs

[m-ou-se]

I tried to expand the current non_fmt_panics but I'm stuck on the fact that the argument passed to panic_str is the result of $crate::concat!("internal error: entered unreachable code: ", $fmt) and I don't know how (if even possible) to get the argument passed to concat or unreachable_2015. Any suggestions ?

Maybe you could make the second case of unreachable_2015 expand to a function call instead: unreachable_display(&$msg) and then define a fn unreachable_display(_: &impl Display) -> !; to do the panic. Then you can detect that function in the lint, similar to how it detects panic_any / panic_str / etc.

[Urgau]

I tried to expand the current non_fmt_panics but I'm stuck on the fact that the argument passed to panic_str is the result of $crate::concat!("internal error: entered unreachable code: ", $fmt) and I don't know how (if even possible) to get the argument passed to concat or unreachable_2015. Any suggestions ?

Maybe you could make the second case of unreachable_2015 expand to a function call instead: unreachable_display(&$msg) and then define a fn unreachable_display(_: &impl Display) -> !; to do the panic. Then you can detect that function in the lint, similar to how it detects panic_any / panic_str / etc.

Added in 8f00aed and update of non_fmt_panic in d8de238.

[m-ou-se]

Nice! Thanks for working on this!

I have a few more comments:

[Urgau]

I've address all of review comments. Let me know if there is something else that I should modify or improve.

[camelid]

@jhpratt It is weird that the changes are so large, but it's almost entirely improvements. There's only one regression, which means it's probably spurious.

[Urgau]

Based on this comment from @m-ou-se:

Can you squash the commits a bit? There's now a few commits that undo changes of previous commits in this PR, potentially making history tracking and backporting a bit complicated. Ideally this change would be backported to 1.59 (beta) too.

Shouldn't this PR be marked as beta-nominated ? or even stable-nominated ?

[jhpratt]

Beta seems reasonable. Not sure we need a point release for this, though.

[Urgau]

Beta seems reasonable. Not sure we need a point release for this, though.

Well #93394 is in the same wane as this PR and was stable-nominated and stable-accepted.

[m-ou-se]

Not sure we need a point release for this, though.

As I was recently told, stable-nominated doesn't mean it's worth a point release. It means that if a new stable point release happens, this should be included.

I'm not sure if even in that case it should be backported, considering this change is a bit more complex than #93394, but it's a discussion that can be had. (But we're getting close to the next release anyway, so a point release seems unlikely at this point.)

[Mark-Simulacrum]

Marking as perf-regression-triaged -- only regression appears to be due to codegen unit reshuffling, not directly related to this PR.

[alice-i-cecile]

This broke bevy_ecs on nightly. Here was our fix: bevyengine/bevy#3889

[Urgau]

@alice-i-cecile This is expected, see #92137 (comment) for more context about this change and why the breaking change is considered acceptable. Also note that this change will probably be at least back-ported to beta.

[jhpratt]

I think I've said this before, but a crater run would be nice to have here. Limited to 2021 edition if it's possible (I don't think it is).

[Urgau]

I think I've said this before, but a crater run would be nice to have here. Limited to 2021 edition if it's possible (I don't think it is).

It will be done as part of the crater run done for every new stable (it's actually done with beta but you see the point).

[jhpratt]

Yeah, I just meant to do it before so we don't have to rush a last-minute revert if it breaks too much.

[apiraino]

Beta backport accepted as per compiler team on Zulip

Stable backport declined (zulip discussion)

@rustbot label +beta-accepted -stable-nominated