Uh oh!
There was an error while loading. Please reload this page.
Use a ParamEnvAnd<Predicate> for caching in ObligationForest - #68475
Conversation
rust-highfive
commented
Jan 23, 2020
(rust_highfive has picked a reviewer for you, use r? to override) |
Aaron1011
commented
Jan 23, 2020
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.
767787b to
90afc07Comparepetrochenkov
commented
Jan 23, 2020
r? @nikomatsakis for review or reassignment |
Aaron1011
commented
Jan 24, 2020
This could probably use a perf run, since it will cause fewer cache hits (though hopefully not significantly fewer). |
jonas-schievink
commented
Jan 25, 2020
@bors try @rust-timer queue |
rust-timer
commented
Jan 25, 2020
Awaiting bors try build completion |
bors
commented
Jan 25, 2020
⌛ Trying commit 90afc07 with merge a1a777c1499a6f109355684d92219c1afbf058f6... |
bors
commented
Jan 25, 2020
☀️ Try build successful - checks-azure |
rust-timer
commented
Jan 25, 2020
Queued a1a777c1499a6f109355684d92219c1afbf058f6 with parent 8ad83af, future comparison URL. |
rust-timer
commented
Jan 26, 2020
Finished benchmarking try commit a1a777c1499a6f109355684d92219c1afbf058f6, comparison URL. |
Aaron1011
commented
Jan 26, 2020
I think the |
nikomatsakis
commented
Jan 27, 2020
Will review soon, thanks @Aaron1011 |
Aaron1011
commented
Feb 8, 2020
@nikomatsakis: Are there any changes that you'd like me to make? |
nikomatsakis
commented
Feb 14, 2020
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+ |
bors
commented
Feb 14, 2020
📌 Commit 90afc07 has been approved by |
…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.