have on_completion record subcycles

[nikomatsakis]

have on_completion record subcycles

Rework on_completion method so that it removes all
provisional cache entries that are "below" a completed
node (while leaving those entries that are not below
the node).

This corrects an imprecise result that could in turn lead
to an incremental compilation failure. Under the old
scheme, if you had:

• A depends on...
  • B depends on A
  • C depends on...
    • D depends on C
• T: 'static

then the provisional results for A, B, C, and D would all
be entangled. Thus, if A was EvaluatedToOkModuloRegions
(because of that final condition), then the result for C and
D would also be demoted to "ok modulo regions".

In reality, though, the result for C depends only on C and itself,
and is not dependent on regions. If we happen to evaluate the
cycle starting from C, we would never reach A, and hence the
result would be "ok".

Under the new scheme, the provisional results for C and D
are moved to the permanent cache immediately and are not affected
by the result of A.

Fixes #83538

r? @Aaron1011

[Aaron1011]

Your description of the issue agrees with my thoughts in #83538 (comment). However, I don't have a very good understanding of the co-inductive cycle handling code (which is why I went with the earlier fix). I'd like to have someone else look over this as well.

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 8c7b7a0 with merge 6fbb7707efd14220bfaa4efa905c7fff7c51cb46...

[bors]

💔 Test failed - checks-actions

[nikomatsakis]

@Aaron1011 maybe it would be good to do a talk through session -- this relates to my goal of building up expertise around the trait system. I'll leave a comment on Zulip.

[nikomatsakis]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit aa83920b7d1d58015fb19b7e71e2887c83e2babc with merge cd2b79ca5ca0c76854b71f4893e2495a6ce19722...

[bors]

💔 Test failed - checks-actions

[nikomatsakis]

OK, I finally found a way to test this issue in a satisfactory way. I've included tests both for the original problem and the ICE we found in rustdoc. I also spent some time talking over how it works with @jackh726, which you can watch here.

r? @jackh726

[jackh726]

Thanks @nikomatsakis for going over this with me (and for all those that watch the recording)

This is a surprisingly simple change for an extremely complex issue. It is subtle though, so regardless of perf results, I want to @bors rollup=never

That being said, r=me when perf is done.

[rust-timer]

Finished benchmarking try commit (61b32d567658b13ad4d86cc5fa7c5a4b76bae0e0): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[nikomatsakis]

@bors r=jackh726

[bors]

📌 Commit 89c58ea has been approved by jackh726

[nikomatsakis]

Nominating for beta and stable backport. These incremental failures are a big problem.

[bors]

⌛ Testing commit 89c58ea with merge 6d395a1...

[bors]

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 6d395a1 to master...

[apiraino]

Both backports have been discussed and pushed to next week's T-compiler meeting (Zulip log)

[pnkfelix]

discussed in steering meeting about incr-comp fingerprints

declining to backport for beta and likewise for stable.