perf(generator): restore incremental caching on the relay-handler branch (TRIM-006) - #73
Merged
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Outcome fork resolved in favor of reshaping the emitted server-only guard so ILLink can eliminate the guarded region, rather than correcting the docs to accept over-retention. Plan opens by turning the gap into a red trimmed- harness pin before any generator edit. PR #72 closed unmerged; its bookkeeping commit stays on TRIM and rides along in TRIM-005's PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…review Plan review REJECTED on direction, pre-implementation. A probe of the HEAD trimmed artifact shows ILLink does eliminate class-factory guarded bodies (ICorrelationContext, IFactoryOnStart, Stopwatch, FactoryOperation, ILogger all absent) — the early-throw guard shape works. TRIM-005 had inherited the opposite claim verbatim from a TRIM-004 Plan Amendment without re-verifying. Real seam: StaticFactoryRenderer.cs:41 and RelayHandlerRenderer.cs:32 point [NeatooFactoryRegistrar] at the consumer's own class, whose DAM(PublicMethods | NonPublicMethods) retains every method — so [Remote] bodies for [Execute] and event-handler classes ship to the browser. Out of TRIM's goal; recorded in the Discovery Log for a direct plan-mode fix rather than queued as a plan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…live break Pre-flight verified the diagnosis at the keyboard rather than inheriting it (TRIM-005's lesson). Findings: TypeInfo and FactoryEventInfo hold the EquatableArray discipline, but RelayHandlerModel exposes Usings, Entries and Diagnostics as IReadOnlyList<T> — a record's synthesized Equals falls back to reference equality there, and the transform allocates fresh lists each run. The relay-handler branch's caching is already broken; the guard is expected to go red on it immediately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every pipeline branch calls RegisterSourceOutput directly on its transform node and does model-building and rendering inside the output stage, so the transform-output record's equality is the entire cache boundary. A record's synthesized Equals falls back to EqualityComparer<T>.Default, which for an IReadOnlyList<T> field means reference equality — and the transform allocates fresh lists on every run. RelayHandlerModel (Usings, Entries, Diagnostics) and EventHandlerEntry (Parameters, ServiceParameters, AllParameters) were all IReadOnlyList<T>, so the relay-handler branch re-rendered on every unrelated edit in the consumer's compilation, with no symptom other than slower incremental builds. Moved onto EquatableArray<T>, the repo's existing value-equatable idiom. The constructors normalize incoming sequences rather than taking the array type, so a future call site cannot reintroduce the defect by passing a plain list. Adds IncrementalCacheTests, which runs the generator twice across an unrelated edit with step tracking enabled and asserts each branch reports Cached or Unchanged. Two details keep it non-vacuous: the edit replaces the syntax tree rather than adding a second one (otherwise every transform is served from upstream cache and the assertion passes regardless), and it is appended so existing spans do not move (TypeInfo and DiagnosticInfo capture line/column). The fixture populates the collection-bearing fields deliberately — empty collections can compare equal and would let a future bad field slip through. TrackingNames plus WithTrackingName on all four nodes let a failure name the branch instead of reporting one anonymous aggregate. Tracking is only recorded when the driver opts in, so normal builds pay nothing. Verified: guard red on RelayHandler (reason Modified) before the fix, green after; negative control confirms the other three branches also go red on a reference-equality field. Emitted output byte-identical — zero drift in the committed Generated/ trees after a rebuild that re-emitted them. DiagnosticTestHelper.RunGenerator is unchanged apart from extracting shared reference-building; the existing diagnostic suite is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gotcha Fills plan 006's Test Evidence table, checks its Acceptance bullets, and adds an Implementation Record covering the red/green cycle and the Step 6 negative control. Two Discovery Log entries. The first confirms pre-flight's prediction held: the relay-handler branch was the one live cache break at HEAD, and emitted output was verified unchanged empirically rather than by inference. The second is the more consequential one. DiagnosticTestHelper loads the generator once per process via Assembly.LoadFrom into a static Lazy, so rebuilding only the generator and re-running with --no-build can let a surviving testhost serve the previously loaded assembly. The negative control caught this: the probe field was provably present in the loaded DLL while the guard still reported green. This applies to every test going through that helper, which is effectively the whole generator suite — a generator fix can appear verified when it was never loaded. CI is unaffected, so it is documented at the seam; the durable fix is recorded as deliberately deferred. Co-Authored-By: Claude Opus 5 (1M context) <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
1 participant
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.
Closes TRIM-006. Also carries the TRIM-005 abandonment and the TRIM-007 bookkeeping commit, which ride along from the
TRIMarc branch.The defect
Every pipeline branch calls
RegisterSourceOutputdirectly on its transform node and does model-building and rendering inside the output stage, so the transform-output record's equality is the entire cache boundary. A record's synthesizedEqualsfalls back toEqualityComparer<T>.Default— reference equality for anIReadOnlyList<T>field — and the transform allocates fresh lists on every run.RelayHandlerModel(Usings,Entries,Diagnostics) andEventHandlerEntry(Parameters,ServiceParameters,AllParameters) were allIReadOnlyList<T>. Consumers with[FactoryEventHandler<T>]classes re-rendered that branch on every unrelated edit anywhere in their compilation, with no symptom other than slower incremental builds.Fixed by moving to
EquatableArray<T>, the repo's existing value-equatable idiom. The constructors normalize incoming sequences rather than taking the array type, so a future call site cannot reintroduce the defect by passing a plain list.The guard
IncrementalCacheTestsruns the generator twice across an unrelated edit with step tracking enabled and asserts each of the four branches reportsCached/Unchanged. Two details keep it from passing vacuously:TypeInfoandDiagnosticInfocapture file paths and line/column, and a shifting edit would fail the guard for a benign reason.The fixture populates the collection-bearing fields deliberately: empty collections can compare equal, which would let a future bad field slip past exactly the check meant to catch it.
TrackingNames+WithTrackingNameon all four nodes make a failure name the branch rather than report one anonymous aggregate. Tracking is only recorded when the driver opts in, so normal builds pay nothing.Verification
RelayHandlerModified(real, pre-existing)UnchangedFactoryClassModified(probe)Cached/UnchangedFactoryInterfaceModified(probe)Cached/UnchangedFactoryEventsModified(probe)Cached/UnchangedThe guard was confirmed red before the fix and green after, so it is proven non-vacuous rather than merely passing. The negative control injected a reference-equality field into
TypeInfoandFactoryEventInfoto prove the three already-passing branches are sensitive too; probes were removed afterward.Emitted output is byte-identical — verified empirically, not inferred from "the renderer is untouched": generated code is committed under
Generated/, andgit statusshows zero drift in both solutions after a rebuild that re-emitted those files.Tests: 601+601 unit, 561+561 integration (5 pre-existing skips), 86+86 Design — 0 failures across net9.0 and net10.0.
Reviewer note — a harness gotcha this uncovered
DiagnosticTestHelperloads the generator once per process viaAssembly.LoadFrominto a staticLazy. Rebuilding only the generator and re-runningdotnet test --no-buildcan let a surviving testhost serve the previously loaded assembly, so a test reports on code no longer on disk. This bit me mid-review: the negative control passed when it should have failed, and a tracked-step dump proved the probe was live in the DLL while the guard reported green.This applies to every test going through that helper — effectively the whole generator suite. A generator fix can appear verified when it was never loaded. CI is unaffected (cold build each run), so it is documented at the seam rather than re-architected; the durable fix (fail fast when the generator DLL predates the test assembly) is recorded in the Discovery Log as deliberately deferred, since this plan's Constraints forbid reworking the shared helper.
Gate status
The Step 5 gate (
test-reviewer, mandatory;code-reviewer, which this plan opted into) has not run yet — opening this PR at the author's direction ahead of it.🤖 Generated with Claude Code