Add a prototype implementation for GetTypeInfo(PatternSyntax) by gafter · Pull Request #26521 · dotnet/roslyn

gafter · 2018-04-30T22:25:03Z

This is a first draft of #26494, and requires more testing.

This is a first draft of dotnet#26494, and requires more testing.

jcouv · 2018-04-30T22:51:14Z

    <Field Name="DeconstructMethodOpt" Type="MethodSymbol" Null="allow"/>
-    <Field Name="Deconstruction" Type="ImmutableArray&lt;BoundPattern&gt;" Null="allow"/>
-    <Field Name="PropertiesOpt" Type="ImmutableArray&lt;(Symbol symbol, BoundPattern pattern)&gt;" Null="allow"/>
+    <Field Name="Deconstruction" Type="ImmutableArray&lt;BoundSubpattern&gt;" Null="allow"/>


Not related to this PR: should Deconstruction be called DeconstructionOpt? #Resolved

We are inconsistent about that. I think we should do a sweep to some consistent state. #Resolved

jcouv · 2018-04-30T22:52:36Z

  </Node>
+
+  <Node Name="BoundSubpattern" Base="BoundNode">
+    <Field Name="Symbol" Type="Symbol" Null="allow"/>


Maybe add a comment. #Resolved

jcouv · 2018-04-30T22:53:10Z

                                TypeSymbol elementType = isError ? CreateErrorType() : elementTypes[i];
                                BoundPattern boundSubpattern = BindVarDesignation(node, tupleDesignation.Variables[i], elementType, isError, diagnostics);
-                                patterns.Add(boundSubpattern);
+                                subPatterns.Add(new BoundSubpattern(node, null, boundSubpattern));


Nit: name null here and below. #Resolved

jcouv · 2018-04-30T22:54:57Z

        if (recursive.PropertiesOpt != null)

Not related to this PR: PropertiesOpt is an immutable array. Why are we checking against null? #Resolved

Refers to: src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs:457 in 3ae4026. [](commit_id = 3ae4026, deletion_comment = False)

jcouv · 2018-04-30T22:57:38Z

            BoundNode highestBoundNode,
            BoundNode boundNodeForSyntacticParent)
        {
+            if (lowestBoundNode is BoundPattern pat)


Nit: pattern can be spelled out #Resolved

jcouv

LGTM with some nits

jcouv · 2018-04-30T23:00:33Z

+                Assert.Equal(ex.Type, typeInfo.Type.ToTestDisplayString());
+                Assert.Equal(ex.ConvertedType, typeInfo.ConvertedType.ToTestDisplayString());
            }
+            Assert.Equal(expected.Length, i);


Could you add testing of GetTypeInfo to some existing ~~types~~ tests as well? #ByDesign

Yes, but not this round. I'm keeping the bug for this new API open until testing is more complete.

In reply to: 185133922 [](ancestors = 185133922)

gafter · 2018-05-01T00:33:02Z

@cston @agocke Could one of you please provide a second review of this?

cston · 2018-05-01T04:46:22Z

+        /// <summary>
+        /// Helper class to remove alias qualifications.
+        /// </summary>
+        class RemoveAliasQual : CSharpSyntaxRewriter


Please consider spelling out Qualifiers. #Resolved

cston · 2018-05-01T04:47:36Z

+                        foundField = CheckIsTupleElement(subPattern.NameColon.Name, (NamedTypeSymbol)declType, name, i, diagnostics);
                        // PROTOTYPE(patterns2): Should the tuple field binding for the name be stored somewhere in the node?

                    }


Unnecessary blank line. #Resolved

jcouv · 2018-05-01T05:10:46Z

Please merge latest patterns changes into features/build-demo once this PR is merged. I'll need this API for IDE logic. Thanks

jcouv · 2018-05-01T15:35:12Z

+                var ex = expected[i++];
+                Assert.Equal(ex.Source, pat.ToString());
+                Assert.Equal(ex.Type, typeInfo.Type.ToTestDisplayString());
+                Assert.Equal(ex.ConvertedType, typeInfo.ConvertedType.ToTestDisplayString());


@gafter Another general discussion: could we reconsider factoring PropertyPatternSyntax (with a type, property sub-pattern and designation) and DeconstructionPatternSyntax (with the same plus parens and list of patterns)?

The parens and list of patterns are pulled into a deconstruction sub-pattern. Then we can have one node with optional type, optional positional sub-pattern and optional property sub-pattern.

I understand that you wanted to have the syntax nodes enforce some constraints (you can't be missing the type, positional and property sub-patterns).

But on the other hand, my experience dealing with the syntax API so far is that it would be easier to deal with and reason about.

what about exposing their common properties through an abstract base class (Type, PropertySubpattern, Designation)?

@Neme12 See #26540

Add a prototype implementation for GetTypeInfo(PatternSyntax)

3ae4026

This is a first draft of dotnet#26494, and requires more testing.

gafter added Area-Compilers Feature Request 4 - In Review A fix for the issue is submitted for review. Feature - Pattern Matching Pattern Matching labels Apr 30, 2018

gafter added this to the 16.0 milestone Apr 30, 2018

gafter self-assigned this Apr 30, 2018

gafter requested review from agocke, cston and jcouv April 30, 2018 22:25

gafter requested a review from a team as a code owner April 30, 2018 22:25

jcouv reviewed Apr 30, 2018

View reviewed changes

jcouv approved these changes Apr 30, 2018

View reviewed changes

jcouv reviewed Apr 30, 2018

View reviewed changes

Minor changes per review, repair a test.

5f2ff5a

cston reviewed May 1, 2018

View reviewed changes

cston approved these changes May 1, 2018

View reviewed changes

jcouv mentioned this pull request May 1, 2018

Some fixes for patterns in IDE #26474

Closed

jcouv reviewed May 1, 2018

View reviewed changes

Minor changes per review.

22fea68

gafter merged commit ce81a76 into dotnet:features/recursive-patterns May 1, 2018

gafter mentioned this pull request May 1, 2018

GetTypeInfo for a PatternSyntax #26494

Closed

gafter removed the 4 - In Review A fix for the issue is submitted for review. label Dec 18, 2018

Conversation

gafter commented Apr 30, 2018

Uh oh!

jcouv Apr 30, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter May 1, 2018 • 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!

jcouv Apr 30, 2018 • edited by gafter 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 Apr 30, 2018 • edited by gafter 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 Apr 30, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Apr 30, 2018 • edited by gafter 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 left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 30, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter May 1, 2018

Choose a reason for hiding this comment

Uh oh!

gafter commented May 1, 2018

Uh oh!

cston May 1, 2018 • edited by gafter 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 1, 2018 • edited by gafter 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 1, 2018

Uh oh!

jcouv May 1, 2018

Choose a reason for hiding this comment

Uh oh!

Neme12 May 1, 2018

Choose a reason for hiding this comment

Uh oh!

gafter May 1, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv Apr 30, 2018 •

edited by gafter

Loading

gafter May 1, 2018 •

edited

Loading

jcouv Apr 30, 2018 •

edited by gafter

Loading

jcouv Apr 30, 2018 •

edited by gafter

Loading

jcouv commented Apr 30, 2018 •

edited by gafter

Loading

jcouv Apr 30, 2018 •

edited by gafter

Loading

jcouv Apr 30, 2018 •

edited by gafter

Loading

cston May 1, 2018 •

edited by gafter

Loading

cston May 1, 2018 •

edited by gafter

Loading