Adjust indent block operations used for VB smart indent in argument lists #25344

DustinCampbell · 2018-03-08T15:40:24Z

Customer scenario

Visual Basic argument lists (and other parenthesized lists, like parameter lists and type parameter lists),
have specialized smart indent behavior. Essentially, arguments after the first argument in a list are
indented to the same position as the first argument. Consider the following code.

Sub M()
    Console.WriteLine("{0} + {1} = {2}", 19, 23, 19 + 23)
End Sub

If the user presses ENTER before the second argument (19), the indentation looks like so.

Sub M()
    Console.WriteLine("{0} + {1} = {2}",
                      19, 23, 19 + 23)
End Sub

However, this can cause indentation issues if the user wants to ident all subsequent arguments differently. Consider the following scenario.

Sub M()
    Console.WriteLine("{0} + {1} = {2}",
        19, 23,
        19 + 23)
End Sub

Now, if the user presses ENTER before the third argument (23), the indentation of the first argument will be used, even though the user had indented the second argument differently:

Sub M()
    Console.WriteLine("{0} + {1} = {2}",
        19,
                      23,
        19 + 23)
End Sub

This problem occurs because a single indent block operation is created for the whole argument list using the indentation of the first argument. This change fixes the issue by creating multiple indent block operations using the indentation of the first argument, and the indentation of any argument that appears on a different line.

Bugs this fixes

Fixes #25323

Workarounds, if any

If the user wishes to format their argument lists in the manner described above, there really isn't a good way to workaround the issue. It's an annoyance and the editor will fight against the user's indentation.

Risk

Low. This change is localized to indentation in argument lists.

Performance impact

Low performance impact. Any new allocations do not happen in hot paths -- only when the user presses enter in an argument list.

Is this a regression from a previous update?

No.

Root cause analysis

I'm surprised we have not heard about this. This problem shows up in very simple cases where a method call has a large number of arguments. However, it is somewhat less common because many users will move the first argument to the next line and indent all arguments together at once.

How was the bug found?

I was reformatting the VB classification unit tests and found this issue to be particularly frustrating.

…ists Visual Basic argument lists (and other parenthesized lists, like parameter lists and type parameter lists), have specialized smart indent behavior. Essentially, arguments after the first argument in a list are indented to the same position as the first argument. Consider the following code. ``` VB Sub M() Console.WriteLine("{0} + {1} = {2}", 19, 23, 19 + 23) End Sub ``` If the user presses ENTER before the second argument (19), the indentation looks like so. ``` VB Sub M() Console.WriteLine("{0} + {1} = {2}", 19, 23, 19 + 23) End Sub ``` However, this can cause indentation issues if the user wants to ident all subsequent arguments differently. Consider the following scenario. ``` VB Sub M() Console.WriteLine("{0} + {1} = {2}", 19, 23, 19 + 23) End Sub ``` Now, if the user presses ENTER before the third argument (23), the indentation of the first argument will be used, even though the user had indented the second argument differently: ``` VB Sub M() Console.WriteLine("{0} + {1} = {2}", 19, 23, 19 + 23) End Sub ``` This problem occurs because a single indent block operation is created for the whole argument list using the indentation of the first argument. This change fixes the issue by creating multiple indent block operations using the indentation of the first argument, and the indentation of any argument that appears on a different line.

sharwell

Needs tests for additional edge case, if only to demonstrate current behavior.

sharwell · 2018-03-08T19:55:22Z

src/EditorFeatures/VisualBasic/Formatting/Indentation/SpecialFormattingOperation.vb

+            For Each argument In argumentList.Arguments
+                Dim lineNumber = sourceText.Lines.GetLineFromPosition(argument.SpanStart).LineNumber
+                If lineNumber > previousLineNumber Then
+                    arguments.Add(argument)


❓ What happens if the argument is split across lines, but doesn't actually start a line?

Method(a + b, c + d) ' <-- What happens if you add , and hit enter here?

I am fine to be better on smart indent on expression. but just as FYI.

both C# and VB never tries to be too smart on multi line expressions. every one has different liking on those on every different cases even for same construct. (that's why formatter never force formatting on expressions)

we do have some special cases like argument or parameter lists, but even for those, we did very simple thing. (so that it can be predictable)

most of time, if expression is more than 2 lines a part, we just give up and act like simple block indent (either align with previous line or 4 space added after previous line)

one thing I hope we don't do is, we forcing people to follow our liking and make them to demand options for it.

Thanks! Good question. I'll add a test or two to cover this scenario.

@heejaechang: We aren't forcing people to follow our liking with this change and no options would ever be required. Instead, we're treating the special case of argument lists (which is different between C# and VB) to be a bit more natural. IOW, this should better match the user's expectations.

…ents span lines

sharwell · 2018-03-08T22:12:15Z

src/EditorFeatures/VisualBasicTest/Formatting/Indentation/SmartIndenterTests.vb

+Class C
+    Sub M()
+        Console.WriteLine(""{0} + {1}"",
+            19, 23 +


📝 This test doesn't cover the case I was describing because 19 is an argument that starts a line. 23 + gets excluded because it is on the same line as 19. I'm looking for a test case where an argument meets both of the following conditions:

The argument is the first argument that starts on the line

The first token of the argument is not the first token on the line

📝 My case isn't as contrives as you'd think. Consider the following:

Method( CallArgument1( x, y, z ), Argument2, ' <-- Press enter here

Sorry, I missed the first + in your original example. Will update.

No problem, the additional ones you did add were good tests too

And FWIW, I would expect that it would indent to the second argument in this case. I don't think that's necessarily correct, but it's a pretty fishy scenario.

…on a line is not the first token and is split across multiple lines

sharwell · 2018-03-08T22:42:04Z

src/EditorFeatures/VisualBasicTest/Formatting/Indentation/SmartIndenterTests.vb


    End Sub
 End Class"

            AssertSmartIndent(
                code,
                indentationLine:=6,
-                expectedIndentation:=16)
+                expectedIndentation:=13)


💡 This is super weird IMO. It would be better to filter arguments to cases where they start on a line. If not in this pull request, then it would be good to file a bug to clean this up "eventually". 😄

If I understand the system correctly, filtering to just arguments that start a line would be even less predictable. This would leave gaps in the argument list where there isn't an indent block operation. In those cases, the indenter would move up the tree to the next indent block operation, which would likely be some node above the argument list. Consider the following code where the user presses ENTER after d,:

Sub M() Method(a + b, c + d, End Sub

If there isn't an indent block operation created for c + d, the indentation would be 4 because the indent block operation from Sub M() .. End Sub would be used.

@heejaechang, please correct me if I'm not understanding the code correctly.

@sharwell: I think the right experience is to for the first line break after the first argument in an argument list to use the same indentation as the first argument. Then, subsequent line breaks would just use the previous line's indentation. However, I couldn't find a way to achieve that in this system. 😞

Skipping arguments because they don't start a line would be equivalent to skipping trailing arguments on the same line. The indentation block from the previous (or first) argument would be used. For the case above, pressing Enter after d would indent to match a.

Ah yes, you're correct. 😄 I think that'd still be a weird experience though.

Sub M() Method(a + b, c + d, | End Sub

smart indentation, first use formatting rule to find indentation, after that it uses simple indentation logic (well, it is bunch of special cases around construct, so logic is simple but code is messy..) to find out where to put caret.
(http://index/?leftProject=Microsoft.CodeAnalysis.EditorFeatures&leftSymbol=poqp1cwzffsy&rightProject=Microsoft.CodeAnalysis.CSharp.EditorFeatures&file=Formatting%5CIndentation%5CCSharpIndentationService.Indenter.cs&line=136)

the simple indentation logic does either following previous line or add 4 more spaces.

the special formatting rule for argument, parameter was added for VB since vb had this behavior where caret aligned to first argument/parameter in the list rather than following previous line indentation or 4 spaces.

ex)
void method(A b,
............| <= here

if one wants this kind of behavior
void method(A b,
....| <= here

one can just remove formatting rule, and let default smart indentation logic to take over.

if one wants to use formatting rule for it, you can use this
http://index/?query=FormattingRu&rightProject=Microsoft.CodeAnalysis.Workspaces&file=Formatting%5CRules%5COperations%5CIndentBlockOption.cs&line=16 and set something like "," as base token, but formatter was not designed to format expressions. it is designed to leave expression exactly as they were as much as possible, so I dont think there is enough options or knobs around expressions.

I think if behavior desired is just letting caret to be either aligned with previous line or 4 more spaces after previous line, it would be better just to let default smart indent behavior to take care of it.

Thanks @heejaechang. I think the right behavior for VB probably is:

Align with the first argument on the second line of an argument list.

Align with the previous line for subsequent lines in an argument list.

If I'm understanding you, removing the current VB formatting rule wouldn't provide that experience, correct?

yep. I think your change is general improvement around this area. I was just saying if we want to make it even better, we can tweak it a bit more.

jinujoseph · 2018-03-08T23:48:37Z

@Pilchie for 15.7.P2 ask mode approval

Pilchie · 2018-03-09T01:26:46Z

Approved - thanks!

DustinCampbell · 2018-03-09T16:11:25Z

Thanks @Pilchie! @heejaechang / @sharwell, I've filed #25371 to track further improvement in this area.

DustinCampbell requested a review from a team as a code owner March 8, 2018 15:40

jinujoseph assigned heejaechang Mar 8, 2018

jinujoseph added this to the 15.7 milestone Mar 8, 2018

heejaechang approved these changes Mar 8, 2018

View reviewed changes

sharwell requested changes Mar 8, 2018

View reviewed changes

Add two unit tests to verify VB smart indentation behavior when argum…

7b9fb74

…ents span lines

sharwell reviewed Mar 8, 2018

View reviewed changes

Add unit test that properly covers scenario where the first argument …

bf9aeb8

…on a line is not the first token and is split across multiple lines

sharwell approved these changes Mar 8, 2018

View reviewed changes

Pilchie added the Approved to merge label Mar 9, 2018

DustinCampbell mentioned this pull request Mar 9, 2018

Improve Visual Basic smart indent for argument lists #25371

Closed

DustinCampbell merged commit 6d6fcc0 into dotnet:dev15.7.x Mar 9, 2018

jinujoseph mentioned this pull request Mar 21, 2018

VB: Smart indent issue with argument lists #25323

Closed

DustinCampbell deleted the issue-25323 branch August 24, 2021 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust indent block operations used for VB smart indent in argument lists #25344

Adjust indent block operations used for VB smart indent in argument lists #25344

DustinCampbell commented Mar 8, 2018

sharwell left a comment

sharwell Mar 8, 2018 •

edited

Loading

heejaechang Mar 8, 2018 •

edited

Loading

heejaechang Mar 8, 2018 •

edited

Loading

DustinCampbell Mar 8, 2018

DustinCampbell Mar 8, 2018 •

edited

Loading

sharwell Mar 8, 2018 •

edited

Loading

DustinCampbell Mar 8, 2018

sharwell Mar 8, 2018

DustinCampbell Mar 8, 2018

sharwell Mar 8, 2018

DustinCampbell Mar 8, 2018 •

edited

Loading

DustinCampbell Mar 8, 2018 •

edited

Loading

sharwell Mar 8, 2018 •

edited

Loading

DustinCampbell Mar 8, 2018

heejaechang Mar 9, 2018 •

edited

Loading

DustinCampbell Mar 9, 2018

heejaechang Mar 9, 2018

jinujoseph commented Mar 8, 2018

Pilchie commented Mar 9, 2018

DustinCampbell commented Mar 9, 2018

Adjust indent block operations used for VB smart indent in argument lists #25344

Adjust indent block operations used for VB smart indent in argument lists #25344

Conversation

DustinCampbell commented Mar 8, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

sharwell left a comment

Choose a reason for hiding this comment

sharwell Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

heejaechang Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

heejaechang Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

sharwell Mar 8, 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

DustinCampbell Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

DustinCampbell Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

sharwell Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinujoseph commented Mar 8, 2018

Pilchie commented Mar 9, 2018

DustinCampbell commented Mar 9, 2018

sharwell Mar 8, 2018 •

edited

Loading

heejaechang Mar 8, 2018 •

edited

Loading

heejaechang Mar 8, 2018 •

edited

Loading

DustinCampbell Mar 8, 2018 •

edited

Loading

sharwell Mar 8, 2018 •

edited

Loading

DustinCampbell Mar 8, 2018 •

edited

Loading

DustinCampbell Mar 8, 2018 •

edited

Loading

sharwell Mar 8, 2018 •

edited

Loading

heejaechang Mar 9, 2018 •

edited

Loading