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

rustc: generalize monomorphic_const_eval to polymorphic constants. #41408

Merged
merged 3 commits into from
Apr 23, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Apr 19, 2017

With the addition of Substs to the query key, we can now evaluate and cache polymorphic constants.

Fixes #23898 by replacing the crippled explicit-discriminant-only local-crate-only lookup_variant_by_id with ConstVal::Variant which can describe variants irrespective of their discriminant.

Fixes #41394 by fixing #23898 (for the original testcase) and by not looping past the first discriminant.

@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb eddyb force-pushed the poly-const-eval branch 2 times, most recently from ccbe157 to faf920a Compare April 20, 2017 17:53
@eddyb
Copy link
Member Author

eddyb commented Apr 21, 2017

ping @rust-lang/compiler This fixes a regression in nightly, should merge it before the beta.

Some(ic) => lookup_const_by_id(tcx, ic.def_id, Substs::empty()),
None => default_value,
Some(ic) => {
let substs = tcx.lift(&impl_data.substs).unwrap();
Copy link
Contributor

@arielb1 arielb1 Apr 21, 2017

Choose a reason for hiding this comment

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

doesn't this need to do the drain_fulfillment_cx_or_panic thing? aka be monomorphize::resolve?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't panic here, but I guess, yeah, it should be using something similar to trans.

Copy link
Contributor

Choose a reason for hiding this comment

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

There used to be a drain_fulfillment_cx without the panic part function. Can you reintroduce it? We need to not drop nested obligations on the floor, because of associated types in impls:

trait Foo {
    const BAR: usize;
}

impl<U, V> Foo for (U, V) where U: Iterator<Item=V>, V: Foo {
    const BAR: usize = V::BAR;
}

impl Foo for u32 { const BAR: usize = 4; }


// Avoid applying substitutions if they're empty, that'd ICE.
let base_ty = if cx.substs.is_empty() {
base_ty
Copy link
Contributor

Choose a reason for hiding this comment

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

this sounds dubious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Empty substitutions are used to signify "polymorphic evaluation". Providing identity substitutions in the right locations is harder.

@shepmaster shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 21, 2017
@eddyb eddyb force-pushed the poly-const-eval branch from faf920a to 18cf0bb Compare April 23, 2017 06:27
@arielb1
Copy link
Contributor

arielb1 commented Apr 23, 2017

r+ modulo the fulfillment_cx problem.

@eddyb eddyb force-pushed the poly-const-eval branch from 18cf0bb to 60faa01 Compare April 23, 2017 07:42
@arielb1
Copy link
Contributor

arielb1 commented Apr 23, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Apr 23, 2017

📌 Commit 60faa01 has been approved by arielb1

@bors
Copy link
Contributor

bors commented Apr 23, 2017

⌛ Testing commit 60faa01 with merge 582d85c...

@bors
Copy link
Contributor

bors commented Apr 23, 2017

💔 Test failed - status-travis

@TimNN
Copy link
Contributor

TimNN commented Apr 23, 2017

[00:10:06] error: expected one of `,`, `.`, `?`, `}`, or an operator, found `None`
[00:10:06]    --> /checkout/src/librustc_const_eval/eval.rs:518:21
[00:10:06]     |
[00:10:06] 518 |                     None => {
[00:10:06]     |                     ^^^^
[00:10:06] 
[00:10:06] error: aborting due to previous error
[00:10:06] 
[00:10:06] error: Could not compile `rustc_const_eval`.
[00:10:06] Build failed, waiting for other jobs to finish...
[00:11:53] error: build failed

@eddyb eddyb force-pushed the poly-const-eval branch from 60faa01 to 8054377 Compare April 23, 2017 08:12
@eddyb
Copy link
Member Author

eddyb commented Apr 23, 2017

@bors r=arielb1

@bors
Copy link
Contributor

bors commented Apr 23, 2017

📌 Commit 8054377 has been approved by arielb1

@bors
Copy link
Contributor

bors commented Apr 23, 2017

⌛ Testing commit 8054377 with merge ea3233d...

@bors
Copy link
Contributor

bors commented Apr 23, 2017

💔 Test failed - status-appveyor

@TimNN
Copy link
Contributor

TimNN commented Apr 23, 2017

@bors
Copy link
Contributor

bors commented Apr 23, 2017

⌛ Testing commit 8054377 with merge 23de823...

bors added a commit that referenced this pull request Apr 23, 2017
rustc: generalize monomorphic_const_eval to polymorphic constants.

With the addition of `Substs` to the query key, we can now evaluate *and cache* polymorphic constants.

Fixes #23898 by replacing the crippled explicit-discriminant-only local-crate-only `lookup_variant_by_id` with `ConstVal::Variant` which can describe variants irrespective of their discriminant.

Fixes #41394 by fixing #23898 (for the original testcase) and by not looping past the first discriminant.
@bors
Copy link
Contributor

bors commented Apr 23, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 23de823 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
6 participants