List patterns: main IDE scenarios and AddRequiredParens #53850

jcouv · 2021-06-03T15:06:17Z

Basic scenarios (first commit):

completion
QuickInfo
formatting
classification

~~More advanced scenarios (second commit):~~
~~- AddRequiredParens~~
~~- RemoveUnecessaryParens~~

Test plan #51289

jcouv · 2021-06-03T15:12:47Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/OperatorPrecedence.cs

@@ -27,6 +27,7 @@ internal enum OperatorPrecedence
        Multiplicative,
        Switch,
        Range,
+        Slice,


Slice,

📝 I don't know what is the correct precedence order in this list. #Resolved

it feels like this shoudl be higher up right? i.e. if i have .. y or z then it's parsed a .. (y or z) not (..y) or z. So .. has lower precedence than than all the binaries. so i would expect this to at least be above ConditionalOr. We should probably note that .. as a range and .. as a slice are very different wrt precedence.

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/SpacingFormattingRule.cs

...rp/Analyzers/AddRequiredParentheses/CSharpAddRequiredPatternParenthesesDiagnosticAnalyzer.cs

CyrusNajmabadi · 2021-06-29T18:39:12Z

src/Workspaces/CSharpTest/Formatting/FormattingTests.cs

+{
+    void M()
+    {
+        _ = this is [ 1, 2, >= 3 ];


_ = this is [1, 2, >= 3] #Resolved

i think this will be like parens. and we'll want to have no spaces around them. similar to foo[1, 2, 3] as well.

...UtilitiesAndExtensions/Compiler/CSharp/Extensions/ParenthesizedExpressionSyntaxExtensions.cs

CyrusNajmabadi · 2021-06-29T18:40:29Z

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/SpacingFormattingRule.cs

+            {
+                // is [ > 0$$]
+                return CreateAdjustSpacesOperation(1, AdjustSpacesOption.ForceSpacesIfOnSingleLine);
+            }


definitely not sure about this. #Resolved

jcouv · 2021-10-19T18:56:08Z

@CyrusNajmabadi I've removed the AddParens change from this PR.
Now, it just covers completion, QuickInfo and formatting. I'll add a classification test as well.

CyrusNajmabadi · 2021-10-19T19:35:09Z

Can you add at least 1-2 indentation tests?

In reply to: 947044345

CyrusNajmabadi · 2021-10-19T19:35:44Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNormalizer.cs

@@ -520,6 +520,39 @@ private static bool NeedsSeparatorForPositionalPattern(SyntaxToken token, Syntax
            return false;
        }

+        private static bool NeedsSeparatorForListPattern(SyntaxToken token, SyntaxToken next)
+        {
+            ListPatternSyntax? listPattern;


Suggested change

ListPatternSyntax? listPattern;

ListPatternSyntax listPattern;

@jcouv your edit to this comment was breaking markdown syntax so I reverted it

CyrusNajmabadi · 2021-10-19T19:36:15Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNormalizer.cs

+            var nextIsOpenBracket = next.IsKind(SyntaxKind.OpenBracketToken);
+            if (nextIsOpenBracket)


Suggested change

var nextIsOpenBracket = next.IsKind(SyntaxKind.OpenBracketToken);

if (nextIsOpenBracket)

if (next.IsKind(SyntaxKind.OpenBracketToken))

@jcouv your edit to this comment was breaking markdown syntax so I reverted it

CyrusNajmabadi · 2021-10-19T19:36:31Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNormalizer.cs

+            var tokenIsOpenBracket = token.IsKind(SyntaxKind.OpenBracketToken);
+            if (tokenIsOpenBracket)


Suggested change

var tokenIsOpenBracket = token.IsKind(SyntaxKind.OpenBracketToken);

if (tokenIsOpenBracket)

if (token.IsKind(SyntaxKind.OpenBracketToken))

@jcouv your edit to this comment was breaking markdown syntax so I reverted it

CyrusNajmabadi · 2021-10-19T19:37:40Z

src/Workspaces/CSharpTest/Formatting/FormattingTests.cs

+{
+    void M()
+    {
+        _ = this is [0, .. var rest];


add test where ..var is the starting state, to ensure a space is added. #Resolved

CyrusNajmabadi · 2021-10-19T19:39:34Z

...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs

+            if (token.IsKind(SyntaxKind.DotDotToken)
+                && token.Parent.IsKind(SyntaxKind.SlicePattern))
+            {
+                return true;


this seems odd. what sort of expressions are legal here? #Resolved

Constants for example

how woudl that work? .. SomeConstant. How would that work? you've got a list there, so how would that list match against a constant?

Put another way, this is not a question if this is legal. It's a question of if the user has a genuine need to be able to write this. If not, then i would say: this is not an expr context as we don't want expr completion items to clutter things up here.

The slice method can return any type. The type could be int (then you can apply a constant pattern to it) or int[] (then you can apply a constant null pattern to it).
Those are technically correct, but not really useful... I'm okay to remove if you think that's better.

yes. please remove for now. i'm highly doubtful this will be an issue in practice. we can add back in the future if someone has a genuinely reasonable case they're running into (and in that case, i think we'd likely do a semantic check for them, rather than muddy teh completion space for everyone else).

jcouv · 2021-10-19T21:37:31Z

this seems... not good :)

It's not crashing ;-P
Also seems to be the same behavior as newline in other brackets (which is already not great):

        [WpfFact, Trait(Traits.Feature, Traits.Features.AutomaticCompletion)]
        public void Array_TODO2()
        {
            var code = @"class C
{
    int [] i = new int $$
}";

            var expected = @"class C
{
    int [] i = new int [
]
}";

            using var session = CreateSession(code);
            Assert.NotNull(session);

            CheckStart(session.Session);
            CheckReturn(session.Session, 0, expected);
        }

In reply to: 947125569

jcouv · 2021-10-19T22:15:24Z

this seems... not good :)

Spent some time trying to figure out indentation when a newline is introduced. Couldn't figure it out.
Given that we don't do it for other constructs with brackets (ie. arrays), I'd propose to handle later. This syntax will typically be on a single line. Let me know if that's okay.
Filled #57244 to track

In reply to: 947146285

CyrusNajmabadi · 2021-10-19T22:24:52Z

Yup. t hat's fine.

In reply to: 947151263

jcouv · 2021-10-21T20:00:17Z

~~Doing some more manual validation, there's still a problem with completing .. var x. I'll investigate and like push a fix this afternoon.~~

Never mind. I was testing stale bits...

CyrusNajmabadi · 2021-10-22T17:52:50Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNormalizer.cs

+            ListPatternSyntax listPattern;
+            if (token.Parent.IsKind(SyntaxKind.ListPattern))
+            {
+                listPattern = (ListPatternSyntax)token.Parent;
+            }
+            else if (next.Parent.IsKind(SyntaxKind.ListPattern))
+            {
+                listPattern = (ListPatternSyntax)next.Parent;
+            }
+            else
+            {
+                return false;
+            }


Suggested change

ListPatternSyntax listPattern;

if (token.Parent.IsKind(SyntaxKind.ListPattern))

{

listPattern = (ListPatternSyntax)token.Parent;

}

else if (next.Parent.IsKind(SyntaxKind.ListPattern))

{

listPattern = (ListPatternSyntax)next.Parent;

}

else

{

return false;

}

var listPattern = token.Parent as ListPatternSyntax ?? next.Parent as ListPatternSyntax;

if (listPattern == null)

return false;

nit, you can ignore this if you want.

CyrusNajmabadi · 2021-10-22T17:53:29Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNormalizer.cs

            }
+            if (NeedsSeparatorForListPattern(token, next))


Suggested change

}

if (NeedsSeparatorForListPattern(token, next))

}

if (NeedsSeparatorForListPattern(token, next))

i'm surpirsed there was no warning on this.

CyrusNajmabadi · 2021-10-22T17:54:50Z

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/SpacingFormattingRule.cs

@@ -436,6 +443,13 @@ public override AbstractFormattingRule WithOptions(AnalyzerConfigOptions options
                return CreateAdjustSpacesOperation(1, AdjustSpacesOption.ForceSpacesIfOnSingleLine);
            }

+            // Slice pattern:
+            // ..var x


Suggested change

// ..var x

// .. var x

jcouv added Area-Compilers Feature - List Patterns labels Jun 3, 2021

jcouv added this to the C# 10 milestone Jun 3, 2021

jcouv self-assigned this Jun 3, 2021

jcouv commented Jun 3, 2021

View reviewed changes

jcouv added Area-IDE PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. and removed Area-Compilers labels Jun 3, 2021

CyrusNajmabadi reviewed Jun 4, 2021

View reviewed changes

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/SpacingFormattingRule.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 4, 2021

View reviewed changes

...rp/Analyzers/AddRequiredParentheses/CSharpAddRequiredPatternParenthesesDiagnosticAnalyzer.cs Outdated Show resolved Hide resolved

jcouv force-pushed the list-patterns branch 4 times, most recently from f05a6c5 to dc68306 Compare June 29, 2021 17:50

CyrusNajmabadi reviewed Jun 29, 2021

View reviewed changes

...UtilitiesAndExtensions/Compiler/CSharp/Extensions/ParenthesizedExpressionSyntaxExtensions.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jun 29, 2021

View reviewed changes

jcouv mentioned this pull request Jun 30, 2021

List patterns: change syntax to use square brackets #54335

Merged

jaredpar modified the milestones: C# 10, 17.0 Jul 13, 2021

jcouv added 4 commits July 13, 2021 12:08

List patterns: main IDE scenarios

93c86a9

Honor bracket spacing preferences

513e1ad

tweak

cddc03d

Add tests with trailing commas

edcbb49

jcouv force-pushed the list-patterns branch from 533ba27 to edcbb49 Compare July 13, 2021 19:09

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 13, 2021

jcouv marked this pull request as ready for review July 13, 2021 23:49

jcouv requested review from a team as code owners July 13, 2021 23:49

jcouv requested a review from CyrusNajmabadi July 13, 2021 23:49

remove one more

7135871

Classification test

a55501f

CyrusNajmabadi reviewed Oct 19, 2021

View reviewed changes

jcouv added 2 commits October 19, 2021 12:55

Address feedback

004bf65

Fix bug with var completion

96343f3

runfoapp bot mentioned this pull request Oct 19, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

jcouv added 2 commits October 19, 2021 14:54

More newline tests

2ff202b

Remove expression context in slice

32964da

jcouv mentioned this pull request Oct 19, 2021

Formatting of list-patterns on multiple lines is incorrect #57244

Closed

Record open issue

7f12f63

jcouv requested a review from CyrusNajmabadi October 20, 2021 06:17

jcouv mentioned this pull request Oct 22, 2021

Test Plan for "List Patterns" #51289

Closed

91 tasks

CyrusNajmabadi reviewed Oct 22, 2021

View reviewed changes

CyrusNajmabadi approved these changes Oct 22, 2021

View reviewed changes

Suggested tweaks

b34d15f

jcouv enabled auto-merge (squash) October 22, 2021 23:38

jcouv merged commit 2259e13 into dotnet:features/list-patterns Oct 23, 2021

jcouv deleted the list-patterns branch October 23, 2021 05:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List patterns: main IDE scenarios and AddRequiredParens #53850

List patterns: main IDE scenarios and AddRequiredParens #53850

jcouv commented Jun 3, 2021 •

edited

Loading

jcouv Jun 3, 2021 •

edited

Loading

CyrusNajmabadi Jun 4, 2021

CyrusNajmabadi Jun 29, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Jun 29, 2021

CyrusNajmabadi Jun 29, 2021 •

edited by jcouv

Loading

jcouv commented Oct 19, 2021

CyrusNajmabadi commented Oct 19, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Oct 19, 2021 •

edited by sharwell

Loading

sharwell Oct 21, 2021

CyrusNajmabadi Oct 19, 2021 •

edited by sharwell

Loading

sharwell Oct 21, 2021

CyrusNajmabadi Oct 19, 2021 •

edited by sharwell

Loading

sharwell Oct 21, 2021

CyrusNajmabadi Oct 19, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Oct 19, 2021 •

edited by jcouv

Loading

jcouv Oct 19, 2021

CyrusNajmabadi Oct 19, 2021

jcouv Oct 19, 2021

CyrusNajmabadi Oct 19, 2021

jcouv commented Oct 19, 2021 •

edited

Loading

jcouv commented Oct 19, 2021 •

edited

Loading

CyrusNajmabadi commented Oct 19, 2021 •

edited by jcouv

Loading

jcouv commented Oct 21, 2021 •

edited

Loading

CyrusNajmabadi Oct 22, 2021

CyrusNajmabadi Oct 22, 2021

CyrusNajmabadi Oct 22, 2021

CyrusNajmabadi Oct 22, 2021

	ListPatternSyntax? listPattern;
	ListPatternSyntax listPattern;

		var nextIsOpenBracket = next.IsKind(SyntaxKind.OpenBracketToken);
		if (nextIsOpenBracket)

	var nextIsOpenBracket = next.IsKind(SyntaxKind.OpenBracketToken);
	if (nextIsOpenBracket)
	if (next.IsKind(SyntaxKind.OpenBracketToken))

		var tokenIsOpenBracket = token.IsKind(SyntaxKind.OpenBracketToken);
		if (tokenIsOpenBracket)

	var tokenIsOpenBracket = token.IsKind(SyntaxKind.OpenBracketToken);
	if (tokenIsOpenBracket)
	if (token.IsKind(SyntaxKind.OpenBracketToken))

List patterns: main IDE scenarios and AddRequiredParens #53850

List patterns: main IDE scenarios and AddRequiredParens #53850

Conversation

jcouv commented Jun 3, 2021 • edited Loading

jcouv Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Slice,

Choose a reason for hiding this comment

CyrusNajmabadi Jun 29, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Jun 29, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Oct 19, 2021

CyrusNajmabadi commented Oct 19, 2021 • edited by jcouv Loading

CyrusNajmabadi Oct 19, 2021 • edited by sharwell Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Oct 19, 2021 • edited by sharwell Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Oct 19, 2021 • edited by sharwell Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Oct 19, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Oct 19, 2021 • edited by jcouv 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

jcouv commented Oct 19, 2021 • edited Loading

jcouv commented Oct 19, 2021 • edited Loading

CyrusNajmabadi commented Oct 19, 2021 • edited by jcouv Loading

jcouv commented Oct 21, 2021 • 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

jcouv commented Jun 3, 2021 •

edited

Loading

jcouv Jun 3, 2021 •

edited

Loading

CyrusNajmabadi Jun 29, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Jun 29, 2021 •

edited by jcouv

Loading

CyrusNajmabadi commented Oct 19, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Oct 19, 2021 •

edited by sharwell

Loading

CyrusNajmabadi Oct 19, 2021 •

edited by sharwell

Loading

CyrusNajmabadi Oct 19, 2021 •

edited by sharwell

Loading

CyrusNajmabadi Oct 19, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Oct 19, 2021 •

edited by jcouv

Loading

jcouv commented Oct 19, 2021 •

edited

Loading

jcouv commented Oct 19, 2021 •

edited

Loading

CyrusNajmabadi commented Oct 19, 2021 •

edited by jcouv

Loading

jcouv commented Oct 21, 2021 •

edited

Loading