Limit the promotion of const fns to the libstd and the `rustc_promotable` attribute#53851

oli-obk · 2018-08-31T11:54:03Z

There are so many questions around promoting const fn calls... it seems saner to try to limit automatic promotion to const fns which were explicitly opted in for promotion.

I added the attribute to all public stable const fns that were already promotable (e.g. not Cell::new) in order to not cause any breakage

r? @eddyb

cc @nikomatsakis

oli-obk · 2018-08-31T12:32:14Z

@bors try

bors · 2018-08-31T12:32:25Z

⌛ Trying commit dc280eb2bc82f51f865c5bc3dc9cd8997cc3d69c with merge 952c7dbfa2628309f557b367929ffc17af20b332...

bors · 2018-08-31T14:50:52Z

☀️ Test successful - status-travis
State: approved= try=True

emilyalbini · 2018-09-01T11:06:57Z

@craterbot run start=master#1114ab684fbad001c4e580326d8eb4d8c4e917d3 end=try#952c7dbfa2628309f557b367929ffc17af20b332 mode=check-only

craterbot · 2018-09-01T11:06:58Z

👌 Experiment pr-53851 created and queued.

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

craterbot · 2018-09-01T11:33:31Z

🚧 Experiment pr-53851 is now running on agent crater-1.

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

craterbot · 2018-09-02T21:59:58Z

🎉 Experiment pr-53851 is completed!
📰 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

oli-obk · 2018-09-03T08:22:29Z

One spurious regression, and one true regression:

Sep 02 20:49:06.306 INFO kablam! error[E0597]: borrowed value does not live long enough
Sep 02 20:49:06.306 INFO kablam!    --> src/apint/constructors.rs:429:11
Sep 02 20:49:06.306 INFO kablam!     |
Sep 02 20:49:06.307 INFO kablam! 429 |               .chain([
Sep 02 20:49:06.307 INFO kablam!     |  ____________________^
Sep 02 20:49:06.307 INFO kablam! 430 | |                 u8::max_value(),
Sep 02 20:49:06.307 INFO kablam! 431 | |                 10,
Sep 02 20:49:06.307 INFO kablam! 432 | |                 42,
Sep 02 20:49:06.307 INFO kablam! 433 | |                 99,
Sep 02 20:49:06.307 INFO kablam! 434 | |                 123
Sep 02 20:49:06.307 INFO kablam! 435 | |             ].into_iter()
Sep 02 20:49:06.307 INFO kablam!     | |_____________^ temporary value does not live long enough
Sep 02 20:49:06.307 INFO kablam! 436 |                .map(|v| *v))
Sep 02 20:49:06.307 INFO kablam! 437 |       }
Sep 02 20:49:06.307 INFO kablam!     |       - temporary value only lives until here
Sep 02 20:49:06.307 INFO kablam!     |
Sep 02 20:49:06.307 INFO kablam!     = note: borrowed value must be valid for the static lifetime...

RalfJung · 2018-09-03T12:50:09Z

... and that could be fixed by using u8::MAX instead. Seems pretty encouraging to me?

oli-obk · 2018-09-03T13:12:53Z

@eddyb said on IRC that we should instead add a whitelist of promotable const fns, and add the currently stable ones to it in order to not have any possibility of breakage even outside crates.io

RalfJung · 2018-09-03T15:15:42Z

Anything, as long as we do it before min_const_fn is stable. :) We could just whitelist libcore for this...

oli-obk · 2018-09-03T15:24:28Z

src/librustc/ty/constness.rs

I collected all the logic in this one file in order to make our lives easier and have less code duplication which might cause odd inconsistencies

oli-obk · 2018-09-03T15:28:03Z

src/librustc_mir/const_eval.rs

I had to remove this because the is_const_fn above is now overly restrictive (it will forbid calling str::len with the const_str_len feature gate but without the const_slice_len feature gate because that calls [u8]::len internally). We already check all the guarantees in the const_qualif pass, there's no need to check them again at evaluation time.

oli-obk · 2018-09-03T15:29:15Z

src/librustc_passes/rvalue_promotion.rs

This is now just a single call to is_promotable_const_fn which seems a major improvement over the code duplication we were doing here before.

oli-obk · 2018-09-03T15:31:20Z

src/test/run-pass/issue-49955-2.rs

calls to Cell::new are not promotable anymore, thus this breaks. The crater run didn't show any cases of people actually writing this kind of code (also needs MIR borrowck)

oli-obk · 2018-09-03T15:32:54Z

src/test/ui/consts/const-eval/issue-52443.stderr

Diagnostics improvement or regression. Depending on your view. We used to stop at the first non-const fn, now we just stop if/when we fail to actually evaluate. An error is reported above about non const fn calls and other non-const things happening here.

Although I like the specificity, this is not actually actionable, is it? I would not block on improving this message though.

We could probably make it a delay_span_bug, but i fear that we'll overlook some case and then run into that hypothetical ICE at some point.

These kind of errors only happen for repeat/array lengths and enum variant discriminant initializers (or constants used in such). Everything else reports only the other errors above.

Alternatively we can try to figure out a scheme that prevents const eval of constants with const qualification errors.

oli-obk · 2018-09-03T15:34:15Z

r? @eddyb

oli-obk · 2018-10-04T06:35:43Z

@bors r=eddyb

bors · 2018-10-04T06:35:43Z

📌 Commit c793391 has been approved by eddyb

bors · 2018-10-04T06:48:20Z

⌛ Testing commit c793391 with merge 088fc73...

@eddyb

Limit the promotion of const fns to the libstd and the `rustc_promotable` attribute There are so many questions around promoting const fn calls... it seems saner to try to limit automatic promotion to const fns which were explicitly opted in for promotion. I added the attribute to all public stable const fns that were already promotable (e.g. not Cell::new) in order to not cause any breakage r? @eddyb cc @nikomatsakis

bors · 2018-10-04T09:17:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 088fc73 to master...

shepmaster · 2019-01-21T17:58:08Z

Broader discussion continues in rust-lang/const-eval#19

rust-highfive assigned RalfJung Aug 31, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2018

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 Sep 1, 2018

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 Sep 2, 2018

oli-obk force-pushed the local_promotion branch from dc280eb to 5f4c14d Compare September 3, 2018 15:21

oli-obk changed the title ~~WIP EXPERIMENT don't promote any const fn calls~~ Don't promote calls to const fns with arguments Sep 3, 2018

oli-obk commented Sep 3, 2018

View reviewed changes

rust-highfive assigned eddyb and unassigned RalfJung Sep 3, 2018

This comment has been minimized.

Sign in to view

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 Oct 2, 2018

kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2018

oli-obk added 5 commits October 3, 2018 10:07

Only promote calls to #[rustc_promotable] const fns

9e46c0b

Update error id to an unused one

1f94384

Fix typo

a9156e9

Poke in the dark and see if bors+windows stops screaming

f5106a2

Move platform dependent output ui tests to compile-fail

c793391

oli-obk force-pushed the local_promotion branch from 6939244 to c793391 Compare October 3, 2018 09:51

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 Oct 4, 2018

bors merged commit c793391 into rust-lang:master Oct 4, 2018

This was referenced Oct 4, 2018

Report const eval error inside the query #53821

Merged

Allow ptr::{null, null_mut} to create pointers to ?Sized types #53954

Closed

Cleanup rustc/ty part 1 #54717

Merged

oli-obk deleted the local_promotion branch October 4, 2018 09:33

SimonSapin mentioned this pull request Oct 5, 2018

Regression: "temporary value does not live long enough" in a static item in Servo #54846

Open

oli-obk mentioned this pull request Oct 26, 2018

Illegal Instruction Promoting const panic! #54305

Closed

This was referenced Nov 9, 2018

apint fails to compile on beta #55806

Closed

Add missing rustc_promotable attribute to unsigned min_value and max_value #55828

Merged

hanna-kruppe mentioned this pull request May 3, 2019

Values created by const fns aren't rvalue static promoted #60502

Open

spastorino mentioned this pull request Jun 10, 2020

Regression: Not well-formed HRTB doesn't compile #55498

Closed

RalfJung mentioned this pull request Aug 12, 2020

Solving the problem with promotion rust-lang/const-eval#53

Closed

Uh oh!

Conversation

oli-obk commented Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented Aug 31, 2018

Uh oh!

bors commented Aug 31, 2018

Uh oh!

bors commented Aug 31, 2018

Uh oh!

emilyalbini commented Sep 1, 2018

Uh oh!

craterbot commented Sep 1, 2018

Uh oh!

craterbot commented Sep 1, 2018

Uh oh!

craterbot commented Sep 2, 2018

Uh oh!

oli-obk commented Sep 3, 2018

Uh oh!

RalfJung commented Sep 3, 2018

Uh oh!

oli-obk commented Sep 3, 2018

Uh oh!

RalfJung commented Sep 3, 2018

Uh oh!

oli-obk Sep 3, 2018

Choose a reason for hiding this comment

Uh oh!

oli-obk Sep 3, 2018

Choose a reason for hiding this comment

Uh oh!

oli-obk Sep 3, 2018

Choose a reason for hiding this comment

Uh oh!

oli-obk Sep 3, 2018

Choose a reason for hiding this comment

Uh oh!

oli-obk Sep 3, 2018

Choose a reason for hiding this comment

Uh oh!

estebank Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

oli-obk Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Sep 3, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

oli-obk commented Oct 4, 2018

Uh oh!

bors commented Oct 4, 2018

Uh oh!

bors commented Oct 4, 2018

Uh oh!

bors commented Oct 4, 2018

Uh oh!

shepmaster commented Jan 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

oli-obk commented Aug 31, 2018 •

edited

Loading