Fixed List pattern is not correctly formatted in the first arm of a switch expression #72269

victor-pogor · 2024-02-25T09:07:21Z

UPD#1: converted to draft PR as other tests fail (CollectionExpressionWrappingTests), not sure the code change is the right one
UPD#2: It seems this bug was introduced by this change

I analyzed in comparison with the case if using property pattern. The code tries to get the indentation from the smallest containing interval, but since the alignment block operation does not calculate the spans correctly, it takes the indentation value from wrong (but indeed smallest) interval.

After code changes the same smallest interval is picked as we have in case the first switch arm is a property pattern:

ListPattern.mp4

…witch expression fixes dotnet#72196

victor-pogor · 2024-02-25T12:53:10Z

...s/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/IndentBlockFormattingRule.cs

-                var lastToken = node.GetLastToken(includeZeroWidth: true);
-                var baseToken = firstToken.GetPreviousToken(includeZeroWidth: true);
-
-                SetAlignmentBlockOperation(list, baseToken, firstToken, lastToken, option);


An attempt (reverting the code + changes state below) that fixes the failing CollectionExpressionWrappingTests

if (node.Parent is not SwitchExpressionArmSyntax) { SetAlignmentBlockOperation(list, baseToken, firstToken, lastToken, option); } else { AddAlignmentBlockOperationRelativeToFirstTokenOnBaseTokenLine(list, bracketPair); }

But I have a feeling this is not the right way :)

this seems generally odd to me. i would expect taht indent block operations would be set for the switch expression, and its arms. Nothing to do with list expressions.

Switch has indent block operations but not his arms. I think they don't need to have.

I think the problem is having wrong base & first tokens for list pattern

var result1 = new int[] { 1, 2 } switch // List pattern { [1] => "one", _ => "unknown" }; var result2 = new int[] { 1, 2 } switch // property pattern clause { { Length: 0 } => "empty", [1] => "one", _ => "unknown" };

First Switch expression arm Base Token Start Token End Token

Property pattern clause { (opening prop pattern) Length } (closing prop pattern)

List pattern expected [ (opening list pattern) 1 ] (closing list pattern)

List pattern actual { (opening switch expression) [ ] (closing list pattern)

victor-pogor · 2024-02-27T22:32:58Z

...s/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/IndentBlockFormattingRule.cs

@@ -257,7 +257,14 @@ private static void AddBracketIndentationOperation(List<IndentBlockOperation> li
            if (!bracketPair.IsValidBracketOrBracePair())
                return;

-            if (node.Parent != null && node.Kind() is SyntaxKind.ListPattern or SyntaxKind.CollectionExpression)
+            if (node.Parent != null && node.Kind() is SyntaxKind.ListPattern)


@CyrusNajmabadi I think the safest way is to separate ListPattern from CollectionExpression and use the logic before this change.

As I know list pattern doesn't support wrapping and we don't need to handle the double indentation issue.

PS: please take a look also at the previous comment

victor-pogor · 2024-02-29T15:26:26Z

@CyrusNajmabadi any feedback here? 🌝

CyrusNajmabadi · 2024-02-29T16:11:23Z

Can look later. On vacation. :+)

victor-pogor · 2024-03-27T09:32:01Z

@CyrusNajmabadi kind reminder :)

sharwell · 2024-03-27T13:35:33Z

...s/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/IndentBlockFormattingRule.cs

@@ -257,7 +257,14 @@ private static void AddBracketIndentationOperation(List<IndentBlockOperation> li
        if (!bracketPair.IsValidBracketOrBracePair())
            return;

-        if (node.Parent != null && node.Kind() is SyntaxKind.ListPattern or SyntaxKind.CollectionExpression)
+        if (node.Parent != null && node.Kind() is SyntaxKind.ListPattern)


💡 The IsKind method is more efficient than Kind

Suggested change

if (node.Parent != null && node.Kind() is SyntaxKind.ListPattern)

if (node.Parent != null && node.IsKind(SyntaxKind.ListPattern))

sharwell · 2024-03-27T13:36:25Z

...s/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/IndentBlockFormattingRule.cs

+            return;
+        }
+
+        if (node.Parent != null && node.Kind() is SyntaxKind.CollectionExpression)
        {
            // Brackets in list patterns are formatted like blocks, so align close bracket with open bracket


💡 This comment seems incorrect now

sharwell · 2024-03-27T13:41:57Z

...s/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/IndentBlockFormattingRule.cs

-        if (node.Parent != null && node.Kind() is SyntaxKind.ListPattern or SyntaxKind.CollectionExpression)
+        if (node.Parent != null && node.Kind() is SyntaxKind.ListPattern)
+        {
+            AddAlignmentBlockOperationRelativeToFirstTokenOnBaseTokenLine(list, bracketPair);


💭 What happens if we omit this line?

What happens if we remove the SetAlignmentBlockOperation call for CollectionExpression?

What happens if we change the call for CollectionExpression to use the same form we have for ListPattern?

sharwell · 2024-03-27T13:42:46Z

src/Workspaces/CSharpTest/Formatting/FormattingTests.cs

+        }
+
+        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/72196")]
+        public async Task TestSwitchExpression_WithDiscardOperator_ShouldFormatListPattern()


📝 There is no list pattern here

sharwell · 2024-03-27T13:43:52Z

src/Workspaces/CSharpTest/Formatting/FormattingTests.cs

+                code: @"
+var result3 = new int[] { 1, 2 } switch
+{
+    { Length: 0 } => ""empty"",
+[1] => ""one"",
+    _ => ""unknown""
+};
+",
+                expected: @"
+var result3 = new int[] { 1, 2 } switch
+{
+    { Length: 0 } => ""empty"",
+    [1] => ""one"",
+    _ => ""unknown""
+};
+");


If these are the same, can you extract a local variable for it?

sharwell · 2024-03-27T13:44:15Z

src/Workspaces/CSharpTest/Formatting/FormattingTests.cs

+                code: @"
+var result3 = new int[] { 1, 2 } switch
+{
+    { Length: 0 } => ""empty"",
+    [1] => ""one"",
+    _ => ""unknown""
+};
+",
+                expected: @"
+var result3 = new int[] { 1, 2 } switch
+{
+    { Length: 0 } => ""empty"",
+    [1] => ""one"",
+    _ => ""unknown""
+};


If these are the same, can you extract a local variable for it?

sharwell · 2024-03-27T13:48:16Z

src/Workspaces/CSharpTest/Formatting/FormattingTests.cs

@@ -10940,5 +10940,128 @@ public async Task TestNormalizeUsingAlias(string text, string expected)
        {
            await AssertFormatAsync(expected, text);
        }
+
+        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/72196")]
+        public async Task TestSwitchExpression_WithFormattedListPattern_ShouldNotFormatListPattern()


In theory, this test should not be necessary. Ideally each call to AssertFormatAsync should verify both of the following:

Formatting code produces expected

If code and expected are not the same, formatting expected also produces expected

Unfortunately, the AssertFormatAsync method is a bit convoluted to applying this change should be done in a separate pull request.

victor-pogor requested a review from a team as a code owner February 25, 2024 09:07

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 25, 2024

victor-pogor marked this pull request as draft February 25, 2024 10:19

Fixed List pattern is not correctly formatted in the first arm of a s…

b385a69

…witch expression fixes dotnet#72196

victor-pogor commented Feb 25, 2024

View reviewed changes

victor-pogor force-pushed the 72196-list-pattern-formatting-fix branch from cefea73 to b385a69 Compare February 26, 2024 10:01

victor-pogor added 2 commits February 28, 2024 00:25

Separated ListPattern and CollectionExpression conditions

a98e9d6

Fixed IDE0055 build pipeline

566aa90

victor-pogor commented Feb 27, 2024

View reviewed changes

Merge branch 'refs/heads/main' into 72196-list-pattern-formatting-fix

5192cac

victor-pogor marked this pull request as ready for review February 28, 2024 12:18

victor-pogor requested a review from CyrusNajmabadi February 29, 2024 10:45

Merge branch 'refs/heads/main' into 72196-list-pattern-formatting-fix

a832459

sharwell reviewed Mar 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed List pattern is not correctly formatted in the first arm of a switch expression #72269

Fixed List pattern is not correctly formatted in the first arm of a switch expression #72269

victor-pogor commented Feb 25, 2024 •

edited

Loading

victor-pogor Feb 25, 2024 •

edited

Loading

CyrusNajmabadi Feb 26, 2024

victor-pogor Feb 27, 2024

victor-pogor Feb 27, 2024 •

edited

Loading

victor-pogor commented Feb 29, 2024

CyrusNajmabadi commented Feb 29, 2024

victor-pogor commented Mar 27, 2024

sharwell Mar 27, 2024

sharwell Mar 27, 2024

sharwell Mar 27, 2024

sharwell Mar 27, 2024

sharwell Mar 27, 2024

sharwell Mar 27, 2024

sharwell Mar 27, 2024

First Switch expression arm	Base Token	Start Token	End Token
Property pattern clause	`{` (opening prop pattern)	`Length`	`}` (closing prop pattern)
List pattern expected	`[` (opening list pattern)	`1`	`]` (closing list pattern)
List pattern actual	`{` (opening switch expression)	`[`	`]` (closing list pattern)

	if (node.Parent != null && node.Kind() is SyntaxKind.ListPattern)
	if (node.Parent != null && node.IsKind(SyntaxKind.ListPattern))

Fixed List pattern is not correctly formatted in the first arm of a switch expression #72269

Are you sure you want to change the base?

Fixed List pattern is not correctly formatted in the first arm of a switch expression #72269

Conversation

victor-pogor commented Feb 25, 2024 • edited Loading

victor-pogor Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victor-pogor Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

victor-pogor commented Feb 29, 2024

CyrusNajmabadi commented Feb 29, 2024

victor-pogor commented Mar 27, 2024

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

victor-pogor commented Feb 25, 2024 •

edited

Loading

victor-pogor Feb 25, 2024 •

edited

Loading

victor-pogor Feb 27, 2024 •

edited

Loading