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

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

Merged
merged 5 commits into from
Oct 4, 2018

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Aug 31, 2018

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 31, 2018

@bors try

@bors
Copy link
Contributor

bors commented Aug 31, 2018

⌛ Trying commit dc280eb2bc82f51f865c5bc3dc9cd8997cc3d69c with merge 952c7dbfa2628309f557b367929ffc17af20b332...

@bors
Copy link
Contributor

bors commented Aug 31, 2018

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

@pietroalbini
Copy link
Member

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

@craterbot
Copy link
Collaborator

👌 Experiment pr-53851 created and queued.

ℹ️ 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 Sep 1, 2018
@craterbot
Copy link
Collaborator

🚧 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
Copy link
Collaborator

🎉 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

@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 Sep 2, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 3, 2018

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
Copy link
Member

RalfJung commented Sep 3, 2018

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

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 3, 2018

@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
Copy link
Member

RalfJung commented Sep 3, 2018

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

@oli-obk 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
use syntax::attr;
use rustc_target::spec::abi;

impl<'a, 'tcx> TyCtxt<'a, 'tcx, 'tcx> {
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 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

@@ -253,9 +250,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeEvaluator {
ecx.goto_block(ret)?; // fully evaluated and done
return Ok(None);
}
return Err(
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 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.

&mut self,
def_id: DefId,
) -> Promotability {
if self.tcx.is_promotable_const_fn(def_id) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

#[inline(never)]
fn tuple_field() -> &'static u32 {
// This test is MIR-borrowck-only because the old borrowck
// doesn't agree that borrows of "frozen" (i.e. without any
// interior mutability) fields of non-frozen temporaries,
// should be promoted, while MIR promotion does promote them.
&(Cell::new(5), 42).1
&(FIVE, 42).1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

| calling non-const fn `<I as std::iter::IntoIterator><std::ops::RangeFrom<usize>>::into_iter`
| inside call to `std::iter::range::<impl std::iter::Iterator for std::ops::RangeFrom<A>><usize>::next`
|
::: $SRC_DIR/libcore/ptr.rs:LL:COL
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

oli-obk commented Sep 3, 2018

r? @eddyb

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

This comment has been minimized.

@bors

This comment has been minimized.

@bors 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 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
Copy link
Contributor Author

oli-obk commented Oct 4, 2018

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Oct 4, 2018

📌 Commit c793391 has been approved by eddyb

@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 Oct 4, 2018
@bors
Copy link
Contributor

bors commented Oct 4, 2018

⌛ Testing commit c793391 with merge 088fc73...

bors added a commit that referenced this pull request Oct 4, 2018
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
Copy link
Contributor

bors commented Oct 4, 2018

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

@shepmaster
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.