Pattern-matching fixes for ITuple, short tuple patterns, and nulls #31056

gafter · 2018-11-09T00:02:54Z

Implement LDM disambiguation rules for Deconstruct vs ITuple
Permit 0-element and 1-element tuple patterns
Fixes Pattern-matching changes for short tuples #30962
Replaces Support 0-element and 1-element tuple patterns #31027
Handle null and nullable input for a var pattern with a tuple designation
Fixes Matching nullables with var pattern #30906
Replaces Handle null and nullable input for a var pattern with a tuple designation #30935
Don't report missing Deconstruct when there is more than one applicable Deconstruct
Fixes "No suitable Deconstruct instance or extension method was found" is incorrect #31031

* Permit 0-element and 1-element tuple patterns Fixes dotnet#30962 Replaces dotnet#31027 * Handle null and nullable input for a var pattern with a tuple designation Fixes dotnet#30906 Replaces dotnet#30935 * Don't report missing Deconstruct when there is more than one applicable Deconstruct Fixes dotnet#31031

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

cston · 2018-11-09T16:04:56Z

            if (symbol is FieldSymbol field && field.IsTupleElement())

IsTupleElement() is an extension method on IFieldSymbol. Perhaps use IsTupleField instead. #WontFix

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs:823 in 102de2f. [](commit_id = 102de2f, deletion_comment = False)

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

cston · 2018-11-09T16:56:10Z

Consider testing static void Deconstruct(this ITuple tuple, ...) { }. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs:649 in 102de2f. [](commit_id = 102de2f, deletion_comment = False)

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs

gafter · 2018-11-13T00:59:51Z

            if (symbol is FieldSymbol field && field.IsTupleElement())

I understand that "tuple element" includes tuple fields. It isn't clear to me that the converse is true, so I'm going to leave it. If the converse is true then we should eliminate the duplicate terminology.

In reply to: 437406528 [](ancestors = 437406528)

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs:823 in 102de2f. [](commit_id = 102de2f, deletion_comment = False)

gafter · 2018-11-13T01:24:00Z

            if (symbol is FieldSymbol field && field.IsTupleElement())

Added #20648 (comment) to track this

In reply to: 438087900 [](ancestors = 438087900,437406528)

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs:823 in 102de2f. [](commit_id = 102de2f, deletion_comment = False)

gafter · 2018-11-13T01:25:17Z

@cston I think this iteration responds to all of your comments. Do you have any others? #Resolved

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

…et/roslyn into rpatterns-DeconstructVsITuple

jcouv · 2018-11-19T23:35:30Z

src/Compilers/CSharp/Test/Syntax/Parsing/PatternParsingTests.cs

@@ -1915,16 +1912,13 @@ public void ParenthesizedExpression_04()
            UsingStatement(@"switch (e) { case (((x: 3))): ; }", TestOptions.RegularWithoutRecursivePatterns,


case (((x: 3))): [](start = 42, length = 16)

Could you add a test with pattern ((3)) too? #Closed

See existing test ParenthesizedExpression_01.

In reply to: 234822088 [](ancestors = 234822088)

jcouv · 2018-11-19T23:40:04Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

+        // Work around https://github.com/dotnet/roslyn/issues/20648: The compiler's internal APIs such as `declType.IsTupleType`
+        // do not correctly treat the non-generic struct `System.ValueTuple` as a tuple type.  We explicitly perform the tests
+        // required to identify it.  When that bug is fixed we should be able to remove this code and its callers.
+        internal static bool IsZeroElementTupleType(TypeSymbol type)


IsZeroElementTupleType [](start = 29, length = 22)

Would it be simpler go get the well-known type ValueTuple and compare them? #Resolved

No; there can be more than one tuple type imported from different assemblies.

In reply to: 234822985 [](ancestors = 234822985)

Sorry, one more question on this: would IsTupleCompatible do the job?

In reply to: 237273866 [](ancestors = 237273866,234822985)

jcouv · 2018-11-19T23:41:34Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

            MethodSymbol deconstructMethod = null;
            ImmutableArray<BoundSubpattern> deconstructionSubpatterns = default;
            if (node.DeconstructionPatternClause != null)
            {
-                deconstructionSubpatterns = BindDeconstructionPatternClause(node.DeconstructionPatternClause, declType, inputValEscape, diagnostics, out deconstructMethod, ref hasErrors);
+                DeconstructionPatternClauseSyntax deconstructClause = node.DeconstructionPatternClause;
+                Debug.Assert(deconstructClause != null);


Debug.Assert(deconstructClause != null); [](start = 16, length = 40)

nit: assertion doesn't add much, since checked two lines above #Resolved

jcouv · 2018-11-19T23:41:59Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

-                deconstructionSubpatterns = BindDeconstructionPatternClause(node.DeconstructionPatternClause, declType, inputValEscape, diagnostics, out deconstructMethod, ref hasErrors);
+                DeconstructionPatternClauseSyntax deconstructClause = node.DeconstructionPatternClause;
+                Debug.Assert(deconstructClause != null);
+                deconstructMethod = null;


deconstructMethod = null; [](start = 16, length = 25)

assignment seem unnecessary since initialized with null already #Resolved

jcouv · 2018-11-19T23:43:56Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

+                DeconstructionPatternClauseSyntax deconstructClause = node.DeconstructionPatternClause;
+                Debug.Assert(deconstructClause != null);
+                deconstructMethod = null;
+                var patterns = ArrayBuilder<BoundSubpattern>.GetInstance(deconstructClause.Subpatterns.Count);


patterns [](start = 20, length = 8)

Rename to patternsBuilder? #Resolved

jcouv · 2018-11-19T23:55:29Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

+                    deconstructMethod = BindDeconstructSubpatterns(deconstructClause, inputValEscape, deconstruct, outPlaceholders, patterns, ref hasErrors, diagnostics);
+                }
+
+                deconstructionSubpatterns = patterns.ToImmutableAndFree();


deconstructionSubpatterns = patterns.ToImmutableAndFree(); [](start = 16, length = 58)

The flow of this method would be easier to follow if BindValueTupleSubpatterns, BindITupleSubpatterns and BindDeconstructSubpatterns returned patterns (which we would store into deconstructionSubpatterns).
BindDeconstructSubpatterns would set deconstructMethod via a secondary output (an out parameter). #ByDesign

They produce multiple subpatterns, not a single pattern, which is why they go into a builder.

In reply to: 234825978 [](ancestors = 234825978)

jcouv · 2018-11-20T00:35:20Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

                var boundSubpattern = new BoundSubpattern(
                    subpatternSyntax,
                    null,
-                    BindPattern(subpatternSyntax.Pattern, elementType, valEscape, false, diagnostics));
+                    BindPattern(subpatternSyntax.Pattern, objectType, valEscape, false, diagnostics));


false [](start = 81, length = 5)

nit: consider naming hasErrors: false (here and below) #Resolved

jcouv · 2018-11-20T00:46:09Z

    if (c2 is C2(1, 2) {}) {}  // [7]

Just to confirm, the type in a pattern cannot be a tuple type. That would be a parsing error, correct?
c2 is (int, int) (1, 2) ...
Do we have a test already? #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs:844 in bbcf265. [](commit_id = bbcf265, deletion_comment = False)

jcouv · 2018-11-20T01:10:17Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

-                        if (inputType.IsTupleType)
+                        var strippedInputType = inputType.StrippedType();
+
+                        void addSubpatternsForTuple(ImmutableArray<TypeSymbolWithAnnotations> elementTypes)


addSubpatternsForTuple [](start = 29, length = 22)

Please move local function to the end of the method to keep the code flow clear. Thanks #Resolved

jcouv · 2018-11-20T01:14:43Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

                            deconstructMethod = deconstruct.ExpressionSymbol as MethodSymbol;
+                            if (!hasErrors)


) [](start = 42, length = 1)

braces? #ByDesign

I'd rather remove unnecessary braces.

In reply to: 234839920 [](ancestors = 234839920)

jcouv · 2018-11-20T01:25:54Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

        }

-        private BoundPattern BindVarDesignation(VarPatternSyntax node, VariableDesignationSyntax designation, TypeSymbol inputType, uint inputValEscape, bool hasErrors, DiagnosticBag diagnostics)
+        private BoundPattern BindVarDesignation(
+            VariableDesignationSyntax node,


node [](start = 38, length = 4)

designation? #ByDesign

This is a convention used widely in the binder: node is the current syntax being processed.

In reply to: 234841661 [](ancestors = 234841661)

jcouv · 2018-11-20T01:30:23Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs

+True
+False
+False";
+            var compVerifier = CompileAndVerify(compilation, expectedOutput: expectedOutput);


var compVerifier = [](start = 12, length = 19)

~~unused local~~ Never mind, that's in every test #Closed

jcouv · 2018-11-20T01:38:09Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs

+using System.Runtime.CompilerServices;
+public class C : ITuple
+{
+    int ITuple.Length => 4;


4; [](start = 25, length = 2)

Consider splitting this test. One test would have the ITuple implementation throw.
Also, three of the comments say the same thing but in different ways... #ByDesign

jcouv · 2018-11-20T01:47:10Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs

+class A: IA, ITuple
+{
+    void IA.Deconstruct(out int X, out int Y) => (X, Y) = (3, 4);
+    int ITuple.Length => 2;


2; [](start = 25, length = 2)

Consider throwing for members that don't participate in execution. (also applies in other tests below) #Resolved

jcouv · 2018-11-20T02:03:52Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs

+            // Here we test the relative priority of steps 2 and 3.
+            // - Found more than one applicable Deconstruct method (even though the type implements ITuple): error
+
+            // case 1: tuple pattern with var subpatterns


case 1 [](start = 15, length = 6)

I didn't understand what "case 1" refers to #Resolved

jcouv

LGTM with some minor suggestions (iteration 6)

Issue reported as dotnet#31279

gafter · 2018-11-28T22:40:03Z

    if (c2 is C2(1, 2) {}) {}  // [7]

We do have parsing tests that show this is a syntax error.

In reply to: 440095022 [](ancestors = 440095022)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs:844 in bbcf265. [](commit_id = bbcf265, deletion_comment = False)

…et/roslyn into rpatterns-DeconstructVsITuple

gafter added Area-Compilers Feature - Pattern Matching Pattern Matching labels Nov 9, 2018

gafter added this to the 16.0.P2 milestone Nov 9, 2018

gafter self-assigned this Nov 9, 2018

gafter requested review from agocke and jcouv November 9, 2018 00:02

gafter requested a review from a team as a code owner November 9, 2018 00:02

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs Outdated Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs Outdated Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs Outdated Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs Outdated Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs Outdated Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs Outdated Show resolved Hide resolved

cston reviewed Nov 9, 2018

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs Show resolved Hide resolved

Minor changes per code review.

dc32a8d

gafter force-pushed the rpatterns-DeconstructVsITuple branch from 96cb362 to dc32a8d Compare November 13, 2018 01:19

gafter added the 4 - In Review A fix for the issue is submitted for review. label Nov 13, 2018

cston reviewed Nov 13, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs Outdated Show resolved Hide resolved

alrz mentioned this pull request Nov 15, 2018

Pattern-matching changes for short tuples #30962

Closed

gafter added 2 commits November 15, 2018 12:58

Merge branch 'features/recursive-patterns' of https://github.com/dotn…

21c4e09

…et/roslyn into rpatterns-DeconstructVsITuple

Merge branch 'features/recursive-patterns' of https://github.com/dotn…

bbcf265

…et/roslyn into rpatterns-DeconstructVsITuple

jcouv reviewed Nov 19, 2018

View reviewed changes

jcouv reviewed Nov 20, 2018

View reviewed changes

jcouv approved these changes Nov 20, 2018

View reviewed changes

jcouv self-assigned this Nov 20, 2018

Skip AddResourceToModule on Linux

187f38a

Issue reported as dotnet#31279

gafter added 2 commits November 28, 2018 15:08

Minor changes per code review.

dfe266a

Merge branch 'features/recursive-patterns' of https://github.com/dotn…

dfca6f5

…et/roslyn into rpatterns-DeconstructVsITuple

gafter merged commit 2ebd138 into dotnet:features/recursive-patterns Nov 29, 2018

This was referenced Nov 29, 2018

Matching nullables with var pattern #30906

Closed

"No suitable Deconstruct instance or extension method was found" is incorrect #31031

Closed

gafter removed 4 - In Review A fix for the issue is submitted for review. labels Dec 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pattern-matching fixes for ITuple, short tuple patterns, and nulls #31056

Pattern-matching fixes for ITuple, short tuple patterns, and nulls #31056

gafter commented Nov 9, 2018

cston commented Nov 9, 2018 •

edited by gafter

Loading

cston commented Nov 9, 2018 •

edited by gafter

Loading

gafter commented Nov 13, 2018

gafter commented Nov 13, 2018

gafter commented Nov 13, 2018 •

edited

Loading

jcouv Nov 19, 2018 •

edited

Loading

gafter Nov 28, 2018

jcouv Nov 19, 2018 •

edited by gafter

Loading

gafter Nov 28, 2018

jcouv Nov 28, 2018

jcouv Nov 19, 2018 •

edited by gafter

Loading

jcouv Nov 19, 2018 •

edited by gafter

Loading

jcouv Nov 19, 2018 •

edited by gafter

Loading

jcouv Nov 19, 2018 •

edited by gafter

Loading

gafter Nov 28, 2018

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv commented Nov 20, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

gafter Nov 28, 2018

jcouv Nov 20, 2018 •

edited by gafter

Loading

gafter Nov 28, 2018

jcouv Nov 20, 2018 •

edited

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv left a comment

gafter commented Nov 28, 2018

		@@ -1915,16 +1912,13 @@ public void ParenthesizedExpression_04()
		UsingStatement(@"switch (e) { case (((x: 3))): ; }", TestOptions.RegularWithoutRecursivePatterns,

		deconstructMethod = deconstruct.ExpressionSymbol as MethodSymbol;
		if (!hasErrors)

Pattern-matching fixes for ITuple, short tuple patterns, and nulls #31056

Pattern-matching fixes for ITuple, short tuple patterns, and nulls #31056

Conversation

gafter commented Nov 9, 2018

cston commented Nov 9, 2018 • edited by gafter Loading

cston commented Nov 9, 2018 • edited by gafter Loading

gafter commented Nov 13, 2018

gafter commented Nov 13, 2018

gafter commented Nov 13, 2018 • edited Loading

jcouv Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Nov 28, 2018

Choose a reason for hiding this comment

jcouv Nov 19, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Nov 28, 2018

Choose a reason for hiding this comment

jcouv Nov 28, 2018

Choose a reason for hiding this comment

jcouv Nov 19, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv Nov 19, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv Nov 19, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv Nov 19, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Nov 28, 2018

Choose a reason for hiding this comment

jcouv Nov 20, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv commented Nov 20, 2018 • edited by gafter Loading

jcouv Nov 20, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv Nov 20, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Nov 28, 2018

Choose a reason for hiding this comment

jcouv Nov 20, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Nov 28, 2018

Choose a reason for hiding this comment

jcouv Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 20, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv Nov 20, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv Nov 20, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

gafter commented Nov 28, 2018

cston commented Nov 9, 2018 •

edited by gafter

Loading

cston commented Nov 9, 2018 •

edited by gafter

Loading

gafter commented Nov 13, 2018 •

edited

Loading

jcouv Nov 19, 2018 •

edited

Loading

jcouv Nov 19, 2018 •

edited by gafter

Loading

jcouv Nov 19, 2018 •

edited by gafter

Loading

jcouv Nov 19, 2018 •

edited by gafter

Loading

jcouv Nov 19, 2018 •

edited by gafter

Loading

jcouv Nov 19, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv commented Nov 20, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading

jcouv Nov 20, 2018 •

edited by gafter

Loading