Skip to content
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

Allow Unreachable terminators unconditionally in const-checking #71691

Merged
merged 1 commit into from
May 1, 2020

Conversation

ecstatic-morse
Copy link
Contributor

If we ever actually reach an Unreachable terminator while executing, the MIR is ill-formed or the user's program is UB due to something like unreachable_unchecked. I don't think we need to forbid these in qualify_min_const_fn.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 29, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Apr 30, 2020

The reason we forbade these is that we wanted to statically prevent these from existing, instead of catching them post-monomorphization. It's not very clear yet what our story on unsound or unconst things for const fn should be.

Although in this case, you're right, nothing problematic can come from it. Even if an unconst/unsound const fn exists, as long as the unsoundness just means evaluation will fail instead of doing surprising behaviour, we should be fine.

r? @RalfJung for confirmation of my above thoughts

@rust-highfive rust-highfive assigned RalfJung and unassigned oli-obk Apr 30, 2020
@RalfJung
Copy link
Member

The dynamic checks are entirely capable of handling Unreachable terminators.

I don't know what the concerns are for static checks, but... naively I'd think they should be fine with this as well; if the code does here that means the static checks already failed to prevent "bad" behavior elsewhere.

What use-cases does this enable?

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Apr 30, 2020

Const-checking currently sees Unreachable terminators in the MIR for the example in #66756. In the PR that fixed this, #66788, it was decided that this should remain an error when neither #![feature(const_if_match)] or #![feature(const_fn)] was enabled. When const_if_match is stabilized, it will be possible for stable users to run into the error in #66756 with no recourse.

I think there's no reason to forbid Unreachable inside a const fn, so we should resolve this separately from #49146.

@RalfJung
Copy link
Member

Makes sense, thanks @ecstatic-morse!

@bors r=oli-obk,RalfJung

@bors
Copy link
Contributor

bors commented Apr 30, 2020

📌 Commit a1aff18 has been approved by oli-obk,RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#71148 (Vec drop and truncate: drop using raw slice *mut [T])
 - rust-lang#71465 (Add a convenience method on `TyCtxt` for checking for thread locals)
 - rust-lang#71567 (Handle build completion message from Cargo)
 - rust-lang#71590 (MIR dump: print pointers consistently with Miri output)
 - rust-lang#71682 (Bump pulldown-cmark)
 - rust-lang#71688 (Allow `Downcast` projections unconditionally in const-checking)
 - rust-lang#71691 (Allow `Unreachable` terminators unconditionally in const-checking)
 - rust-lang#71719 (Update backtrace-sys)

Failed merges:

r? @ghost
@bors bors merged commit 1b62bb6 into rust-lang:master May 1, 2020
@ecstatic-morse ecstatic-morse deleted the const-unreachable branch October 6, 2020 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants