fix: Hot reload no longer warns about callers in other assemblies that it already patched - #3033
Merged
Merged
Conversation
Labels built from a resolved MethodBase used Type.ToString for parameter types (List`1[System.Int32], System.Int32[,]), while worker rows and call-site hits carry Cecil's spelling (List`1<System.Int32>, System.Int32[0...,0...]). The ledgers compare labels by ordinal equality, so methods with constructed generic or multidimensional-array parameters silently missed their matches: the skipped-writer warning claimed the property keeps its default value although an earlier patch still assigns it, pause points found no unapplied row, and the stale, superseded, and signature-change lookups could miss (#2903). - FormatMethodLabel walks the parameter type structurally (byref, pointer, array rank, generic parameter, generic type from its definition's FullName plus argument names, otherwise FullName) and keeps reflection's '+' for nested types; FormatMethodLabelParts and the worker are unchanged. - A Cecil parity test compares the MethodBase label with the label built from the same method's metadata spelling for 23 parameter shapes, and two end-to-end tests repeat the skipped-writer-with-earlier-patch case with List<int> and int[,] writers; restoring ToString makes both fail with the default-value wording. - The pause-point domain test and the patcher label test expect the metadata spelling, outdated comments about the mismatch are updated, and the skill output reference says how Method spells parameter types.
Each group's signature-change gate covers only its own entries, so a compiled caller in another assembly was always named in the warning, even when this run (in an earlier or a later group) or an earlier run had patched it and it no longer runs the compiled body that calls the old signature. Groups run one assembly at a time, so no check at gate time can see what a later group patches. - The gate now records the uncovered call-site hits in structured form in a run-scoped HotReloadRunStaleSignatureWarnings instead of formatting text. BuildResult drops every caller whose (assembly, label) is active when the run ends, before de-duplicating the display by wire key, and keeps the warning text unchanged. Reading the state at the end also keeps a caller that a later group peeled back to its compiled body. - HotReloadActivePatchInfo carries the declaring assembly name, so a method with the same label in another assembly is not taken for the caller. - Known limits stay on the safe or documented side: a patched body that still calls the removed method is not detected (shims are not scanned), and callers inside lambdas or local functions keep compiler-generated names and stay listed. The return-type gate is unchanged and now pinned: a caller in another assembly still gates the change even when patched. Tests: 14 unit tests for the run-scoped filter, a coverage test that the collection keeps one hit per assembly-qualified caller, and 7 end-to-end cases (caller patched in an earlier or later group, host-only rerun with an active or unpatched caller, caller peeled later in the same run, active caller still calling the removed method, return-type change stays gated). Turning the filter off, ignoring the assembly, de-duplicating before filtering, or de-duplicating the collection by wire key each fails the expected tests.
The hot reload docs said a return-type change applies once this or an earlier reload patched every compiled caller. A caller in another assembly still gates the change even after being patched, because its patch is compiled against the compiled assembly, where the old signature still exists; an end-to-end test now pins that. The SKILL.md summary and the gate paragraph now limit the "already patched" case to the same assembly. The paragraph on the warning for renamed, re-parameterized, or deleted methods now says a caller whose patch is active when the reload ends is left out, and names the limits: what the patched body calls is not checked, a copy the JIT inlined or a delegate created before the patch can still reach the old method, and a call inside a lambda or local function stays listed under its compiler-generated name. SKILL.md stays at 7,996 of 8,000 bytes. The generated copies under .claude and .agents are regenerated and match the source byte for byte.
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: hatayama/unity-cli-loop/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (17)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe signature gate now retains stale call-site records for run-level processing. At the end of a reload, warning generation filters callers against active patches, using assembly and method labels. Documentation and tests describe and check the gate and warning behavior. ChangesStale signature call-site capture
Run-level warning filtering
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Possibly related PRs
Merge Risk: ⚪ Minimal · up to Hot reload's stale-signature warning now omits callers already patched, including callers in other assemblies, and still gates return-type changes. The change is well covered by unit and end-to-end tests, and no merge-blocking risk was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change affects warning selection, not permission to patch code. Assembly-qualified caller matching and return-type compatibility checks are preserved. Behavior during overlapping or interrupted reloads is not fully established, so the assessment retains some uncertainty. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 41.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 86 functions across 21 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Base automatically changed from
fix/hot-reload-generic-parameter-labels
to
main
September 29, 2026 23:51
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.
Summary
User Impact
Changes
This pull request is stacked on #3031. It matches callers by method label, and #3031 changes how labels spell parameter types.
Verification
sync-tool-docs --checkpass.Closes #2896