Use a ParamEnvAnd<Predicate> for caching in ObligationForest
#68475
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I don't have an example of unsound code that's current accepted due to this. However, I ran into this when working on a fix for #52843 - I attempted to register two |
This comment has been minimized.
This comment has been minimized.
Previously, we used a plain `Predicate` to cache results (e.g. successes and failures) in ObligationForest. However, fulfillment depends on the precise `ParamEnv` used, so this is unsound in general. This commit changes the impl of `ForestObligation` for `PendingPredicateObligation` to use `ParamEnvAnd<Predicate>` instead of `Predicate` for the associated type. The associated type and method are renamed from 'predicate' to 'cache_key' to reflect the fact that type is no longer just a predicate.
Aaron1011
force-pushed
the
fix/forest-caching
branch
from
767787b
to
90afc07
Compare
|
r? @nikomatsakis for review or reassignment |
|
This could probably use a perf run, since it will cause fewer cache hits (though hopefully not significantly fewer). |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 90afc07 with merge a1a777c1499a6f109355684d92219c1afbf058f6... |
|
☀️ Try build successful - checks-azure |
|
Queued a1a777c1499a6f109355684d92219c1afbf058f6 with parent 8ad83af, future comparison URL. |
|
Finished benchmarking try commit a1a777c1499a6f109355684d92219c1afbf058f6, comparison URL. |
|
I think the |
|
Will review soon, thanks @Aaron1011 |
|
@nikomatsakis: Are there any changes that you'd like me to make? |
|
Apologies for the long delay. This is a good PR, and actually an important step as part of the "move rustc closer to chalk goal", so .. great! The perf impacts look small, though I'm not sure on what basis we can declare them to be noise? But I hope you're right! @bors r+ |
|
📌 Commit 90afc07 has been approved by |
bors
added
S-waiting-on-bors
…omatsakis Use a `ParamEnvAnd<Predicate>` for caching in `ObligationForest` Previously, we used a plain `Predicate` to cache results (e.g. successes and failures) in ObligationForest. However, fulfillment depends on the precise `ParamEnv` used, so this is unsound in general. This commit changes the impl of `ForestObligation` for `PendingPredicateObligation` to use `ParamEnvAnd<Predicate>` instead of `Predicate` for the associated type. The associated type and method are renamed from 'predicate' to 'cache_key' to reflect the fact that type is no longer just a predicate.
Rollup of 7 pull requests Successful merges: - #68129 (Correct inference of primitive operand type behind binary operation) - #68475 (Use a `ParamEnvAnd<Predicate>` for caching in `ObligationForest`) - #68856 (typeck: clarify def_bm adjustments & add tests for or-patterns) - #69051 (simplify_try: address some of eddyb's comments) - #69128 (Fix extra subslice lowering) - #69150 (Follow-up to #68848) - #69164 (Update pulldown-cmark dependency) Failed merges: r? @ghost
Previously, we used a plain
Predicateto cache results (e.g. successesand failures) in ObligationForest. However, fulfillment depends on the
precise
ParamEnvused, so this is unsound in general.This commit changes the impl of
ForestObligationforPendingPredicateObligationto useParamEnvAnd<Predicate>instead ofPredicatefor the associated type. The associated type and method arerenamed from 'predicate' to 'cache_key' to reflect the fact that type is
no longer just a predicate.