Extended property patterns: main IDE scenarios by jcouv · Pull Request #53280 · dotnet/roslyn

jcouv · 2021-05-08T17:51:29Z

Changes in this PR: completion (recommend symbols in extended pattern chain, don't recommend "not" and such after a .) and formatting
Added tests for FAR. Manually validated QuickInfo, GoToDef, Rename (which share much of the same infrastructure)
Not sure if we want GenerateVariable to work on extended property patterns, so added a note to test plan.

A note on completion:
We parse is { Property.$$ } as a member access expression (Property.) in a constant pattern, inside a property pattern.
This is correct and will remain that way due to list-patterns. But the binding/lookup rules for Property in a list-pattern are different than those in an extended-property-pattern. That means that we can't directly rely on a compiler API (such as GetTypeInfo which works to get the type of the member access in a fully-typed extended property pattern is { Property.X: ... }) to get the types of the properties in the chain.

FYI @alrz

Test plan: #52468

CyrusNajmabadi · 2021-05-08T17:59:54Z

Suggested change

IEnumerable<ISymbol> members = semanticModel.LookupSymbols(position, type);

var members = semanticModel.LookupSymbols(position, type);

``` #Resolved

CyrusNajmabadi · 2021-05-08T18:00:12Z

Suggested change

var matches = members.Where(m => m.Name == name).ToArray();

var matches = members.WhereAsArray(m => m.Name == name);

``` #Resolved

why not pass in 'name' into GetCandidatePropertiesAndFields?

why not pass in 'name' into GetCandidatePropertiesAndFields?

This local function is used twice. One of the usages doesn't have a name to filter on.

WhereAsArray

I don't think there's such extension on IEnumerable

this can use ImmutableArray from bottom to top :)

CyrusNajmabadi · 2021-05-08T18:00:19Z

Suggested change

static IEnumerable<ISymbol> GetCandidatePropertiesAndFields(Document document, int position, SemanticModel semanticModel, ITypeSymbol type)

static ImmutableArray<ISymbol> GetCandidatePropertiesAndFields(Document document, SemanticModel semanticModel, int position, ITypeSymbol type)

``` #Resolved

CyrusNajmabadi · 2021-05-08T18:00:51Z

Suggested change

type = matches[0] switch

{

IPropertySymbol property => property.Type,

IFieldSymbol field => field.Type,

_ => null

};

return type;

return matches[0] switch

{

IPropertySymbol property => property.Type,

IFieldSymbol field => field.Type,

_ => null

};

``` #Resolved

…rns' into property-ide

jcouv · 2021-05-10T20:55:17Z

/azp run

azure-pipelines · 2021-05-10T20:55:40Z

Azure Pipelines successfully started running 3 pipeline(s).

jcouv · 2021-05-10T22:05:51Z

Test plan: #51199

CyrusNajmabadi · 2021-05-12T19:36:44Z

+                Await state.AssertNoCompletionSession()
+                Assert.Contains("is { CProperty.IntProperty: 2, CProperty.ShortProperty: 3", state.GetLineTextFromCaretPosition(), StringComparison.Ordinal)
+            End Using
+        End Function


can we have a test where the user is writing a new proeprty like this above an existing pattern property? #Resolved

Added CompletionOnExtendedPropertyPattern_AlreadyTestedByNestedPattern

that adds a new property after an existing property. i'd like a test where we'er adding a prop-pattern before An existing property. basically like this:

_ = c is { $$CProperty: { IntProperty: 2 }

i want to make sure the following namr (or dotted name) doesn't throw off completion before this.

also a test of just this:

`_ = c is { $$CProperty

i.e. you're trying to write a dotted name before a name that is not part of a prop pattern.

CyrusNajmabadi · 2021-05-12T19:37:42Z

+                // `c is { X. }`
+
+                type = GetMemberAccessType(type, memberNameAccess.Expression, document, semanticModel, position);
+            }


i found this code confusing. esp passing in the type to return a type. could we just compute the type in whatever way is appropriate given the presence of a memberNameAccess or not? #Resolved

I didn't understand the suggestion.
In the example of c is { X. } we start with the type of c (given by GetTypeInfo), then we're going to access members (just X in this case) and we'll end up with the type of c.X as the starting point for completion.

I've attemped a change, but I'm not sure it's what you had in mind.

CyrusNajmabadi · 2021-05-12T19:37:53Z

            var type = semanticModel.GetTypeInfo(pattern, cancellationToken).ConvertedType;
-            if (type == null)
+
+            if (memberNameAccess is not null)


this is a weir dname. should it be memberAccessName? #Resolved

CyrusNajmabadi · 2021-05-12T19:38:39Z

-            var alreadyTestedMembers = new HashSet<string>(propertyPatternClause.Subpatterns.Select(
-                // PROTOTYPE(extended-property-patterns) ExpressionColon
-                p => p.NameColon?.Name.Identifier.ValueText).Where(s => !string.IsNullOrEmpty(s)));
+            if (propertyPatternClause is not null)


this test seems unnecessary (you checked it for null above). #Resolved

Thanks. It was the wrong test

CyrusNajmabadi · 2021-05-12T19:41:07Z

+                    return null;
+                }
+
+                return GetMemberType(type, name, document, semanticModel, position);


i'm pretty confused with teh data flow here :) any way to make thsi simpler by just separating out the 'is dotted name' and 'is identifer name' cases? #Resolved

CyrusNajmabadi · 2021-05-12T19:42:59Z

+            static IEnumerable<ISymbol> GetCandidatePropertiesAndFields(Document document, SemanticModel semanticModel, int position, ITypeSymbol type)
+            {
+                var members = semanticModel.LookupSymbols(position, type);
+                return members.Where(m => m.CanBeReferencedByName &&


Suggested change

return members.Where(m => m.CanBeReferencedByName &&

return members.WhereAsArray(m => m.CanBeReferencedByName &&

``` #Resolved

acn just be WhereAsArray :)

CyrusNajmabadi · 2021-05-12T19:43:08Z

+                };
+            }
+
+            static IEnumerable<ISymbol> GetCandidatePropertiesAndFields(Document document, SemanticModel semanticModel, int position, ITypeSymbol type)


Suggested change

static IEnumerable<ISymbol> GetCandidatePropertiesAndFields(Document document, SemanticModel semanticModel, int position, ITypeSymbol type)

static ImmutableArray<ISymbol> GetCandidatePropertiesAndFields(Document document, SemanticModel semanticModel, int position, ITypeSymbol type)

``` #Resolved

CyrusNajmabadi · 2021-05-12T19:44:03Z

+                {
+                    IPropertySymbol property => property.Type,
+                    IFieldSymbol field => field.Type,
+                    _ => null


Suggested change

_ => null

_ => throw ExceptionUtilities.Unreachable;

``` #Resolved

CyrusNajmabadi · 2021-05-12T19:44:16Z

+            {
+                var members = GetCandidatePropertiesAndFields(document, semanticModel, position, type);
+                var matches = members.Where(m => m.Name == name).ToArray();
+                if (matches.Length is 0 or > 1)


Suggested change

if (matches.Length is 0 or > 1)

if (matches.Length != 1)

``` #Resolved

CyrusNajmabadi · 2021-05-12T19:45:12Z

+
+            return default;
+
+            (PropertyPatternClauseSyntax, MemberAccessExpressionSyntax) makeResult(SyntaxToken token)


Suggested change

(PropertyPatternClauseSyntax, MemberAccessExpressionSyntax) makeResult(SyntaxToken token)

(PropertyPatternClauseSyntax, MemberAccessExpressionSyntax) MakeResult(SyntaxToken token)

``` #Resolved

CyrusNajmabadi · 2021-05-12T19:45:44Z

+            if (token.IsKind(SyntaxKind.CommaToken, SyntaxKind.OpenBraceToken))
            {
-                return default;
+                return token.Parent.IsKind(SyntaxKind.PropertyPatternClause) ? makeResult(token) : default;


this is a bit weird. yo uahve the local functino. but it's called in one place. #Resolved

CyrusNajmabadi · 2021-05-12T19:46:58Z

+            if (token.IsKind(SyntaxKind.CommaToken, SyntaxKind.OpenBraceToken))
            {
-                return default;
+                return token.Parent.IsKind(SyntaxKind.PropertyPatternClause) ? makeResult(token) : default;


Suggested change

return token.Parent.IsKind(SyntaxKind.PropertyPatternClause) ? makeResult(token) : default;

return token.Parent is { Kind: (int)SyntaxKind.PropertyPatternClause, Parent: PatternSyntax } propertyPattern ? (propertyPattern, null) : default;

``` #Resolved

CyrusNajmabadi · 2021-05-12T19:49:28Z

+                var memberNameAccess = token.Parent;
+                if (!memberNameAccess.IsKind(SyntaxKind.SimpleMemberAccessExpression)
+                    || !memberNameAccess.Parent.Parent.IsKind(SyntaxKind.Subpattern)
+                    || !memberNameAccess.Parent.Parent.Parent.IsKind(SyntaxKind.PropertyPatternClause))
+                {
+                    return default;
+                }
+
+                return ((PropertyPatternClauseSyntax)memberNameAccess.Parent.Parent.Parent, (MemberAccessExpressionSyntax)memberNameAccess);


Suggested change

var memberNameAccess = token.Parent;

if (!memberNameAccess.IsKind(SyntaxKind.SimpleMemberAccessExpression)

|| !memberNameAccess.Parent.Parent.IsKind(SyntaxKind.Subpattern)

|| !memberNameAccess.Parent.Parent.Parent.IsKind(SyntaxKind.PropertyPatternClause))

{

return default;

}

return ((PropertyPatternClauseSyntax)memberNameAccess.Parent.Parent.Parent, (MemberAccessExpressionSyntax)memberNameAccess);

var memberNameAccess = token.Parent;

if (memberNameAccess is { Kind: (int)SyntaxKind.SimpleMemberAccessExpression), Parent: { Parent: SubpatternSyntax { Parent: PropertyPatternClauseSyntax propertyPatternClause } } } memberAccess))

{

return (propertyPatternClause, memberAccess);

}

``` #Resolved

CyrusNajmabadi · 2021-05-12T19:50:18Z

+            if (leftToken.IsKind(SyntaxKind.DotToken))
+            {
+                return false;
+            }


doc? #Resolved

CyrusNajmabadi

overall looking good. just some cleanup/doc requests.

CyrusNajmabadi · 2021-05-13T16:29:20Z

            TestNormalizeStatement(actual, expected);
        }

+        [Fact(Skip = "PROTOTYPE")]


can we link this to an actual tracking bug? #Resolved

No need. This is going into a feature branch and all PROTOTYPE markers need to be addressed (or converted to issues) before merging the feature.

…rns' into property-ide

CyrusNajmabadi · 2021-05-16T18:12:11Z

+            var propertyPatternType = semanticModel.GetTypeInfo((PatternSyntax)propertyPatternClause.Parent, cancellationToken).ConvertedType;
+            // For simple property patterns, the type we want is the "input type" of the property pattern, ie the type of `c` in `c is { $$ }`.
+            // For extended property patterns, we get the type by following the chain of members that we have so far, ie
+            // the type of `c.Property` for `c is { Property.$$ }` and the type of `c.Property1.Property2` for `c is { Property1.Property2.$$ }`.


CyrusNajmabadi · 2021-05-16T18:13:39Z

+
+            // We have to figure out the type of the extended property ourselves, because
+            // the semantic model could not provide the answer we want in incomplete syntax:
+            // `c is { X. }`


where are we landing with collection patterns? do we have tests that this shows the right thing when 'X' binds to something in scope?

List patterns are in progress in another feature branch (Ali is currently working on binding/lowering for those in PR #53299).
I did my best here to assume the two features will eventually meet, so hopefully merging should be smooth.

CyrusNajmabadi · 2021-05-16T18:15:01Z

+                    IsFieldOrReadableProperty(m) &&
+                    !m.IsImplicitlyDeclared &&
+                    !m.IsStatic &&
+                    m.IsEditorBrowsable(document.ShouldHideAdvancedMembers(), semanticModel.Compilation));


i dont' think IsEditorBrowsable is right in this method (it should be at the top level). Wit hthis approach, if you had:

x.y.z. you wouldn't show anything if y was EB(never). Instead, we want to still lookup the members properly, but just not show the EB properties/fields at the end when determining the final list of members to show.

Good catch.
Fixed but FYI I wasn't able to test (attribute is ignored in source).

in general we just do a cross language test. write the type in VB, consume in C#. But i'm ok if you dont' want to write such a test here.

Thanks, that worked (added test that fails before fix but passes now) :-)

CyrusNajmabadi

LGTM, but we should fix the EB issue.

jcouv added Area-IDE Feature - Pattern Matching Pattern Matching labels May 8, 2021

jcouv self-assigned this May 8, 2021

CyrusNajmabadi reviewed May 8, 2021

View reviewed changes

Extended property patterns: main IDE scenarios

f7b084c

jcouv force-pushed the property-ide branch from 8530afd to f7b084c Compare May 9, 2021 18:20

Address feedback

5799e65

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

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

Merge remote-tracking branch 'dotnet/features/extended-property-patte…

dcaaa9a

…rns' into property-ide

jcouv requested a review from CyrusNajmabadi May 10, 2021 20:54

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

CyrusNajmabadi reviewed May 12, 2021

View reviewed changes

CyrusNajmabadi suggested changes May 12, 2021

View reviewed changes

Julien Couvreur added 3 commits May 13, 2021 00:17

Address feedback

e06fd2b

tweak

c7db195

Add test

3991496

CyrusNajmabadi reviewed May 13, 2021

View reviewed changes

Comment thread ...tures/CSharp/Portable/Completion/CompletionProviders/PropertySubPatternCompletionProvider.cs

Julien Couvreur added 2 commits May 14, 2021 12:35

Fix completion *before* another pattern

b97cc8e

WhereAsArray

2438e4c

jcouv force-pushed the property-ide branch from a5e8284 to 2438e4c Compare May 15, 2021 19:37

Julien Couvreur added 2 commits May 15, 2021 19:33

Merge remote-tracking branch 'dotnet/features/extended-property-patte…

377fe10

…rns' into property-ide

Adjust after merging latest changes

8da08e4

jcouv requested a review from CyrusNajmabadi May 16, 2021 18:02

CyrusNajmabadi reviewed May 16, 2021

View reviewed changes

CyrusNajmabadi approved these changes May 16, 2021

View reviewed changes

Julien Couvreur added 2 commits May 16, 2021 22:49

Only check hidden members for final list

81722cc

Add test for hidden fix

997360a

jcouv merged commit 2436749 into dotnet:features/extended-property-patterns May 17, 2021

jcouv deleted the property-ide branch May 17, 2021 21:07

	IEnumerable<ISymbol> members = semanticModel.LookupSymbols(position, type);
	var members = semanticModel.LookupSymbols(position, type);
	``` #Resolved

	var matches = members.Where(m => m.Name == name).ToArray();
	var matches = members.WhereAsArray(m => m.Name == name);
	``` #Resolved

	static IEnumerable<ISymbol> GetCandidatePropertiesAndFields(Document document, int position, SemanticModel semanticModel, ITypeSymbol type)
	static ImmutableArray<ISymbol> GetCandidatePropertiesAndFields(Document document, SemanticModel semanticModel, int position, ITypeSymbol type)
	``` #Resolved

	return members.Where(m => m.CanBeReferencedByName &&
	return members.WhereAsArray(m => m.CanBeReferencedByName &&
	``` #Resolved

	_ => null
	_ => throw ExceptionUtilities.Unreachable;
	``` #Resolved

	if (matches.Length is 0 or > 1)
	if (matches.Length != 1)
	``` #Resolved


		return default;

		(PropertyPatternClauseSyntax, MemberAccessExpressionSyntax) makeResult(SyntaxToken token)

	return token.Parent.IsKind(SyntaxKind.PropertyPatternClause) ? makeResult(token) : default;
	return token.Parent is { Kind: (int)SyntaxKind.PropertyPatternClause, Parent: PatternSyntax } propertyPattern ? (propertyPattern, null) : default;
	``` #Resolved

Conversation

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

Uh oh!

Uh oh!

CyrusNajmabadi May 8, 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!

CyrusNajmabadi May 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 8, 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!

CyrusNajmabadi May 8, 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 10, 2021

Uh oh!

azure-pipelines bot commented May 10, 2021

Uh oh!

jcouv commented May 10, 2021

Uh oh!

CyrusNajmabadi May 12, 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!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 12, 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!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 12, 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!

CyrusNajmabadi May 12, 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!

CyrusNajmabadi May 12, 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!

CyrusNajmabadi May 12, 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!

CyrusNajmabadi May 12, 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!

CyrusNajmabadi May 12, 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!

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

Uh oh!

jcouv commented May 8, 2021 •

edited

Loading

CyrusNajmabadi May 8, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 8, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 8, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 8, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 12, 2021 •

edited by jcouv

Loading

CyrusNajmabadi May 13, 2021 •

edited by jcouv

Loading