Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Mar 9, 2018
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4fa524b
Adjust indent block operations used for VB smart indent in argument l…
DustinCampbell Mar 8, 2018
7b9fb74
Add two unit tests to verify VB smart indentation behavior when argum…
DustinCampbell Mar 8, 2018
bf9aeb8
Add unit test that properly covers scenario where the first argument …
DustinCampbell Mar 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion src/EditorFeatures/VisualBasic/Formatting/Indentation/SpecialFormattingOperation.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.Formatting.Indentation
End If

AddIndentBlockOperations(Of ParameterListSyntax)(list, node, Function(n) Not n.OpenParenToken.IsMissing AndAlso n.Parameters.Count > 0)
AddIndentBlockOperations(Of ArgumentListSyntax)(list, node, Function(n) Not n.OpenParenToken.IsMissing AndAlso n.Arguments.Count > 0 AndAlso n.Arguments.Any(Function(a) Not a.IsMissing))
AddArgumentListIndentBlockOperations(list, node)
AddIndentBlockOperations(Of TypeParameterListSyntax)(list, node, Function(n) Not n.OpenParenToken.IsMissing AndAlso n.Parameters.Count > 0, indentationDelta:=1)
End Sub

Expand Down Expand Up @@ -106,6 +106,62 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.Formatting.Indentation
baseToken, startToken, endToken, TextSpan.FromBounds(baseToken.Span.End, closeBrace.Span.End), indentationDelta, IndentBlockOption.RelativePosition))
End Sub

Private Sub AddArgumentListIndentBlockOperations(operations As List(Of IndentBlockOperation), node As SyntaxNode)
Dim argumentList = TryCast(node, ArgumentListSyntax)
If argumentList Is Nothing OrElse
argumentList.OpenParenToken.IsMissing OrElse
argumentList.Arguments.Count = 0 OrElse
argumentList.Arguments.All(Function(a) a.IsMissing) Then
Return
End If

Dim openBrace = argumentList.GetFirstToken(includeZeroWidth:=True)
Dim closeBrace = argumentList.GetLastToken(includeZeroWidth:=True)

Dim sourceText = node.SyntaxTree.GetText()

' First, create a list of all arguments that start lines.
Dim arguments = New List(Of ArgumentSyntax)

Dim previousLineNumber = -1
For Each argument In argumentList.Arguments
Dim lineNumber = sourceText.Lines.GetLineFromPosition(argument.SpanStart).LineNumber
If lineNumber > previousLineNumber Then
arguments.Add(argument)
Copy link
Member

@sharwell sharwell Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ 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?

Copy link
Contributor

@heejaechang heejaechang Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@heejaechang heejaechang Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@DustinCampbell DustinCampbell Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

previousLineNumber = lineNumber
End If
Next

' Next create indent block operations using the arguments that start each line.
' These indent block operations span to the start of the next argument starting a line,
' or the close brace of the argument list if there isn't another argument.
For i = 0 To arguments.Count - 1
Dim argument = arguments(i)
Dim baseToken = argument.GetFirstToken(includeZeroWidth:=True)
Dim startToken = baseToken.GetNextToken(includeZeroWidth:=True)

Dim endToken As SyntaxToken
Dim span As TextSpan

If i < arguments.Count - 1 Then
Dim nextArgument = arguments(i + 1)
Dim firstToken = nextArgument.GetFirstToken(includeZeroWidth:=True)

endToken = firstToken.GetPreviousToken(includeZeroWidth:=True)

span = TextSpan.FromBounds(baseToken.Span.End, firstToken.SpanStart)
Else
endToken = closeBrace.GetPreviousToken(includeZeroWidth:=True)
span = TextSpan.FromBounds(baseToken.Span.End, closeBrace.Span.End)
End If

Dim operation = FormattingOperations.CreateRelativeIndentBlockOperation(
baseToken, startToken, endToken, span, indentationDelta:=0, IndentBlockOption.RelativePosition)

operations.Add(operation)
Next
End Sub

Public Overrides Sub AddAlignTokensOperations(operations As List(Of AlignTokensOperation), node As SyntaxNode, optionSet As OptionSet, nextAction As NextAction(Of AlignTokensOperation))
MyBase.AddAlignTokensOperations(operations, node, optionSet, nextAction)

Expand Down
73 changes: 73 additions & 0 deletions src/EditorFeatures/VisualBasicTest/Formatting/Indentation/SmartIndenterTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2802,6 +2802,79 @@ End Class

<WorkItem(3293, "https://github.com/dotnet/roslyn/issues/3293")>
<WpfFact, Trait(Traits.Feature, Traits.Features.SmartIndent)>
Public Sub TestSmartIndentInArgumentLists1()
Dim code = "
Class C
Sub M()
Console.WriteLine(""{0} + {1}"",

End Sub
End Class"

AssertSmartIndent(
code,
indentationLine:=4,
expectedIndentation:=26)
End Sub

<WorkItem(3293, "https://github.com/dotnet/roslyn/issues/3293")>
<WpfFact, Trait(Traits.Feature, Traits.Features.SmartIndent)>
Public Sub TestSmartIndentInArgumentLists2()
Dim code = "
Class C
Sub M()
Console.WriteLine(""{0} + {1}"",
19,

End Sub
End Class"

AssertSmartIndent(
code,
indentationLine:=5,
expectedIndentation:=12)
End Sub

<WorkItem(3293, "https://github.com/dotnet/roslyn/issues/3293")>
<WpfFact, Trait(Traits.Feature, Traits.Features.SmartIndent)>
Public Sub TestSmartIndentInArgumentLists3()
Dim code = "
Class C
Sub M()
Console.WriteLine(""{0} + {1}"",
19, 23 +
Copy link
Member

@sharwell sharwell Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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:

  1. The argument is the first argument that starts on the line
  2. 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

19,

End Sub
End Class"

AssertSmartIndent(
code,
indentationLine:=6,
expectedIndentation:=12)
End Sub

<WorkItem(3293, "https://github.com/dotnet/roslyn/issues/3293")>
<WpfFact, Trait(Traits.Feature, Traits.Features.SmartIndent)>
Public Sub TestSmartIndentInArgumentLists4()
Dim code = "
Class C
Sub M()
Console.WriteLine(""{0} + {1}"",
19, 23,
19,

End Sub
End Class"

AssertSmartIndent(
code,
indentationLine:=6,
expectedIndentation:=16)
End Sub

<WorkItem(25323, "https://github.com/dotnet/roslyn/issues/25323")>
<WpfFact, Trait(Traits.Feature, Traits.Features.SmartIndent)>
Public Sub TestSmartIndentAtCaseBlockEndUntabbedComment()
Dim code = <code>Class Program
Public Sub M()
Expand Down