Implement pattern-matching via ITuple #28859

gafter · 2018-07-26T18:38:26Z

This implements positional pattern-matching using the interface ITuple.

jcouv · 2018-07-26T19:29:17Z

Consider updating the feature document (or start one there isn't already) or feature spec to explain expected behavior. Thanks #Resolved

jcouv · 2018-07-26T21:54:31Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -1778,6 +1782,12 @@
    <Field Name="VariableAccess" Type="BoundExpression" Null="allow"/>
  </Node>

+  <Node Name="BoundITuplePattern" Base="BoundPattern">
+    <Field Name="getLengthMethod" Type="MethodSymbol" Null="disallow"/>


getLengthMethod [](start = 17, length = 15)

nit: Not sure about capitalization #Closed

jcouv · 2018-07-26T21:56:23Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

+  <Node Name="BoundITuplePattern" Base="BoundPattern">
+    <Field Name="getLengthMethod" Type="MethodSymbol" Null="disallow"/>
+    <Field Name="getItemMethod" Type="MethodSymbol" Null="disallow"/>
+    <Field Name="Deconstruction" Type="ImmutableArray&lt;BoundSubpattern&gt;" Null="disallow"/>


Deconstruction [](start = 17, length = 14)

nit: name probably can be tweaked, since no deconstruction involved. #Closed

jcouv · 2018-07-26T22:00:39Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -1177,6 +1177,10 @@
  <Node Name="BoundDagPropertyEvaluation" Base="BoundDagEvaluation">
    <Field Name="Property" Type="PropertySymbol" Null="disallow"/>
  </Node>
+  <Node Name="BoundDagIndexEvaluation" Base="BoundDagEvaluation">


BoundDagIndexEvaluation [](start = 14, length = 23)

Do we need to customize the Display property for the new bound nodes (in Formatting.cs)? #Closed

No, that is for nodes that extend BoundExpression but have no type. #Closed

jcouv · 2018-07-26T22:16:10Z

src/Compilers/CSharp/Test/Emit/CodeGen/ITuplePatternTests.cs

+#endif
+
+        [Fact]
+        public void XYZZY()


XYZZY [](start = 20, length = 5)

Probably not intentional #Closed

This file should be merged with PatternMatchingTests4.cs

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

jcouv · 2018-07-26T22:52:56Z

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

                //             case (1, 2):
-                Diagnostic(ErrorCode.ERR_MissingDeconstruct, "(1, 2)").WithArguments("object", "2").WithLocation(10, 18)
+                Diagnostic(ErrorCode.ERR_MissingDeconstruct, "(1, 2)").WithArguments("object", "2").WithLocation(8, 18)
                );


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

Not related to this PR, but this reminded me:
I'm working on an IDE PR to generate a Deconstruct method when it's missing.
But that fixer is negatively affected by the fact that we have two diagnostics (as visible in this test). The IDE doesn't like when there are two diagnostics on the same span...
I'd like to brainstorm how to reduce this to a single diagnostic, while still providing good information to users. I'll stop by tomorrow or next week to discuss. #Closed

jcouv · 2018-07-26T22:54:04Z

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

@@ -1093,15 +1091,15 @@ public static void M(object o)
    }
 }
 ";
-            var compilation = CreatePatternCompilation(source);
+            var compilation = CreateCompilationWithMscorlib45(source); // doesn't have ITuple


doesn't have ITuple [](start = 74, length = 19)

Would this test behave differently if the ITuple type was found? #Resolved

Yes; compilation would pass and it would use ITuple for the matching. #Closed

~~Sorry, I didn't follow. The parameter object o doesn't implement ITuple.~~
Oh, I see the description in the issue covers this.
Some documentation would be good

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

jcouv · 2018-07-26T23:16:23Z

src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs

@@ -570,6 +570,9 @@ public void AllWellKnownTypes()
                    case WellKnownType.ExtSentinel:
                        // Not a real type
                        continue;
+                    case WellKnownType.System_Runtime_CompilerServices_ITuple:


Actually, ITuple has shipped (more details on where). And it may be available in ValueTupleRef which is referenced above (I don't remember exactly what that ref was based off of). #Closed

jcouv · 2018-07-26T23:27:58Z

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

+            // Use a version of the platform APIs that lack ITuple
+            var compilation = CreateCompilationWithMscorlib40AndSystemCore(source, options: TestOptions.ReleaseExe);
+            compilation.VerifyDiagnostics(
+                // (7,32): error CS1061: 'object' does not contain a definition for 'Deconstruct' and no accessible extension method 'Deconstruct' accepting a first argument of type 'object' could be found (are you missing a using directive or an assembly reference?)


CS1061 [](start = 33, length = 6)

Should the error message mention ITuple? #Resolved

Something like "Pattern matching in this case would work if we could find the type 'System.Runtime.CompilerServices.ITuple'"? If ITuple were available then we wouldn't be looking for an extension method.

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

Hum, I don't really know how to incorporate that into the message, and adding a third diagnostic would be pretty bad :-(
I don't have a good idea for this, but it feels like we're not guiding users to one of the possible solutions... :-S

In reply to: 206681158 [](ancestors = 206681158,205631111)

jcouv · 2018-07-26T23:43:21Z

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

+{
+    [CompilerTrait(CompilerFeature.Patterns)]
+    public class ITuplePatternTests : PatternMatchingTestBase
+    {


{ [](start = 4, length = 1)

I'd like to see the IL produced for at least one case. Thanks #WontFix

Not blocking this PR: are we going to expose the ITuple-related information through public API? (ie. what semantic model scenario should be tested?)

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

There is no design for semantic model support for pattern-matching for ITuple. It is hard to imagine an IDE scenario in which that would be helpful.

In reply to: 205633786 [](ancestors = 205633786,205633476)

jcouv · 2018-07-27T00:03:32Z

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

+        Console.WriteLine(t is (3, 0, 5)); // false
+        Console.WriteLine(t is (3, 4, 5, 6)); // false
+    }
+    public void Deconstruct() {}


Could you add a variant of such scenario (where the existence of Deconstruct interferes with binding to ITuple) but the Deconstruct method has a non-void return type? #Resolved

jcouv

I'll finish reviewing tomorrow.

jcouv · 2018-07-30T19:14:41Z

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

+                return false;
+            }
+
+            if (((CSharpParseOptions)node.SyntaxTree.Options).LanguageVersion < MessageID.IDS_FeatureRecursivePatterns.RequiredVersion())


LanguageVersion [](start = 62, length = 15)

Feels like we should use the LanguageVersion from the compilation (ie. max of all trees) rather than from a single tree. #Closed

The compilation's language version isn't the max. It requires that the trees have the same language version (else it throws an exception) and uses that. However, I agree that Compilation.LanguageVersion should be used here.

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

jcouv · 2018-07-30T19:19:17Z

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

+                );
+        }
+    }
+}


Could you add a test with obsolete ITuple interface? #Resolved

Sure, but I wouldn't expect an error. The error would appear on the API that declares that it implements the interface.

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

jcouv · 2018-07-30T19:21:32Z

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

+            // declared to implement `ITuple` but contain no `Deconstruct` methods.
+            if (declType != (object)Compilation.GetSpecialType(SpecialType.System_Object) &&
+                declType != (object)Compilation.DynamicType &&
+                declType != (object)iTupleType &&


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

I'm surprised we're using reference equality. Shouldn't we be using symbol equality (.Equals)? #Resolved

They are the same for "original" type symbols, which all of these are.

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

jcouv · 2018-07-30T19:26:57Z

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

+            ArrayBuilder<BoundPatternBinding> bindings)
+        {
+            var syntax = pattern.Syntax;
+            var patternLength = pattern.Deconstruction.Length;


var [](start = 12, length = 3)

nit: int #WontFix

Isn't it obvious that a length is an int? Why is this line different than all the others?

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

It's a nit, but I don't see a good reason to use var for type names that are so short (like int or bool). For long type names I can see some benefits ;-) #Closed

I see them as distracting unless they convey something relevant to the reader. #Closed

jcouv

Done with review pass (iteration 1).

gafter · 2018-07-31T20:46:42Z

That is tracked by #28975 (there really isn't yet a separate doc for C# 8 pattern-matching changes).

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

gafter · 2018-07-31T22:43:46Z

@jcouv I believe I've responded to all of your comments. #Resolved

jcouv

LGTM Thanks (iteration 2)

cston · 2018-08-01T15:38:23Z

src/Compilers/Core/Portable/WellKnownMembers.cs

@@ -3409,6 +3423,8 @@ static WellKnownMembers()
                "Ceiling",                                  // System_Math__CeilingDouble
                "Floor",                                    // System_Math__FloorDouble
                "Truncate",                                 // System_Math__TruncateDouble
+                "get_Item",                                 // System_Runtime_CompilerServices_ITuple__Index


Index [](start = 103, length = 5)

Item or get_Item. (If get_Item, then Length should be get_Length.) #Resolved

cston · 2018-08-01T15:39:39Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -5420,4 +5420,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_PointerTypeInPatternMatching" xml:space="preserve">
    <value>Pattern-matching is not permitted for pointer types.</value>
  </data>
+  <data name="ERR_ArgumentNameInITuplePattern" xml:space="preserve">
+    <value>Element names are not permitted when patttern-matching via 'System.Runtime.CompilerServices.ITuple'.</value>


patttern [](start = 48, length = 8)

Typo #Resolved

cston · 2018-08-01T15:53:58Z

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

+            var objectType = Compilation.GetSpecialType(SpecialType.System_Object);
+            var deconstruction = node.DeconstructionPatternClause;
+            var patterns = ArrayBuilder<BoundSubpattern>.GetInstance(deconstruction.Subpatterns.Count);
+            for (int i = 0; i < deconstruction.Subpatterns.Count; i++)


for (int i = 0; i < deconstruction.Subpatterns.Count; i++) [](start = 12, length = 58)

Consider using foreach. #Resolved

cston · 2018-08-01T16:00:33Z

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

+            BoundExpression deconstruct = MakeDeconstructInvocationExpression(
+                node.Subpatterns.Count, inputPlaceholder, node, diagnostics, outPlaceholders: out ImmutableArray<BoundDeconstructValuePlaceholder> outPlaceholders);
+            deconstructMethod = deconstruct.ExpressionSymbol as MethodSymbol;
+            if (deconstructMethod is null)
            {


Consider keeping else with a single return patterns.ToImmutableAndFree(); outside of the if / else. #Resolved

cston · 2018-08-01T20:51:33Z

src/Compilers/CSharp/Portable/BoundTree/BoundDagEvaluation.cs

+        public override bool Equals(object obj) => obj is BoundDagIndexEvaluation other && this.Equals(other);
+        public override bool Equals(BoundDagEvaluation obj) => obj is BoundDagIndexEvaluation other && this.Equals(other);
+        public override int GetHashCode() => base.GetHashCode() ^ this.Index;
+        public bool Equals(BoundDagIndexEvaluation other)


public bool Equals(BoundDagIndexEvaluation other) [](start = 8, length = 49)

Are we calling this overload directly, other than in overloads above? If not, then it seems like we can get away with one overload in this class.

public override bool Equals(BoundDagEvaluation obj) { return base.Equals(other) && this.Index == ((BoundDagIndexEvaluation).Index; } ``` #Resolved

cston · 2018-08-01T21:03:18Z

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

+        Console.WriteLine(t is (3, 4, 5)); // TRUE
+        Console.WriteLine(t is (3, 0, 5)); // false
+        Console.WriteLine(t is (3, 4, 5, 6)); // false
+    }


} [](start = 4, length = 1)

Consider adding a case that is not ITuple:

Console.WriteLine(new object() is (3, 4, 5)); // false ``` #Resolved

cston · 2018-08-01T21:07:05Z

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

+{
+    public class ITuple
+    {
+        int Length => 3;


int [](start = 8, length = 3)

Consider making these properties public so it's more obvious the failure is due to the class rather than inaccessible properties. #Resolved

cston · 2018-08-01T21:10:48Z

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

+    {
+        M(new C());
+    }
+    public static void M<T>(T t) where T: C


where T: C [](start = 33, length = 10)

Perhaps also test where T : ITuple and when T is unconstrained. #Resolved

gafter · 2018-08-01T23:38:18Z

@cston I have made changes to address all of your review comments. Do you have any other comments? #Resolved

cston · 2018-08-01T23:42:36Z

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

+                if (subpatternSyntax.NameColon != null)
+                {
+                    // error: name not permitted in ITuple deconstruction
+                    diagnostics.Add(ErrorCode.ERR_ArgumentNameInITuplePattern, subpatternSyntax.NameColon.Location);


diagnostics.Add(ErrorCode.ERR_ArgumentNameInITuplePattern, subpatternSyntax.NameColon.Location); [](start = 20, length = 96)

Please add a test for this case. #Resolved

gafter · 2018-08-07T17:21:54Z

@cston I have added the test you mentioned. Do you have any other comments? #Resolved

cston · 2018-08-07T17:35:54Z

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

+    }
+}
+";
+            // Use a version of the platform APIs that lack ITuple


Is this comment correct? #Resolved

Adjust VB test for well known types.

Implement pattern-matching via ITuple

572ed34

Fixes dotnet#27746

gafter added Area-Compilers Feature Request Feature - Pattern Matching Pattern Matching labels Jul 26, 2018

gafter added this to the 16.0 milestone Jul 26, 2018

gafter self-assigned this Jul 26, 2018

gafter requested review from agocke and cston July 26, 2018 18:38

gafter requested a review from a team as a code owner July 26, 2018 18:38

jcouv reviewed Jul 26, 2018

View reviewed changes

jcouv reviewed Jul 27, 2018

View reviewed changes

jcouv self-assigned this Jul 29, 2018

jcouv reviewed Jul 30, 2018

View reviewed changes

Respond to comments on pattern-matching using ITuple

f8f9900

jcouv approved these changes Jul 31, 2018

View reviewed changes

cston reviewed Aug 1, 2018

View reviewed changes

Changes recommended during code review.

1b4d070

cston reviewed Aug 1, 2018

View reviewed changes

Add test for ERR_ArgumentNameInITuplePattern

c84817f

cston reviewed Aug 7, 2018

View reviewed changes

cston approved these changes Aug 7, 2018

View reviewed changes

gafter added 3 commits August 7, 2018 15:53

Adjust comment.

ef7f052

Adjust VB test for well known types.

Adjust VB test for new WellKnownMembers

691acf9

Remove non-language test.

7372e86

gafter closed this Aug 13, 2018

gafter reopened this Aug 13, 2018

gafter merged commit 10eaa84 into dotnet:features/recursive-patterns Aug 14, 2018

gafter mentioned this pull request Aug 14, 2018

Patterns: matching the tuple pattern using ITuple #27746

Closed

Implement pattern-matching via ITuple #28859

Implement pattern-matching via ITuple #28859

Conversation

gafter commented Jul 26, 2018

jcouv commented Jul 26, 2018 • edited by gafter Loading

jcouv Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 26, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv Jul 26, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Jul 26, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 26, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 26, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 27, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 30, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 30, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 30, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Jul 31, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

gafter commented Jul 31, 2018

gafter commented Jul 31, 2018 • edited Loading

jcouv left a comment

Choose a reason for hiding this comment

cston Aug 1, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Aug 1, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Aug 1, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Aug 1, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Aug 1, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Aug 1, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Aug 1, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

cston Aug 1, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter commented Aug 1, 2018 • edited Loading

cston Aug 1, 2018 • edited by gafter Loading

Choose a reason for hiding this comment

gafter commented Aug 7, 2018 • edited Loading

jcouv commented Jul 26, 2018 •

edited by gafter

Loading

jcouv Jul 26, 2018 •

edited

Loading

jcouv Jul 26, 2018 •

edited

Loading

jcouv Jul 26, 2018 •

edited

Loading

gafter Jul 26, 2018 •

edited

Loading

jcouv Jul 26, 2018 •

edited

Loading

jcouv Jul 26, 2018 •

edited by gafter

Loading

jcouv Jul 26, 2018 •

edited by gafter

Loading

gafter Jul 26, 2018 •

edited by jcouv

Loading

jcouv Jul 26, 2018 •

edited

Loading

jcouv Jul 26, 2018 •

edited

Loading

jcouv Jul 26, 2018 •

edited by gafter

Loading

jcouv Jul 26, 2018 •

edited by gafter

Loading

jcouv Jul 27, 2018 •

edited by gafter

Loading

jcouv Jul 30, 2018 •

edited

Loading

jcouv Jul 30, 2018 •

edited by gafter

Loading

jcouv Jul 30, 2018 •

edited by gafter

Loading

jcouv Jul 30, 2018 •

edited by gafter

Loading

jcouv Jul 31, 2018 •

edited

Loading

gafter Jul 31, 2018 •

edited by jcouv

Loading

gafter commented Jul 31, 2018 •

edited

Loading

cston Aug 1, 2018 •

edited by gafter

Loading

cston Aug 1, 2018 •

edited by gafter

Loading

cston Aug 1, 2018 •

edited by gafter

Loading

cston Aug 1, 2018 •

edited by gafter

Loading

cston Aug 1, 2018 •

edited by gafter

Loading

cston Aug 1, 2018 •

edited by gafter

Loading

cston Aug 1, 2018 •

edited by gafter

Loading

cston Aug 1, 2018 •

edited by gafter

Loading

gafter commented Aug 1, 2018 •

edited

Loading

cston Aug 1, 2018 •

edited by gafter

Loading

gafter commented Aug 7, 2018 •

edited

Loading

cston Aug 7, 2018 •

edited by gafter

Loading