Emit a hard error when a panic occurs during const-eval #85704

Aaron1011 · 2021-05-26T02:04:39Z

Previous, a panic during const evaluation would go through the
const_err lint. This PR ensures that such a panic always causes
compilation to fail.

rust-highfive · 2021-05-26T02:04:41Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2021-05-26T02:04:43Z

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

davidtwco · 2021-05-26T16:56:26Z

@bors r+

bors · 2021-05-26T16:56:27Z

📌 Commit ec0819b03e2c9e756e25a61963384101d9355485 has been approved by davidtwco

RalfJung · 2021-05-26T17:45:13Z

compiler/rustc_mir/src/const_eval/error.rs

            }
            // We must *always* hard error on these, even if the caller wants just a lint.
            err_inval!(Layout(LayoutError::SizeOverflow(_))) => true,
+            InterpError::MachineStop(err) => err.is_hard_err(),


I don't think this is right. Check the comment below where must_error is used:

if must_error { // The `message` makes little sense here, this is a more serious error than the // caller thinks anyway. // See <https://github.com/rust-lang/rust/pull/63152>.

We are now entering that code path for panics, aren't we?

I guess ignoring the message works out in this case... but the comment is still wrong or at least misleading now.

compiler/rustc_middle/src/mir/interpret/error.rs

RalfJung · 2021-05-26T17:47:34Z

@bors r-
I left some comments.

davidtwco · 2021-05-26T21:41:20Z

r? @RalfJung

Aaron1011 · 2021-05-30T16:01:07Z

@RalfJung: I've refactored the code to better handle the message parameter. This makes the error message for panics slightly worse (it says 'any use of this value will cause an error', like it does on master). However, displaying a nicer message without simply ignoring message will require some additional refactoring, since the calling code is assuming that we're emitting a lint.

RalfJung · 2021-05-30T16:21:24Z

This makes the error message for panics slightly worse (it says 'any use of this value will cause an error', like it does on master). However, displaying a nicer message without simply ignoring message will require some additional refactoring, since the calling code is assuming that we're emitting a lint.

I think if we want to use a different "lint title" (the thing displayed on the first line, no idea what its proper name is), then we should do so consistently for all errors, not just for panics. So, I agree with this. Some people argued that the title should already say what kind of error we have -- I am not opposed, I just think this should be done for panics and other CTFE errors alike.

Also that entire code should probably be refactored, it does some wacky things and I have no idea why, and haven't yet dared touch this code.

compiler/rustc_mir/src/const_eval/error.rs

RalfJung · 2021-05-30T16:23:20Z

compiler/rustc_mir/src/transform/const_prop.rs

@@ -55,7 +55,9 @@ macro_rules! throw_machine_stop_str {
                write!(f, $($tt)*)
            }
        }
-        impl rustc_middle::mir::interpret::MachineStopType for Zst {}
+        impl rustc_middle::mir::interpret::MachineStopType for Zst {
+            fn is_hard_err(&self) -> bool { false }


This is redundant now.

RalfJung · 2021-05-30T16:24:28Z

Looking good! Feel free to bors r=RalfJung once CI is green.
@bors delegate+

bors · 2021-05-30T16:24:30Z

✌️ @Aaron1011 can now approve this pull request

Previous, a panic during const evaluation would go through the `const_err` lint. This PR ensures that such a panic always causes compilation to fail.

Aaron1011 · 2021-05-30T17:00:48Z

@bors r=RalfJung

bors · 2021-05-30T17:00:49Z

📌 Commit 2779fc1 has been approved by RalfJung

bors · 2021-05-31T04:40:18Z

⌛ Testing commit 2779fc1 with merge d84a9199537f1ccd5f8b42eed4a09e8296f45b14...

rust-log-analyzer · 2021-05-31T05:00:45Z

The job dist-x86_64-apple-alt failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

      Memory: 14 GB
      Boot ROM Version: VMW71.00V.13989454.B64.1906190538
      Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
      SMC Version (system): 2.8f0
      Serial Number (system): VM2mYAuJnmkX

hw.ncpu: 3
hw.byteorder: 1234
hw.memsize: 15032385536

bors · 2021-05-31T05:03:22Z

💔 Test failed - checks-actions

Aaron1011 · 2021-05-31T05:10:27Z

@bors retry

bors · 2021-05-31T12:41:59Z

⌛ Testing commit 2779fc1 with merge d9feaaa...

bors · 2021-05-31T15:22:46Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing d9feaaa to master...

rust-highfive · 2021-05-31T15:23:14Z

📣 Toolstate changed by #85704!

Tested on commit d9feaaa.
Direct link to PR: #85704

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).

@oli-obk

Tested on commit rust-lang/rust@d9feaaa. Direct link to PR: <rust-lang/rust#85704> 💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).

lqd · 2021-06-01T06:48:59Z

For people trying to understand the rationale of this change, see #85194 where the decision was made for turning the lint into a hard error. ~~And since this forward-incompatibility lint was tracked in #71800, that issue should probably be closed.~~

RalfJung · 2021-06-01T07:24:33Z

#71800 tracks making all CTFE errors hard errors.
This here does it just for panics.

rust-highfive assigned davidtwco May 26, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 26, 2021

Aaron1011 mentioned this pull request May 26, 2021

Stabilize RFC 2345: Allow panicking in constants #85194

Closed

davidtwco approved these changes May 26, 2021

View reviewed changes

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 May 26, 2021

RalfJung reviewed May 26, 2021

View reviewed changes

compiler/rustc_middle/src/mir/interpret/error.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 26, 2021

rust-highfive assigned RalfJung and unassigned davidtwco May 26, 2021

Aaron1011 force-pushed the const-panic-hard-err branch from ec0819b to 58deca2 Compare May 30, 2021 15:58

Aaron1011 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 30, 2021

RalfJung reviewed May 30, 2021

View reviewed changes

compiler/rustc_mir/src/const_eval/error.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 30, 2021

View reviewed changes

Emit a hard error when a panic occurs during const-eval

2779fc1

Previous, a panic during const evaluation would go through the `const_err` lint. This PR ensures that such a panic always causes compilation to fail.

Aaron1011 force-pushed the const-panic-hard-err branch from d5e94c5 to 2779fc1 Compare May 30, 2021 17:00

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 May 30, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 31, 2021

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 May 31, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label May 31, 2021

bors merged commit d9feaaa into rust-lang:master May 31, 2021

rustbot added this to the 1.54.0 milestone May 31, 2021

rust-highfive mentioned this pull request May 31, 2021

miri no longer builds after rust-lang/rust#85704 #85861

Closed

bors mentioned this pull request May 31, 2021

Run RemoveZsts pass at mir-opt-level=1 #83417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit a hard error when a panic occurs during const-eval #85704

Emit a hard error when a panic occurs during const-eval #85704

Aaron1011 commented May 26, 2021

rust-highfive commented May 26, 2021

rust-highfive commented May 26, 2021

davidtwco commented May 26, 2021

bors commented May 26, 2021

RalfJung May 26, 2021

RalfJung May 26, 2021

RalfJung commented May 26, 2021 •

edited

Loading

davidtwco commented May 26, 2021

Aaron1011 commented May 30, 2021

RalfJung commented May 30, 2021

RalfJung May 30, 2021

RalfJung commented May 30, 2021

bors commented May 30, 2021

Aaron1011 commented May 30, 2021

bors commented May 30, 2021

bors commented May 31, 2021

rust-log-analyzer commented May 31, 2021

bors commented May 31, 2021

Aaron1011 commented May 31, 2021

bors commented May 31, 2021

bors commented May 31, 2021

rust-highfive commented May 31, 2021

lqd commented Jun 1, 2021 •

edited

Loading

RalfJung commented Jun 1, 2021

Emit a hard error when a panic occurs during const-eval #85704

Emit a hard error when a panic occurs during const-eval #85704

Conversation

Aaron1011 commented May 26, 2021

rust-highfive commented May 26, 2021

rust-highfive commented May 26, 2021

davidtwco commented May 26, 2021

bors commented May 26, 2021

RalfJung May 26, 2021

Choose a reason for hiding this comment

RalfJung May 26, 2021

Choose a reason for hiding this comment

RalfJung commented May 26, 2021 • edited Loading

davidtwco commented May 26, 2021

Aaron1011 commented May 30, 2021

RalfJung commented May 30, 2021

RalfJung May 30, 2021

Choose a reason for hiding this comment

RalfJung commented May 30, 2021

bors commented May 30, 2021

Aaron1011 commented May 30, 2021

bors commented May 30, 2021

bors commented May 31, 2021

rust-log-analyzer commented May 31, 2021

bors commented May 31, 2021

Aaron1011 commented May 31, 2021

bors commented May 31, 2021

bors commented May 31, 2021

rust-highfive commented May 31, 2021

lqd commented Jun 1, 2021 • edited Loading

RalfJung commented Jun 1, 2021

RalfJung commented May 26, 2021 •

edited

Loading

lqd commented Jun 1, 2021 •

edited

Loading