-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
evaluation cache, freshen each predicate by itself #102713
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@@ -2253,7 +2245,8 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { | |||
previous_stack: TraitObligationStackList<'o, 'tcx>, | |||
obligation: &'o TraitObligation<'tcx>, | |||
) -> TraitObligationStack<'o, 'tcx> { | |||
let fresh_trait_pred = obligation.predicate.fold_with(&mut self.freshener); | |||
let fresh_trait_pred = | |||
obligation.predicate.fold_with(&mut self.infcx.freshener_keep_static()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was marked as outdated.
This comment was marked as outdated.
@bors try @rust-timer queue |
14584fb
to
da92870
Compare
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit da928700181422639a4c06f0ac832f3a87e65180 with merge 79e77056cd5cd1e6cd8cb883026e53a7f1ff0659... |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
1 similar comment
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit da928700181422639a4c06f0ac832f3a87e65180 with merge f30cafb00d844ad0086efc9765084f4e994286f5... |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
1 similar comment
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit da928700181422639a4c06f0ac832f3a87e65180 with merge 0cb6fd9f0ff8333a6cdc3b11e06197e42a2c8da5... |
☀️ Try build successful - checks-actions |
Queued 0cb6fd9f0ff8333a6cdc3b11e06197e42a2c8da5 with parent 8c71b67, future comparison URL. |
Finished benchmarking commit (0cb6fd9f0ff8333a6cdc3b11e06197e42a2c8da5): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Footnotes |
@bors try |
r=me but I think we should do a crater run |
⌛ Trying commit da928700181422639a4c06f0ac832f3a87e65180 with merge 7f168b18704203996d3698515c88a036a9300946... |
☀️ Try build successful - checks-actions |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
da92870
to
b28b235
Compare
this changes some recursive paths from Comparing the trait ref using structural eq, this previously failed as we had different fresh vars, now it succeeds: rust/compiler/rustc_trait_selection/src/traits/select/mod.rs Lines 846 to 853 in 1536ab1
This succeeds even for different fresh vars: rust/compiler/rustc_trait_selection/src/traits/select/mod.rs Lines 916 to 928 in 1536ab1
@jyn514 previously looked into changing |
wip code here: https://github.com/jyn514/rust/pull/new/normalize-docs |
☔ The latest upstream changes (presumably #104600) made this pull request unmergeable. Please resolve the merge conflicts. |
when freshening each obligation by itself we have to correctly deal with inference constraints from the cyclic call. Otherwise we get the following issue: trait Foo {} // assume `Foo` is coinductive here
struct Wrapper<T>(T);
impl<T> Foo for Wrapper<Wrapper<T>>
where
Wrapper<T>: Foo
{} query: does
To deal with this evaluate has to return inference results which will only be added in the implementation of the trait system refactor initiative. |
update test for inductive canonical cycles the previous test always resulted in a cycle 😅 cc rust-lang/chalk#787. I checked with rust-lang#102713 and this is the only test which fails with that PR. r? `@jackh726`
update test for inductive canonical cycles the previous test always resulted in a cycle 😅 cc rust-lang/chalk#787. I checked with rust-lang#102713 and this is the only test which fails with that PR. r? ``@jackh726``
I think this change is correct. Here's my understanding of this:
This previously wasn't done was so that we don't consider
Foo<?0>: Trait
requiringFoo<?0>: Trait
(which is trivially a cycle), the same asFoo<?0>: Trait
requiringFoo<?1>: Trait
, which is an infinite chain instead.For inductive obligations, cycles and infinite chains should both result in ambiguity, so for these this doesn't matter.
For coinductive obligations, cycles result in success and I believe that infinitely diverging chains should also be a success. That's my understanding given my limited experience with coq and the general intuition of "coinduction means stuff is true unless proven otherwise, induction means stuff is wrong unless proven otherwise".
r? @nikomatsakis cc @rust-lang/types