Repository navigation
Fix CS1612 for struct sub-property paths in C# expression source gen - #35006
Merged
Merged
Conversation
C# expressions like {Margin.Top} where an intermediate property returns
a value type (struct) previously failed with CS1612 because the source
gen emitted __source.Margin.Top = __value as the setter.
Now generates copy-modify-writeback when the intermediate property is
settable:
var __t0 = __source.Margin; __t0.Top = __value; __source.Margin = __t0;
When the intermediate property has no setter and the target is two-way,
emits null setter with MAUIX2017 warning.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35006Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35006" |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes C# expression source generation for bindings that include struct (value-type) intermediates, avoiding CS1612 by emitting a copy–modify–writeback setter when possible and introducing a dedicated warning when it isn’t.
Changes:
- Add expression “write kind” analysis and generate struct copy–modify–writeback setters for settable struct-intermediate property chains.
- Introduce diagnostic MAUIX2017 (
StructIntermediateNotSettable) and register it in analyzer release tracking. - Add a SourceGen XAML/unit-test scenario exercising a struct sub-property path (
{Margin.Top}).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Controls/src/SourceGen/SetPropertyHelpers.cs |
Adds writeability analysis + emits struct copy–modify–writeback setters; updates diagnostics emitted when setters can’t be generated. |
src/Controls/src/SourceGen/Descriptors.cs |
Defines new warning descriptor MAUIX2017 for unsettable struct intermediates in two-way scenarios. |
src/Controls/src/SourceGen/AnalyzerReleases.Unshipped.md |
Registers MAUIX2017 in unshipped analyzer rules list. |
src/Controls/tests/Xaml.UnitTests/CSharpExpressions.sgen.xaml |
Adds XAML scenario binding to Margin.Top. |
src/Controls/tests/Xaml.UnitTests/CSharpExpressions.sgen.xaml.cs |
Adds Margin to the VM and a test validating the struct sub-property binding updates on INPC. |
Comment on lines
984
to
985
| // Walk the dot-separated property chain (also handle ?. null-conditional access) | ||
| var parts = expr.Replace("?.", ".").Split('.'); |
There was a problem hiding this comment.
GetExpressionWriteInfo normalizes null-conditional access by doing expr.Replace("?.", "."). When ExpressionWriteKind.StructCopyWriteback is selected, WriteStructCopyWritebackSetter will then emit plain . member access and can throw NullReferenceException for expressions that relied on ?. (e.g., User?.Point.X). Consider either preserving ?. semantics (emit appropriate null-guards) or treating any chain containing ?. as not writable (return NotWritable) to avoid generating an unsafe setter.
Suggested change
| // Walk the dot-separated property chain (also handle ?. null-conditional access) | |
| var parts = expr.Replace("?.", ".").Split('.'); | |
| // Null-conditional access cannot be safely written back without preserving short-circuit semantics. | |
| // Reject such expressions rather than normalizing `?.` into `.` and generating unsafe setters. | |
| if (expr.Contains("?.", StringComparison.Ordinal)) | |
| return notWritable; | |
| // Walk the dot-separated property chain | |
| var parts = expr.Split('.'); |
| /// For example, "Name" is writable if Name has a public set accessor, but "ReadOnlyProp" is not | ||
| /// if it only has a getter (expression-bodied or getter-only property). | ||
| /// Returns false for complex expressions that are not simple property chains. | ||
| /// <summary> |
There was a problem hiding this comment.
There’s an extra nested XML doc opener (/// <summary>) immediately before the ExpressionWriteKind docs. This will produce malformed XML documentation / warnings; remove the duplicate /// <summary> line so the comment block is well-formed.
Suggested change
| /// <summary> |
Comment on lines
+150
to
+151
| Source gen emits null setter (one-way) because Thickness is a value type, | ||
| avoiding CS1612 from `source.Margin.Top = value`. --> |
There was a problem hiding this comment.
The comment says source gen emits a null setter / one-way binding because Thickness is a value type. With the new struct copy-writeback support, {Margin.Top} is now a settable property chain (when intermediates are settable), so the generated setter should no longer be null. Please update the comment to reflect the new behavior (copy-modify-writeback) or avoid asserting anything about setter generation here.
Suggested change
| Source gen emits null setter (one-way) because Thickness is a value type, | |
| avoiding CS1612 from `source.Margin.Top = value`. --> | |
| For settable struct property chains such as Margin.Top, source gen uses | |
| copy-modify-writeback semantics rather than assigning through the struct directly. --> |
Comment on lines
+842
to
+845
| page.BindingContext = vm; | ||
|
|
||
| // {Margin.Top} creates a one-way binding (no setter, since Thickness is a struct) | ||
| Assert.Equal(47, page.thicknessSubGrid.HeightRequest); |
There was a problem hiding this comment.
The test name/comment claim {Margin.Top} creates a one-way binding with no setter because Thickness is a struct. After this change the generator can emit a copy-modify-writeback setter for struct intermediates, so “no setter because struct” is misleading. Consider renaming the test (and updating the comment) to focus on the scenario it actually validates (e.g., that struct sub-property paths don’t break source-gen / update on INPC).
This was referenced Apr 20, 2026
This comment has been minimized.
This comment has been minimized.
MauiBot
added
s/agent-review-incomplete
s/agent-reviewed
This comment has been minimized.
This comment has been minimized.
Contributor
|
/review -b feature/refactor-copilot-yml |
kubaflo
requested changes
May 25, 2026
kubaflo
left a comment
kubaflo
left a comment
Contributor
There was a problem hiding this comment.
Could you check the ai's suggestions?
Contributor
|
/review -b feature/enhanced-reviewer -p android |
Remove stray analyzer release entries, fix XML docs, and add coverage for copy-writeback and MAUIX2017 diagnostics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MauiBot
removed
the
s/agent-review-in-progress
github-actions
Bot
added
s/agent-ready-for-rerun
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
added
s/agent-changes-requested
MauiBot
reviewed
Jul 23, 2026
MauiBot
left a comment
MauiBot
left a comment
Collaborator
There was a problem hiding this comment.
AI Review Summary
@StephaneDelcroix — new AI review results are available based on this last commit:
e740d43. To request a fresh review after new comments or commits, comment/review rerun.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ✅ PASSED
Platform: ANDROID · Base: main · Merge base: 657fb516
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🧪 CSharpExpressionDiagnosticsTests CSharpExpressionDiagnosticsTests |
✅ FAIL — 37s | ✅ PASS — 24s |
📄 CSharpExpressions.sgen CSharpExpressions.sgen |
🛠️ BUILD ERROR | 🔍 NO MATCH |
📄 UserInfo UserInfo |
🛠️ BUILD ERROR | 🔍 NO MATCH |
🔴 Without fix — 🧪 CSharpExpressionDiagnosticsTests: FAIL ✅ · 37s
Error-relevant lines (filtered from the build log):
at Microsoft.Maui.Controls.SourceGen.UnitTests.CSharpExpressionDiagnosticsTests.GetOnlyStructIntermediate_OnTwoWayProperty_ReportsMAUIX2017AndOmitsSetter() in /_/src/Controls/tests/SourceGen.UnitTests/CSharpExpressionDiagnosticsTests.cs:line 547
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
🟢 With fix — 🧪 CSharpExpressionDiagnosticsTests: PASS ✅ · 24s
(no coded error found; showing last 1200 chars)
s.CSharpExpressionDiagnosticsTests.ExplicitDotPrefix_NoStaticTypeConflictWarning [29 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.CSharpExpressionDiagnosticsTests.CSharpExpression_WithoutPreviewFeatures_ReportsMAUIX2012 [16 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.CSharpExpressionDiagnosticsTests.BareIdentifier_AmbiguousBetweenMarkupAndProperty_ReportsMAUIX2007 [52 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.CSharpExpressionDiagnosticsTests.ExplicitThisPrefix_NoAmbiguousError [36 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.CSharpExpressionDiagnosticsTests.ComplexExpression_OnTwoWayProperty_EmitsInfoDiagnostic [28 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.CSharpExpressionDiagnosticsTests.SingleQuotedLiteral_MethodExpectsString_GeneratesStringLiteral [84 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.CSharpExpressionDiagnosticsTests.BareIdentifier_ExplicitExpressionSyntax_NoAmbiguityWarning [56 ms]
Passed Microsoft.Maui.Controls.SourceGen.UnitTests.CSharpExpressionDiagnosticsTests.ExplicitDotPrefix_NoAmbiguousError [63 ms]
Test Run Successful.
Total tests: 19
Passed: 19
Total time: 5.8065 Seconds
🔴 Without fix — 📄 CSharpExpressions.sgen: 🛠️ BUILD ERROR · 56s
Error-relevant lines (filtered from the build log):
/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Generated/Microsoft.Maui.Controls.SourceGen/Microsoft.Maui.Controls.SourceGen.XamlGenerator/CSharpExpressions.sgen.xaml.xsg.cs(861,28): error CS1612: Cannot modify the return value of 'SimpleViewModel.Margin' because it is not a variable [/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Controls.Xaml.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Generated/Microsoft.Maui.Controls.SourceGen/Microsoft.Maui.Controls.SourceGen.XamlGenerator/CSharpExpressions.sgen.xaml.xsg.cs(872,28): error CS1612: Cannot modify the return value of 'SimpleViewModel.Margin' because it is not a variable [/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Controls.Xaml.UnitTests.csproj]
🟢 With fix — 📄 CSharpExpressions.sgen: 🔍 NO MATCH · 62s
(no coded error found; showing last 1200 chars)
ps://learn.microsoft.com/dotnet/maui/fundamentals/data-binding/compiled-bindings for more information. [/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Controls.Xaml.UnitTests.csproj]
Controls.Xaml.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Xaml.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Xaml.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Xaml.UnitTests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.21] Discovering: Microsoft.Maui.Controls.Xaml.UnitTests
[xUnit.net 00:00:04.11] Discovered: Microsoft.Maui.Controls.Xaml.UnitTests
[xUnit.net 00:00:04.12] Starting: Microsoft.Maui.Controls.Xaml.UnitTests
[xUnit.net 00:00:04.14] Finished: Microsoft.Maui.Controls.Xaml.UnitTests
No test matches the given testcase filter `CSharpExpressions.sgen` in /home/vsts/work/1/s/artifacts/bin/Controls.Xaml.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Xaml.UnitTests.dll
🔴 Without fix — 📄 UserInfo: 🛠️ BUILD ERROR · 38s
Error-relevant lines (filtered from the build log):
/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Generated/Microsoft.Maui.Controls.SourceGen/Microsoft.Maui.Controls.SourceGen.XamlGenerator/CSharpExpressions.sgen.xaml.xsg.cs(861,28): error CS1612: Cannot modify the return value of 'SimpleViewModel.Margin' because it is not a variable [/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Controls.Xaml.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Generated/Microsoft.Maui.Controls.SourceGen/Microsoft.Maui.Controls.SourceGen.XamlGenerator/CSharpExpressions.sgen.xaml.xsg.cs(872,28): error CS1612: Cannot modify the return value of 'SimpleViewModel.Margin' because it is not a variable [/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Controls.Xaml.UnitTests.csproj]
🟢 With fix — 📄 UserInfo: 🔍 NO MATCH · 23s
(no coded error found; showing last 1200 chars)
aml.UnitTests.InternalsHiddenAssembly -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml.UnitTests.InternalsHiddenAssembly/Debug/net10.0/Microsoft.Maui.Controls.Xaml.UnitTests.InternalsHiddenAssembly.dll
Controls.Xaml.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Xaml.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Xaml.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Xaml.UnitTests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.52] Discovering: Microsoft.Maui.Controls.Xaml.UnitTests
[xUnit.net 00:00:06.70] Discovered: Microsoft.Maui.Controls.Xaml.UnitTests
[xUnit.net 00:00:06.72] Starting: Microsoft.Maui.Controls.Xaml.UnitTests
[xUnit.net 00:00:06.75] Finished: Microsoft.Maui.Controls.Xaml.UnitTests
No test matches the given testcase filter `UserInfo` in /home/vsts/work/1/s/artifacts/bin/Controls.Xaml.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Xaml.UnitTests.dll
⚠️ Failure Details
- 🛠️ CSharpExpressions.sgen without fix: build failed before tests could run
/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Generated/Microsoft.Maui.Controls.SourceGen/Microsoft.Maui.Controls.SourceGen.XamlGenerator/CSharpExpressions.sgen.xaml.xsg.cs(861,28): error CS16...
- 🛠️ UserInfo without fix: build failed before tests could run
/home/vsts/work/1/s/src/Controls/tests/Xaml.UnitTests/Generated/Microsoft.Maui.Controls.SourceGen/Microsoft.Maui.Controls.SourceGen.XamlGenerator/CSharpExpressions.sgen.xaml.xsg.cs(861,28): error CS16...
- 🔍 CSharpExpressions.sgen with fix: test filter matched 0 tests
- filter:
CSharpExpressions.sgen
- filter:
- 🔍 UserInfo with fix: test filter matched 0 tests
- filter:
UserInfo
- filter:
📁 Fix files reverted (2 files)
src/Controls/src/SourceGen/Descriptors.cssrc/Controls/src/SourceGen/SetPropertyHelpers.cs
📱 UI Tests — Button,Label,Layout
Detected UI test categories: Button,Label,Layout
✅ Deep UI tests — 366 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 |
71/73 ✓ | — |
Label |
96/98 ✓ | — |
Layout |
199/202 ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
🔗 Regression Cross-Reference
🔍 Regression Cross-Reference
✗ Revert risks detected — this PR removes 1 line(s) previously added by labeled bug-fix PRs.
| File | Fix PR | Fixed issue(s) | Risk | Reverted line |
|---|---|---|---|---|
src/Controls/src/SourceGen/SetPropertyHelpers.cs |
#34137 | #34075 | ✗ REVERT | if (analysis.IsSettable && IsExpressionWritable(expression, dataTypeSymbol, context)) |
Action required: Verify that issues #34075 do not re-regress before merging.
🧪 Regression Tests to Verify
These tests were added by the fix PRs being reverted. They must still pass:
| Fix PR | Type | Test | Filter |
|---|---|---|---|
| #34137 | XamlUnitTest | Maui34075ViewModel | Maui34075ViewModel |
🧪 Regression Test Results
✅ PASSED — 2 passed, 0 failed, 0 skipped
| Fix PR | Test | Type | Result |
|---|---|---|---|
| #34727 | Maui34726 | XamlUnitTest | ✅ PASSED |
| #34137 | Maui34075ViewModel | XamlUnitTest | ✅ PASSED |
📋 Pre-Flight — Context & Validation
Issue: #35006 - Fix CS1612 for struct sub-property paths in C# expression source gen
PR: #35006 - Fix CS1612 for struct sub-property paths in C# expression source gen
Platforms Affected: android (test platform); source generation/XAML behavior is cross-platform
Files Changed: 3 implementation, 3 test
Key Findings
- PR changes C# expression source generation for struct sub-property setters, adding copy-modify-writeback for paths such as
{Margin.Top}. - PR adds MAUIX2017 for read-only struct intermediates and tests settable/read-only
Thickness.Topcases. - Public PR discussion includes an unresolved inline concern that null-conditional access is normalized from
?.to., which can make generated struct writeback setters dereference null. - Impacted UI test categories: NONE (source generator/XAML unit-test-only behavior; no direct host-app UI category).
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: medium
Errors: 1 | Warnings: 0 | Suggestions: 0
Key code review findings:
- ✗
src/Controls/src/SourceGen/SetPropertyHelpers.cs:984normalizes?.to., whileWriteStructCopyWritebackSetteremits plain dereferences, so{User?.Margin.Top}can throw instead of preserving null-conditional no-op semantics.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35006 | Generate copy-modify-writeback setters for settable struct intermediates; warn/omit setter for read-only struct intermediates. | ✅ PASSED (Gate) | SetPropertyHelpers.cs, Descriptors.cs, analyzer release file, tests |
Original PR; gate was already completed externally. |
🔬 Code Review — Deep Analysis
Code Review — PR #35006
Independent Assessment
What this changes: Adds source-generator support for two-way C# expression bindings through struct sub-properties like {Margin.Top} by emitting copy-modify-writeback setters instead of direct __source.Margin.Top = __value, plus a new MAUIX2017 warning when a struct intermediate cannot be written back.
Inferred motivation: Avoid generated CS1612 failures and make settable struct sub-property paths usable in typed bindings.
Reconciliation with PR Narrative
Author claims: The PR fixes CS1612 for struct intermediate paths, emits null plus MAUIX2017 when writeback is impossible, and adds tests for Thickness.Top.
Agreement/disagreement: The simple settable/read-only struct scenarios match the claim. The implementation does not preserve null-conditional semantics for struct paths containing ?..
Prior Review Reconciliation
| Prior ❌ Error Finding | Source | Status | Evidence |
|---|---|---|---|
Null-conditional access is normalized from ?. to ., making generated struct writeback setters unsafe. |
Copilot inline review / MauiBot review | ❌ Unresolved | Current PR code still uses expr.Replace("?.", ".").Split('.') and WriteStructCopyWritebackSetter reconstructs all access with plain .. |
Blast Radius Assessment
- Runs for all instances: No — limited to XAML C# expression source generation.
- Startup impact: No direct runtime startup impact, but generated app code can fail at runtime for affected bindings.
- Static/shared state: No new mutable shared runtime state.
CI Status
- Required-check result: tool-unavailable via
gh pr checks --required(gh auth loginrequired). - Classification: undetermined by required-check command; supplemental public check-run API showed current head checks completed successfully.
- Action taken: capped confidence; no GitHub comments posted.
Findings
❌ Error — Null-conditional struct writeback drops ?. and can dereference null
src/Controls/src/SourceGen/SetPropertyHelpers.cs:984 normalizes ?. to ., and WriteStructCopyWritebackSetter later emits plain dereferences at src/Controls/src/SourceGen/SetPropertyHelpers.cs:1072.
For a two-way expression such as {User?.Margin.Top}, the generated setter can become equivalent to:
var __t0 = __source.User.Margin;
__t0.Top = __value;
__source.User.Margin = __t0;If User is null, this throws instead of preserving null-conditional no-op behavior. Carry conditional-access metadata into the writer and guard before dereferencing, or decline to generate a setter for struct writeback chains containing ?..
⚠️ Warning — None.
💡 Suggestion — None.
Failure-Mode Probing
- Simple
{Margin.Top}: copy-modify-writeback correctly preserves otherThicknessedges. - Read-only
{ReadOnlyMargin.Top}on two-way target: emits MAUIX2017 and no setter, as intended. - Null-conditional
{User?.Margin.Top}: unsafe; conditional access is erased before setter emission. - Deep non-null struct chains: reverse writeback stack appears sound.
Verdict: NEEDS_CHANGES
Confidence: medium
Summary: The core struct writeback approach is reasonable, but the current implementation regresses/null-breaks valid null-conditional expression paths. This prior error-level finding remains unresolved and should be fixed before merge.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix-1 | Reject null-conditional struct writeback and fall back to no setter/diagnostic behavior. | ✅ PASS tests, not selected | 2 files | Safe but conservative; loses two-way behavior for {User?.Margin.Top}. |
| 2 | try-fix-2 | Preserve ?. metadata and emit guarded struct copy/writeback setters. |
❌ FAIL expert review | 2 files | Tests passed, but direct value-type paths like {User?.Age} would still generate invalid setters. |
| 3 | try-fix-3 | General guarded setters for null-conditional non-nullable value-type paths, including struct writeback and direct terminals. | ✅ PASS | 2 files | Selected; no blocking expert-review findings. |
| PR | PR #35006 | Generate copy-modify-writeback setters for settable struct intermediates; warn/omit setter for read-only struct intermediates. | ✅ PASSED (Gate) | 6 files | Original PR; leaves unresolved ?. normalization/null-deref risk. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Candidate 1: reject unsupported null-conditional struct writeback safely. |
| maui-expert-reviewer | 2 | Yes | Candidate 2: preserve null-conditional semantics with guarded copy/writeback. |
| maui-expert-reviewer | 3 | Yes | Candidate 3: extend guarded setters to direct null-conditional non-nullable value-type terminal paths. |
| maui-expert-reviewer | Final | No blocking findings | Candidate 3 is a credible improvement; only non-blocking field-terminal coverage risk noted. |
Exhausted: No — stopped because Candidate #3 passed the executed test commands and was demonstrably better than the PR fix and prior candidates.
Selected Fix: Candidate #3 — preserves null-conditional semantics, fixes nullable-lifted value-type setter generation, passes focused source-gen tests plus mandatory regression commands, and cleared final expert review.
Mandatory Regression Tests
dotnet test src/Controls/tests/Xaml.UnitTests/Controls.Xaml.UnitTests.csproj --filter "Maui34726" --no-restorepassed for every candidate that reached regression testing.dotnet test src/Controls/tests/Xaml.UnitTests/Controls.Xaml.UnitTests.csproj --filter "Maui34075ViewModel" --no-restorewas run for every candidate that reached regression testing; in this environment it exited 0 with "No test matches".
📝 Recommended PR Title & Description
Assessment: ✏️ Recommend updating — the current metadata accurately describes the raw struct writeback fix, but the winning fix also preserves null-conditional semantics and handles nullable-lifted value-type terminals.
Recommended title
SourceGen: Fix struct and null-conditional sub-property setters in C# expressions
Recommended description
## Description
C# expressions like `{Margin.Top}` where an intermediate property returns a value type (struct) previously failed with **CS1612** because the source gen naively emitted `__source.Margin.Top = __value` as the setter lambda.
The fix also preserves null-conditional semantics for paths such as `{User?.Margin.Top}` and `{User?.Age}`. These expressions nullable-lift the getter result, so generated setters must guard the nullable receiver before assigning instead of normalizing `?.` to `.` and dereferencing null.
### Fix
The source gen now handles struct sub-property paths and null-conditional value-type paths with these behaviors:
| Scenario | Generated setter |
|----------|-----------------|
| Struct intermediate settable | Copy-modify-writeback pattern |
| Struct intermediate settable behind `?.` | Guarded copy-modify-writeback pattern |
| Null-conditional non-nullable value-type terminal | Guarded assignment using `GetValueOrDefault()` |
| Null-conditional reference-type terminal | Existing direct C# null-conditional assignment |
| Struct intermediate not settable + two-way target | `null` + **MAUIX2017** warning |
| Terminal not writable / complex expression | `null` (existing behavior) |
**Copy-modify-writeback example** for `{Margin.Top}`:
```csharp
// Before (CS1612):
(__source, __value) => __source.Margin.Top = __value
// After:
(__source, __value) => {
var __t0 = __source.Margin;
__t0.Top = __value;
__source.Margin = __t0;
}
For deeper chains (e.g., A.B.C where both A and B are structs), the pattern stacks correctly.
Guarded null-conditional example for {User?.Margin.Top}:
(__source, __value) => {
var __n0 = __source.User;
if (__n0 is not null) {
var __t0 = __n0.Margin;
__t0.Top = __value.GetValueOrDefault();
__n0.Margin = __t0;
}
}Changes
SetPropertyHelpers.cs— ReplacedIsExpressionWritablewithGetExpressionWriteInfo(returns write kind + chain info), added property-chain setter generation with struct copy/writeback, preserved null-conditional chain metadata, and emits guarded setters for nullable-lifted non-nullable value-type pathsDescriptors.cs— AddedMAUIX2017 StructIntermediateNotSettablewarningAnalyzerReleases.Unshipped.md— Registered MAUIX2017CSharpExpressionDiagnosticsTests.cs— Added coverage for read-only struct intermediates, null-conditional struct intermediates, and null-conditional value-type terminalsCSharpExpressions.sgen.xaml[.cs]— Added{Margin.Top}test coverage forThicknessstruct sub-property binding with INPC
Platforms Tested
- Android
</details>
---
<details>
<summary><strong>🏁 Report — Final Recommendation</strong></summary>
<br/>
## Comparative Fix Report — PR #35006
### Winner
**Selected candidate:** `pr-plus-reviewer`
`pr-plus-reviewer` keeps the PR's core fix for CS1612 on struct sub-property setters, then applies the expert reviewer's required correction for null-conditional paths. It is materially equivalent to `try-fix-3`, which passed the focused source-gen tests and mandatory regression commands, but is classified as the PR fix plus reviewer feedback.
### Candidate comparison
| Rank | Candidate | Regression result | Review result | Assessment |
|---:|---|---|---|---|
| 1 | `pr-plus-reviewer` | ✅ PASS, via same implementation as `try-fix-3` | ✅ No blocking findings | Best overall: preserves struct copy/writeback, adds guarded null-conditional semantics, and covers direct nullable-lifted value-type terminals. |
| 2 | `try-fix-3` | ✅ PASS | ✅ No blocking findings | Same technical solution as the winner, but as an alternate try-fix candidate rather than the PR plus applied reviewer feedback. |
| 3 | `try-fix-1` | ✅ PASS | ⚠️ Conservative behavior | Safe, but rejects null-conditional struct writeback instead of preserving two-way behavior for `{.User?.Margin.Top}`. |
| 4 | `pr` | ✅ Gate passed | ❌ Blocking expert finding | Fixes the primary `{Margin.Top}` CS1612 scenario, but loses `?.` semantics and can dereference null for `{.User?.Margin.Top}`. |
| 5 | `try-fix-2` | ✅ PASS | ❌ Blocking expert finding | Fixes guarded struct writeback, but leaves direct null-conditional value-type terminals such as `{.User?.Age}` with invalid setter generation. |
No candidate was reported as failing the mandatory regression tests; if a candidate had failed regression testing, it would be ranked below all candidates that passed. The ranking above therefore prioritizes passed regression evidence first, then expert correctness findings and behavior preservation.
### Rationale
The raw PR is directionally correct and covered the reported CS1612 root cause, but the expert review identified a real adjacent correctness issue introduced by normalizing null-conditional member access before generating writeback setters. `try-fix-1` avoids the crash by omitting the setter, but sacrifices valid two-way behavior. `try-fix-2` improves guarded struct writeback but misses nullable-lifted direct value-type terminals.
`pr-plus-reviewer` / `try-fix-3` is the most complete solution: it retains the submitted PR's struct copy-modify-writeback behavior, preserves no-op semantics for null-conditional chains, unwraps nullable setter values only for non-nullable value-type terminals, and leaves reference-type null-conditional assignment on the direct C# path.
</details>
</details>
<!-- SESSION:e740d43 END -->
---
<details>
<summary><strong>🧭 Next Steps</strong> — review latest findings</summary>
<br/>
No alternative fix was selected for this run. Review the session findings and CI results before merging.
</details>
MauiBot
reviewed
Jul 23, 2026
MauiBot
left a comment
MauiBot
left a comment
Collaborator
There was a problem hiding this comment.
Expert Review — 1 findings
See inline comments for details.
| if (isStruct) | ||
| { | ||
| var tempVar = $"__t{tempIndex++}"; | ||
| writer.Write($"var {tempVar} = {currentAccessor}.{name}; "); |
Collaborator
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Logic and Correctness — The struct writeback generator always emits a plain dereference here. Because GetExpressionWriteInfo normalizes ?. to . before building the chain, a binding such as {.User?.Margin.Top} is classified as StructCopyWriteback and generates var __t0 = __source.User.Margin; ... __source.User.Margin = __t0;. When User is null this setter throws instead of preserving null-conditional assignment/no-op semantics. Please preserve the null-conditional segment in the writeback chain and guard the generated setter (with a regression test for a nullable object before a struct sub-property).
MauiBot
removed
the
s/agent-review-in-progress
kubaflo
approved these changes
Jul 24, 2026
StephaneDelcroix
added a commit
that referenced
this pull request
Jul 27, 2026
Resolve merge conflicts in the C# expressions source generator: - CSharpExpressionHelpers.cs: keep both the xmlns-prefix/attached-property helpers (this PR) and the interpolated-string identifier extraction + lambda method-group detection helpers (from inflight/current). - CSharpExpressionDiagnosticsTests.cs and CSharpExpressions.sgen.xaml.cs: keep the appended test methods from both branches. Also resolve a pre-existing MAUIX2017 diagnostic-ID collision on inflight/current (#35093 LambdaMethodGroupReference vs #35006 StructIntermediateNotSettable). LambdaMethodGroupReference keeps MAUIX2017; StructIntermediateNotSettable is renumbered to MAUIX2018 (descriptor, analyzer release notes, and its test updated accordingly). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2c02c03e-d835-437a-bb14-f9f2d0e108f3
kubaflo
added a commit
that referenced
this pull request
Jul 28, 2026
…35006) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description C# expressions like `{Margin.Top}` where an intermediate property returns a value type (struct) previously failed with **CS1612** because the source gen naively emitted `__source.Margin.Top = __value` as the setter lambda. ### Fix The source gen now handles struct sub-property paths with three behaviors: | Scenario | Generated setter | |----------|-----------------| | Struct intermediate **settable** | Copy-modify-writeback pattern | | Struct intermediate **not settable** + two-way target | `null` + **MAUIX2017** warning | | Terminal not writable / complex expression | `null` (existing behavior) | **Copy-modify-writeback example** for `{Margin.Top}`: ```csharp // Before (CS1612): (__source, __value) => __source.Margin.Top = __value // After: (__source, __value) => { var __t0 = __source.Margin; __t0.Top = __value; __source.Margin = __t0; } ``` For deeper chains (e.g., `A.B.C` where both A and B are structs), the pattern stacks correctly. ### Changes - `SetPropertyHelpers.cs` — Replaced `IsExpressionWritable` with `GetExpressionWriteInfo` (returns write kind + chain info) and added `WriteStructCopyWritebackSetter` - `Descriptors.cs` — Added `MAUIX2017 StructIntermediateNotSettable` warning - `AnalyzerReleases.Unshipped.md` — Registered MAUIX2017 - `CSharpExpressions.sgen.xaml[.cs]` — Added `{Margin.Top}` test (Thickness struct sub-property binding with INPC) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
kubaflo
added a commit
that referenced
this pull request
Jul 29, 2026
…35006) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description C# expressions like `{Margin.Top}` where an intermediate property returns a value type (struct) previously failed with **CS1612** because the source gen naively emitted `__source.Margin.Top = __value` as the setter lambda. ### Fix The source gen now handles struct sub-property paths with three behaviors: | Scenario | Generated setter | |----------|-----------------| | Struct intermediate **settable** | Copy-modify-writeback pattern | | Struct intermediate **not settable** + two-way target | `null` + **MAUIX2017** warning | | Terminal not writable / complex expression | `null` (existing behavior) | **Copy-modify-writeback example** for `{Margin.Top}`: ```csharp // Before (CS1612): (__source, __value) => __source.Margin.Top = __value // After: (__source, __value) => { var __t0 = __source.Margin; __t0.Top = __value; __source.Margin = __t0; } ``` For deeper chains (e.g., `A.B.C` where both A and B are structs), the pattern stacks correctly. ### Changes - `SetPropertyHelpers.cs` — Replaced `IsExpressionWritable` with `GetExpressionWriteInfo` (returns write kind + chain info) and added `WriteStructCopyWritebackSetter` - `Descriptors.cs` — Added `MAUIX2017 StructIntermediateNotSettable` warning - `AnalyzerReleases.Unshipped.md` — Registered MAUIX2017 - `CSharpExpressions.sgen.xaml[.cs]` — Added `{Margin.Top}` test (Thickness struct sub-property binding with INPC) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
kubaflo
added a commit
that referenced
this pull request
Aug 7, 2026
…35006) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description C# expressions like `{Margin.Top}` where an intermediate property returns a value type (struct) previously failed with **CS1612** because the source gen naively emitted `__source.Margin.Top = __value` as the setter lambda. ### Fix The source gen now handles struct sub-property paths with three behaviors: | Scenario | Generated setter | |----------|-----------------| | Struct intermediate **settable** | Copy-modify-writeback pattern | | Struct intermediate **not settable** + two-way target | `null` + **MAUIX2017** warning | | Terminal not writable / complex expression | `null` (existing behavior) | **Copy-modify-writeback example** for `{Margin.Top}`: ```csharp // Before (CS1612): (__source, __value) => __source.Margin.Top = __value // After: (__source, __value) => { var __t0 = __source.Margin; __t0.Top = __value; __source.Margin = __t0; } ``` For deeper chains (e.g., `A.B.C` where both A and B are structs), the pattern stacks correctly. ### Changes - `SetPropertyHelpers.cs` — Replaced `IsExpressionWritable` with `GetExpressionWriteInfo` (returns write kind + chain info) and added `WriteStructCopyWritebackSetter` - `Descriptors.cs` — Added `MAUIX2017 StructIntermediateNotSettable` warning - `AnalyzerReleases.Unshipped.md` — Registered MAUIX2017 - `CSharpExpressions.sgen.xaml[.cs]` — Added `{Margin.Top}` test (Thickness struct sub-property binding with INPC) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
kubaflo
added a commit
that referenced
this pull request
Aug 12, 2026
…35006) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description C# expressions like `{Margin.Top}` where an intermediate property returns a value type (struct) previously failed with **CS1612** because the source gen naively emitted `__source.Margin.Top = __value` as the setter lambda. ### Fix The source gen now handles struct sub-property paths with three behaviors: | Scenario | Generated setter | |----------|-----------------| | Struct intermediate **settable** | Copy-modify-writeback pattern | | Struct intermediate **not settable** + two-way target | `null` + **MAUIX2017** warning | | Terminal not writable / complex expression | `null` (existing behavior) | **Copy-modify-writeback example** for `{Margin.Top}`: ```csharp // Before (CS1612): (__source, __value) => __source.Margin.Top = __value // After: (__source, __value) => { var __t0 = __source.Margin; __t0.Top = __value; __source.Margin = __t0; } ``` For deeper chains (e.g., `A.B.C` where both A and B are structs), the pattern stacks correctly. ### Changes - `SetPropertyHelpers.cs` — Replaced `IsExpressionWritable` with `GetExpressionWriteInfo` (returns write kind + chain info) and added `WriteStructCopyWritebackSetter` - `Descriptors.cs` — Added `MAUIX2017 StructIntermediateNotSettable` warning - `AnalyzerReleases.Unshipped.md` — Registered MAUIX2017 - `CSharpExpressions.sgen.xaml[.cs]` — Added `{Margin.Top}` test (Thickness struct sub-property binding with INPC) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
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
C# expressions like
{Margin.Top}where an intermediate property returns a value type (struct) previously failed with CS1612 because the source gen naively emitted__source.Margin.Top = __valueas the setter lambda.Fix
The source gen now handles struct sub-property paths with three behaviors:
null+ MAUIX2017 warningnull(existing behavior)Copy-modify-writeback example for
{Margin.Top}:For deeper chains (e.g.,
A.B.Cwhere both A and B are structs), the pattern stacks correctly.Changes
SetPropertyHelpers.cs— ReplacedIsExpressionWritablewithGetExpressionWriteInfo(returns write kind + chain info) and addedWriteStructCopyWritebackSetterDescriptors.cs— AddedMAUIX2017 StructIntermediateNotSettablewarningAnalyzerReleases.Unshipped.md— Registered MAUIX2017CSharpExpressions.sgen.xaml[.cs]— Added{Margin.Top}test (Thickness struct sub-property binding with INPC)