Add support for generating fields/properties for property patterns. #10873

CyrusNajmabadi · 2016-04-26T00:01:02Z

CyrusNajmabadi · 2016-04-26T00:01:12Z

Tagging @dotnet/roslyn-ide

gafter · 2016-04-26T02:37:01Z

@CyrusNajmabadi This will have to wait until after #10866; I'm removing property patterns from the code base before insertion into future, and any changes to the pattern-matching code will cause merge heck.
#Closed

DustinCampbell · 2016-12-01T15:44:07Z

@CyrusNajmabadi, @gafter: Should we close this ancient PR until we're ready for property patterns? #Closed

CyrusNajmabadi · 2017-09-22T22:23:32Z

@CyrusNajmabadi, @gafter: Should we close this ancient PR until we're ready for property patterns?

Works for me. If we add property patterns back in someone can resurrect this :) #Closed

CyrusNajmabadi · 2018-03-28T17:46:33Z

@gafter Can you give me a set of interesting examples that demonstrate how people can use recursive patterns? I want to make sure this features handles all the ones that make sense. Thanks! #Closed

gafter · 2018-03-28T18:02:12Z

Yes, I will prepare a set of examples. #Closed

CyrusNajmabadi · 2018-03-28T18:04:26Z

Awesome, thanks. It doesn't have to be huge. Just like 1 (or 2) tops, for each interesting new construct. And maybe some special 'oddities' (if they exist) to be aware of. #Closed

gafter · 2018-03-29T17:53:26Z

I will include some complete examples here in a moment, but just to point out some oddities first...

A property pattern checks that the input value is non-null, and that each named property matches the given pattern. But you can use no patterns if you just want a non-null check:

if (o is {})

The switch expression (parser and code gen) has some fixes pending that are not integrated yet, so it is broken in features/recursive-patterns at this moment, so better to focus on the switch statement and is-pattern expression. #Resolved

CyrusNajmabadi · 2018-03-29T17:57:39Z

if (o is {})

Good case. Just to verify, you should be able to write the following right:

if (o is { [|X|] is int i })

And this just means: "'o' is non-null and has a field/property 'X' that matches the pattern 'int i'"?

#Resolved

CyrusNajmabadi · 2018-03-29T18:18:53Z

Also, do we have a list of all patterns written down somewhere, i'd like to test out a variety of combinations. What i know of off the top of my head:

Type pattern: int i
Constant pattern: 0
Allow-null-of-any-type-pattern: var x
non-null property pattern: { ... }
non-null + type + property pattern: X { ... }
Discard pattern: _

Did we do deconstruction patterns? i.e. something like SomeType(...)? If so, what syntax did we settle on for that?

#Resolved

gafter · 2018-03-29T18:20:06Z

Here is a little sample demonstrating the interaction of tuples and patterns. Possibly not relevant to this PR

using System;

public class Door
{
    public DoorState State;

    public enum DoorState { Opened, Closed, Locked }

    public enum Action { Open, Close, Lock, Unlock }

    public void Act(Action action, bool haveKey = false)
    {
        Console.Write($"{State} {action}{(haveKey ? " withKey" : null)}");
        State = ChangeState0(State, action, haveKey);
        Console.WriteLine($" -> {State}");
    }

    public static DoorState ChangeState0(DoorState state, Action action, bool haveKey = false)
    {
        switch ((state, action))
        {
            case (DoorState.Opened, Action.Close):
                return DoorState.Closed;
            case (DoorState.Closed, Action.Open):
                return DoorState.Opened;
            case (DoorState.Closed, Action.Lock) when haveKey:
                return DoorState.Locked;
            case (DoorState.Locked, Action.Unlock) when haveKey:
                return DoorState.Closed;
            case var (oldState, _):
                return oldState;
        }
    }

    public static DoorState ChangeState1(DoorState state, Action action, bool haveKey = false) =>
        (state, action) switch {
            (DoorState.Opened, Action.Close) => DoorState.Closed,
            (DoorState.Closed, Action.Open) => DoorState.Opened,
            (DoorState.Closed, Action.Lock) when haveKey => DoorState.Locked,
            (DoorState.Locked, Action.Unlock) when haveKey => DoorState.Closed,
            _ => state };
}

class Program
{
    static void Main(string[] args)
    {
        var door = new Door();
        door.Act(Door.Action.Close);
        door.Act(Door.Action.Lock);
        door.Act(Door.Action.Lock, true);
        door.Act(Door.Action.Open);
        door.Act(Door.Action.Unlock);
        door.Act(Door.Action.Unlock, true);
        door.Act(Door.Action.Open);
    }
}

#Resolved

CyrusNajmabadi · 2018-03-29T18:20:38Z

Also, inside a property-pattern, what syntax are the field/property pattern matchers? At one point it was { X is int i }, is that still the case? or did we go with another syntax that isn't just a list of "IsPatternExpression"s? #Resolved

gafter · 2018-03-29T18:21:09Z

Here is a possibly more relevant sample, though there are only a couple of examples of property patterns in it.

using System;

class Program2
{
    static StatementSyntax s =
        new ExpressionStatementSyntax(
            new AssignmentExpressionSyntax(
                new SimpleNameSyntax(new SyntaxToken("x")),
                new SimpleNameSyntax(new SyntaxToken("x"))));

    public static void Test()
    {
        if (s is
            ExpressionStatementSyntax(
                AssignmentExpressionSyntax(
                    SimpleNameSyntax(SyntaxToken(var leftName)),
                    SimpleNameSyntax(SyntaxToken(var rightName))
                    ) a) &&
            leftName == rightName)
        {
            Console.WriteLine("self-assignment!");
        }
    }

    public static void Test2()
    {
        if (s is
            ExpressionStatementSyntax(
                AssignmentExpressionSyntax(
                    SimpleNameSyntax { Identifier: SyntaxToken(var leftName) },
                    SimpleNameSyntax { Identifier: SyntaxToken(var rightName) }
                    ) a) &&
            leftName == rightName)
        {
            Console.WriteLine("self-assignment!");
        }
    }

    public static void Test3()
    {
        if (s is
            ExpressionStatementSyntax(
                AssignmentExpressionSyntax(
                    SimpleNameSyntax(var leftToken),
                    SimpleNameSyntax(var rightToken)
                    ) a) &&
            leftToken.ValueText == rightToken.ValueText)
        {
            Console.WriteLine("self-assignment!");
        }
    }
}











abstract class SyntaxNode
{
    public int Location => 0;
}
class SyntaxToken : SyntaxNode
{
    public string ValueText;
    public SyntaxToken(string ValueText) => this.ValueText = ValueText;
    public void Deconstruct(out string ValueText) => ValueText = this.ValueText;
}
class StatementSyntax : SyntaxNode
{
}
class ExpressionSyntax : SyntaxNode
{
}
class ExpressionStatementSyntax : StatementSyntax
{
    public ExpressionSyntax Expression;
    public ExpressionStatementSyntax(ExpressionSyntax Expression) => this.Expression = Expression;
    public void Deconstruct(out ExpressionSyntax Expression) => Expression = this.Expression;
}
class AssignmentExpressionSyntax : ExpressionSyntax
{
    public ExpressionSyntax Left;
    public ExpressionSyntax Right;
    public AssignmentExpressionSyntax(ExpressionSyntax Left, ExpressionSyntax Right) => (this.Left, this.Right) = (Left, Right);
    public void Deconstruct(out ExpressionSyntax Left, out ExpressionSyntax Right) => (Left, Right) = (this.Left, this.Right);
}
class SimpleNameSyntax : ExpressionSyntax
{
    public SyntaxToken Identifier;
    public SimpleNameSyntax() { }
    public SimpleNameSyntax(SyntaxToken Identifier) => this.Identifier = Identifier;
    public void Deconstruct(out SyntaxToken Identifier) => Identifier = this.Identifier;
}

#Resolved

CyrusNajmabadi · 2018-03-29T18:35:43Z

Ah interesting:

is ExpressionStatementSyntax(
                AssignmentExpressionSyntax(
                    SimpleNameSyntax { Identifier: SyntaxToken(var leftName) },
                    SimpleNameSyntax { Identifier: SyntaxToken(var rightName) }
                    ) a)

So the "ExpressionStatementSyntax ( .. )``` is a type+deconstruction pattern? Ensure that the value is of htat type, and if so, deconstruct it and check if the deconstructed parts match teh subpatterns?

Then Identifier: SyntaxToken(var leftName) is a field/property checking pattern in a property-pattern? So the syntax is "Name pattern"? #Resolved

gafter · 2018-03-30T03:59:52Z

Yes, you've got that. In its most general form, a recursive pattern can be

Type ( SubpatternList ) { PropertySubpatternList } Identifier

You can omit any of the parts, except that you can't have only a type, only an identifier, or nothing.
#Resolved

gafter · 2018-03-30T04:01:14Z

See also dotnet/csharplang#1054 and note that we added a shorthand discard pattern _. #Resolved

…nto generateVarPatterns

CyrusNajmabadi · 2018-04-11T23:05:56Z

Tagging @dotnet/roslyn-ide Please review this update of "generate variable" to understand and properly generate properties when used in property-patterns.

CyrusNajmabadi · 2018-04-12T22:01:39Z

@dotnet/roslyn-ide @jmarolf @jcouv can someone take a look at this PR? Thanks!

Neme12 · 2018-04-12T22:08:35Z

src/EditorFeatures/CSharpTest/GenerateVariable/GenerateVariableTests.cs

+
+    class Blah
+    {
+        public int X { get; internal set; }


internal is redundant here

Neme12 · 2018-04-12T22:13:17Z

src/EditorFeatures/CSharpTest/GenerateVariable/GenerateVariableTests.cs

+        public int X { get; internal set; }
+    }
+}", parseOptions: WithPatternsEnabled);
+        }


consider adding tests with some error cases, eg. Blah { X: { [|Y|]: int i } } where X doesn't exist, and consider adding an example with more than 1 property

consider adding tests with some error cases, eg. Blah { X: { [|Y|]: int i } } where X doesn't exist,
sure

and consider adding an example with more than 1 property

It's unclear what that would be testing. We don't generate multiple props at a time for example.

it would be testing that this doesn't crash

Neme12 · 2018-04-12T22:18:54Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs

-        {
-            return IsObjectInitializerNamedAssignmentIdentifier(node, out var unused);
-        }
+            => IsObjectInitializerNamedAssignmentIdentifier(node, out var unused);


nit: discard

Neme12 · 2018-04-12T22:20:38Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs

+               node.Parent.IsParentKind(SyntaxKind.SubpatternElement);
+
+        public bool IsPropertySubpattern(SyntaxNode node)
+            => node.Kind() == SyntaxKind.PropertySubpattern;


does this belong here even though it's specific to C#?

SyntaxFacts is a superset of both languages. Either language can simply return 'false' for anything they dont' support.

Neme12 · 2018-04-12T22:24:07Z

src/Features/Core/Portable/GenerateMember/AbstractGenerateMemberService.cs

@@ -154,6 +154,17 @@ protected AbstractGenerateMemberService()
                isStatic = false;
                return;
            }
+            else if (syntaxFacts.IsNameOfSubpatternElement(expression) &&
+                     syntaxFacts.IsPropertySubpattern(expression.Parent.Parent.Parent))


expression.Parent.Parent.Parent is really not clear here... if you want to think of syntaxFacts as language agnostic, this kind of check doesn't make sense in this place. Consider adding an out argument for subpatternElement to IsNameOfSubpatternElement so you could only do one Parent check.

Neme12 · 2018-04-12T22:25:08Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

            }

-            private static bool DecomposeBinaryOrAssignmentExpression(ExpressionSyntax expression, out SyntaxToken operatorToken, out ExpressionSyntax left, out ExpressionSyntax right)
+            private static bool DecomposeBinaryOrAssignmentExpression(SyntaxNode node, out SyntaxToken operatorToken, out ExpressionSyntax left, out ExpressionSyntax right)


why was this changed?

Because the type inferred now works across expressions and patterns uniformly.

I don't see how that's relevant here. This method is called DecomposeBinaryOrAssignmentExpression and still only handles expressions.

sorry, i thought you were asking a general question about the use of Expression vs SyntaxNode. I can make this ExpressionSyntax, though then i still have to add casts. That's not a big deal, just something that wasn't necessary if this was typed as SyntaxNode.

Neme12 · 2018-04-12T22:26:41Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

+                if (expression != null)
+                {
+                    expression = expression.WalkUpParentheses();
+                    node = expression;


nit: consider pattern matching: if (node is ExpressionSyntax expression) node = expression.WalkUpParentheses()

Neme12 · 2018-04-12T22:31:10Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

@@ -899,7 +909,7 @@ private int GetArgumentListIndex(AttributeArgumentListSyntax attributeArgumentLi
                return (tokenIndex + 1) / 2;
            }

-            private IEnumerable<TypeInferenceInfo> InferTypeInBinaryOrAssignmentExpression(ExpressionSyntax binop, SyntaxToken operatorToken, ExpressionSyntax left, ExpressionSyntax right, ExpressionSyntax expressionOpt = null, SyntaxToken? previousToken = null)
+            private IEnumerable<TypeInferenceInfo> InferTypeInBinaryOrAssignmentExpression(SyntaxNode binop, SyntaxToken operatorToken, ExpressionSyntax left, ExpressionSyntax right, ExpressionSyntax expressionOpt = null, SyntaxToken? previousToken = null)


This seems like it should still be ExpressionSyntax

true. fixed.

Neme12 · 2018-04-12T22:34:07Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

+                    var memberTypes = 
+                        parentTypes.SelectMany(ti => GetMemberReturnTypes(ti.InferredType, identifier.Identifier.ValueText))
+                                   .SelectAsArray(t => new TypeInferenceInfo(t));
+


why is all this logic necessary? doesn't the compiler return a symbol for X?

Good question. Let me try that.

Hrm.. Doesn't work. Not sure if that's intentional or not. @gafter can you shed some light here? The scenario is the following code:

void M2() { object o = null; if (o is Blah { X: { Y: int i } }) //<-- call semanticModel.GetSymbolInfo on 'X' here. { } } class Frob { } class Blah { public Frob X; }

If I call semanticModel.GetSymbolInfo(id) on the identifier node for 'X', i don't get anything back. Is that expected? Or should i get back the the field symbol declared inside Blah? My intuition is 'yes', but i know the compiler has some very particular views about types/symbols and whatnot in patterns.

Can you clarify the expected behavior here of hte semantic model? Thanks!

The semantic model is not implemented for this syntactic location yet.

@gafter Is the expectation that it will be at some later point? I just want to know what i should expect to work in the future so i can write the feature code in the appropriate manner :)

For example, you explained in the past how patterns don't have types, so i know to not even expect to be able to use .GetTypeInfo on a pattern. But the rules about things like getting symbol info on these parts of a pattern are a bit less clear.

Thanks!

Yes, I intend that identifiers referenced within a pattern will have SymbolInfo working. I will work on that next (though I am out for about a week, so it will be more than a week before I have it working).

Sounds good. I'll wait for that to be done.

Neme12 · 2018-04-12T22:35:09Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

+            }
+
+            private ImmutableArray<ITypeSymbol> GetMemberReturnTypes(ITypeSymbol type, string memberName)
+            {


Neme12 · 2018-04-12T23:36:02Z

src/EditorFeatures/CSharpTest/GenerateVariable/GenerateVariableTests.cs

+
+    class Blah
+    {
+        public Frob X;


is there a similar test wehre X is a property rather than field?

it would be good to have one to exercise both cases in GetMemberReturnTypes

jaredpar · 2018-04-19T16:51:17Z

We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them.

If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you!

CyrusNajmabadi · 2018-04-19T18:11:56Z

@jaredpar please reopen this PR. This is work that @gafter has asked me to do as part of the pattern-matching feature work.

If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you!

This PR is intentionally against @gafter's feature branch as that's where all the pattern work is happening.

Note that work has been made in this PR as of a week ago, please don't close the PRs that are still actively being worked on, thanks :D

jaredpar · 2018-04-19T20:31:36Z

@CyrusNajmabadi it's apparently not just you. I can't re-open this PR either. I'm not sure what is going on here.

sharwell · 2018-04-19T20:37:35Z

@CyrusNajmabadi You need to create a branch generateVarPatterns at commit d9a8bf3 in your fork in order for this pull request to be reopened.

CyrusNajmabadi · 2018-04-19T20:50:38Z

@sharwell can you clarify that? I already have a branch called generateVarExpressions . So i can't create one. Should i create another branch with a different name? Should i delete the existing branch? A set of steps would be appreciated here.

CyrusNajmabadi · 2018-04-19T20:50:59Z

@jaredpar Can we put a hold on closing out old PRs until a set of reasonable steps can be provided to contributers on how to actually reopen things? Thanks!

jaredpar · 2018-04-19T20:52:21Z

@CyrusNajmabadi this seems to be the only PR that was affected. I've re-opened several others with no ill effects.

CyrusNajmabadi · 2018-04-19T21:58:37Z

Perhaps this is because it's a feature branch? I also don't seem to be able to reopen: #10420

CyrusNajmabadi · 2018-04-24T04:00:07Z

I could not figure out how to reopen this. So i've opened #26355 instead.

Add support for generating fields/properties for property patterns.

b0b146d

dnfclas added the cla-already-signed label Apr 26, 2016

gafter added the New Language Feature - Pattern Matching Pattern Matching label Apr 26, 2016

Pilchie added the Area-IDE label May 15, 2016

sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Sep 8, 2017

CyrusNajmabadi closed this Sep 22, 2017

CyrusNajmabadi mentioned this pull request Mar 27, 2018

Generate field/property not offered for complex pattern member. #9090

Closed

Merge branch 'features/recursive-patterns' into generateVarPatterns

d67cdcc

CyrusNajmabadi changed the base branch from features/patterns to features/recursive-patterns March 28, 2018 00:19

CyrusNajmabadi reopened this Mar 28, 2018

CyrusNajmabadi requested a review from a team as a code owner March 28, 2018 00:20

Update feature.

bdbd341

gafter self-assigned this Mar 29, 2018

gafter added this to the 16.0 milestone Mar 29, 2018

CyrusNajmabadi added 2 commits April 11, 2018 12:52

remove dead code.

a960ef0

Merge remote-tracking branch 'upstream/features/recursive-patterns' i…

e18d12f

…nto generateVarPatterns

Neme12 reviewed Apr 12, 2018

View reviewed changes

Make method strongly typed.

d9a8bf3

Neme12 reviewed Apr 12, 2018

View reviewed changes

jaredpar added the Resolution-Expired The request is closed due to inactivity under our contribution review policy. label Apr 19, 2018

jaredpar closed this Apr 19, 2018

CyrusNajmabadi mentioned this pull request Apr 19, 2018

For comment: Roslyn team SLA for addressing community PRs and bug submissions #26266

Open

CyrusNajmabadi mentioned this pull request Apr 24, 2018

Add support for generating fields/properties for property patterns. #26355

Merged

Add support for generating fields/properties for property patterns. #10873

Add support for generating fields/properties for property patterns. #10873

Conversation

CyrusNajmabadi commented Apr 26, 2016

CyrusNajmabadi commented Apr 26, 2016

gafter commented Apr 26, 2016 • edited Loading

DustinCampbell commented Dec 1, 2016 • edited by gafter Loading

CyrusNajmabadi commented Sep 22, 2017 • edited by gafter Loading

CyrusNajmabadi commented Mar 28, 2018 • edited by gafter Loading

gafter commented Mar 28, 2018 • edited Loading

CyrusNajmabadi commented Mar 28, 2018 • edited by gafter Loading

gafter commented Mar 29, 2018 • edited Loading

CyrusNajmabadi commented Mar 29, 2018 • edited by gafter Loading

CyrusNajmabadi commented Mar 29, 2018 • edited by gafter Loading

gafter commented Mar 29, 2018 • edited Loading

CyrusNajmabadi commented Mar 29, 2018 • edited by gafter Loading

gafter commented Mar 29, 2018 • edited Loading

CyrusNajmabadi commented Mar 29, 2018 • edited by gafter Loading

gafter commented Mar 30, 2018 • edited Loading

gafter commented Mar 30, 2018 • edited Loading

CyrusNajmabadi commented Apr 11, 2018

CyrusNajmabadi commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

jaredpar commented Apr 19, 2018

CyrusNajmabadi commented Apr 19, 2018

jaredpar commented Apr 19, 2018

sharwell commented Apr 19, 2018 • edited Loading

CyrusNajmabadi commented Apr 19, 2018

CyrusNajmabadi commented Apr 19, 2018

jaredpar commented Apr 19, 2018

CyrusNajmabadi commented Apr 19, 2018 • edited Loading

CyrusNajmabadi commented Apr 24, 2018

gafter commented Apr 26, 2016 •

edited

Loading

DustinCampbell commented Dec 1, 2016 •

edited by gafter

Loading

CyrusNajmabadi commented Sep 22, 2017 •

edited by gafter

Loading

CyrusNajmabadi commented Mar 28, 2018 •

edited by gafter

Loading

gafter commented Mar 28, 2018 •

edited

Loading

CyrusNajmabadi commented Mar 28, 2018 •

edited by gafter

Loading

gafter commented Mar 29, 2018 •

edited

Loading

CyrusNajmabadi commented Mar 29, 2018 •

edited by gafter

Loading

CyrusNajmabadi commented Mar 29, 2018 •

edited by gafter

Loading

gafter commented Mar 29, 2018 •

edited

Loading

CyrusNajmabadi commented Mar 29, 2018 •

edited by gafter

Loading

gafter commented Mar 29, 2018 •

edited

Loading

CyrusNajmabadi commented Mar 29, 2018 •

edited by gafter

Loading

gafter commented Mar 30, 2018 •

edited

Loading

gafter commented Mar 30, 2018 •

edited

Loading

Neme12 Apr 12, 2018 •

edited

Loading

Neme12 Apr 12, 2018 •

edited

Loading

sharwell commented Apr 19, 2018 •

edited

Loading

CyrusNajmabadi commented Apr 19, 2018 •

edited

Loading