Skip to content

Always match the union payload with a property pattern in the STJ source generator - #131801

Open
akoeplinger wants to merge 1 commit into
dotnet:mainfrom
akoeplinger:stj-union-deconstructor-property-pattern
Open

Always match the union payload with a property pattern in the STJ source generator#131801
akoeplinger wants to merge 1 commit into
dotnet:mainfrom
akoeplinger:stj-union-deconstructor-property-pattern

Conversation

@akoeplinger

Copy link
Copy Markdown
Member

Follow-up to the review feedback on #131569.

That PR fixed infinite recursion in the generated union deconstructor for recursive union types by matching the payload with a { Value: T x } property pattern, but only for the case whose type is the union type itself; every other case kept a bare type pattern.

Per @AlekseyTs's suggestion, this generates all cases that way unconditionally, which removes the special case from the emitter and is more robust:

  • It does not rely on the compiler's union-unwrapping rule for type patterns. Per the unions proposal, a type pattern T applied to a union is equivalent to T or { Value: T }, so a bare type pattern happens to work for cases that are not the union type itself only because the union instance can never match them.
  • It always verifies the payload rather than the union instance, so no arm can become a trivially-matching catch-all that shadows the arms after it.

Generated code before:

return value switch
{
    null => ((Type?)null, (object?)null),
    bool caseValue0 => (typeof(bool), (object?)caseValue0),
    { Value: RecursiveNat caseValue1 } => (typeof(RecursiveNat), (object?)caseValue1),
};

After:

return value switch
{
    null => ((Type?)null, (object?)null),
    { Value: bool caseValue0 } => (typeof(bool), (object?)caseValue0),
    { Value: RecursiveNat caseValue1 } => (typeof(RecursiveNat), (object?)caseValue1),
};

No behavior change is expected, so no new tests are added; #131569 already added recursive-union coverage in both case-declaration orders.

Testing

  • System.Text.Json.SourceGeneration.Roslyn4.4.Tests — 10932 passed, 0 failed
  • System.Text.Json.SourceGeneration.Roslyn3.11.Tests — 475 passed, 0 failed
  • System.Text.Json.SourceGeneration.Roslyn4.4.Unit.Tests — 282 passed, 0 failed

cc @eiriktsarpalis @AlekseyTs

Note

This PR was created with the assistance of GitHub Copilot.

…erator

Follow-up to the review feedback on dotnet#131569. The generated union
deconstructor only used the `{ Value: T x }` form for the case whose type
is the union type itself and kept a bare type pattern for every other case.
Emitting the property pattern unconditionally is more robust: it does not
depend on the compiler's union-unwrapping rule for type patterns, it always
verifies the payload rather than the union instance, and it removes the
special case from the emitter.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8591ab4e-b1d7-40df-80f0-9a476a7bfd9d
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the System.Text.Json source generator’s union metadata emitter so the generated union deconstructor switch arms always match the union payload via a { Value: ... } property pattern, rather than sometimes using a bare type pattern on the union itself.

Changes:

  • Removes the special-case branch for “recursive union” cases in the union deconstructor emitter.
  • Emits { Value: <case-type> <local> } patterns for all union deconstructor arms unconditionally.
  • Updates the inline rationale comment explaining why payload property patterns are preferred.

Comment on lines +785 to +792
// Match the payload through a property pattern rather than applying a type
// pattern to the union itself. A type pattern `T` applied to a union is
// equivalent to `T or { Value: T }`, so for a case whose type is the union
// type itself the union instance also matches: that would bind the union
// rather than its payload -- making the converter recurse on the same value
// forever -- and would render every later arm unreachable. The explicit form
// binds only the unwrapped value and behaves the same for all other cases.
writer.WriteLine($"{{ Value: {caseSpec.PatternType.FullyQualifiedName} caseValue{deconArmIndex} }} => (typeof({caseSpec.CaseType.FullyQualifiedName}), (object?)caseValue{deconArmIndex}),");

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm the tests pass. I'll defer to area experts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants