-
Notifications
You must be signed in to change notification settings - Fork 13.4k
made unreachable_unchecked
panic in debug mode
#100021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. Please see the contribution instructions for more information. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
i.e. if Played with it a little, looks like i'm wrong. |
I think this behavior change would need discussion and an FCP. Nominating for a meeting. |
We briefly discussed this in the libs-api meeting. We're in favor of this change, but we'd like to make sure this doesn't result in an unwinding panic, since that will most likely not be helpful, and might make things worse. |
☔ The latest upstream changes (presumably #102315) made this pull request unmergeable. Please resolve the merge conflicts. |
// SAFETY: the safety contract for `intrinsics::unreachable` must | ||
// be upheld by the caller. | ||
unsafe { intrinsics::unreachable() } | ||
if cfg!(debug_assertions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making this
if cfg!(debug_assertions) { | |
if cfg!(debug_assertions) && !cfg!(miri) { |
since the comment on line 99 implies that Miri will already give good diagnostics for hitting unreachable
.
// SAFETY: the safety contract for `intrinsics::unreachable` must | ||
// be upheld by the caller. | ||
unsafe { intrinsics::unreachable() } | ||
if cfg!(debug_assertions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use assert_unsafe_precondition
instead?
@cbrownsey any updates to this pr? |
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
Added a call to
unreachable
whenunreachable_unchecked
is compiled withdebug_assertions
enabled. I don't know if this is an issue, but this shouldn't cause any problems with const fns, as panicking in const fns was added in the release that this was made const.