Replace the global fulfillment cache with the evaluation cache #42840
Conversation
arielb1
force-pushed
the
poison-smoke-and-mirrors
branch
from
447f780
to
d93605e
Compare
shepmaster
added
the
S-waiting-on-author
arielb1
force-pushed
the
poison-smoke-and-mirrors
branch
from
d93605e
to
f8f8d7c
Compare
arielb1
changed the title
src/librustc/traits/select.rs
Outdated
| } else { | ||
| debug!("evaluate_stack({:?}) --> recursive, inductive", | ||
| stack.fresh_trait_ref); | ||
| return EvaluatedToErr; |
There was a problem hiding this comment.
So we've been working through this logic in the context of chalk -- it turns out that error isn't really the right answer here. In particular, it can lead you to "overconfidence". The correct answer probably requires iteration (at least, that's what we're doing in chalk). Maybe might be tolerable for now, but may break stuff, I'm not sure.
PR rust-lang/chalk#47 by @scalexm outlines the chalk strategy, which is based on a technique called tabling. In short, you start out by saying error, but then -- if you find solutions -- you iterate again, and this time, on the cycle, you return the solution that you found. This may allow you to find a second solution, in which case you can report ambiguity (no unique solution).
This example test is relevant -- on the first iteration, we encounter a cycle testing whether exists<T> { T: Foo } (that is, S<T>: Foo inquires whether S<T>: Foo), but we also encounter a solution -- T = i32. This then enables (on the second round) us to uncover that S<i32>: Foo is another solution. (Indeed, there are infinitely many, iirc.)
There was a problem hiding this comment.
The current logic actually reports an overflow error in this case, which aborts compilation. Maybe it's better to do that here too?
There was a problem hiding this comment.
It seems like we return "OK" for the "evaluation" result, but abort in the fulfillment cx..? If we aborted in both, it might cause trouble? Maybe worth a try though.
There was a problem hiding this comment.
The other option is EvaluatedToAmbiguity.
arielb1
force-pushed
the
poison-smoke-and-mirrors
branch
3 times, most recently
from
6fc1b35
to
d66f6c6
Compare
aidanhs
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #43008) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@arielb1 sorry for delay, I promise a review soon. =) |
|
r=me after rebase |
Previously, coinductive matching was only blocked on the fulfillment path, and ignored on the evaluation path.
This is just duplicating the logic from the old fulfillment cache, so I'm not sure it is 100% correct, but it should not be more wrong than the old logic.
The evaluation cache already exists, and it can handle differing parameter environments natively. Fixes rust-lang#39970. Fixes rust-lang#42796.
This helps avoid cache pollution. Also add more comments explaining that.
|
@arielb1 I did the rebase locally because I wanted to check some experiments; just pushed it, hope you don't mind. =) |
nikomatsakis
force-pushed
the
poison-smoke-and-mirrors
branch
from
d66f6c6
to
b7b965a
Compare
|
@bors r+ |
|
📌 Commit b7b965a has been approved by |
Replace the global fulfillment cache with the evaluation cache This uses the new "Chalk" ParamEnv refactoring to check "global" predicates in an empty environment, which should be correct because global predicates aren't affected by a consistent environment. Fixes #39970. Fixes #42796. r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
This uses the new "Chalk" ParamEnv refactoring to check "global" predicates in an empty environment, which should be correct because global predicates aren't affected by a consistent environment.
Fixes #39970.
Fixes #42796.
r? @nikomatsakis