[XAML] XIHR: remove the now-dead __version field and StableContentHash - #36912
Conversation
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
3b72408 to
92b8f4d
Compare
da50ac9 to
9626ba6
Compare
92b8f4d to
fe6f656
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36912Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36912" |
There was a problem hiding this comment.
Pull request overview
This PR removes the generated XIHR __version field and the now-unused GeneratorHelpers.StableContentHash helper, and updates XIHR source-gen tests/docs accordingly. The goal is to simplify the incremental hot reload generator by eliminating write-only identity stamping that is no longer read after the diagnostics/classification changes introduced in #36833.
Changes:
- Stop emitting/stamping
__versionin generatedInitializeComponent()output and remove theStableContentHashhelper. - Update XIHR generator/writer comments to reflect the current “empty vs non-empty UpdateComponent body” behavior.
- Update SourceGen unit tests to drop
__versionassertions and refocus on registration/tracking + reverse-transition behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/tests/SourceGen.UnitTests/XamlIncrementalHotReloadE2ETests.cs | Removes __version-identity assertions; refocuses the revert test on reverse-transition UC behavior. |
| src/Controls/tests/SourceGen.UnitTests/XamlIncrementalHotReloadCSharpExpressionTests.cs | Updates the test to validate registration emission rather than __version field emission. |
| src/Controls/tests/SourceGen.UnitTests/InitializeComponent/IncrementalHotReloadICTests.cs | Removes __version-specific tests/assertions and keeps coverage focused on IHR artifacts. |
| src/Controls/src/SourceGen/XamlHotReloadState.cs | Updates comments to remove __version-related wording while keeping internal version bookkeeping. |
| src/Controls/src/SourceGen/XamlGenerator.cs | Updates comments around IHR state seeding/diff timing to reflect current behavior. |
| src/Controls/src/SourceGen/UpdateComponentCodeWriter.cs | Updates remarks/docs for the new UC shape; (also contains a doc-comment formatting issue noted in review). |
| src/Controls/src/SourceGen/ProjectItem.cs | Updates EnableIncrementalHotReload docs to describe Track/Register behavior (no __version). |
| src/Controls/src/SourceGen/InitializeComponentCodeWriter.cs | Removes __version field emission and stamping; keeps registry + Track emission. |
| src/Controls/src/SourceGen/GeneratorHelpers.cs | Removes StableContentHash implementation and related documentation. |
| /// <summary> | ||
| /// Assembles a complete <c>UpdateComponent()</c> source file from accumulated patch bodies. | ||
| /// Each patch body becomes an <c>if (__version == N) { ... }</c> block inside the single method. | ||
| /// </summary> | ||
| /// <summary> |
| Assert.DoesNotContain("__version", text, StringComparison.Ordinal); | ||
| Assert.DoesNotContain("XamlComponentRegistry.Register", text, StringComparison.Ordinal); | ||
| } |
| @@ -55,68 +54,7 @@ | |||
| Assert.DoesNotContain("__version", text, StringComparison.Ordinal); | |||
| Assert.DoesNotContain("XamlComponentRegistry.Register", text, StringComparison.Ordinal); | |||
…h (Tomas review) Follow-up to #36833 addressing Tomas Matousek's review comment on GeneratorHelpers (the content-hash algorithm choice). Once diagnostics classify a delta by the empty-vs-non-empty UpdateComponent() body (not by a stamped identity), nothing reads the generated __version field anymore — it became write-only. So rather than pick a different hash (xxHash128 etc.), remove the field and its hash entirely: - InitializeComponent no longer emits `private int __version` nor stamps it; drop the `__version = 0` reset on the legacy ResourceProvider2 fallback path. - Remove GeneratorHelpers.StableContentHash (its only consumer was the __version stamp). - Update the obsolete __version-era doc comments across the SourceGen writers. - Tests: drop the three __version-field assertions (Enabled_VersionField*/SetAtEndOfMethod), the CSharpExpression version-field assertion, and the __version parts of the determinism test; repurpose them to the surviving registration / reverse-transition behavior. No behavior change: __version was unused. XamlHotReloadState's internal Version counter (bookkeeping only, never emitted) is untouched — the broader static-cache concern Tomas raised is tracked separately. SourceGen.UnitTests: 460 green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fe6f656 to
84e5dc0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Controls/src/SourceGen/UpdateComponentCodeWriter.cs:149
- The XML doc comment here has a duplicated
<summary>tag (/// <summary>twice). This produces malformed XML documentation (and can trigger CS1570/CS1584 warnings, which may break the build under warnings-as-errors). Remove the extra<summary>line so the comment has a single<summary>block.
/// <summary>
/// <summary>
/// Generates the <c>UpdateComponent()</c> method body from a single baseline→current patch.
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@StephaneDelcroix — new AI review results are available based on this last commit:
84e5dc0.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ❌ FAILED
Platform: WINDOWS · Base: net11.0 · Merge base: c6dc61fa
🩺 Test does not reproduce the bug — ran the same in both states (PASS without fix, PASS with fix). The repro test is not exercising the issue. Strengthen the test before reviewing the fix.
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🧪 IncrementalHotReloadICTests IncrementalHotReloadICTests |
❌ PASS — 337s | ✅ PASS — 37s |
🧪 XamlIncrementalHotReloadCSharpExpressionTests XamlIncrementalHotReloadCSharpExpressionTests |
❌ PASS — 88s | ✅ PASS — 30s |
🧪 XamlIncrementalHotReloadE2ETests XamlIncrementalHotReloadE2ETests |
❌ PASS — 87s | ✅ PASS — 29s |
🔴 Without fix — 🧪 IncrementalHotReloadICTests: PASS ❌ · 337s
(no coded error found; showing last 1200 chars)
5.101)
[xUnit.net 00:00:00.12] Discovering: Microsoft.Maui.Controls.SourceGen.UnitTests
[xUnit.net 00:00:00.25] Discovered: Microsoft.Maui.Controls.SourceGen.UnitTests
[xUnit.net 00:00:00.26] Starting: Microsoft.Maui.Controls.SourceGen.UnitTests
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Enabled_RootNodeNotRegistered_ChildrenAreRegistered [2 s]
[xUnit.net 00:00:02.84] Finished: Microsoft.Maui.Controls.SourceGen.UnitTests
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Enabled_MultipleChildren_MultipleRegisterCalls [77 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Enabled_RegisterCallForChildNode [21 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Enabled_RegisterCallContainsNodeId [21 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Default_NoVersionField [244 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Enabled_DiagnosticsAndIHR_BothFeaturesPresent [22 ms]
Test Run Successful.
Total tests: 6
Passed: 6
Total time: 3.4700 Seconds
🟢 With fix — 🧪 IncrementalHotReloadICTests: PASS ✅ · 37s
(no coded error found; showing last 1200 chars)
5.101)
[xUnit.net 00:00:00.12] Discovering: Microsoft.Maui.Controls.SourceGen.UnitTests
[xUnit.net 00:00:00.29] Discovered: Microsoft.Maui.Controls.SourceGen.UnitTests
[xUnit.net 00:00:00.30] Starting: Microsoft.Maui.Controls.SourceGen.UnitTests
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Enabled_RootNodeNotRegistered_ChildrenAreRegistered [2 s]
[xUnit.net 00:00:02.89] Finished: Microsoft.Maui.Controls.SourceGen.UnitTests
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Enabled_MultipleChildren_MultipleRegisterCalls [76 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Enabled_RegisterCallForChildNode [23 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Enabled_RegisterCallContainsNodeId [19 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Default_NoVersionField [249 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.IncrementalHotReloadICTests.Enabled_DiagnosticsAndIHR_BothFeaturesPresent [21 ms]
Test Run Successful.
Total tests: 6
Passed: 6
Total time: 3.7471 Seconds
🔴 Without fix — 🧪 XamlIncrementalHotReloadCSharpExpressionTests: PASS ❌ · 88s
(no coded error found; showing last 1200 chars)
aui.Controls.SourceGen.UnitTests
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.CSharpExpression_NullCoalescingChange_GeneratesUC [59 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.Binding_PathChange_GeneratesSetBinding [71 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.DynamicResource_Change_GeneratesSetDynamicResource [76 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.CSharpExpression_MethodCallChange_GeneratesUC [77 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.CSharpExpression_TernaryChange_GeneratesUC [79 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.CSharpExpression_ConcatenationChange_GeneratesUC [73 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.CSharpExpression_OperatorAliasChange_GeneratesUC [83 ms]
Test Run Successful.
Total tests: 17
Passed: 17
Total time: 4.7291 Seconds
🟢 With fix — 🧪 XamlIncrementalHotReloadCSharpExpressionTests: PASS ✅ · 30s
(no coded error found; showing last 1200 chars)
aui.Controls.SourceGen.UnitTests
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.CSharpExpression_NullCoalescingChange_GeneratesUC [57 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.Binding_PathChange_GeneratesSetBinding [80 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.DynamicResource_Change_GeneratesSetDynamicResource [50 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.CSharpExpression_MethodCallChange_GeneratesUC [89 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.CSharpExpression_TernaryChange_GeneratesUC [78 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.CSharpExpression_ConcatenationChange_GeneratesUC [76 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadCSharpExpressionTests.CSharpExpression_OperatorAliasChange_GeneratesUC [66 ms]
Test Run Successful.
Total tests: 17
Passed: 17
Total time: 4.6937 Seconds
🔴 Without fix — 🧪 XamlIncrementalHotReloadE2ETests: PASS ❌ · 87s
(no coded error found; showing last 1200 chars)
_InitializeComponent_IsByteIdentical_ToInitialGeneration(bodyV1: "<Label Text=\"Hi\" />", bodyV2: "<Label Text=\"Bye\" />") [27 ms]
Skipped Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.ResourceAdded_AppliedViaHotReload [1 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.ResourceAdded_CompilesCleanly [407 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.UpdateComponent_OnInheritedXamlClass_CompilesWithoutHidingWarning [104 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.RootContentReplaced_CompilesCleanly [88 ms]
Skipped Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.ResourceRemoved_AppliedViaHotReload [1 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.ReverseEdit_UC_RestoresBaselineValue_WithoutRetainingIntermediate [21 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.ResourceRemoved_CompilesCleanly [157 ms]
Test Run Successful.
Total tests: 18
Passed: 11
Skipped: 7
Total time: 4.9210 Seconds
🟢 With fix — 🧪 XamlIncrementalHotReloadE2ETests: PASS ✅ · 29s
(no coded error found; showing last 1200 chars)
_InitializeComponent_IsByteIdentical_ToInitialGeneration(bodyV1: "<Label Text=\"Hi\" />", bodyV2: "<Label Text=\"Bye\" />") [28 ms]
Skipped Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.ResourceAdded_AppliedViaHotReload [1 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.ResourceAdded_CompilesCleanly [417 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.UpdateComponent_OnInheritedXamlClass_CompilesWithoutHidingWarning [124 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.RootContentReplaced_CompilesCleanly [88 ms]
Skipped Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.ResourceRemoved_AppliedViaHotReload [1 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.ReverseEdit_UC_RestoresBaselineValue_WithoutRetainingIntermediate [21 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.XamlIncrementalHotReloadE2ETests.ResourceRemoved_CompilesCleanly [187 ms]
Test Run Successful.
Total tests: 18
Passed: 11
Skipped: 7
Total time: 4.7456 Seconds
⚠️ Failure Details
- ❌ IncrementalHotReloadICTests PASSED without fix (should fail) — tests don't catch the bug
- ❌ XamlIncrementalHotReloadCSharpExpressionTests PASSED without fix (should fail) — tests don't catch the bug
- ❌ XamlIncrementalHotReloadE2ETests PASSED without fix (should fail) — tests don't catch the bug
📁 Fix files reverted (6 files)
src/Controls/src/SourceGen/GeneratorHelpers.cssrc/Controls/src/SourceGen/InitializeComponentCodeWriter.cssrc/Controls/src/SourceGen/ProjectItem.cssrc/Controls/src/SourceGen/UpdateComponentCodeWriter.cssrc/Controls/src/SourceGen/XamlGenerator.cssrc/Controls/src/SourceGen/XamlHotReloadState.cs
📱 UI Tests — Button,Label,Layout
Detected UI test categories: Button,Label,Layout
✅ Deep UI tests — 344 passed, 0 failed across 3 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Button |
69/70 ✓ | — |
Label |
91/94 ✓ | — |
Layout |
184/188 ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
📋 Pre-Flight — Context & Validation
Issue: N/A - Follow-up to #36833 / review feedback on XIHR content identity
PR: #36912 - [XAML] XIHR: remove the now-dead __version field and StableContentHash
Platforms Affected: all (source generator behavior; tests requested on windows)
Files Changed: 6 implementation, 3 test
Key Findings
- The PR is a single squashed commit removing the generated private
__versionfield,StableContentHash, and version stamping from the XIHR source generator path. - The surviving XAML-change signal is the always-emitted
UpdateComponent()method with empty vs non-empty body, established by the stacked #36833 behavior. - Targeted tests are SourceGen unit tests; no UI/device categories are directly impacted.
- Public GitHub metadata showed prior inline comments about malformed XML docs and test coverage; no prior ❌ error-level review finding was found.
- Gate result supplied by the caller already failed and was not rerun.
Code Review Summary
Verdict: NEEDS_DISCUSSION
Confidence: low
Errors: 0 | Warnings: 1 | Suggestions: 0
Key code review findings:
- ⚠
src/Controls/src/SourceGen/UpdateComponentCodeWriter.cs:148has consecutive/// <summary>tags, leaving malformed XML docs after removing obsolete__versionwording.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #36912 | Remove the private generated __version field and StableContentHash; rely on always-present empty/non-empty UpdateComponent() as the XIHR signal |
❌ FAILED (Gate supplied by caller) | GeneratorHelpers.cs, InitializeComponentCodeWriter.cs, ProjectItem.cs, UpdateComponentCodeWriter.cs, XamlGenerator.cs, XamlHotReloadState.cs, SourceGen tests |
Original PR; code review found no functional error but did find a malformed XML doc warning |
🔬 Code Review — Deep Analysis
Code Review — PR #36912
Independent Assessment
What this changes: Removes XIHR-generated __version field/stamping and StableContentHash, relying on always-present empty/non-empty UpdateComponent() for XAML-change classification.
Inferred motivation: The version field is now write-only after the prior empty-UC diagnostics change.
Reconciliation with PR Narrative
Author claims: __version is unused; no behavior change; tests updated.
Agreement/disagreement: Matches the code. Search found no remaining StableContentHash callers; remaining __version references are comments/tests outside this PR’s touched lines.
Prior Review Reconciliation
No prior ❌ Error findings found. Prior Copilot comments flagged the duplicated XML doc summary and test-coverage suggestions; the doc issue still applies.
Blast Radius Assessment
- Runs for all instances: only XIHR-enabled generated XAML types.
- Startup impact: no app startup path; source-generator/generated-code path only.
- Static/shared state: existing
XamlHotReloadStateremains; this PR does not add new shared state.
CI Status
- Required-check result:
gh pr checks --requiredunavailable (gh auth loginrequired). Public check-runs showmaui-prfailed. - Classification: undetermined. Public timeline shows RunOniOS failure and Blazor cancellation; AzDO logs were not anonymously readable.
- Action taken: invoked
azdo-build-investigator; confidence capped low and no LGTM.
External Output Contract
| Consumer token/pattern | Producer location | Producer emission condition | Consumer assumption | Ordinary negative case | Downstream effect |
|---|---|---|---|---|---|
| N/A | N/A | No changed regex/string classifier for external tool output | N/A | N/A | N/A |
Findings
⚠️ Warning — Malformed XML documentation remains
src/Controls/src/SourceGen/UpdateComponentCodeWriter.cs:148
The PR deletes the obsolete first summary’s body/closing tag but leaves its opening /// <summary> immediately before the new /// <summary>, producing nested/malformed XML docs.
❌ Error
None found.
💡 Suggestion
None.
Failure-Mode Probing
- Fresh XIHR instance after removing
__version: still registers nodes and callsTrack(this). - Live hot reload after patch:
UpdateComponent()remains always emitted; non-empty body remains the change signal. - Revert/no-op edit: empty body path still refreshes cached state without emitting version-dependent code.
- External consumers of
__version: no repository readers found; generated field was private.
Verdict: NEEDS_DISCUSSION
Confidence: low
Summary: The code change appears conceptually sound, with one doc-comment warning. CI is currently red/undetermined, so this cannot be LGTM until the failing maui-pr result is classified or rerun cleanly.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #36912 | Remove generated private __version and StableContentHash; use always-emitted empty/non-empty UpdateComponent() body as the XIHR signal | ❌ FAILED (Gate supplied by caller: tests pass without fix) | 6 implementation, 3 test | Code-review found no functional error; one malformed XML doc warning remains |
| 1 | try-fix | Generated static hot-reload marker method plus runtime marker lookup | ✅ PASS | UpdateComponentCodeWriter.cs; XamlIncrementalHotReloadHandler.cs | Passed targeted tests but was broader than PR and added generated/runtime contract surface. |
| 2 | try-fix | Runtime UpdateComponent IL fingerprint cache | ✅ PASS | XamlIncrementalHotReloadHandler.cs | Passed targeted tests but added runtime state/lifetime/threading risk and still relied on IL inspection. |
| 3 | try-fix | Method metadata attribute on UpdateComponent | ❌ FAIL | UpdateComponentCodeWriter.cs; XamlIncrementalHotReloadHandler.cs; XamlHotReloadUpdateAttribute.cs | Failed build: public attribute triggered RS0016 PublicAPI analyzer errors. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Generated static marker method plus runtime marker lookup |
| maui-expert-reviewer | 2 | Yes | Runtime UpdateComponent() IL fingerprint cache after candidate 1 added too much contract surface |
| maui-expert-reviewer | 3 | Yes | Method metadata attribute after candidate 2 added runtime state |
| maui-expert-reviewer | 4 | No | Remaining alternatives either duplicate PR #36912 or add generated/runtime/public API contract surface |
Exhausted: Yes
Selected Fix: PR #36912 — Candidate 1 and 2 passed the targeted tests but are not demonstrably better because they add broader runtime/generated contracts or state. Candidate 3 failed public API analyzers. The main reviewable gap is test quality: the supplied gate already showed all repro tests pass without the fix, so no candidate can be proven by the current tests.
Gate / Baseline Limitation
The gate was not rerun. Existing gate artifacts report Gate ❌ FAILED because the tests pass both without and with the PR fix. EstablishBrokenBaseline.ps1 could not be reused during this loop because the shared worktree contains unrelated dirty files; to avoid destructive cleanup, candidates were tested as overlays on the PR state and the blocker is recorded in each try-fix-N/content.md.
📝 Recommended PR Title & Description
Assessment: ✏️ Recommend updating — the current title and description accurately describe the submitted PR, but the selected winning candidate adds an explicit generated/runtime XIHR marker contract, so the metadata would be stale if the winning fix is applied.
Recommended title
[XAML] XIHR: remove __version and mark generated XAML-change deltas
Recommended description
### Description
Follow-up to #36833 addressing @tmat's review comment on `GeneratorHelpers.StableContentHash` (that `xxHash128` would be a better hash for the `__version` content identity).
Once #36833 moved diagnostics to classify a delta by the XAML-change signal instead of a stamped identity, nothing needs the generated `__version` field anymore — it is write-only. So instead of swapping the hash algorithm, this removes the field and its hash entirely, then makes the XAML-change signal explicit with a generated marker method:
- `InitializeComponent` no longer emits `private int __version` nor stamps it; drops the `__version = 0` reset on the legacy `ResourceProvider2` fallback path.
- Removes `GeneratorHelpers.StableContentHash` (its only consumer was the `__version` stamp).
- `UpdateComponent` generation emits `internal static bool HasXamlHotReloadChange()` alongside the always-present `UpdateComponent()` method, returning `true` only when the generated body carries a XAML patch.
- `XamlIncrementalHotReloadHandler` prefers the generated marker when classifying updated types, falling back to the existing empty/non-empty `UpdateComponent()` IL-body check for compatibility.
- Updates the obsolete `__version`-era doc comments across the SourceGen writers.
- Tests: drops the three `__version`-field assertions and the `__version` parts of the determinism test, repurposing them to the surviving registration / reverse-transition behavior.
**No app behavior change intended** — `__version` was unused, and the marker represents the same XAML-change classification that previously came from the generated `UpdateComponent()` body shape.
### Scope note
`@tmat`'s second comment (the mutable static `XamlHotReloadState` cache being brittle when Roslyn runs source generators on multiple/throwaway compilations) is a **separate, larger architectural question** and is **not** addressed here — it's tracked for a dedicated follow-up. This PR is limited to removing the dead field/hash and making the XIHR change signal explicit.
### Testing
`SourceGen.UnitTests`: targeted XIHR SourceGen tests passed for the selected candidate on Windows. The automated gate still reported that the repro tests pass without the fix, so the tests should be strengthened before treating them as proof of the regression.
### Note on base
Stacked on #36833 (the `__version` field is only dead *after* that PR's empty-UC diagnostics). Targets `feature/xihr-deterministic-versioning`; GitHub will retarget to `net11.0` once #36833 merges.
🏁 Report — Final Recommendation
Comparative Report — PR #36912
Ranking
| Rank | Candidate | Regression/test result | Assessment |
|---|---|---|---|
| 1 | try-fix-1 |
PASS in targeted SourceGen command | Best eligible candidate under the required ranking rule. It passes the targeted candidate tests and avoids try-fix-2's runtime fingerprint cache/lifetime state and try-fix-3's public API failure. It does add a generated/runtime marker contract and reflection lookup, so it is broader than the raw PR, but it is the strongest candidate among those whose candidate tests passed. |
| 2 | try-fix-2 |
PASS in targeted SourceGen command | Also passes targeted tests, but it adds runtime mutable state keyed by Type, lock contention, fingerprint lifetime concerns, and a broad catch around IL-body inspection. This is a larger blast radius than try-fix-1. |
| 3 | pr-plus-reviewer |
FAILED supplied gate / not proven | Raw PR plus the expert reviewer's XML-doc cleanup. Functionally the cleanest implementation, but the supplied gate still failed because the tests pass without the fix, so it must rank below passing candidates. |
| 4 | pr |
FAILED supplied gate / not proven | Minimal and architecturally preferable if the gate were meaningful: removes dead __version and StableContentHash and relies on the #36833 empty/non-empty UpdateComponent() body contract. However, the supplied gate failed, and the PR also leaves a malformed XML doc comment. |
| 5 | try-fix-3 |
FAIL | Fails build/analyzers because it introduces a public XamlHotReloadUpdateAttribute without PublicAPI entries, and the public API addition is disproportionate for an internal XIHR signal. |
Candidate Comparison
pr
The submitted PR is the smallest functional change: once #36833 made the runtime and diagnostics classify XIHR deltas by the UpdateComponent() body, the generated __version field became write-only. Removing the field and GeneratorHelpers.StableContentHash is conceptually correct and keeps generated output simpler. The drawbacks are evidence-related: the supplied gate failed because tests pass without the fix, and the diff leaves a malformed XML documentation comment in UpdateComponentCodeWriter.
pr-plus-reviewer
This candidate applies the expert reviewer's actionable feedback by deleting the duplicate /// <summary> opener in UpdateComponentCodeWriter.GenerateUpdateComponent(...). That makes it strictly better than pr on code quality with no behavior change. It still inherits the same failed supplied gate result, so under the instruction that candidates failing regression tests rank lower than passing candidates, it cannot win over try-fix-1 or try-fix-2.
try-fix-1
This candidate adds an explicit generated internal static bool HasXamlHotReloadChange() marker and makes the runtime handler prefer that marker over IL-body-size classification. It passed the targeted SourceGen command. Compared with the PR, it broadens the generated/runtime contract and adds reflection invocation, but it is less risky than a runtime fingerprint cache and avoids public API surface.
try-fix-2
This candidate compares current UpdateComponent() IL bytes against a runtime cache seeded from Track(instance). It passed targeted tests, but the design adds static mutable state, locking, type-keyed lifetime concerns, and another broad fallback path around IL inspection. Those risks make it weaker than try-fix-1 despite passing the same targeted command.
try-fix-3
This candidate annotates UpdateComponent() with a framework attribute. It failed the build with RS0016 PublicAPI analyzer errors and would add public API for an internal source-generator/runtime handshake. It ranks below every non-build-failing candidate.
Winner
try-fix-1 wins because it is the highest-ranked candidate with passing candidate tests and fewer architectural risks than try-fix-2. The raw PR / pr-plus-reviewer path remains architecturally cleaner, but the supplied gate failure means those candidates cannot outrank passing candidates under the user's explicit ranking rule.
🧭 Next Steps — alternative fix proposed (try-fix-1)
Automated review — alternative fix proposed
The expert-reviewer evaluation compared the PR fix against automatically generated candidates and selected try-fix-1 as the strongest fix.
Why: try-fix-1 wins because it is the highest-ranked candidate with passing candidate tests while avoiding try-fix-2's runtime fingerprint cache/state and try-fix-3's public API analyzer failure. The raw PR/pr-plus-reviewer path is cleaner architecturally, but the supplied gate failed, so those candidates must rank below passing candidates.
Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.
Candidate diff (try-fix-1)
diff --git a/src/Controls/src/SourceGen/UpdateComponentCodeWriter.cs b/src/Controls/src/SourceGen/UpdateComponentCodeWriter.cs
index 3891139cc6..4d33a363ff 100644
--- a/src/Controls/src/SourceGen/UpdateComponentCodeWriter.cs
+++ b/src/Controls/src/SourceGen/UpdateComponentCodeWriter.cs
@@ -178,6 +178,8 @@ static class UpdateComponentCodeWriter
using (PrePost.NewBlock(codeWriter))
{
codeWriter.WriteLine($"[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]");
+ codeWriter.WriteLine($"internal static bool HasXamlHotReloadChange() => {(!string.IsNullOrWhiteSpace(patchBody)).ToString().ToLowerInvariant()};");
+ codeWriter.WriteLine();
codeWriter.WriteLine($"internal void UpdateComponent()");
// The method is ALWAYS emitted (member stability / no EnC churn), but its BODY is empty when
diff --git a/src/Controls/src/Xaml/HotReload/XamlIncrementalHotReloadHandler.cs b/src/Controls/src/Xaml/HotReload/XamlIncrementalHotReloadHandler.cs
index 6b23c6f5fa..667e822e62 100644
--- a/src/Controls/src/Xaml/HotReload/XamlIncrementalHotReloadHandler.cs
+++ b/src/Controls/src/Xaml/HotReload/XamlIncrementalHotReloadHandler.cs
@@ -98,7 +98,7 @@ public static class XamlIncrementalHotReloadHandler
if (ucMethod is null)
continue; // not an incremental-XAML type at all
- if (IsEmptyUpdateComponent(ucMethod))
+ if (!HasXamlHotReloadChange(type, ucMethod))
continue; // XAML unchanged in this delta (e.g. a pure C# edit) — not a XAML change
handledTypes.Add(type);
@@ -174,6 +174,25 @@ public static class XamlIncrementalHotReloadHandler
// trim/AOT warning). This is safe: XIHR is a dev-time (Hot Reload) feature gated by
// RuntimeFeature.IsIncrementalHotReloadEnabled, which is off for Release/publish, so this method is
// never reached under trimming/AOT — the "trimming may change method bodies" caveat cannot apply.
+ [System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessage("Trimming", "IL2026:RequiresUnreferencedCode",
+ Justification = "XIHR is a dev-time feature disabled under trimming/AOT (Release); IsEmptyUpdateComponent is never reached there.")]
+ static bool HasXamlHotReloadChange(Type type, MethodInfo ucMethod)
+ {
+#pragma warning disable IL2070, IL2075
+ var markerMethod = type.GetMethod(
+ "HasXamlHotReloadChange",
+ BindingFlags.NonPublic | BindingFlags.Static,
+ binder: null,
+ types: Type.EmptyTypes,
+ modifiers: null);
+#pragma warning restore IL2070, IL2075
+
+ if (markerMethod?.ReturnType == typeof(bool))
+ return (bool)markerMethod.Invoke(null, null)!;
+
+ return !IsEmptyUpdateComponent(ucMethod);
+ }
+
[System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessage("Trimming", "IL2026:RequiresUnreferencedCode",
Justification = "XIHR is a dev-time feature disabled under trimming/AOT (Release); IsEmptyUpdateComponent is never reached there.")]
static bool IsEmptyUpdateComponent(MethodInfo ucMethod)
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions?
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: faeea9ac-6b99-49b8-b73e-2b85de04801e
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Controls/src/SourceGen/UpdateComponentCodeWriter.cs:149
- There is a duplicated XML doc tag here (
/// <summary>appears twice in a row), which makes the doc comment malformed and can lead to XML documentation warnings/errors.
/// <summary>
/// <summary>
/// Generates the <c>UpdateComponent()</c> method body from a single baseline→current patch.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description
Follow-up to #36833 addressing @tmat's review comment on
GeneratorHelpers.StableContentHash(thatxxHash128would be a better hash for the__versioncontent identity).Once #36833 moved diagnostics to classify a delta by the empty-vs-non-empty
UpdateComponent()body (rather than a stamped identity), nothing reads the generated__versionfield anymore — it is write-only. So instead of swapping the hash algorithm, this removes the field and its hash entirely:InitializeComponentno longer emitsprivate int __versionnor stamps it; drops the__version = 0reset on the legacyResourceProvider2fallback path.GeneratorHelpers.StableContentHash(its only consumer was the__versionstamp).__version-era doc comments across the SourceGen writers.__version-field assertions and the__versionparts of the determinism test, repurposing them to the surviving registration / reverse-transition behavior.No behavior change —
__versionwas unused.Scope note
@tmat's second comment (the mutable staticXamlHotReloadStatecache being brittle when Roslyn runs source generators on multiple/throwaway compilations) is a separate, larger architectural question and is not addressed here — it's tracked for a dedicated follow-up. This PR is limited to removing the dead field/hash.Testing
SourceGen.UnitTests: 460 green.Note on base
Stacked on #36833 (the
__versionfield is only dead after that PR's empty-UC diagnostics). Targetsfeature/xihr-deterministic-versioning; GitHub will retarget tonet11.0once #36833 merges.