fix: Hot reload warnings and pause points now recognize methods with generic or multidimensional-array parameters - #3031
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.
|
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 (2)
📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe Editor now formats resolved method parameter types using recursive .NET metadata-style spelling. Tests cover label parity and skipped-writer warning attribution for constructed generics and multidimensional arrays. Output documentation describes the parameter spelling. ChangesMethod Label Consistency
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The change aligns method labels and improves warning attribution for complex parameter types. No merge-blocking issue was identified; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change corrects method-name matching without changing which methods are selected for patching or reversion. Risk is low: externally visible method strings change, and runtime behavior and downstream client compatibility have not been fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 51 functions across 8 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 |
1 participant
Summary
List<int>) or multidimensional arrays (such asint[,]), so warnings, pause points, and stale or superseded lookups about such methods are accurate.User Impact
List<int>orint[,]parameter, the Editor spelled the method name differently from the transform worker (List`1[System.Int32]versusList`1<System.Int32>), so the two names never matched. As a result:uloop compile, even when an earlier hot reload of the writer was still active and assigning it;Methodfield in hot reload output now spells these parameter types asSystem.Collections.Generic.List`1<System.Int32>andSystem.Int32[0...,0...].Changes
<>, multidimensional arrays with0...bounds, and nested types with+. Labels built from worker rows are unchanged.Methodspells parameter types.Verification
List<int>andint[,]writers; temporarily restoring the old spelling makes both fail with the default-value wording.uloop run-tests: the parity, patcher, and domain classes (88 tests), six label-related classes (61 tests), and 35 orchestrator end-to-end tests pass.check-skill-size,sync-tool-docs --check, file-length, and complexity checks pass.Closes #2903