Repository navigation
Remove Try-Both matching mode for unions. - #84897
Conversation
…enarios" This reverts only compiler changes from commit 1979ab1.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 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. |
There was a problem hiding this comment.
Pull request overview
This PR updates the C# compiler’s union pattern-matching implementation to remove the “try-both” (instance-or-value) matching mode, aligning diagnostics and bound-tree representation with the new behavior and adjusting tests/resources accordingly (see issue #84883 / csharplang #10302).
Changes:
- Simplifies union-matching lowering by removing the “instance arm” and representing union matching as a single value-pattern rewrite.
- Refactors bound nodes and binder plumbing from
UnionMatchingMode(for most patterns) to a booleanIsUnionMatching. - Updates CS8780 wording and expected diagnostics across pattern-matching and flow tests, plus localized resource strings.
Reviewed changes
Copilot reviewed 40 out of 42 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs | Updates CS8780 message expectations |
| src/Compilers/CSharp/Test/Emit3/Semantics/PatternMatchingTests3.cs | Updates CS8780 message expectations |
| src/Compilers/CSharp/Test/Emit3/Semantics/PatternMatchingTests_NullableTypes.cs | Updates CS8780 message expectations |
| src/Compilers/CSharp/Test/Emit3/Semantics/PatternMatchingTests_ListPatterns.cs | Updates CS8780 message expectations |
| src/Compilers/CSharp/Test/Emit3/FlowAnalysis/PatternsVsRegions.cs | Updates CS8780 message expectations |
| src/Compilers/CSharp/Test/Emit3/FlowAnalysis/FlowTests.cs | Updates CS8780 message expectations |
| src/Compilers/CSharp/Test/CSharp15/ClosedClassesTests.cs | Updates diagnostics for union/closed-class scenarios |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hant.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hans.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf | Updates localized CS8780 message |
| src/Compilers/CSharp/Portable/CSharpResources.resx | Updates CS8780 message text |
| src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IsOperator.cs | Adjusts constant folding call shape |
| src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AsOperator.cs | Adjusts constant folding call shape |
| src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs | Regenerates bound-node APIs for new fields |
| src/Compilers/CSharp/Portable/BoundTree/BoundRelationalPattern.cs | Updates validation to IsUnionMatching |
| src/Compilers/CSharp/Portable/BoundTree/BoundRecursivePattern.cs | Updates validation to IsUnionMatching |
| src/Compilers/CSharp/Portable/BoundTree/BoundPatternWithUnionMatching.cs | Simplifies union-matching pattern shape |
| src/Compilers/CSharp/Portable/BoundTree/BoundPattern.cs | Replaces UnionMatchingMode with IsUnionMatching |
| src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml | Updates bound-node schema for union matching |
| src/Compilers/CSharp/Portable/BoundTree/BoundNegatedPattern.cs | Updates validation to IsUnionMatching |
| src/Compilers/CSharp/Portable/BoundTree/BoundListPattern.cs | Updates validation to IsUnionMatching |
| src/Compilers/CSharp/Portable/BoundTree/BoundITuplePattern.cs | Updates validation to IsUnionMatching |
| src/Compilers/CSharp/Portable/BoundTree/BoundConstantPattern.cs | Defines IsUnionMatching based on mode |
| src/Compilers/CSharp/Portable/Binder/UnionMatchingRewriter.cs | Rewrites union patterns as value-property patterns |
| src/Compilers/CSharp/Portable/Binder/SwitchExpressionArmBinder.cs | Updates binder call to new signature |
| src/Compilers/CSharp/Portable/Binder/SwitchBinder.cs | Updates binder call to new signature |
| src/Compilers/CSharp/Portable/Binder/SwitchBinder_Patterns.cs | Updates binder call to new signature |
| src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs | Updates asserts to IsUnionMatching |
| src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder_CheckOrReachability.cs | Updates pattern rewrites to new fields |
| src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs | Refactors union matching type-compat checks |
| src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs | Refactors is/as constant analysis helpers |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml:2475
- The BoundNodes.xml doc comment has a grammatical error: it refers to two fields (ValueProperty and ValuePattern) but uses the singular verb "represents".
The ValueProperty and ValuePattern represents a Union matching operation
pending a rewrite to a BoundRecursivePattern.
src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml:2491
- The BoundNodes.xml doc comment refers to "Pattern.NarrowedType", but the field is named ValuePattern; this makes the comment ambiguous/inaccurate.
NarrowedType is Pattern.NarrowedType
src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs:1928
- This PR changes union pattern binding semantics (removing Try-Both matching) to address #84883, but there doesn't appear to be a targeted regression test covering the reported scenario (open generic union + struct case + variable designation in the case pattern). Without a dedicated test, it's easy for future refactors to reintroduce CS8780 in that corner case.
Consider adding a compiler test based on the issue repro (e.g., in C#15 union pattern tests) asserting that ValUnion<T> u => u switch { ValCase(var v) => v } compiles without CS8780.
NamedTypeSymbol? unionValueMatchingInputType = PrepareForUnionMatchingIfAppropriateAndReturnUnionMatchingInputType(node, ref inputType, ref unionType, diagnostics);
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
hasErrors |= CheckValidPatternType(typeSyntax, inputType, patternType.Type, diagnostics: diagnostics, ref useSiteInfo, out _) ||
(unionType is not null && CheckValidPatternTypeForUnionCases(typeSyntax, unionType, patternType.Type, diagnostics: diagnostics, ref useSiteInfo));
diagnostics.Add(typeSyntax, useSiteInfo);
isUnionMatching = unionValueMatchingInputType is not null;
|
@RikkiGibson, @jjonescz, @333fred Please review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs:1918
- This PR is marked as closing #84883, but I couldn’t find a compiler regression test that exercises the reported scenario (open generic union + struct case + pattern variable designation should not produce CS8780). Please add a targeted test (likely under
src/Compilers/CSharp/Test/CSharp15/UnionsTests.cs) that uses the repro from the issue and asserts the code compiles with no diagnostics.
private void CheckCompatibilityOfInputWithPatternType(
SyntaxNode node,
ExpressionSyntax typeSyntax,
NamedTypeSymbol? unionType,
TypeSymbol inputType,
BoundTypeExpression patternType,
ref bool hasErrors,
BindingDiagnosticBag diagnostics,
out bool isUnionMatching)
There was a problem hiding this comment.
Do we have a test for #84883, i.e., a recursive pattern like the following?
record struct ValCase(int Value);
union ValUnion<T>(ValCase);
static int FromVal<T>(ValUnion<T> u) =>
u switch { ValCase(var v) => v };There was a problem hiding this comment.
No, we don't. There are existing tests that observe the fact that the union instance isn't matched
|
@RikkiGibson, @333fred For a second review |
* upstream/main: (730 commits) Improve recovery for repeated partial type modifiers (#84934) Add standalone C# LSP telemetry (#84874) Suppress AI artifact audit failure issues (#84925) Remove empty ExternalAccessAspNetCoreResources.resx (#84939) Update helix job monitor version (#84940) Add HasPendingUpdates to HotReloadService.Updates to be used in dotnet-watch (#84891) Add LocalizableBranches parameter for OneLocBuild (#84933) Pin version of Microsoft.CodeAnalysis.Analyzers (#84924) Centralize record and union keyword checks (#84928) Caching compiler: support binary additional texts (#84916) Remove unused AdditionalTextComparer (#84917) Remove Try-Both matching mode for unions. (#84897) Update CodeStyleAnalyzerVersion to 5.9.0 (#84919) Fix/84847 source generated rename conflict (#84849) Return MethodNotFound for unsupported LSP method dispatch (#84892) [main] Update dependencies from dotnet/arcade (#84896) [main] Update dependencies from dotnet/arcade (#84882) Track status for feature "Type Parameter Inference from Constraints" (#84869) [main] Source code updates from dotnet/dotnet (#84879) Stop labeling Loc PRs as community (#84878) ...
See dotnet/csharplang#10302.
Closes #84883.
Microsoft Reviewers: Open in CodeFlow