Extended property patterns: address PROTOTYPE comments by jcouv · Pull Request #53594 · dotnet/roslyn

jcouv · 2021-05-21T17:09:41Z

Addresses the remaining PROTOTYPE markers and work items.

Fixes #52956 (tracking usage of fields)
Fixes #53484 (deal with fallout of better error recovery in patterns parsing)

IDE changes: adds ability to infer type and generate variable c is { Property.|[YetUndefined|]: ... }

Test plan #52468

jcouv · 2021-05-21T17:21:43Z

...esAndExtensions/Workspace/CSharp/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs


                        foreach (var subPattern in positionalPart.Subpatterns)
                        {
-                            // PROTOTYPE(extended-property-patterns) ExpressionColon


📝 We don't need to handle ExpressionColon in positional patterns

jcouv · 2021-05-21T17:27:10Z

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

+    public void M()
+    {
+        _ = this is { Field1.Field2.Field3: {} };
+        _ = this is { Field4: {} };


📝 We previously didn't count this as a read on Field4 (existing bug got fixed). Previously produced: warning CS0169: The field 'C.Field4' is never used

jcouv · 2021-05-21T17:29:48Z

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

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


📝 ~~We need to discuss how to properly represent such scenarios (involving nullable value types) in IOperation and CFG. There's existing PROTOTYPE markers tracking this.~~

We're representing is { Field1.Field2.Field3: null } as is { Field1: { Field2: { Field 3: null } } } #Closed

jcouv · 2021-05-21T17:30:49Z

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

+{
+    public unsafe void M(S* s)
+    {
+        if (0 is s->X) {}


📝 Previously resulted in a parsing error

jcouv · 2021-05-21T17:39:01Z

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

+  Pattern:
+    IRecursivePatternOperation (OperationKind.RecursivePattern, Type: null) (Syntax: '{ Field1.Fi ... ld4: null }') (InputType: A, NarrowedType: A, DeclaredSymbol: null, MatchedType: A, DeconstructSymbol: null)
+      DeconstructionSubpatterns (0)
+      PropertySubpatterns (2):


📝 One way we could solve the problem is representing like the following, using a new IOperation node that has an array of property references onto implicit receivers (as opposed to a chain).

IPropertySubpatternOperation (OperationKind.PropertySubpattern, Type: null) (Syntax: 'Field1.Fiel ... ield3: null') Member: IExtendedPropertyPatternOperation // new Properties: IFieldReferenceOperation: B? A.Field1 (OperationKind.FieldReference, Type: A) (Syntax: 'Field1') Instance Receiver: IInstanceReferenceOperation (ReferenceKind: PatternInput) (OperationKind.InstanceReference, Type: B, IsImplicit) (Syntax: 'Field1') IFieldReferenceOperation: C? B.Field2 (OperationKind.FieldReference, Type: C) (Syntax: 'Field2') Instance Receiver: IInstanceReferenceOperation (ReferenceKind: PatternInput) (OperationKind.InstanceReference, Type: B, IsImplicit) (Syntax: 'Field2') IFieldReferenceOperation: System.Object C.Field3 (OperationKind.FieldReference, Type: System.Object) (Syntax: 'Field3') Instance Receiver: IInstanceReferenceOperation (ReferenceKind: PatternInput) (OperationKind.InstanceReference, Type: C, IsImplicit) (Syntax: 'Field3')

#Closed

If we decide to use ConvertedType here:

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

Lines 2038 to 2039 in 313caf1

// PROTOTYPE(extended-property-patterns) Should we capture unwrapped nullables in nested members as the ConvertedType?

return new CSharpTypeInfo(member.Type, member.Type, nullability: default, convertedNullability: default, Conversion.Identity);

I think it would make sense to also use IConversionOperation to represent a nullable value unwrap in the chain.

From chat with Fred and Aleksey, I'll adjust the IOperation tree to take advantage of the fact that the feature is syntactic sugar. c is { Prop1.Prop2: null } -> c is { Prop1: { Prop2: null } }. We can do the same for IOperation.

alrz · 2021-05-21T17:35:31Z

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

+                {
+                    expr = allowExtendedProperties
+                        ? CheckFeatureAvailability(expr, MessageID.IDS_FeatureExtendedPropertyPatterns)
+                        : AddError(expr, ErrorCode.ERR_IdentifierExpected);


Instead of this I think it would be simpler to just check ExpressionColon.Expression is IdentifierName at use-sites and report. That would make this a semantic error but we're already checking invalid names there. #Resolved

alrz · 2021-05-21T17:38:49Z

eng/targets/Settings.props

        -->
-        <!-- PROTOTYPE(extended-property-patterns) PublicAPIs -->
-        <NoWarn>$(NoWarn);1573;1591;1701;RS0016</NoWarn>
+        <NoWarn>$(NoWarn);1573;1591;1701</NoWarn>


Thanks a lot for sorting this out. Have we fixed the code action or this is done manually? (I have no idea how this could be done manually) #Resolved

I've done it semi-manually: copy sections of generated code into real files, then trigger publicAPI fixer on those, repeat... It's better than manually editting, by far :-)

alrz · 2021-05-21T17:39:25Z

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

            {
                var subpatternSyntax = node.Subpatterns[i];
+                // If the expression before the colon isn't just a name, we'll have reported a parsing error.
+                Debug.Assert(subpatternSyntax.NameColon is not null || subpatternSyntax.ExpressionColon is null || subpatternSyntax.ExpressionColon.HasErrors);


When subpatternSyntax.ExpressionColon is null NameColon is also null. #Resolved

jcouv · 2021-05-24T17:30:33Z

@alrz @dotnet/roslyn-compiler for review. Thanks

cston · 2021-05-24T17:53:54Z

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

                    }
+                    else if (subPattern.ExpressionColon != null)
+                    {
+                        diagnostics.Add(ErrorCode.ERR_IdentifierExpected, subPattern.ExpressionColon.Expression.Location);


ERR_IdentifierExpected

Wasn't this reported in the parser? #Resolved

Clarified offline. The scenario is is (A.B: <pattern>, ...)

cston · 2021-05-24T17:54:15Z

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

                }
+                else if (subpatternSyntax.ExpressionColon != null)
+                {
+                    diagnostics.Add(ErrorCode.ERR_IdentifierExpected, subpatternSyntax.ExpressionColon.Expression.Location);


ERR_IdentifierExpected

Wasn't this reported in the parser? Same question below. #Resolved

cston · 2021-05-24T17:59:56Z

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

+                                    {
+                                        while (member is not null && !member.HasErrors)
+                                        {
+                                            if (_sourceAssembly is not null && member.Symbol is FieldSymbol field)


_sourceAssembly is not null

Consider checking this in the outer if, before sub is .... #Closed

cston · 2021-05-24T18:00:38Z

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

+                                {
+                                    if (sub is BoundPropertySubpattern { Member: var member })
+                                    {
+                                        while (member is not null && !member.HasErrors)


!member.HasErrors

Is this check necessary? #Closed

cston · 2021-05-24T18:58:29Z

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

+                    // `.LastProp: <pattern operation>` portion (treated as `{ LastProp: <pattern operation> }`)
+                    IPatternOperation pattern = (IPatternOperation)Create(subpattern.Pattern);
+                    result = createPropertySubpattern(member.Symbol, pattern, inputType, nameSyntax, isSingle: member.Receiver is null);
+                }


Consider moving this case before the do { ... } while (...); loop. It doesn't look like it shares anything with the rest of the loop. #Closed

cston · 2021-05-24T18:59:00Z

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

+                }
+                else
+                {
+                    // Create an operation for a preceding property acess:


acess

Typo. #Closed

cston · 2021-05-24T19:00:20Z

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

+
+            int getExtendedPropertySlot(BoundPropertySubpatternMember member, int inputSlot)
+            {
+                if (member is null || member.Symbol is null)


member is null

Is this ever true? #Closed

cston · 2021-05-25T04:21:13Z

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

+            // or
+            // `.LastProp: <pattern operation>` portion (treated as `{ LastProp: <pattern operation> }`)
+            var nameSyntax = member.Syntax;
+            var inputType = member.Receiver?.Type.StrippedType().GetPublicSymbol() ?? matchedType;


member.Receiver?.Type.StrippedType().GetPublicSymbol() ?? matchedType

It looks like this value is/was shared between the two createPropertySubpattern() calls. (Sorry for missing that with my earlier comment.) Perhaps add a local function to calculate the value. #Closed

cston · 2021-05-25T04:27:33Z

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

-                    Conversion: CommonConversion (Exists: True, IsIdentity: False, IsNumeric: False, IsReference: True, IsUserDefined: False) (MethodSymbol: null)
-                    Operand: 
-                      ILiteralOperation (OperationKind.Literal, Type: null, Constant: null) (Syntax: 'null')
+          IPropertySubpatternOperation (OperationKind.PropertySubpattern, Type: null, IsImplicit) (Syntax: 'prop')


IsImplicit

Why are the IPropertySubpatternOperation instances marked as implicit? I wouldn't expect to have implicit parent nodes with explicit children. #Closed

Only one IOperation node can be explicit for a given piece of syntax. The property reference below is the IOperation node associated to that syntax which isn't implicit.

cston · 2021-05-25T04:41:55Z

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

+            var compilation = CreateCompilation(source, parseOptions: TestOptions.Regular9);
+            compilation.VerifyEmitDiagnostics(
+                // (8,22): error CS8652: The feature 'extended property patterns' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.
+                //         _ = this is (Property.Property: null, Property: null);


Property.Property

It seems surprising that we're parsing this as an extended property pattern, rather than just a syntax error at the '.'. Is there a reason to allow nested properties here? #Resolved

Both positional and recurisve patterns use the same syntax node SubpatternSyntax and share the same parsing logic (ParseSubpatternElement).
There's no reason beyond that.

cston · 2021-05-25T04:44:04Z

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

+            var compilation = CreateCompilation(program, parseOptions: TestOptions.RegularWithExtendedPropertyPatterns);
+            compilation.VerifyEmitDiagnostics(
+                // (7,21): error CS0122: 'C1.Prop1' is inaccessible due to its protection level
+                //         _ = c1 is { Prop1.Prop2: null };


Prop2

Should we report an error on Prop2 as well?

It looks like we don't report an error on Prop2 on a similar dotted reference outside of patterns (see sharplab.io) so this is consistent, but the missing second error in both cases seem unexpected. #WontFix

That is not part of this PR, and because the behavior matches pre-existing member access behavior, I'll leave as-is.
I don't think this is simple to fix. BindInstanceMemberAccess returns with a null ExpressionSymbol in this scenario, so when we get to Prop2 we're only looking at an error type...

cston · 2021-05-25T04:52:47Z

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

+        public class FlowAnalysisTests : FlowTestBase
+        {
+            [Fact]
+            public void RegionInIsPattern01()


RegionInIsPattern01

Is this testing a new scenario? Should this test be moved to another file? #Resolved

An IDE test crashed in an earlier iteration of this PR. I created a compiler-level repro. It follows many other flow analysis tests, but I prefered to keep it together with the rest of the feature.

cston · 2021-05-25T04:54:54Z

src/Compilers/Test/Core/Compilation/OperationTreeVerifier.cs

            char[] newLineChars = Environment.NewLine.ToCharArray();
            string actual = actualOperationTree.Trim(newLineChars);
-            actual = actual.Replace("\"", "\"\"");
+            actual = actual.Replace("\"", "\"\"").Replace(" \n", "\n").Replace(" \r", "\r");


Replace(" \n", "\n").Replace(" \r", "\r")

Just curious: what required the changes to Verify()? #Resolved

I've been annoyed for a long time about the extra space output by the IOperation tests. This PR has a number of IOperation tests where I ran into this issue.
This change allows the space to be ignored in comparison and removed in "actual".

cston · 2021-05-25T04:57:42Z

...esAndExtensions/Workspace/CSharp/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

+                        }

-                    return result.ToImmutableAndFree();
+                        return result.ToImmutableAndFree();


Consider extracting a helper method for use here and in if above. #Closed

jcouv · 2021-05-25T18:56:47Z

@333fred @dotnet/roslyn-compiler for a second review. Thanks

CyrusNajmabadi · 2021-05-25T21:38:23Z

...esAndExtensions/Workspace/CSharp/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

+
+                IEnumerable<TypeInferenceInfo> InferTypeInSubpatternCore(ExpressionSyntax expression)
                {
                    var result = ArrayBuilder<TypeInferenceInfo>.GetInstance();


Suggested change

var result = ArrayBuilder<TypeInferenceInfo>.GetInstance();

using var result = TemporaryAray<TypeInferenceInfo>.Empty;

``` #Resolved

TemporaryArray is preferred when the expected case is a small number of results (4 or less).

CyrusNajmabadi · 2021-05-25T21:38:31Z

...esAndExtensions/Workspace/CSharp/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

@@ -1478,8 +1492,6 @@ private IEnumerable<TypeInferenceInfo> InferTypeInSubpattern(

                    return result.ToImmutableAndFree();


Suggested change

return result.ToImmutableAndFree();

return result.ToImmutableAndClear();

``` #Resolved

CyrusNajmabadi

IDE side lgtm.

333fred · 2021-05-26T21:19:35Z

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

+        {
+            case var x when e.Property1.Property2.ToString() == null: // 1
+                break;
+            case { Property1.Property2: not null }:


not nul

Nit: consider just making this a theory with all the different pure tests (such as both null and not null).

333fred

Compiler side LGTM (commit 12)

alrz · 2021-05-27T09:10:38Z

...esAndExtensions/Workspace/CSharp/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

+                    if (subpattern.NameColon != null)
+                    {
+                        return InferTypeInSubpatternCore(subpattern.NameColon.Name);
+                    }
+                    else if (subpattern.ExpressionColon is { Expression: MemberAccessExpressionSyntax memberAccess })
+                    {
+                        return InferTypeInSubpatternCore(memberAccess.Name);
+                    }


nit: You could probably just call InferTypeInSubpatternCore(subpattern.ExpressionColon.Expression) instead of these two ifs. We've verified GetSymbolInfo works with a member access. #Resolved

alrz · 2021-06-13T07:48:08Z

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

+            if (this is c!) {}
+            if (this is (c!)) {}
+            if (this is Type!) {} // 1
+            if (this is ContainerType!.Type) {} // 2
+            if (this is ContainerType.Type!) {} // 3


@jcouv Why is it expected to accept suppression in the first two?

Those already compile (in C# 9).

I get parsing errors on sharplab default branch.

https://sharplab.io/#v2:D4AQTAjAsAUAxAOwK4BsUEMBGKCmACHBLXWEAZj3DwGE8BvWPJyikAFjwFkAKASnsbMhAYwD2CAM4AXGnmF4AvHmRoA3IKFMA+orxSAFgEsJeY3ICE6mEIC+sG0A

I think patterns did not accept suppression in the previous version of the compiler.

Oops, I used sharplab on main, which already has this change...
Filed #54071

Extended property patterns: address PROTOTYPE comments

95a950f

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

jcouv self-assigned this May 21, 2021

ghost added the Area-Compilers label May 21, 2021

jcouv added the Area-IDE label May 21, 2021

jcouv requested a review from CyrusNajmabadi May 21, 2021 17:20

jcouv commented May 21, 2021

View reviewed changes

formatting

062fff4

alrz reviewed May 21, 2021

View reviewed changes

Julien Couvreur added 2 commits May 22, 2021 00:27

Adjust IOperation and fix regressions

f5ca3bd

Learn from pure null tests on extended properties early in switches

a08ee8f

jcouv force-pushed the property-patterns branch from 7e1b64f to a08ee8f Compare May 23, 2021 21:12

Use binding error instead

2ae8251

jcouv force-pushed the property-patterns branch from de2379f to 2ae8251 Compare May 23, 2021 21:31

Julien Couvreur added 2 commits May 23, 2021 16:44

Fix tests

ee7416a

Fix flow analysis issue

f057729

jcouv marked this pull request as ready for review May 24, 2021 05:49

jcouv requested review from a team as code owners May 24, 2021 05:49

tweak name

e577d0e

cston reviewed May 24, 2021

View reviewed changes

Julien Couvreur added 2 commits May 24, 2021 17:51

Address feedback

6945eb1

Address feedback

715546c

cston reviewed May 25, 2021

View reviewed changes

Address feedback

44c1373

jcouv requested a review from cston May 25, 2021 05:56

cston approved these changes May 25, 2021

View reviewed changes

CyrusNajmabadi reviewed May 25, 2021

View reviewed changes

CyrusNajmabadi approved these changes May 25, 2021

View reviewed changes

Address feedback from Cyrus

717e253

333fred reviewed May 26, 2021

View reviewed changes

333fred approved these changes May 26, 2021

View reviewed changes

Use a theory

60393b0

alrz approved these changes May 27, 2021

View reviewed changes

Address feedback from Ali

ae4ea73

jcouv merged commit 9cd9209 into dotnet:features/extended-property-patterns May 27, 2021

jcouv deleted the property-patterns branch May 27, 2021 20:34

This was referenced May 27, 2021

Reject null-suppression operator in patterns #53484

Closed

False warning: pattern matching on a field doesn't count as a use #52956

Closed

alrz reviewed Jun 13, 2021

View reviewed changes

	// PROTOTYPE(extended-property-patterns) Should we capture unwrapped nullables in nested members as the ConvertedType?
	return new CSharpTypeInfo(member.Type, member.Type, nullability: default, convertedNullability: default, Conversion.Identity);

	var result = ArrayBuilder<TypeInferenceInfo>.GetInstance();
	using var result = TemporaryAray<TypeInferenceInfo>.Empty;
	``` #Resolved

		@@ -1478,8 +1492,6 @@ private IEnumerable<TypeInferenceInfo> InferTypeInSubpattern(

		return result.ToImmutableAndFree();

	return result.ToImmutableAndFree();
	return result.ToImmutableAndClear();
	``` #Resolved

Conversation

jcouv commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 21, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv May 21, 2021 • 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!

alrz May 21, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

alrz May 21, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz May 21, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz May 21, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 24, 2021

Uh oh!

cston May 24, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 24, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 24, 2021 • 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!

cston May 24, 2021 • 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!

cston May 24, 2021 • 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!

cston May 24, 2021 • 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!

cston May 24, 2021 • 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!

cston May 25, 2021 • 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!

cston May 25, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 21, 2021 •

edited

Loading

jcouv May 21, 2021 •

edited by 333fred

Loading

jcouv May 21, 2021 •

edited

Loading

alrz May 21, 2021 •

edited

Loading

alrz May 21, 2021 •

edited by jcouv

Loading

alrz May 21, 2021 •

edited by jcouv

Loading

alrz May 21, 2021 •

edited by jcouv

Loading

cston May 24, 2021 •

edited by jcouv

Loading

cston May 24, 2021 •

edited by jcouv

Loading

cston May 24, 2021 •

edited

Loading

cston May 24, 2021 •

edited

Loading

cston May 24, 2021 •

edited

Loading

cston May 24, 2021 •

edited

Loading

cston May 24, 2021 •

edited

Loading

cston May 25, 2021 •

edited

Loading

cston May 25, 2021 •

edited

Loading

cston May 25, 2021 •

edited by jcouv

Loading

cston May 25, 2021 •

edited by jcouv

Loading

cston May 25, 2021 •

edited by jcouv

Loading

cston May 25, 2021 •

edited by jcouv

Loading

cston May 25, 2021 •

edited

Loading

CyrusNajmabadi May 25, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 25, 2021 •

edited by jcouv

Loading

333fred May 26, 2021 •

edited

Loading

alrz May 27, 2021 •

edited by jcouv

Loading