Allow Abort terminators in all const-contexts#77512
Conversation
These appear along the cleanup path inside functions with `#[unwind(aborts)]`. We don't const-check the cleanup path anyways, since const-eval already has "abort-on-panic" semantics and there's often drops that would otherwise be forbidden, so the check wasn't really preventing anything anyways.
rust-highfive
added
the
S-waiting-on-review
ecstatic-morse
force-pushed
the
const-checking-allow-abort
branch
2 times, most recently
from
61f1f00 to
c412c30
Compare
ecstatic-morse
force-pushed
the
const-checking-allow-abort
branch
from
c412c30 to
14c3705
Compare
| } | ||
|
|
||
| TerminatorKind::Assert { .. } | ||
| TerminatorKind::Abort |
There was a problem hiding this comment.
Maybe add a comment here saying that Abort terminators are only used for #[unwind(aborts)] and since CTFE does not unwind, they are thus unreachable?
There was a problem hiding this comment.
I could just span_bug here actually. AFAICT, there are only Aborts in "cleanup" blocks.
There was a problem hiding this comment.
That sounds like a good idea but does not replace a comment. :)
|
|
||
| #![feature(unwind_attributes, const_panic)] | ||
|
|
||
| // `#[unwind(aborts)]` is okay for a `const fn`. We don't unwind in const-eval anyways. |
There was a problem hiding this comment.
Why did you add two tests? Our test suite is already huge.^^
There was a problem hiding this comment.
One is check-pass, the other fails to compile.
There was a problem hiding this comment.
I can see that, I just do not understand their respective purpose. Specifically, "consts/unwind-abort" does not even run any CTFE, and the other test already checks that the Abort terminator is accepted.
There was a problem hiding this comment.
🤷 I'm happy with the test coverage in this PR.
There was a problem hiding this comment.
I am trying to understand what unwind-abort.rs tests that the other test does not already cover.
My concern is not too little coverage; my concern is unnecessarily growing the number of UI tests.
There was a problem hiding this comment.
I try to split up passing and failing tests when possible. One is solely a test of const-checking and involves no const-eval, the other is a sanity check to ensure that the CTFE engine doesn't do anything weird when evaluating something with #[unwind(aborts)].
There was a problem hiding this comment.
I think this unnecessarily leads to an excessive number of tests in our UI test suite that is already so big that it becomes hard to run the entire thing, but whatever.
Could you add comments to these tests explaining what they are meant to test?
Co-authored-by: Ralf Jung <post@ralfj.de>
|
@bors r+ |
|
📌 Commit 6ae1da3 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 11 pull requests Successful merges: - rust-lang#75853 (Use more intra-doc-links in `core::fmt`) - rust-lang#75928 (Remove trait_selection error message in specific case) - rust-lang#76329 (Add check for doc alias attribute at crate level) - rust-lang#77219 (core::global_allocator docs link to std::alloc::GlobalAlloc) - rust-lang#77395 (BTreeMap: admit the existence of leaf edges in comments) - rust-lang#77407 (Improve build-manifest to work with the improved promote-release) - rust-lang#77426 (Include scope id in SocketAddrV6::Display) - rust-lang#77439 (Fix missing diagnostic span for `impl Trait` with const generics, and add various tests for `min_const_generics` and `const_generics`) - rust-lang#77471 (BTreeMap: refactoring around edges, missed spots) - rust-lang#77512 (Allow `Abort` terminators in all const-contexts) - rust-lang#77514 (Replace some once(x).chain(once(y)) with [x, y] IntoIter) Failed merges: r? `@ghost`
5 participants
We never unwind during const-eval, so we basically have these semantics already. Also I just figured out that these only appear along the cleanup path, which doesn't get const-checked. In other words, this doesn't actually change behavior: the
check-passtest I added compiles just fine on nightly.r? @RalfJung
cc @rust-lang/wg-const-eval