Make IPropertySubpattern Internal by 333fred · Pull Request #33178 · dotnet/roslyn

333fred · 2019-02-06T19:40:37Z

Updates the structure of IPropertySubpattern to be what we decided in the IOperation design meeting, and makes the implementation internal until we finalize C# 8.
Fixes #33018.

Part of dotnet#33018

…patterns.

mavasani · 2019-02-06T19:45:09Z

src/Workspaces/Core/Portable/Extensions/OperationExtensions.cs

                        return ValueUsageInfo.Write;

-                    case IRecursivePatternOperation _:
+                    case IOperation iop when iop.GetType().GetInterfaces().Any(i => i.Name == "IRecursivePatternOperation"):


Thanks for updating this. Can you also file a tracking issue to revert this IDE change? #Resolved

Added a note to #33175.

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

mavasani · 2019-02-06T19:49:13Z

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

+                            constantValue: default, isImplicit: isImplicit);
+                    }
+                case null:
+                    return null;


Should this also return an invalid operation? At least the doc comment here indicates Member is always non-null. If we do decide to allow null, we should make this explicit in doc comments. #Resolved

AlekseyTs · 2019-02-06T20:19:13Z

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

+        internal override IOperation VisitPropertySubpattern(IPropertySubpatternOperation operation, object argument)
+        {
+            return new PropertySubpatternOperation(
+                semanticModel: null,


semanticModel: null, [](start = 16, length = 20)

Semantic model should be preserved by the Cloner. #Closed

AlekseyTs · 2019-02-06T20:35:39Z

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

+                operation.Syntax,
+                IsImplicit(operation),
+                Visit(operation.Member),
+                (IPatternOperation)Visit(operation.Pattern));


(IPatternOperation)Visit(operation.Pattern) [](start = 16, length = 43)

It looks like in this case we are not setting the right substitution for the InstanceReferenceKind.PatternInput for the case when the operation.Pattern is a recursive pattern. Given that majority of nodes are internal, I'd suggest to not substitute InstanceReferenceKind.PatternInput with a capture in this PR and leave it as a regular node. We will have to figure out the right way to deal with this later. #Closed

Added a note to #33175 to follow up.

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

AlekseyTs · 2019-02-06T20:41:24Z

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

-
+                              IPropertySubpatternOperation (OperationKind.None, Type: null) (Syntax: 'Item1: var z')
+                                Member: 
+                                  IFieldReferenceOperation: System.Int32 (System.Int32 X, System.Int32 Y).Item1 (OperationKind.FieldReference, Type: System.Int32, IsImplicit) (Syntax: 'Item1')


IsImplicit [](start = 163, length = 10)

Is this supposed to be implicit? #Closed

AlekseyTs · 2019-02-06T20:43:13Z

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

+        internal IOperation CreatePropertySubpatternMember(Symbol symbol, SyntaxNode syntax)
+        {
+            var nameSyntax = (syntax is SubpatternSyntax subpatSyntax ? subpatSyntax.NameColon?.Name : null) ?? syntax;
+            bool isImplicit = nameSyntax != syntax;


nameSyntax != syntax [](start = 30, length = 20)

Should this condition be inversed? It looks like we are lacking a test that simply asserts IOperation tree shape for this code path, consider adding one. #Closed

AlekseyTs · 2019-02-06T20:47:49Z

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

+                    {
+                        var constantValue = field.ConstantValue is null ? default(Optional<object>) : new Optional<object>(field.ConstantValue);
+                        var receiver = new InstanceReferenceOperation(
+                            InstanceReferenceKind.PatternInput, _semanticModel, nameSyntax, field.ContainingType, constantValue, isImplicit: true);


field.ContainingType [](start = 92, length = 20)

It feels like field.ContainingType is not always the right value to use. The field can come from a base type. I think we would want all PatternInputs within the same pattern to use the same type, matching the MatchedType. #Closed

AlekseyTs · 2019-02-06T20:48:58Z

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

+                case PropertySymbol property:
+                    {
+                        var receiver = new InstanceReferenceOperation(
+                            InstanceReferenceKind.PatternInput, _semanticModel, nameSyntax, property.ContainingType, constantValue: default, isImplicit: true);


property.ContainingType [](start = 92, length = 23)

Same comment. #Closed

AlekseyTs · 2019-02-06T20:49:54Z

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

+                        var receiver = new InstanceReferenceOperation(
+                            InstanceReferenceKind.PatternInput, _semanticModel, nameSyntax, property.ContainingType, constantValue: default, isImplicit: true);
+                        return new PropertyReferenceOperation(
+                            property, receiver, ImmutableArray<IArgumentOperation>.Empty, _semanticModel, nameSyntax, property.Type.TypeSymbol,


ImmutableArray.Empty [](start = 48, length = 40)

In an error case, could we ran into a parameterized property? #Closed

It does not appear so. The closest I could get was an IConstantPatternOperation child of the IPropertyPatternOperation. Using an indexer like below doesn't parse close to cleanly at all, it results in multiple IncompleteMembers on the class.

class C { (int x, int y) this[int i] { get => default; set {} } void M(object o, bool b) { b = /*<bind>*/o is C { [1]: (var x, var y) }/*</bind>*/; } }

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

AlekseyTs · 2019-02-06T20:52:34Z

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

+            BoundSubpattern subpattern,
+            SyntaxNode syntax,
+            SemanticModel semanticModel)
+            : base(semanticModel, syntax, false)


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

Is this for Implicit? Consider using named parameter. #Closed

AlekseyTs · 2019-02-06T20:56:49Z

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

+
+        internal override void VisitPropertySubpattern(IPropertySubpatternOperation operation)
+        {
+            Assert.True(operation.Member is IMemberReferenceOperation);


Assert.True(operation.Member is IMemberReferenceOperation); [](start = 12, length = 59)

What about error cases? #Closed

AlekseyTs · 2019-02-06T20:59:57Z

Done with review pass (iteration 2) #Closed

333fred · 2019-02-06T22:01:06Z

@AlekseyTs addressed feedback.

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

333fred · 2019-02-06T22:04:55Z

@dotnet/roslyn-compiler for a second review.

AlekseyTs · 2019-02-06T22:11:18Z

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

+        public override ImmutableArray<IPropertySubpatternOperation> CreatePropertySubpatterns()
+        {
+            return _boundRecursivePattern.Properties.IsDefault ? ImmutableArray<IPropertySubpatternOperation>.Empty :
+                _boundRecursivePattern.Properties.SelectAsArray((p, fac) => fac.CreatePropertySubpattern(p, MatchedType), _operationFactory);


MatchedType [](start = 108, length = 11)

Is this going to prevent the lambda from caching the delegate? Consider passing it in a tuple parameter, together with operation factory. #Closed

AlekseyTs · 2019-02-06T22:19:37Z

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

+                default:
+                    // We should expose the symbol in this case somehow:
+                    // https://github.com/dotnet/roslyn/issues/33175
+                    return OperationFactory.CreateInvalidOperation(_semanticModel, nameSyntax, ImmutableArray<IOperation>.Empty, isImplicit: true);


true [](start = 141, length = 4)

Should we use isImplicit instead of hard coding true? #Closed

AlekseyTs · 2019-02-06T22:20:13Z

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

+      PropertySubpatterns (1):
+          IPropertySubpatternOperation (OperationKind.None, Type: null, IsInvalid) (Syntax: 'NotFound: int x')
+            Member: 
+              IInvalidOperation (OperationKind.Invalid, Type: null, IsInvalid, IsImplicit) (Syntax: 'NotFound')


IsImplicit [](start = 79, length = 10)

It feels like this shouldn't be implicit. #Closed

AlekseyTs · 2019-02-06T22:26:04Z

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

+{
+    void M1(object o, bool b)
+    {
+        b = /*<bind>*/o is C1 { Prop[1]: var x }/*</bind>*/;


Prop[1] [](start = 32, length = 7)

Consider adding a test without indexing, just the property name #Closed

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

AlekseyTs · 2019-02-06T22:33:53Z

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

+        internal override void VisitPropertySubpattern(IPropertySubpatternOperation operation)
+        {
+            Assert.True(operation.Member is IMemberReferenceOperation || operation.Member is IInvalidOperation);
+            var member = (IMemberReferenceOperation)operation.Member;


(IMemberReferenceOperation)operation.Member; [](start = 25, length = 44)

Is this going to throw when operation.Member is IInvalidOperation? #Closed

Is this not getting caught by tests? Even when IOperation hook is enabled?

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

I should probably run these tests through the hook, I haven't. But no, this visitor isn't called on our regular IOperation tests.

In reply to: 254480510 [](ancestors = 254480510,254480084)

AlekseyTs · 2019-02-06T22:40:23Z

It looks like there are legitimate test failures. #Closed

AlekseyTs · 2019-02-06T22:40:37Z

Done with review pass (iteration 3) #Closed

333fred · 2019-02-06T22:43:32Z

There were on the first run, second run hasn't completed yet.

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

333fred · 2019-02-06T22:54:43Z

@AlekseyTs addressed feedback.

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

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

AlekseyTs · 2019-02-06T23:00:56Z

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

+            {
+                case IFieldSymbol field:
+                case IPropertySymbol prop:
+                case IErrorTypeSymbol error:


case IErrorTypeSymbol error: [](start = 16, length = 28)

This is probably not a valid case any more. #Resolved

AlekseyTs

LGTM (iteration 4), assuming the tests are passing.

cston · 2019-02-06T23:29:25Z

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

+            SyntaxNode syntax,
+            bool isImplicit,
+            IOperation member,
+            IPatternOperation pattern)


Minor point: It looks like constructors for other IOperation implementations have child arguments before semanticModel, syntax, isImplicit arguments. #Resolved

cston · 2019-02-06T23:39:13Z

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

+
+        internal IOperation CreatePropertySubpatternMember(Symbol symbol, ITypeSymbol matchedType, SyntaxNode syntax)
+        {
+            var nameSyntax = (syntax is SubpatternSyntax subpatSyntax ? subpatSyntax.NameColon?.Name : null) ?? syntax;


(syntax is SubpatternSyntax subpatSyntax ? subpatSyntax.NameColon?.Name : null) [](start = 29, length = 79)

Could remove the : null with (syntax as SubpatternSyntax)?.NameColon?.Name. #Resolved

cston · 2019-02-06T23:42:14Z

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

+            {
+                case FieldSymbol field:
+                    {
+                        var constantValue = field.ConstantValue is null ? default(Optional<object>) : new Optional<object>(field.ConstantValue);


field.ConstantValue [](start = 123, length = 19)

Should this be field.ContantValue.Value?

If so, please use the helper method:

var constantValue = ConvertToOptional(field.ConstantValue);

Please add a test for a field with constant value if possible. #Resolved

cston · 2019-02-06T23:51:34Z

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

+                case FieldSymbol field:
+                    {
+                        var constantValue = field.ConstantValue is null ? default(Optional<object>) : new Optional<object>(field.ConstantValue);
+                        var receiver = new InstanceReferenceOperation(


InstanceReferenceOperation [](start = 43, length = 26)

Are we testing with static fields and properties?

class C { static int F; static void M(object o) { bool b = o is C { F: 1 } c; } } ``` #Resolved

cston · 2019-02-06T23:53:14Z

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

        private static readonly IConvertibleConversion s_boxedIdentityConversion = Conversion.Identity;

-        private static Optional<object> ConvertToOptional(ConstantValue value)
+        internal static Optional<object> ConvertToOptional(ConstantValue value)


internal [](start = 8, length = 8)

Is this change necessary? #Resolved

333fred · 2019-02-07T06:16:09Z

@cston none of these comments seem blocking, so I've added notes to #33175 to follow up. #Resolved

…m in dotnet#33175

…33175 (#45454)

gafter and others added 2 commits January 31, 2019 17:02

Draft of changes to IOperation for property patterns

21fe487

Part of dotnet#33018

Finalized internalization and implementation details of recursive sub…

d2523e5

…patterns.

333fred added Area-Compilers Feature - IOperation IOperation Feature - Flow Analysis Flow Analysis labels Feb 6, 2019

333fred requested review from a team and AlekseyTs February 6, 2019 19:40

333fred requested a review from a team as a code owner February 6, 2019 19:40

mavasani reviewed Feb 6, 2019

View reviewed changes

AlekseyTs reviewed Feb 6, 2019

View reviewed changes

Address PR Feedback.

2c3d73b

AlekseyTs reviewed Feb 6, 2019

View reviewed changes

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IIsPatternExpression.cs Show resolved Hide resolved

AlekseyTs reviewed Feb 6, 2019

View reviewed changes

Additional PR feedback.

22e474c

AlekseyTs reviewed Feb 6, 2019

View reviewed changes

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IIsPatternExpression.cs Show resolved Hide resolved

AlekseyTs reviewed Feb 6, 2019

View reviewed changes

AlekseyTs approved these changes Feb 6, 2019

View reviewed changes

This was referenced Feb 6, 2019

Bad Error in Recursive Pattern with Indexed Properties #33189

Closed

Follow up on IPropertySubpatternOperation Issues #33175

Open

cston reviewed Feb 6, 2019

View reviewed changes

gafter approved these changes Feb 7, 2019

View reviewed changes

333fred closed this Feb 7, 2019

333fred reopened this Feb 7, 2019

cston approved these changes Feb 7, 2019

View reviewed changes

333fred merged commit 81e9db8 into dotnet:master Feb 7, 2019

333fred deleted the rpatterns-33018 branch February 7, 2019 08:07

gafter mentioned this pull request Feb 11, 2019

Tuples in public API of IRecursivePatternOperation #32756

Closed

gafter pushed a commit to gafter/roslyn that referenced this pull request Jun 25, 2020

Address review comments from dotnet#33178 documented in comment strea…

d49d5b4

…m in dotnet#33175

gafter mentioned this pull request Jun 25, 2020

Address review comments from #33178 documented in comment stream in #33175 #45454

Merged

gafter pushed a commit that referenced this pull request Jul 6, 2020

Address review comments from #33178 documented in comment stream in #…

3138112

…33175 (#45454)

Conversation

333fred commented Feb 6, 2019

Uh oh!

mavasani Feb 6, 2019 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

mavasani Feb 6, 2019 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented Feb 6, 2019

Uh oh!

333fred commented Feb 6, 2019

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mavasani Feb 6, 2019 •

edited by 333fred

Loading

mavasani Feb 6, 2019 •

edited by 333fred

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs commented Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited

Loading

AlekseyTs commented Feb 6, 2019 •

edited by 333fred

Loading

AlekseyTs commented Feb 6, 2019 •

edited

Loading

AlekseyTs Feb 6, 2019 •

edited by gafter

Loading

cston Feb 6, 2019 •

edited by gafter

Loading

cston Feb 6, 2019 •

edited by gafter

Loading

cston Feb 6, 2019 •

edited by gafter

Loading

cston Feb 6, 2019 •

edited by gafter

Loading

cston Feb 6, 2019 •

edited by gafter

Loading

333fred commented Feb 7, 2019 •

edited by gafter

Loading