IOperation for recursive pattern and switch expression #31967

gafter · 2018-12-20T21:51:27Z

Despite the title, I have not done switch expression nodes in this PR. That will be a later PR.
Part of #27749

Work in progress dotnet#27749

src/Compilers/Core/Portable/Operations/IRecursivePatternOperation.cs

src/Compilers/Core/Portable/Operations/IDeclarationPatternOperation.cs

src/Compilers/Core/Portable/Operations/IRecursivePatternOperation.cs

src/Compilers/Core/Portable/Operations/OperationNodes.cs

src/Compilers/Core/Portable/Operations/IRecursivePatternOperation.cs

333fred · 2018-12-20T22:30:55Z

src/Compilers/Core/Portable/Operations/IRecursivePatternOperation.cs

@@ -13,6 +13,25 @@ namespace Microsoft.CodeAnalysis.Operations
    /// </remarks>
    public interface IRecursivePatternOperation : IPatternOperation


Testing around HasErrors, such as when there is no DeconstructSymbol but there are patterns. #Resolved

333fred · 2018-12-20T22:33:30Z

src/Compilers/Core/Portable/Operations/IRecursivePatternOperation.cs

+        /// <see cref="ISymbol"/>/<see cref="IPatternOperation"/> pairs within it.
+        /// If there is no property subpattern, this is a default immutable array.
+        /// </summary>
+        ImmutableArray<(ISymbol, IPatternOperation)> PropertySubpatterns { get; }


(ISymbol, IPatternOperation) [](start = 23, length = 28)

Need to get some feedback on whether we should be using Tuples here. We could potentially replace with a struct instead. #Resolved

src/Compilers/Core/Portable/Operations/OperationCloner.cs

src/Compilers/Core/Portable/Operations/OperationNodes.cs

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IIsPatternExpression.cs

jcouv · 2018-12-26T18:04:36Z

src/Compilers/Core/Portable/Operations/OperationNodes.cs

@@ -7854,6 +7855,9 @@ public override IEnumerable<IOperation> Children
        /// Constant value of the pattern.
        /// </summary>
        public abstract IOperation Value { get; }
+
+        public ITypeSymbol InputType { get; }


InputType [](start = 27, length = 9)

What does the input type represent? Could it be removed?
I'd expect the constant/value to have a type and that's it.
Also, I don't see InputType in any of the test outputs. #Closed

Yes, I haven't modified the outputs of the tests to keep this PR small. The input type of a pattern is part of the spec of the meaning of a pattern. Noted in #32012 that the tests should be modified to display it.

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

I looked at the spec, but saw not mention of "input type".
In e is <pattern>, isn't the input type just the type of e?

Update: I think I found it: "If the type is omitted, we take it to be the static type of the input value."

In reply to: 244056434 [](ancestors = 244056434,244029510)

jcouv · 2018-12-26T18:05:16Z

src/Compilers/Core/Portable/Operations/OperationNodes.cs

+
+        public ITypeSymbol MatchedType { get; }
+
+        public ITypeSymbol InputType { get; }


InputType [](start = 27, length = 9)

Same question on the InputType of the declaration pattern. #Resolved

src/Compilers/Core/Portable/Operations/OperationNodes.cs

src/Test/Utilities/Portable/Compilation/OperationTreeVerifier.cs

jcouv · 2018-12-26T18:18:02Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

+Microsoft.CodeAnalysis.Operations.IRecursivePatternOperation.DeconstructSymbol.get -> Microsoft.CodeAnalysis.ISymbol
+Microsoft.CodeAnalysis.Operations.IRecursivePatternOperation.DeconstructionSubpatterns.get -> System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Operations.IPatternOperation>
+Microsoft.CodeAnalysis.Operations.IRecursivePatternOperation.MatchedType.get -> Microsoft.CodeAnalysis.ITypeSymbol
+Microsoft.CodeAnalysis.Operations.IRecursivePatternOperation.PropertySubpatterns.get -> System.Collections.Immutable.ImmutableArray<(Microsoft.CodeAnalysis.ISymbol, Microsoft.CodeAnalysis.Operations.IPatternOperation)>


(Microsoft.CodeAnalysis.ISymbol, Microsoft.CodeAnalysis.Operations.IPatternOperation) [](start = 132, length = 85)

I believe this is the first usage of tuples in our public API.
I think we should have an IPropertySubpatternOperation for this. Or maybe we provide two arrays.

Elsewhere in this change, I see uses of Item1 and Item2 that result from this decision. #Resolved

Noted in #32012

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

jcouv · 2018-12-26T18:26:13Z

src/Compilers/Core/Portable/Operations/IDeclarationPatternOperation.cs

+        /// For example, in C# the pattern `var x` will match a null input,
+        /// while the pattern `string x` will not.
+        /// </summary>
+        bool MatchesNull { get; }


MatchesNull [](start = 13, length = 11)

Do we need this property storing this information, or can we derive MatchesNull from other parts?
If the latter, maybe we should remove the property and provide an extension method for convenience instead. #ByDesign

No, it cannot be derived from other information.

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

jcouv · 2018-12-26T18:30:20Z

src/Compilers/Core/Portable/Operations/OperationCloner.cs

@@ -47,7 +47,13 @@ internal override IOperation VisitNoneOperation(IOperation operation, object arg
        private ImmutableArray<T> VisitArray<T>(ImmutableArray<T> nodes) where T : IOperation
        {
            // clone the array
-            return nodes.SelectAsArray(n => Visit(n));
+            return nodes.IsDefault ? default : nodes.SelectAsArray(n => Visit(n));


IsDefault [](start = 25, length = 9)

Are default arrays allowed in operations? I suspect that empty arrays are used presently. #Resolved

jcouv · 2018-12-26T18:32:44Z

src/Compilers/Core/Portable/Operations/OperationCloner.cs

@@ -564,7 +581,9 @@ public override IOperation VisitMethodBodyOperation(IMethodBodyOperation operati

        public override IOperation VisitDiscardOperation(IDiscardOperation operation, object argument)
        {
-            return new DiscardOperation(operation.DiscardSymbol, ((Operation)operation).OwningSemanticModel, operation.Syntax, operation.Type, operation.ConstantValue, operation.IsImplicit);
+            return new DiscardOperation(
+                operation is IPatternOperation pat ? pat.InputType : operation.DiscardSymbol?.Type,


operation is IPatternOperation pat ? pat.InputType : operation.DiscardSymbol?.Type, [](start = 16, length = 83)

I didn't understand why cloning would require such logic.
Assuming we agreed that discard operation should contain an InputType, then cloning should just copy it. #Resolved

This is disentangled in #32018

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

jcouv · 2018-12-26T18:34:53Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

@@ -1837,21 +1838,30 @@ private IDeclarationPatternOperation CreateBoundDeclarationPatternOperation(Boun
                variable = ((BoundDiscardExpression)boundDeclarationPattern.VariableAccess).ExpressionSymbol;
            }

+            ITypeSymbol inputType = boundDeclarationPattern.InputType;
+            bool acceptsNull = boundDeclarationPattern.IsVar;


acceptsNull [](start = 17, length = 11)

What about x is null? If the constant is null, then we should probably set AcceptsNull to true.
If the intention is to capture IsVar, then maybe the pattern should have an IsVar property instead.
As Fred pointed out in a test, should AcceptsNull be true when matching types that can't be null in the first place (like int)? #Closed

This is for the declaration pattern only.
The spec for a var pattern is that it accepts null. The fact that it will never get null in some circumstances doesn't affect that fact.

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

jcouv · 2018-12-26T18:48:07Z

src/Test/Utilities/Portable/Compilation/TestOperationVisitor.cs

+            }
+
+            AssertEx.Equal(children, operation.Children);
+        }


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

Consider verifying that the symbol part of PropertySubpatterns contains non-null symbols.
Consider verifying that DeconstructSymbol is either a method, a type or null. #Resolved

Noted in #32012 for follow-up.

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

jcouv · 2018-12-26T18:52:24Z

src/Compilers/Core/Portable/Operations/OperationNodes.cs

+                if (_lazyPropertySubpatterns.IsDefault)
+                {
+                    var propertySubpatterns = CreatePropertySubpatterns();
+                    foreach (var d in propertySubpatterns)


d [](start = 33, length = 1)

nit: d should be p or subpattern. #Resolved

jcouv · 2018-12-26T18:54:22Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationNodes.cs

+        }
+        public override ImmutableArray<IPatternOperation> CreateDeconstructionSubpatterns()
+        {
+            return _deconstructionSubpatterns.SelectAsArray(p => (IPatternOperation)_operationFactory.Create(p.Pattern));


p [](start = 60, length = 1)

nit: p should be d #ByDesign

No, it is a subpattern.

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

jcouv · 2018-12-26T19:03:01Z

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IIsPatternExpression.cs

+  Expression: 
+    IParameterReferenceOperation: tuple (OperationKind.ParameterReference, Type: (System.Int32 X, System.Int32 Y)) (Syntax: 'tuple')
+  Pattern: 
+    IRecursivePatternOperation (Declared Symbol: (System.Int32 X, System.Int32 Y) y, MatchedType: (System.Int32 X, System.Int32 Y), Deconstruct Symbol: null)


(System.Int32 X, System.Int32 Y) [](start = 49, length = 32)

The declared symbol having tuple names feels unexpected. Does that reflect the language in some way?
I would expect that y is an (int, int).
Did we discuss allowing names in positional patterns? I'd expect the names in output to come from there tuple is (a: 1, b: 2) y, rather than from input.
Please also add test with Deconstruct. I'd expect the element names to come from the Deconstruct method, if anything. #Closed

If the type is not given in the pattern, the type of the variable is its input type. Using Deconstruct would not change that (it does not produce a tuple type).

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

jcouv

Done with review pass (iteration 5)

…et/roslyn into rpatterns-27749

…to rpatterns-27749

gafter · 2018-12-26T22:55:34Z

I believe I have responded to all comments. I intend to address outstanding issues in #32012 and #32018. @jcouv are you OK with this being integrated into the feature branch as a basis for furhter development?

Add and implement IDiscardPatternOperation, ISwitchExpressionOperation, ISwitchExpressionArmOperation

jcouv

I didn't understand why postpone addressing the feedback. Tracking such fine-grained feedback as github issues and without PROTOTYPE markers incurs a loss of context, and I don't see the benefit.

Two things that stand out and should be prioritized in the next PR:

aligning printed labels with property names in test verifier
revert handling of default arrays (assuming ioperations indeed uses empty arrays)

gafter · 2018-12-27T06:25:10Z

@jcouv I don’t want to have to deal with merge conflicts in the multiple PRs I have in progress. The items you suggest are already done in a follow up PR.

src/Compilers/Core/Portable/Operations/OperationNodes.cs

AlekseyTs · 2018-12-27T16:59:57Z

src/Compilers/Core/Portable/Operations/OperationCloner.cs

@@ -47,7 +47,13 @@ internal override IOperation VisitNoneOperation(IOperation operation, object arg
        private ImmutableArray<T> VisitArray<T>(ImmutableArray<T> nodes) where T : IOperation
        {
            // clone the array
-            return nodes.SelectAsArray(n => Visit(n));
+            return nodes.IsDefault ? default : nodes.SelectAsArray(n => Visit(n));


return nodes.IsDefault ? default : nodes.SelectAsArray(n => Visit(n)); [](start = 12, length = 70)

If we don't exposed default arrays, why is this change necessary? #Closed

AlekseyTs · 2018-12-27T17:02:18Z

src/Compilers/Core/Portable/Operations/OperationCloner.cs

+        private ImmutableArray<(U, T)> VisitArray<U, T>(ImmutableArray<(U, T)> nodes) where T : IOperation
+        {
+            // clone the array
+            return nodes.IsDefault ? default : nodes.SelectAsArray(n => (n.Item1, Visit(n.Item2)));


nodes.IsDefault [](start = 19, length = 15)

If we don't exposed default arrays, why this special case is necessary? #Closed

AlekseyTs · 2018-12-27T17:04:55Z

src/Compilers/Core/Portable/Operations/OperationCloner.cs

+            return nodes.IsDefault ? default : nodes.SelectAsArray(n => Visit(n));
+        }
+
+        private ImmutableArray<(U, T)> VisitArray<U, T>(ImmutableArray<(U, T)> nodes) where T : IOperation


U [](start = 72, length = 1)

Please remove type parameter U and replace its references with ISymbol. This way we can be sure that we are not skipping any IOperation nodes during the cloning operation.

AlekseyTs · 2018-12-27T17:13:17Z

It looks like this PR is subsumed by #32018. I am going to review that PR instead. Consider closing this one.

…onArmOperation (#32018) - Adds further IOperation nodes for recursive patterns: IDiscardPatternOperation, ISwitchExpressionOperation, ISwitchExpressionArmOperation - Follow-up to #31967 per #32012 - Issue #32012 also describes additional open issues to be addressed later. - Update compiler test plan. - Also revert some changes accidentally checked in to the patterns branch.

IOperation for recursive pattern and switch expression

1b3094b

Work in progress dotnet#27749

gafter added Concept-API This issue involves adding, removing, clarification, or modification of an API. Area-Compilers Feature - Pattern Matching Pattern Matching Feature - IOperation IOperation labels Dec 20, 2018

gafter added this to the 16.0.P2 milestone Dec 20, 2018

gafter self-assigned this Dec 20, 2018

gafter requested a review from a team as a code owner December 20, 2018 21:51