Extended property patterns: IOperation and Symbols #53082

alrz · 2021-05-01T18:51:11Z

Test plan: #52468

Split BoundPropertySubpattern and BoundPositionalSubpattern
Introduce BoundPropertySubpatternMember to keep track of symbol/syntax mapping
Adjust IOperation (There's an open issue on operation tree representation of unwrapped nullables: Test plan for "extended property patterns" #52468 (comment))

alrz · 2021-05-04T14:50:27Z

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

+ if (this is { Prop.Prop: null }) 
+ {
+ this.Prop.ToString();
+ this.Prop.Prop.ToString(); // 1
+ }


I have no idea why this works already. There's no changes to account for nested members in this PR.

jcouv · 2021-05-05T18:53:43Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs

- Symbol last = subpattern.Symbols.Last();
- // PROTOTYPE(extended-property-patterns) For a chain of members, the input type is no longer `inputType`
- if (last.ContainingType.Equals(inputType, TypeCompareKind.AllIgnoreOptions))
+ if (subpattern.Member is { Symbol: Symbol symbol, Receiver: var receiver } member &&


var

I didn't understand how the nullability analysis of the chain of accesses falls out from this implementation. I would have expected we'd analyze the nullability of the receiver first (the chain of receivers is not null) then learn that the current member/symbol is also not null.

nit: please use an explicit type here for readability #Closed

Me neither. I'll have to investigate to see where the state is produced.

nit: please use an explicit type here for readability

We don't want the null check here. Filed a suggestion to address this: dotnet/csharplang#4724

Let's keep a PROTOTYPE marker here then. Thanks

jcouv · 2021-05-05T18:57:10Z

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

+ }
+
+ [Fact]
+ public void ExtendedPropertyPatterns_Nullability()


Could you add a simliar test with two types (each having a property/field)?

class C1 { C2? Prop1 { get; } } class C2 { object? Prop2 { get; } } ``` #Closed

jcouv · 2021-05-05T21:17:38Z

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

+ Console.WriteLine(p is { X.Y: {}, Y.X: {} });
+ }
+}
+class P


It would be better to split this type into two (one containing X and one for Y) #Closed

jcouv · 2021-05-05T21:20:01Z

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

+ AssertEmpty(model.GetSymbolInfo(subpatterns[0]));
+ AssertEmpty(model.GetSymbolInfo(subpatterns[0].ExpressionColon));
+ var xy = subpatterns[0].ExpressionColon.Expression;
+ var xySymbol = model.GetSymbolInfo(xy);


Please also verify GetTypeInfo on those various pieces of syntax. #Closed

jcouv · 2021-05-05T21:22:30Z

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

+ Diagnostic(ErrorCode.ERR_PropertyLacksGet, "action").WithArguments("action").WithLocation(8, 38)
+ };
+
+ VerifyOperationTreeAndDiagnosticsForTest<IsPatternExpressionSyntax>(source, expectedOperationTree, expectedDiagnostics, parseOptions: TestOptions.RegularWithExtendedPropertyPatterns);


Let's also verify the control flow graph with VerifyFlowGraphAndDiagnosticsForTest (for a positive case above). PROTOTYPE for follow-up is okay too. #Closed

jcouv

Done with review pass (iteration 12)

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

alrz · 2021-05-06T11:57:40Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -2029,6 +2033,10 @@ private static bool IsUserDefinedTrueOrFalse(BoundUnaryOperator @operator)
 pattern.InputType, pattern.NarrowedType, nullability: default, convertedNullability: default,
 Compilation.Conversions.ClassifyBuiltInConversion(pattern.InputType, pattern.NarrowedType, ref discardedUseSiteInfo));
 }
+ if (lowestBoundNode is BoundPropertySubpatternMember member)
+ {
+ return new CSharpTypeInfo(member.Type, member.Type, nullability: default, convertedNullability: default, Conversion.Identity);


I guess we want to return the unwrapped type + nullable conversion if this is a nested member? If so, we probably need to capture this info from binding. Also I'm not sure what is the expected NullabilityInfo here.

Sounds like GetTypeInfo on subpattern name wasn't handled before or am I missing something?

cston · 2021-05-06T17:26:17Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs

- if (last.ContainingType.Equals(inputType, TypeCompareKind.AllIgnoreOptions))
+ // PROTOTYPE(extended-property-patterns): Investigate if we need to visit nested members; is there a test gap?
+ if (subpattern.Member is { Symbol: Symbol symbol, Receiver: var receiver } member &&
+ symbol.ContainingType.Equals(receiver is not null ? receiver.Type : inputType, TypeCompareKind.AllIgnoreOptions))


symbol.ContainingType.Equals(receiver is not null ? receiver.Type : inputType, TypeCompareKind.AllIgnoreOptions)

It's not clear to me why we compare the containing type to the receiver or input type using Equals(). Could the receiver or input type be a derived type?

The logic is unchanged from before but it sounds like the check is actually redundant. Remove?

I do recall one or two tests failing without the check but I couldn't find it now. I'll remove it to see if CI fails.

If no tests fail without this check, is there a simple test with a derived type that we could add that would demonstrate the difference in behavior?

I think this is the one: RecursivePatternNullInferenceWithDowncast_01 but now it's green.

I actually went back to main and rechecked. No test fails. I might be missing something though.

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

alrz · 2021-05-06T20:41:36Z

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

- property.GetPublicSymbol(), ImmutableArray<IArgumentOperation>.Empty, createReceiver(), _semanticModel, nameSyntax, property.Type.GetPublicSymbol(),
- isImplicit: isImplicit);
+ return new PropertyReferenceOperation(property.GetPublicSymbol(), ImmutableArray<IArgumentOperation>.Empty,
+ createReceiver(), _semanticModel, member.Syntax, property.Type.StrippedType().GetPublicSymbol(), isImplicit: false);


I don't think we should use StrippedType here. Maybe only if this is a nested member?

jcouv

LGTM Thanks (iteration 17). Will merge as soon as CI is green.

alrz · 2021-05-07T20:14:30Z

I think both #53082 (comment) and #53082 (comment) need to be revised based on the expected behavior.

jcouv · 2021-05-08T07:06:06Z

I think both #53082 (comment) and #53082 (comment) need to be revised based on the expected behavior.

Would you mind capturing those follow-up comments with PROTOTYPE markers? I'm afraid we'll lose or forget them otherwise.

alrz · 2021-05-09T08:34:14Z

@RikkiGibson Integration legs are consistently failing on x86. Is this normal?

jcouv · 2021-05-10T19:36:29Z

azp /run

jcouv · 2021-05-10T19:37:09Z

/azp run

azure-pipelines · 2021-05-10T19:37:31Z

Azure Pipelines successfully started running 3 pipeline(s).

dotnet-issue-labeler bot added the Area-Compilers label May 1, 2021

alrz marked this pull request as ready for review May 3, 2021 16:45

alrz requested review from a team as code owners May 3, 2021 16:45

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 4, 2021

jcouv mentioned this pull request May 5, 2021

Implement extended property patterns #52139

Merged

alrz added 11 commits May 5, 2021 19:24

Split BoundPropertySubpattern

9fe8451

Fixup code

0ed5192

Fixup IOperation

80898a1

Add IOperation tests

eeceee6

Introduce BoundPropertySubpatternMember

6c524f4

Fixup

7ffc35a

Revert code

cd022e1

Adjust baseline

1daeb2b

Remove unreachable code

2dc689d

Misc

5a1f62b

Fixup code

70a124a

alrz force-pushed the prop-patterns-symbols branch from 259e110 to 70a124a Compare May 5, 2021 15:17

alrz commented May 5, 2021

View reviewed changes

alrz requested review from jcouv and 333fred May 5, 2021 15:19

Fixup tests

3ea34d9

alrz removed request for a team May 5, 2021 16:11

jcouv self-assigned this May 5, 2021

jcouv added this to the C# 10 milestone May 5, 2021

jcouv reviewed May 5, 2021

View reviewed changes

alrz added 3 commits May 6, 2021 15:11

Tweaks

5c44180

Add PROTOTYPE comment

0e41923

PR feedback

fe23447

alrz commented May 6, 2021

View reviewed changes

cston reviewed May 6, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs Outdated Show resolved Hide resolved

alrz added 2 commits May 6, 2021 23:52

PR feedback

d46b412

Attempt to remove the ContainingType check

8c6ce2c

alrz commented May 6, 2021

View reviewed changes

cston approved these changes May 7, 2021

View reviewed changes

jcouv approved these changes May 7, 2021

View reviewed changes

jcouv enabled auto-merge (squash) May 7, 2021 19:24

jcouv disabled auto-merge May 8, 2021 07:05

Add PROTOTYPE comments

1cb3251

jcouv enabled auto-merge (squash) May 10, 2021 20:24

jcouv merged commit 2a82d69 into dotnet:features/extended-property-patterns May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended property patterns: IOperation and Symbols #53082

Extended property patterns: IOperation and Symbols #53082

alrz commented May 1, 2021 •

edited

Loading

alrz May 4, 2021

jcouv May 5, 2021 •

edited

Loading

alrz May 5, 2021

jcouv May 5, 2021

jcouv May 5, 2021 •

edited

Loading

jcouv May 5, 2021 •

edited

Loading

jcouv May 5, 2021 •

edited

Loading

jcouv May 5, 2021 •

edited

Loading

jcouv left a comment

alrz May 6, 2021 •

edited

Loading

alrz May 6, 2021 •

edited

Loading

cston May 6, 2021 •

edited

Loading

alrz May 6, 2021

alrz May 6, 2021

cston May 6, 2021

alrz May 6, 2021 •

edited

Loading

alrz May 6, 2021 •

edited

Loading

alrz May 6, 2021

jcouv left a comment

alrz commented May 7, 2021 •

edited

Loading

jcouv commented May 8, 2021

alrz commented May 9, 2021

jcouv commented May 10, 2021

jcouv commented May 10, 2021

azure-pipelines bot commented May 10, 2021

Extended property patterns: IOperation and Symbols #53082

Extended property patterns: IOperation and Symbols #53082

Conversation

alrz commented May 1, 2021 • edited Loading

alrz May 4, 2021

Choose a reason for hiding this comment

jcouv May 5, 2021 • edited Loading

Choose a reason for hiding this comment

alrz May 5, 2021

Choose a reason for hiding this comment

jcouv May 5, 2021

Choose a reason for hiding this comment

jcouv May 5, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv May 5, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv May 5, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv May 5, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

alrz May 6, 2021 • edited Loading

Choose a reason for hiding this comment

alrz May 6, 2021 • edited Loading

Choose a reason for hiding this comment

cston May 6, 2021 • edited Loading

Choose a reason for hiding this comment

alrz May 6, 2021

Choose a reason for hiding this comment

alrz May 6, 2021

Choose a reason for hiding this comment

cston May 6, 2021

Choose a reason for hiding this comment

alrz May 6, 2021 • edited Loading

Choose a reason for hiding this comment

alrz May 6, 2021 • edited Loading

Choose a reason for hiding this comment

alrz May 6, 2021

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

alrz commented May 7, 2021 • edited Loading

jcouv commented May 8, 2021

alrz commented May 9, 2021

jcouv commented May 10, 2021

jcouv commented May 10, 2021

azure-pipelines bot commented May 10, 2021

alrz commented May 1, 2021 •

edited

Loading

jcouv May 5, 2021 •

edited

Loading

jcouv May 5, 2021 •

edited

Loading

jcouv May 5, 2021 •

edited

Loading

jcouv May 5, 2021 •

edited

Loading

jcouv May 5, 2021 •

edited

Loading

alrz May 6, 2021 •

edited

Loading

alrz May 6, 2021 •

edited

Loading

cston May 6, 2021 •

edited

Loading

alrz May 6, 2021 •

edited

Loading

alrz May 6, 2021 •

edited

Loading

alrz commented May 7, 2021 •

edited

Loading