You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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. :(
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:
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.
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 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.
I'm not sure I follow - can't we remove the concat special case and forward directly to panic, like todo does?
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); }
@m-ou-se@Urgau Will this be in 1.59.0? I don't see it in the release notes, but I do see the fix in the beta git branch. If it's going to be in 1.59.0, could the release notes be updated to mention it?
Yes, as I said it's in the beta branch but not mentioned in the release notes for the upcoming 1.59.0. So if it's confirmed that it will be in 1.59.0, then it would be great if the release notes could be updated to mention it.
@jqnatividad Already reported here with a response here, but basically fixing this rendering issue would required a complete rebuild of all the artifacts of 1.59.0 which don't seems worth while.
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.
And
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:
Activity
[-]unimplemented!("{}") works on Rust 2021[/-][+]unreachable!("{}") works on Rust 2021[/+]m-ou-se commentedon Dec 20, 2021
todo!()andunimplemented!()are implemented like this:rust/library/core/src/macros/mod.rs
Lines 738 to 741 in 23f6923
They never accepted
todo!("{}")ortodo!(123)since their arguments are always directly passed toformat_args!(). So, they didn't have to change for Rust 2021.unreachable!()however, is implemented differently:rust/library/core/src/macros/mod.rs
Lines 592 to 602 in 23f6923
It doesn't just forward to
panic!()orformat_args!(), but instead it does its ownconcatshenanigans, and has special handling of the single argument case.m-ou-se commentedon Dec 20, 2021
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.
m-ou-se commentedon Dec 20, 2021
Behavior:
m-ou-se commentedon Dec 20, 2021
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.jyn514 commentedon Dec 20, 2021
I'm not sure I follow - can't we remove the concat special case and forward directly to panic, like todo does?
m-ou-se commentedon Dec 20, 2021
@jyn514 Then
unreachable!(123)would stop working on Rust 2015/2018.m-ou-se commentedon Dec 20, 2021
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:
joshtriplett commentedon Dec 20, 2021
Making
unreachablebuilt-in to fix this on Rust 2021 seems reasonable to me.jhpratt commentedon Dec 21, 2021
IMO as long as there's a crater run fixing this is reasonable. It's certainly a bug.
48 remaining items
Squashed commit of the following:
fix unreachable macro calls for rust 2021 (#3889)
Arnavion commentedon Feb 22, 2022
@m-ou-se @Urgau Will this be in 1.59.0? I don't see it in the release notes, but I do see the fix in the
betagit branch. If it's going to be in 1.59.0, could the release notes be updated to mention it?jhpratt commentedon Feb 22, 2022
This is in fact on beta: playground
Arnavion commentedon Feb 22, 2022
Yes, as I said it's in the beta branch but not mentioned in the release notes for the upcoming 1.59.0. So if it's confirmed that it will be in 1.59.0, then it would be great if the release notes could be updated to mention it.
Urgau commentedon Feb 22, 2022
Yes it will be 1.59.0. cc @Mark-Simulacrum about the change not being in the release notes.
Mark-Simulacrum commentedon Feb 22, 2022
#94265 includes a note about this into the 1.59 release notes.
jqnatividad commentedon Feb 24, 2022
Folks, reading the 1.59 release notes, it's odd that the markdown link to this issue is not rendering.
https://github.com/rust-lang/rust/blob/stable/RELEASES.md#compatibility-notes
Urgau commentedon Feb 24, 2022
@jqnatividad Already reported here with a response here, but basically fixing this rendering issue would required a complete rebuild of all the artifacts of 1.59.0 which don't seems worth while.
jyn514 commentedon Mar 3, 2022
Why was there no future-incompat warning for this? The first time I saw it was when a stable compiler upgrade broke my code.
Urgau commentedon Mar 3, 2022
See #92137 (comment), in particular:
And
fix unreachable macro calls for rust 2021 (bevyengine#3889)
Auto merge of #93179 - Urgau:unreachable-2021, r=m-ou-se,oli-obk