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

make UB during CTFE a hard error #86194

Merged
merged 3 commits into from
Jun 19, 2021
Merged

Conversation

RalfJung
Copy link
Member

This is a next step for #71800. const_err has been a future-incompatibility lint for 4 months now since #80394 (and err-by-default for many years before that), so I think we could try making it a proper hard error at least in some situations.

I didn't yet adjust the tests, since I first want to gauge the fall-out via crater.
Cc @rust-lang/wg-const-eval

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @davidtwco

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 10, 2021
@RalfJung
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Jun 10, 2021

⌛ Trying commit 98a710fc2a08c585a96b27a7667bd46a6447787b with merge 8552849d5ae3b6c66b89a0ac2e9c1e3ef5cde346...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 10, 2021

☀️ Try build successful - checks-actions
Build commit: 8552849d5ae3b6c66b89a0ac2e9c1e3ef5cde346 (8552849d5ae3b6c66b89a0ac2e9c1e3ef5cde346)

@RalfJung
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-86194 created and queued.
🤖 Automatically detected try build 8552849d5ae3b6c66b89a0ac2e9c1e3ef5cde346
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2021
@craterbot
Copy link
Collaborator

🚧 Experiment pr-86194 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'll re-assign to someone more involved in const eval.

@davidtwco
Copy link
Member

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned davidtwco Jun 16, 2021
@craterbot
Copy link
Collaborator

🎉 Experiment pr-86194 is completed!
📊 10 regressed and 5 fixed (167396 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 18, 2021
@RalfJung
Copy link
Member Author

RalfJung commented Jun 18, 2021

As far as I can see, these are all spurious. :-)
Cc @rust-lang/lang -- I am proposing to make UB errors during const evaluation hard errors (instead of err-by-default const_err lints). Crater showed no regressions. See #71800 for some background and the long-term plan.

@rust-log-analyzer

This comment has been minimized.

@joshtriplett
Copy link
Member

@RalfJung This seems consistent with @rust-lang/lang consensus.

Thanks for working on this!

@@ -1,5 +1,4 @@
// build-fail

// error-pattern: evaluation of constant value failed
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not great, but arguably the old test already did not match in anything interesting in the error annotations either.

@bors 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 Jun 18, 2021
@nikomatsakis
Copy link
Contributor

Maybe that was premature :) I can't remember the state of things here.

@RalfJung
Copy link
Member Author

Still needs a review; currently @oli-obk is assigned.

@@ -1,5 +1,8 @@
#![feature(const_raw_ptr_deref)]
#![feature(const_ptr_offset_from)]
#![feature(core_intrinsics)]

use std::intrinsics::ptr_offset_from;
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this test to call the intrinsic directly, so that we can properly match on the spans for the error messages.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 18, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jun 18, 2021

📌 Commit 7475661 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 18, 2021
@bors
Copy link
Contributor

bors commented Jun 18, 2021

⌛ Testing commit 7475661 with merge ec57c60...

@bors
Copy link
Contributor

bors commented Jun 19, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing ec57c60 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 19, 2021
@bors bors merged commit ec57c60 into rust-lang:master Jun 19, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 19, 2021
@RalfJung RalfJung deleted the const-ub-hard-error branch June 20, 2021 10:57
@ehuss
Copy link
Contributor

ehuss commented Jun 21, 2021

@RalfJung Would it make sense to add a note to https://github.com/rust-lang/reference/blob/master/src/const_eval.md that mentions that undefined behavior is an error? I'm not sure if the ub chapter covers what MIRI checks for, or if those checks should be explicitly listed?

@RalfJung
Copy link
Member Author

RalfJung commented Jun 21, 2021

RFC 3016 contains some explanation of which kinds of UB CTFE will find. (That's less than what Miri-the-tool will find, mostly for performance reasons.)

But in general we don't want to make stable guarantees about the set of UB detected by CTFE. So not sure sure if that should be in the reference.

@pthariensflame
Copy link
Contributor

Is this worth putting in RELEASES.md? It’s not there right now.

@joshtriplett
Copy link
Member

Yes, I think it should be.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 25, 2022
… r=estebank

make const_err show up in future breakage reports

As tracked in rust-lang#71800, const_err should become a hard error Any Day Now (TM). I'd love to move forward with that sooner rather than later; it has been deny-by-default for many years and a future incompat lint since rust-lang#80394 (landed more than a year ago). Some CTFE errors are already hard errors since rust-lang#86194. But before we truly make it a hard error in all cases, we now have one more intermediate step we can take -- to make it show up in future breakage reports.

Cc `@rust-lang/wg-const-eval`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 26, 2022
… r=estebank

make const_err show up in future breakage reports

As tracked in rust-lang#71800, const_err should become a hard error Any Day Now (TM). I'd love to move forward with that sooner rather than later; it has been deny-by-default for many years and a future incompat lint since rust-lang#80394 (landed more than a year ago). Some CTFE errors are already hard errors since rust-lang#86194. But before we truly make it a hard error in all cases, we now have one more intermediate step we can take -- to make it show up in future breakage reports.

Cc `@rust-lang/wg-const-eval`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 26, 2022
… r=estebank

make const_err show up in future breakage reports

As tracked in rust-lang#71800, const_err should become a hard error Any Day Now (TM). I'd love to move forward with that sooner rather than later; it has been deny-by-default for many years and a future incompat lint since rust-lang#80394 (landed more than a year ago). Some CTFE errors are already hard errors since rust-lang#86194. But before we truly make it a hard error in all cases, we now have one more intermediate step we can take -- to make it show up in future breakage reports.

Cc ``@rust-lang/wg-const-eval``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 26, 2022
… r=estebank

make const_err show up in future breakage reports

As tracked in rust-lang#71800, const_err should become a hard error Any Day Now (TM). I'd love to move forward with that sooner rather than later; it has been deny-by-default for many years and a future incompat lint since rust-lang#80394 (landed more than a year ago). Some CTFE errors are already hard errors since rust-lang#86194. But before we truly make it a hard error in all cases, we now have one more intermediate step we can take -- to make it show up in future breakage reports.

Cc ```@rust-lang/wg-const-eval```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 26, 2022
… r=estebank

make const_err show up in future breakage reports

As tracked in rust-lang#71800, const_err should become a hard error Any Day Now (TM). I'd love to move forward with that sooner rather than later; it has been deny-by-default for many years and a future incompat lint since rust-lang#80394 (landed more than a year ago). Some CTFE errors are already hard errors since rust-lang#86194. But before we truly make it a hard error in all cases, we now have one more intermediate step we can take -- to make it show up in future breakage reports.

Cc ````@rust-lang/wg-const-eval````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 26, 2022
… r=estebank

make const_err show up in future breakage reports

As tracked in rust-lang#71800, const_err should become a hard error Any Day Now (TM). I'd love to move forward with that sooner rather than later; it has been deny-by-default for many years and a future incompat lint since rust-lang#80394 (landed more than a year ago). Some CTFE errors are already hard errors since rust-lang#86194. But before we truly make it a hard error in all cases, we now have one more intermediate step we can take -- to make it show up in future breakage reports.

Cc `````@rust-lang/wg-const-eval`````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. 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.