Add a strategy FnMut to inject behavior into the flush cycle#157620
Draft
Daniel-B-Smith wants to merge 5 commits into
Draft
Add a strategy FnMut to inject behavior into the flush cycle#157620Daniel-B-Smith wants to merge 5 commits into
Daniel-B-Smith wants to merge 5 commits into
Conversation
…ep graph Adds `FileEncoder<'a>` with a phantom `_ref: Option<&'a u64>` field so the lifetime propagates virally through `EncoderState`, `GraphEncoder`, `CurrentDepGraph`, `DepGraphData`, `DepGraph`, and `GlobalCtxt<'tcx>`. Uses `#[may_dangle]` on the `Drop` impl to break the drop-check cycle that arises when `GlobalCtxt<'tcx>` (which contains `DepGraph<'tcx>`) is stored inside the `OnceLock` whose reference determines `'tcx`. Removes `DepGraph` from `Linker` (which must escape a `for<'tcx>` closure) and replaces it with a plain `WorkProductMap` clone, updating `save_work_product_index` accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- PhantomData<&'a ()>: replace Option<&'a u64> with a zero-sized covariant marker matching the existing MemDecoder<'a> idiom; no runtime field needed - Gate Drop entirely on #[cfg(debug_assertions)]: release builds have no Drop impl and therefore no dropck constraint; debug builds keep may_dangle to break the GlobalCtxt self-referential cycle - Restore assert_ignored() in codegen_and_build_linker before extracting previous_work_products, preserving the debug invariant that was silently dropped when save_work_product_index stopped taking &DepGraph - Option<WorkProductMap>: make the incremental-only invariant explicit in Linker, and deduplicate the repeated incremental.is_some() guards with a shared binding Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous approach added #[may_dangle] to `impl Drop for FileEncoder<'a>` to suppress the dropck constraint on `'a`. This works but requires the dropck_eyepatch feature and uses unsafe. The cleaner fix: keep Drop off FileEncoder<'a> entirely. Move the `finished: bool` flag into a concrete `FinishGuard` struct (debug builds only) that owns the Drop assertion. Because FinishGuard has no lifetime parameters, its Drop impl does not constrain 'a at all, so the self-referential cycle through GlobalCtxt<'tcx> never forms. FileEncoder<'a> has no explicit Drop impl. Its _marker: PhantomData<&'a ()> also has no Drop. Dropck therefore adds no constraint on 'a, making may_dangle and the dropck_eyepatch feature gate unnecessary. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rustbot
added
A-query-system
This comment has been minimized.
This comment has been minimized.
…> marker The viral lifetime cascade only required `serialize` to accept `FileEncoder<'_>`; the accompanying `TyCtxt<'_>` -> `TyCtxt<'tcx>` change introduced a named lifetime that elision already handled. Revert it to keep the signature minimal. FileEncoder carries its phantom lifetime via `_ref: Option<&'a u64>`, which is the point of the exercise. Drop semantics stay on the lifetime-free `FinishGuard` so `FileEncoder<'a>` has no Drop impl and imposes no dropck constraint on `'a`, avoiding the GlobalCtxt<'tcx> self-referential cycle without may_dangle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Linker can no longer hold DepGraph<'tcx> (invariant in its new lifetime, so it cannot escape the for<'tcx> closure in create_and_enter_global_ctxt), so it extracts the WorkProductMap instead. Previously that extraction deep-cloned the whole map at link time. Store DepGraphData::previous_work_products as Arc<WorkProductMap> instead. The map is wrapped once at load (Arc::new in DepGraph::new), and the getter now returns a cheap Arc::clone, so Linker retains the map via a refcount bump rather than copying every WorkProduct and its saved_files strings. The singular previous_work_product() getter is unchanged (reaches through Deref); save_work_product_index still takes &WorkProductMap via as_deref(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a proof of concept PR to confirm that we do not need to fork FileEncoder in #154724.
I used AI to generate the PR because, as you can tell, adding that lifetime parameter was viral. For now, I'm trying to get a sense of the scope and performance impact of this change.
I have reviewed the change and the only thing I don't fully understand is the changes to DepGraph. Here is the AI explanation:
DepGraph<'a> became invariant in 'a.
Deep inside it now holds Lock<Option<FileEncoder<'a>>>, and Lock/UnsafeCell
is invariant. So DepGraph<'tcx> can't be subtyped to a shorter/longer
lifetime.
Linker has to escape a for<'tcx> closure.
In rustc_driver_impl, the linker is returned out of:
whose bound is
for<'tcx> FnOnce(TyCtxt<'tcx>) -> T. T is higher-ranked —it can't depend on 'tcx. When Linker stored dep_graph: DepGraph<'tcx>
(invariant), 'tcx got trapped inside the closure, producing the
"lifetime may not live long enough" error we hit in
rustc_driver_impl/src/lib.rs.
So DepGraph was dropped from Linker.
But link() only ever used the dep graph for two things:
stale work-product files
Those two uses were relocated.
assert_ignored() moved up to codegen_and_build_linker (queries.rs), where
the TyCtxt is still in scope inside the closure. And previous_work_products()
— a lifetime-free WorkProductMap (UnordMap<WorkProductId, WorkProduct>) — is
now cloned out while still inside the closure and stored in Linker directly.
save_work_product_index followed.
Since its caller no longer holds a DepGraph and the only thing the function
needed from it was previous_work_products(), the parameter was narrowed from
&DepGraph to &WorkProductMap. The assert_ignored() line inside it was deleted
because that check now happens at the extraction site instead.
So the removal is a consequence of the invariance: Linker couldn't carry the
lifetime-bearing DepGraph<'tcx> across the closure boundary, so it carries the
plain WorkProductMap instead, and the function signature was simplified to match
what it actually consumes.
One tradeoff worth naming: this swapped a cheap Arc clone (the old DepGraph) for
a full WorkProductMap clone. It's once, at link time, off the hot path — but it
is a new allocation that didn't exist before.