Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extended property patterns: address PROTOTYPE comments #53594

Merged

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented May 21, 2021

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 jcouv added this to the C# 10 milestone May 21, 2021
@jcouv jcouv self-assigned this May 21, 2021
@jcouv jcouv added the Area-IDE label May 21, 2021
@jcouv jcouv requested a review from CyrusNajmabadi May 21, 2021 17:20
@@ -1559,7 +1580,6 @@ private IEnumerable<TypeInferenceInfo> GetTypesForRecursivePattern(RecursivePatt

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public void M()
{
_ = this is { Field1.Field2.Field3: {} };
_ = this is { Field4: {} };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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

}

[Fact]
public void ExtendedPropertyPatterns_IOperation_FieldsInStructs()
Copy link
Member Author

@jcouv jcouv May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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

{
public unsafe void M(S* s)
{
if (0 is s->X) {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Previously resulted in a parsing error

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):
Copy link
Member Author

@jcouv jcouv May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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

Copy link
Member

@alrz alrz May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to use ConvertedType here:

// 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@alrz alrz May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -208,8 +208,7 @@
and hence suppress this warning until we get closer to release and a more
thorough documentation story
-->
<!-- PROTOTYPE(extended-property-patterns) PublicAPIs -->
<NoWarn>$(NoWarn);1573;1591;1701;RS0016</NoWarn>
<NoWarn>$(NoWarn);1573;1591;1701</NoWarn>
Copy link
Member

@alrz alrz May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

@@ -873,10 +900,12 @@ internal static bool IsZeroElementTupleType(TypeSymbol type)
for (int i = 0; i < node.Subpatterns.Count; i++)
{
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);
Copy link
Member

@alrz alrz May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jcouv jcouv marked this pull request as ready for review May 24, 2021 05:49
@jcouv jcouv requested review from a team as code owners May 24, 2021 05:49
@jcouv
Copy link
Member Author

jcouv commented May 24, 2021

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

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

@cston cston May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ERR_IdentifierExpected

Wasn't this reported in the parser? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (subpatternSyntax.NameColon != null)
{
// error: name not permitted in ITuple deconstruction
diagnostics.Add(ErrorCode.ERR_ArgumentNameInITuplePattern, subpatternSyntax.NameColon.Location);
}
else if (subpatternSyntax.ExpressionColon != null)
{
diagnostics.Add(ErrorCode.ERR_IdentifierExpected, subpatternSyntax.ExpressionColon.Expression.Location);
Copy link
Member

@cston cston May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ERR_IdentifierExpected

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

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

@cston cston May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_sourceAssembly is not null

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

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

@cston cston May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!member.HasErrors

Is this check necessary? #Closed

// `.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);
}
Copy link
Member

@cston cston May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
else
{
// Create an operation for a preceding property acess:
Copy link
Member

@cston cston May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acess

Typo. #Closed


int getExtendedPropertySlot(BoundPropertySubpatternMember member, int inputSlot)
{
if (member is null || member.Symbol is null)
Copy link
Member

@cston cston May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

member is null

Is this ever true? #Closed

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

@cston cston May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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')
Copy link
Member

@cston cston May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsImplicit

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
Copy link
Member

@cston cston May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 };
Copy link
Member

@cston cston May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

public class FlowAnalysisTests : FlowTestBase
{
[Fact]
public void RegionInIsPattern01()
Copy link
Member

@cston cston May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RegionInIsPattern01

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -57,9 +57,9 @@ public static void Verify(string expectedOperationTree, string actualOperationTr
{
char[] newLineChars = Environment.NewLine.ToCharArray();
string actual = actualOperationTree.Trim(newLineChars);
actual = actual.Replace("\"", "\"\"");
actual = actual.Replace("\"", "\"\"").Replace(" \n", "\n").Replace(" \r", "\r");
Copy link
Member

@cston cston May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".


return result.ToImmutableAndFree();
return result.ToImmutableAndFree();
Copy link
Member

@cston cston May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jcouv jcouv requested a review from cston May 25, 2021 05:56
@jcouv
Copy link
Member Author

jcouv commented May 25, 2021

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


return SpecializedCollections.EmptyEnumerable<TypeInferenceInfo>();

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

@CyrusNajmabadi CyrusNajmabadi May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

return result.ToImmutableAndFree();
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDE side lgtm.

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

@333fred 333fred May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not nul

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

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiler side LGTM (commit 12)

Comment on lines 1465 to 1472
if (subpattern.NameColon != null)
{
return InferTypeInSubpatternCore(subpattern.NameColon.Name);
}
else if (subpattern.ExpressionColon is { Expression: MemberAccessExpressionSyntax memberAccess })
{
return InferTypeInSubpatternCore(memberAccess.Name);
}
Copy link
Member

@alrz alrz May 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jcouv jcouv merged commit 9cd9209 into dotnet:features/extended-property-patterns May 27, 2021
@jcouv jcouv deleted the property-patterns branch May 27, 2021 20:34
Comment on lines +938 to +942
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jcouv jcouv Jun 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those already compile (in C# 9).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants