unreachable!("{}") works on Rust 2021 · Issue #92137 · rust-lang/rust...
source link: https://github.com/rust-lang/rust/issues/92137
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
The behavior of unreachable does not depend the edition of the crate that invoked the macro. That's fine for todo!() and unimplemented!() due to the way they are implemented, but in #92068, it was discovered that unreachable!() behaves subtly different, due to the way it has a special one-argument case. Looks like missed this when working on the 2021 panic macros. :(
changed the title unimplemented!("{}") works on Rust 2021
unreachable!("{}") works on Rust 2021
todo!() and unimplemented!() are implemented like this:
macro_rules! todo {
() => ($crate::panic!("not yet implemented"));
($($arg:tt)+) => ($crate::panic!("not yet implemented: {}", $crate::format_args!($($arg)+)));
}
They never accepted todo!("{}") or todo!(123) since their arguments are always directly passed to format_args!(). So, they didn't have to change for Rust 2021.
unreachable!() however, is implemented differently:
macro_rules! unreachable {
() => ({
$crate::panic!("internal error: entered unreachable code")
});
($msg:expr $(,)?) => ({
$crate::unreachable!("{}", $msg)
});
($fmt:expr, $($arg:tt)*) => ({
$crate::panic!($crate::concat!("internal error: entered unreachable code: ", $fmt), $($arg)*)
});
}
It doesn't just forward to panic!() or format_args!(), but instead it does its own concat shenanigans, and has special handling of the single argument case.
So then the question is: Do we consider this a bug in Rust 2021 and fix it in Rust 1.59 for that edition? Or leave this in and only fix it in a later edition (or never)?
There's quite a few cases on crates.io of Rust 2015/2018 code depending on this behavior, but Rust 2021 is very new at this point. Adding this change/fix to Rust 2021 probably doesn't break anyone, yet.
Behavior:
unreachable!(); // panic message: "internal error: entered unreachable code"
unreachable!(123); // panic message: "internal error: entered unreachable code: 123"
unreachable!("{}"); // panic message: "internal error: entered unreachable code: {}"
unreachable!("{}", 123); // panic message: "internal error: entered unreachable code: 123"
We can't fix unreachable without making it a builtin macro, like panic!(). That's because (unlike e.g. debug_assert), this macro doesn't just forward to assert/panic, but has its own problematic implementation with a special case that shouldn't exist.
We can't fix
unreachablewithout making it a builtin macro, likepanic!(). That's because (unlike e.g.debug_assert), this macro doesn't just forward to assert/panic, but has its own problematic implementation with a special case that shouldn't exist.
I'm not sure I follow - can't we remove the concat special case and forward directly to panic, like todo does?
I'm not sure I follow - can't we remove the concat special case and forward directly to panic, like todo does?
@jyn514 Then unreachable!(123) would stop working on Rust 2015/2018.
From a quick grep through crates.io, it looks like there's about 15 crates on crates.io depending on the current behavior that would break if we made it work like todo!(). Here's a few examples from the grep results:
unreachable!(message);
unreachable!(&error)
unreachable!(format!("Cannot compute gcd of {:?} and {:?}", x, y))
Err(err) => unreachable!(err),
_ => unreachable!(format!("Unsupported socket family {}!", raw.family)),
l => unreachable!(format!("got unexpected length {0:?}", l)),
x => unreachable!(x),
Error { message } => unreachable!(message),
unreachable!(false);
unreachable!(msg)
unreachable!(&format!("n = {:?}", n)),
_ => unreachable!(ERROR_MSG),
_ => unreachable!(format!("{} is invalid month!", month)),
hash => { unreachable!(hash); }
Making unreachable built-in to fix this on Rust 2021 seems reasonable to me.
IMO as long as there's a crater run fixing this is reasonable. It's certainly a bug.
It'd probably be better to fix this sooner rather than later, as the edition is still relatively new.
Short of eager macro expansion, this would require unreachable! becoming a built-in macro, correct?
Thanks; not sure how I missed that comment. I won't have time until Tuesday at the earliest (and likely not that early), but I can give it a shot if no one else wants to before I get to it.
Just to be sure, the proposed change will also fix the fact that unreachable!("{x}") and unreachable!("{}", x) don't behave the same in 1.58+ with E2021, right? (The former prints internal error: entered unreachable code: {x} because it's not parsed as a format string with implicit args.)
@jhpratt I've give it a shot today and basically everything works except that unreachable!("{x}") doesn't because:
error: there is no argument named `x`
--> /home/archie/Projects/RustProjects/rust/src/test/ui/macros/unreachable-2021.rs:8:5
|
LL | unreachable!("x is {x}");
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: did you intend to capture a variable `x` from the surrounding scope?
= note: to avoid ambiguity, `format_args!` cannot capture variables when the format string is expanded from a macro
= note: this error originates in the macro `$crate::concat` (in Nightly builds, run with -Z macro-backtrace for more info)
This is due to the fact that the macro append to the format the string "internal error: entered unreachable code: " with:
$crate::panic!($crate::concat!("internal error: entered unreachable code: ", $fmt), $($arg)*)If anyone has a idea on how to solve it, let me know or if you want you can also continue my work.
As mentioned, making it a built-in macro seems to be the way to go. My intent was to pass as much as possible off to the panic macro handler. Good luck!
@jhpratt I tried to do it the same way as the panic macro, make it a builtin macro that redirects toward one of these two new macro unreachable_2015 and unreachable_2021.
Btw, the compilation error comes format_args! who only accept literal string for the formatting one when trying to capture from a surrounding scope. The only way I see to resolve the compilation error is to find a way to bypass either concat or the check inside format_args.
@Urgau what about doing the concatenation using internal compiler APIs? I don't know them off the top of my head, but there's probably a way to concatenate the strings from within the compiler without using concat!.
My intended solution involved manually prepending the relevant text to the format and passing the result of that to the method that expands the panic! macro. This should handle the 2015/2021 edition split automatically. You won't be able to use the concat! macro because eager expansion doesn't exist.
I managed to get it working by adding an unstable format_args_capture_non_literal feature to format_args!.
The actual the logic is the same as the previous one.
@camelid @jhpratt Do you think that this is an acceptable solution (at least for a PR) ?
A quick once-over of it shows no obvious issues. I'd personally prefer a solution like I described because I think it would limit the amount of compiler magic, but I'm not on any teams.
I'd also prefer avoiding a internal-use-only feature gate that turns off error checks.
I'd also prefer avoiding a internal-use-only feature gate that turns off error checks.
Technically it doesn't turn off but instead turn on a future possibility of RFC that defined capturing arguments but I get your concern.
Over the concern you two have expressed I will not submit a pr and also will not try to implement @jhpratt proposed solution because it involves TokenStream and I don't know enough about them.
You could still open a PR and get feedback from macro experts about how to do this without the feature gate. Your code could probably be adapted fairly easily.
Open #93179 with my solution. Let's see what the reviewer think's about it.
cc @rust-lang/libs-api @rust-lang/project-edition-2021
We have a few weeks left before the next beta branches off, so we have time for an FCP without delaying this to a later Rust version.
Summary: unreachable!() turns out to be broken in a similar way as panic and assert were in Rust 2015 and Rust 2018: unreachable!("{}") worked 'fine'. This was missed, because this isn't the case for todo!() and unimplemented!(). This was unfortunately not included in the 2021 edition panic changes, but it seems best to add it to the 2021 edition now, only a few versions late. This won't break any Rust 2015/2018 code, but this is breaking for any Rust 2021 code using unreachable!(var) or unreachable!(123) or unreachable!("{}"). All 'normal' cases of this macro are unaffected. There's probably not many problematic invocations of this macro in general, and much less in Rust 2021 code, considering how recently that edition has been available.
#93179 contains the fix and upgrades the panic migration lint to also cover this case. Every instance of the problematic case of unreachable!() in Rust 2015/2018 will get a diagnostic with a correct (machine applicable) suggestion.
I just see this as a bug that needs fixing, as the decision to make the panic macros consistent in 2021 has already been made. But since this is technically breakage, let's quickly check some boxes or raise some concerns:
@rfcbot merge
Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
Copy link
rfcbot commented 5 days ago
This is now entering its final comment period, as per the review above.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK