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 Abort terminators in all const-contexts #77512

Merged
merged 6 commits into from
Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions compiler/rustc_mir/src/transform/check_consts/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,6 @@ pub trait NonConstOp: std::fmt::Debug {
fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> DiagnosticBuilder<'tcx>;
}

#[derive(Debug)]
pub struct Abort;
impl NonConstOp for Abort {
fn status_in_item(&self, ccx: &ConstCx<'_, '_>) -> Status {
mcf_status_in_item(ccx)
}

fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> DiagnosticBuilder<'tcx> {
mcf_build_error(ccx, span, "abort is not stable in const fn")
}
}

#[derive(Debug)]
pub struct FloatingPointOp;
impl NonConstOp for FloatingPointOp {
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir/src/transform/check_consts/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -874,13 +874,13 @@ impl Visitor<'tcx> for Validator<'mir, 'tcx> {
}

TerminatorKind::InlineAsm { .. } => self.check_op(ops::InlineAsm),
TerminatorKind::Abort => self.check_op(ops::Abort),

TerminatorKind::GeneratorDrop | TerminatorKind::Yield { .. } => {
self.check_op(ops::Generator(hir::GeneratorKind::Gen))
}

TerminatorKind::Assert { .. }
TerminatorKind::Abort
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could just span_bug here actually. AFAICT, there are only Aborts in "cleanup" blocks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a good idea but does not replace a comment. :)

| TerminatorKind::Assert { .. }
| TerminatorKind::FalseEdge { .. }
| TerminatorKind::FalseUnwind { .. }
| TerminatorKind::Goto { .. }
Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/consts/const-eval/unwind-abort.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#![feature(unwind_attributes, const_panic)]

#[unwind(aborts)]
const fn foo() {
panic!() //~ evaluation of constant value failed
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
}

const _: () = foo(); //~ any use of this value will cause an error
ecstatic-morse marked this conversation as resolved.
Show resolved Hide resolved

fn main() {
let _ = foo();
}
21 changes: 21 additions & 0 deletions src/test/ui/consts/const-eval/unwind-abort.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
error[E0080]: evaluation of constant value failed
--> $DIR/unwind-abort.rs:5:5
|
LL | panic!()
| ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/unwind-abort.rs:5:5
|
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: any use of this value will cause an error
--> $DIR/unwind-abort.rs:8:15
|
LL | const _: () = foo();
| --------------^^^^^-
| |
| referenced constant has errors
|
= note: `#[deny(const_err)]` on by default

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0080`.
17 changes: 17 additions & 0 deletions src/test/ui/consts/unwind-abort.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// check-pass

#![feature(unwind_attributes, const_panic)]

// `#[unwind(aborts)]` is okay for a `const fn`. We don't unwind in const-eval anyways.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add two tests? Our test suite is already huge.^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One is check-pass, the other fails to compile.

Copy link
Member

@RalfJung RalfJung Oct 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷 I'm happy with the test coverage in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

#[unwind(aborts)]
const fn foo() {
panic!()
}

const fn bar() {
foo();
}

fn main() {
bar();
}