Some fixes for patterns in IDE by jcouv · Pull Request #26474 · dotnet/roslyn

jcouv · 2018-04-28T14:26:55Z

Addressed:

When typing a property-based pattern, completion should be offered for the name of the property. (this is working for many cases, but not all. A more general solution will use the upcoming GetTypeInfo API from the compiler)
switch keyword offered for switch expression.
Manually verified that UpgradeProject is properly offered.
spacing between type and positional pattern
missing spacing before positional pattern
typing e is { gets a newline before brace

Not yet addressed:

typing e switch { then return produces badly indented closing brace. Formatting the document doesn't fix the issue. So that's likely a formatting problem.
completion isn't triggered in some contexts (inside property pattern in switch expression).

        _ = this switch
        {
        }; // typing this introduces extra newline and a bad indent on closing brace

        _ = this switch { { P1: 1 } => 1, _ => 2 }; // missing completion on P1

jcouv · 2018-04-28T14:27:34Z

                {
                    // This occurs in error cases.
-                    Debug.Assert(recursive.HasAnyErrors);
+                    //Debug.Assert(recursive.HasAnyErrors);


I won't merge this change. It's to unblock manual validation in IDE.
Filed #26467 #Closed

Fix for that is in #26481

jcouv · 2018-04-28T14:57:10Z

Should a colon be displayed in the completion? Prop:
Should typing enter or tab automatically enter the colon?

Neme12 · 2018-04-28T16:10:49Z

+                }
+            }
+
+            return false;


can this reuse the same logic that is and as keywords use? those also show up generally after an expression #Closed

Excellent idea. Thanks #Closed

Neme12 · 2018-04-28T16:11:51Z

+        }
+
+        // `switch` follows expressions in switch expressions
+        internal static bool IsAfterExpression(CSharpSyntaxContext context)


why internal? #Closed

Neme12 · 2018-04-28T18:35:19Z

consider also verifying "P1" and "P2" is absent here #Closed

Neme12 · 2018-04-28T18:35:51Z

nit: extra comma #Closed

Neme12 · 2018-04-28T18:37:08Z

consider also verifying "P2" is absent here #Resolved

consider adding a test that has a comma inside a nested pattern, such as:
this is Program { P2: (1, $$ or this is Program { P2: { something: 1, $$ we don't want P1 here, but we do want after the closing ) or } and a comma

Neme12 · 2018-04-28T18:38:37Z

consider adding a test where the properties are private but inside Program and therefore accessible #Closed

Good idea. Note that is allowed. #Closed

Neme12 · 2018-04-28T18:40:16Z

what about setter only properties? should they be offered or not? #Closed

The shouldn't be offered (just like we don't offer read-only properties in object initializers). Fixed.
Thanks! #Closed

Neme12 · 2018-04-28T18:42:50Z

is there a test where this type (Program) is explicitly different from the switch expression type? #Closed

Add PropertiesInRecursivePattern_WithOtherType #Closed

Neme12 · 2018-04-28T18:45:02Z

consider adding a nested test where P2 itself is a field #Closed

Added PropertiesInRecursivePattern_NestedInField #Closed

Neme12 · 2018-04-28T18:50:37Z

you're not actually testing that P1 is absent, which seems like it was the purpose of this test #Closed

Neme12 · 2018-04-28T18:53:54Z

is there an IsKind overload that takes two arguments? so you could get rid of the || #Closed

Neme12 · 2018-04-28T18:54:30Z

nit: I would probably consider making these separate variables instead of reassigning, but up to you #ByDesign

or just inlining this directly: var members = semanticModel.LookupSymbols(position, type).Where #Resolved

nit: Please use 'var'. #ByDesign

I get an error in assignment below if I use var here. Leaving as-is.

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

Neme12 · 2018-04-28T18:56:09Z

why is this separate? #Closed

Neme12 · 2018-04-28T18:56:42Z

nit: should be untestedMember #Closed

Neme12 · 2018-04-28T18:57:26Z

nit: => #Closed

Neme12 · 2018-04-28T18:58:17Z

consider naming TryGetPatternType #Closed

can this be static? #Closed

CyrusNajmabadi · 2018-04-30T23:46:57Z

+                        case IPropertySymbol propertySymbol:
+                            return (propertySymbol.Type, token.GetLocation());
+                        case IFieldSymbol fieldSymbol:
+                            return (fieldSymbol.Type, token.GetLocation());


is returning this location really needed here? is it not suffiient to just find the token in the calling method and just use that locatoin? isn't the location just used for things like simplification?

CyrusNajmabadi · 2018-04-30T23:47:41Z

                context.IsStatementContext ||
-                context.IsGlobalStatementContext;
+                context.IsGlobalStatementContext ||
+                context.IsIsOrAsContext; // ie. following an expression


this seems super bizarre. If you really want to reuse this property, then call it IsIsOrAsOrSwitchContext #Closed

CyrusNajmabadi · 2018-04-30T23:47:52Z

            return false;
        }

+        // PROTOTYPE(recursive-patterns): Rename this method


yes plz. #Closed

possibly: FollowsCompleteExpression #Closed

CyrusNajmabadi · 2018-04-30T23:48:15Z

+        public static bool IsColonInCasePatternSwitchLabel(this SyntaxToken token)
+        {
+            return token.Kind() == SyntaxKind.ColonToken && token.Parent is CasePatternSwitchLabelSyntax;
+        }


nit: => #Closed

CyrusNajmabadi · 2018-04-30T23:48:54Z

+            {
+                AddSuppressWrappingIfOnSingleLineOperation(list, switchExpressionArm.GetFirstToken(), switchExpressionArm.GetLastToken());
+                return;
+            }


do you need a similar suppressoin for the entire switch expression? #Closed

Done. Thanks #Closed

CyrusNajmabadi · 2018-04-30T23:49:15Z


+                    if (currentToken.IsPattern())
+                    {
+                        return null;


why? #Closed

Thanks. I've added a suppression on switch expression and pattern case. Then I removed this special case (on similar one below, at line 49). #Closed

Hum, after I removed this, I get a newline before the brace when typing _ = e is {.
It's not showing up in tests for some reason, only in IDE at the moment.

@CyrusNajmabadi
Would you know a good way to test the formatting-as-you-type experience? That will help me capture and investigate behaviors.
In particular, I need to find out what generates a newline when typing x is {. Then I can see what prevents the same from happening when typing if (true) { }, and do the same.

CyrusNajmabadi · 2018-04-30T23:49:59Z


+                    if (currentToken.IsPattern())
+                    {
+                        return null;


why? this doesn't seem right to me. We would want to adjust newlines. We'd just all want that be overridden by a supression if it was on a single line to start with. But if this is spread over multiple lines, we would want this fixed up, right?

CyrusNajmabadi · 2018-04-30T23:50:53Z

+    {
+        _ = this is {
+        P1: 1,
+        P2: 2


indentation seems wrong here.

I've tried various things in IndentBlockFormattingRules, but none seems to have any effect.

@CyrusNajmabadi @heejaechang Any tips on this?

let me see.

this kind of thing (expression level), we do not force indentation. looks like it is doing right thing. it is maintaining relative indentation from "_"

if test was written

_ = this is { P1 : 1 ...

it will maintain relative indentation between _ and P1.

same way something like below is formatted.

method(1, 2, 4);

and format, you will see relative indentation of 2 and 4 are maintained aginst method

The latest commit I pushed finally has some effect. But it's super weird. Only the first arm of the switch expression got indented... I'll continue poking around.

heejaechang · 2018-05-01T04:08:41Z

+            var endToken = arms.Last().GetLastToken(includeZeroWidth: true);
+            var span = CommonFormattingHelpers.GetSpanIncludingTrailingAndLeadingTriviaOfAdjacentTokens(startToken, endToken);
+
+            AddIndentBlockOperation(list, switchExpression.OpenBraceToken, startToken, endToken, IndentBlockOption.RelativeToFirstTokenOnBaseTokenLine);


do you want to really force this? that means users can't put it anywhere else?

Yes, I think so. The switch expression should be formatted strictly, like a switch statement (except a bit simpler).

heejaechang · 2018-05-01T04:11:01Z

+                //      }
+                // ```
+                // Each sub-pattern gets a new line
+                return CreateAdjustNewLinesOperation(1, AdjustNewLinesOption.ForceLines);


we usually never use force line in general formatting rule. that means, user can never put lines between propertysubpatternSyntax.

is this syntax type only exist in your feature branch?

PropertySubpatternSyntax

Yes, this syntax is in features/recursive-patterns (and also in features/build-demo).

~~This is analogous to the colon in switch labels (see below, line 124).~~
I'm mistaken. The handling of switch labels below is doing PreserveLines. I'll fix.

heejaechang · 2018-05-01T04:17:55Z

+    {
+        _ = this switch
+        {
+            { P1: 1 } => true,


I think you dont have new line rule for ","

CyrusNajmabadi · 2018-05-01T05:00:04Z

+            }
+
+            if (token.Parent.IsKind(SyntaxKind.PropertySubpattern) == false ||
+                token.Parent.Parent.IsKind(SyntaxKind.PropertyPattern) == false)


== false and || feels weird.... why not just if token.parent.iskind() && token.parent.parent.iskind() ? token : default? #Closed

the parent of a PropertySubpattern can also be a DeconstructionPattern:

(1, 2) { P1: 1, P2: 2 }

CyrusNajmabadi · 2018-05-01T05:01:47Z

+            // PROTOTYPE(NullableReferenceTypes): 
+            // All the cases above should be replaced by using the semantic model's GetTypeInfo (ConvertedType) once that is integrated
+
+            return default;


i feel like there's a bunch of a logic shared here and between the work i'm doing in my PR where i update the TypeInferenceService... i don't have a great suggestion on how to unify things though :-/

The logic for TryGetPatternType can be removed once Neal's PR for GetTypeInfo gets merged.

There will still be some holes. For instance, when typing a property pattern inside a positional pattern and the positional pattern isn't completely typed yet, so the Deconstruct method may not be fully resolved yet. But I think we'll fix that from compiler side (best overload candidate should be returned in that case, for IDE to use (just as in SignatureHelp).

Beyond that, I don't expect much need for inference from IDE side in patterns.

CyrusNajmabadi · 2018-05-01T05:02:34Z

+                if (token.Parent is PropertySubpatternSyntax subpattern)
+                {
+                    return new HashSet<string>(subpattern.SubPatterns.Select(
+                        p => p.NameColon?.Name?.Identifier.ValueText).Where(s => !string.IsNullOrEmpty(s)));


if you have subpatterns, can you not have a NameColon or Name? Are those really optional? if so... i'm curious why....

See error syntax, which shows no NameColon.
I don't know about no Name, but I preferred to be defensive.

Neme12 · 2018-05-01T09:30:51Z

+        {
+            // is Goo { $$
+            // is Goo { P1: 0, $$
+            var propertyPattern = (PropertyPatternSyntax)token.Parent.Parent;


This can also be a DeconstructionPatternSyntax

Neme12 · 2018-05-01T09:39:35Z

+        }
+
+        [Fact(Skip = "PROTOTYPE(patterns2)")]
+        public async Task PropertiesInRecursivePattern_WithPositionalPattern()


Please fix this. You need to take into account that the parent of a PropertySubpattern can also be a DeconstructionPattern, not just PropertyPattern, everywhere you expect that to be the case.

See https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLRIMaOQSwG4SoAOsMECAdsgDQwhTIzUAmIA1AD4ACADAARcAjAG4AsACguAZgEAmPgGE+Abwl91AmVwAsfALIAKAJRqNq8Rst8A+nwC8fGAAscyPq74HB1PrKMq+AAVBED5vINlQ+QBfMQsNaNN1JPcKGCDBAIBzCBhhPkT49Rw0iOzc/MLLFJ0+ABEIDAB7KhgEOAwYAya4dJL0qB8evtLgEyKVFMsoez5gWZ44y0LooA===

then make sure it works both with and without a type just like with a PropertyPattern:

this is C (1, 2) { P1: 1, P2: 2 }; this is (1, 2) { P1: 1, P2: 2 };

Because this is a different SyntaxNode and you need to have separate cases to get the type in each one, I would expect additional tests such as
PropertiesInRecursivePattern_UseStaticTypeFromIs_DeconstructionPattern
PropertiesInRecursivePattern_UseStaticTypeFromSwitchStatement_DeconstructionPattern and
PropertiesInRecursivePattern_WithOtherType_DeconstructionPattern

I don't plan to expand the cases handled by the completion provider in this PR. As noted, we have a design to supercede this approach, using GetTypeInfo. I'm waiting for that to be merged.

Note: even when you do that, you will still have to update the code to get the DeconstructionPattern in order to use GetTypeInfo on it. Right now I found 2 places in the code that assume the parent can only be a PropertyPattern and bail out if it's not.

jcouv · 2018-05-02T03:35:58Z

I'm going to close this PR in favor of:

Completion for property-based pattern and 'switch' keyword in switch expression #26553 (the completion changes)
Formatting for recursive patterns #26555 (the formatting changes, still WIP)

jcouv added 3 commits April 27, 2018 23:33

Fix brace and property completion for property sub-pattern

63607b5

Workaround for assertion - DO NOT MERGE

4001afe

Detect static type for pattern

2502889

jcouv added Area-IDE Feature - Pattern Matching Pattern Matching labels Apr 28, 2018

jcouv added this to the 16.0 milestone Apr 28, 2018

jcouv self-assigned this Apr 28, 2018

jcouv requested review from a team as code owners April 28, 2018 14:26

jcouv commented Apr 28, 2018

View reviewed changes

Add support for fields in property-based patterns. More tests

2ff2644

jcouv force-pushed the patterns-ide branch from a6d5863 to 2ff2644 Compare April 28, 2018 15:15

Complete switch keyword for switch expressions

8fc6d0d

Neme12 reviewed Apr 28, 2018

View reviewed changes

CyrusNajmabadi reviewed Apr 30, 2018

View reviewed changes

jcouv added 3 commits April 30, 2018 18:53

Address some PR feedback from Neme

8055535

Trying to indent inside switch expression

3280d92

Suppress on whole switch expression and pattern case

86831a3

heejaechang reviewed May 1, 2018

View reviewed changes

Address PR feedback from Cyrus

f46c681

CyrusNajmabadi reviewed May 1, 2018

View reviewed changes

Fix spacing between type and positional pattern

5a8499d

Neme12 reviewed May 1, 2018

View reviewed changes

Julien Couvreur added 4 commits May 1, 2018 12:32

No newline when typing 'e is {'

b85e685

Anchor arm expressions to arm

2c92124

Newlines for commas in switch expression

935247a

Fix indentation on arms of switch expression

104219d

This was referenced May 2, 2018

Completion for property-based pattern and 'switch' keyword in switch expression #26553

Merged

Formatting for recursive patterns #26555

Merged

jcouv closed this May 2, 2018

jcouv mentioned this pull request May 2, 2018

ObjectCreationInitializer completion doesn't escape keywords #26560

Closed

Conversation

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

Uh oh!

Uh oh!

jcouv Apr 28, 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!

gafter Apr 28, 2018 • 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 Apr 28, 2018

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 28, 2018 • 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 Apr 28, 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!

Neme12 Apr 28, 2018 • 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!

Neme12 Apr 28, 2018 • 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!

Neme12 Apr 28, 2018 • 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!

Neme12 Apr 28, 2018 • 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!

Neme12 Apr 28, 2018

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 28, 2018 • 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 Apr 28, 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!

Neme12 Apr 28, 2018 • 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 Apr 28, 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!

Neme12 Apr 28, 2018 • 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 Apr 28, 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!

Neme12 Apr 28, 2018 • 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 Apr 28, 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!

Neme12 Apr 28, 2018 • 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!

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

Uh oh!

jcouv commented Apr 28, 2018 •

edited

Loading

jcouv Apr 28, 2018 •

edited

Loading

gafter Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

jcouv Apr 28, 2018 •

edited

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

jcouv Apr 28, 2018 •

edited

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

jcouv Apr 28, 2018 •

edited

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

jcouv Apr 28, 2018 •

edited

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

jcouv Apr 28, 2018 •

edited

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 30, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

Neme12 Apr 28, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 30, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 30, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 30, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 30, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 30, 2018 •

edited by jcouv

Loading

jcouv May 1, 2018 •

edited

Loading