Skip to content
This repository was archived by the owner on Jul 12, 2022. It is now read-only.

Commit 72fc879

Browse files
jaredparjaredpar
committed
Fix VB trivia handling with comments + attrs
The explicit modifier code for VB wasn't correctly handling the leading trivia on types and methods. It mistakenly grabbed and replaced the trivia on the node instead of the appropriate keyword token when there were no other modifiers. This resulted in strange effects like comments being moved below attributes. Fixed the first modifier case to use the keyword for trivial handling instead.
1 parent 32fec62 commit 72fc879

File tree

2 files changed

+193
-19
lines changed

2 files changed

+193
-19
lines changed

‎src/Microsoft.DotNet.CodeFormatting.Tests/Rules/ExplicitVisibilityRuleTests.cs

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,42 @@ Friend Enum E
448448
Verify(text, expected, runFormatter: false, languageName: LanguageNames.VisualBasic);
449449
}
450450

451+
[Fact]
452+
public void TypeWithCommentAndAttribute()
453+
{
454+
var text = @"
455+
' Hello
456+
<Attr>
457+
Class C
458+
End Class";
459+
460+
var expected = @"
461+
' Hello
462+
<Attr>
463+
Friend Class C
464+
End Class";
465+
466+
Verify(text, expected, runFormatter: false, languageName: LanguageNames.VisualBasic);
467+
}
468+
469+
[Fact]
470+
public void TypePartialWithCommentAndAttribute()
471+
{
472+
var text = @"
473+
' Hello
474+
<Attr>
475+
Partial Class C
476+
End Class";
477+
478+
var expected = @"
479+
' Hello
480+
<Attr>
481+
Friend Partial Class C
482+
End Class";
483+
484+
Verify(text, expected, runFormatter: false, languageName: LanguageNames.VisualBasic);
485+
}
486+
451487
/// <summary>
452488
/// It is interesting to note that nested typs in VB.Net default to public accessibility
453489
/// instead of private as C# does.
@@ -506,6 +542,28 @@ End Sub
506542
Verify(text, expected, runFormatter: false, languageName: LanguageNames.VisualBasic);
507543
}
508544

545+
[Fact]
546+
public void MethodWithCommentAndAttribute()
547+
{
548+
var text = @"
549+
Class C
550+
' A Comment
551+
<Attr>
552+
Sub M1()
553+
End Sub
554+
End Class";
555+
556+
var expected = @"
557+
Friend Class C
558+
' A Comment
559+
<Attr>
560+
Public Sub M1()
561+
End Sub
562+
End Class";
563+
564+
Verify(text, expected, runFormatter: false, languageName: LanguageNames.VisualBasic);
565+
}
566+
509567
[Fact]
510568
public void Fields()
511569
{
@@ -546,6 +604,36 @@ End Sub
546604
Verify(text, expected, runFormatter: false, languageName: LanguageNames.VisualBasic);
547605
}
548606

607+
[Fact]
608+
public void ConstructorsWithCommentAndAttribute()
609+
{
610+
var text = @"
611+
Class C
612+
' Hello
613+
<Attr>
614+
Sub New()
615+
End Sub
616+
' Hello
617+
<Attr>
618+
Shared Sub New()
619+
End Sub
620+
End Class";
621+
622+
var expected = @"
623+
Friend Class C
624+
' Hello
625+
<Attr>
626+
Public Sub New()
627+
End Sub
628+
' Hello
629+
<Attr>
630+
Shared Sub New()
631+
End Sub
632+
End Class";
633+
634+
Verify(text, expected, runFormatter: false, languageName: LanguageNames.VisualBasic);
635+
}
636+
549637
[Fact]
550638
public void InterfaceMembers()
551639
{

‎src/Microsoft.DotNet.CodeFormatting/Rules/ExplicitVisibilityRule.VisualBasic.cs

Lines changed: 105 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -31,34 +31,64 @@ internal VisualBasicVisibilityRewriter(Document document, CancellationToken canc
3131
public override SyntaxNode VisitClassBlock(ClassBlockSyntax originalNode)
3232
{
3333
var node = (ClassBlockSyntax)base.VisitClassBlock(originalNode);
34-
var begin = (ClassStatementSyntax)EnsureVisibility(node.ClassStatement, node.ClassStatement.Modifiers, (x, l) => x.WithModifiers(l), () => GetTypeDefaultVisibility(originalNode));
34+
var begin = (ClassStatementSyntax)EnsureVisibility(
35+
node.ClassStatement,
36+
node.ClassStatement.ClassKeyword,
37+
node.ClassStatement.Modifiers,
38+
(x, k) => x.WithClassKeyword(k),
39+
(x, l) => x.WithModifiers(l),
40+
() => GetTypeDefaultVisibility(originalNode));
3541
return node.WithClassStatement(begin);
3642
}
3743

3844
public override SyntaxNode VisitStructureBlock(StructureBlockSyntax originalNode)
3945
{
4046
var node = (StructureBlockSyntax)base.VisitStructureBlock(originalNode);
41-
var begin = (StructureStatementSyntax)EnsureVisibility(node.StructureStatement, node.StructureStatement.Modifiers, (x, l) => x.WithModifiers(l), () => GetTypeDefaultVisibility(originalNode));
47+
var begin = (StructureStatementSyntax)EnsureVisibility(
48+
node.StructureStatement,
49+
node.StructureStatement.StructureKeyword,
50+
node.StructureStatement.Modifiers,
51+
(x, k) => x.WithStructureKeyword(k),
52+
(x, l) => x.WithModifiers(l),
53+
() => GetTypeDefaultVisibility(originalNode));
4254
return node.WithStructureStatement(begin);
4355
}
4456

4557
public override SyntaxNode VisitInterfaceBlock(InterfaceBlockSyntax originalNode)
4658
{
4759
var node = (InterfaceBlockSyntax)base.VisitInterfaceBlock(originalNode);
48-
var begin = (InterfaceStatementSyntax)EnsureVisibility(node.InterfaceStatement, node.InterfaceStatement.Modifiers, (x, l) => x.WithModifiers(l), () => GetTypeDefaultVisibility(originalNode));
60+
var begin = (InterfaceStatementSyntax)EnsureVisibility(
61+
node.InterfaceStatement,
62+
node.InterfaceStatement.InterfaceKeyword,
63+
node.InterfaceStatement.Modifiers,
64+
(x, k) => x.WithInterfaceKeyword(k),
65+
(x, l) => x.WithModifiers(l),
66+
() => GetTypeDefaultVisibility(originalNode));
4967
return node.WithInterfaceStatement(begin);
5068
}
5169

5270
public override SyntaxNode VisitModuleBlock(ModuleBlockSyntax node)
5371
{
5472
node = (ModuleBlockSyntax)base.VisitModuleBlock(node);
55-
var begin = (ModuleStatementSyntax)EnsureVisibility(node.ModuleStatement, node.ModuleStatement.Modifiers, (x, l) => x.WithModifiers(l), () => SyntaxKind.FriendKeyword);
73+
var begin = (ModuleStatementSyntax)EnsureVisibility(
74+
node.ModuleStatement,
75+
node.ModuleStatement.ModuleKeyword,
76+
node.ModuleStatement.Modifiers,
77+
(x, k) => x.WithModuleKeyword(k),
78+
(x, l) => x.WithModifiers(l),
79+
() => SyntaxKind.FriendKeyword);
5680
return node.WithModuleStatement(begin);
5781
}
5882

5983
public override SyntaxNode VisitEnumBlock(EnumBlockSyntax node)
6084
{
61-
var enumStatement = (EnumStatementSyntax)EnsureVisibility(node.EnumStatement, node.EnumStatement.Modifiers, (x, l) => x.WithModifiers(l), () => GetDelegateTypeDefaultVisibility(node));
85+
var enumStatement = (EnumStatementSyntax)EnsureVisibility(
86+
node.EnumStatement,
87+
node.EnumStatement.EnumKeyword,
88+
node.EnumStatement.Modifiers,
89+
(x, k) => x.WithEnumKeyword(k),
90+
(x, l) => x.WithModifiers(l),
91+
() => GetDelegateTypeDefaultVisibility(node));
6292
return node.WithEnumStatement(enumStatement);
6393
}
6494

@@ -69,7 +99,13 @@ public override SyntaxNode VisitMethodStatement(MethodStatementSyntax node)
6999
return node;
70100
}
71101

72-
return EnsureVisibility(node, node.Modifiers, (x, l) => x.WithModifiers(l), () => SyntaxKind.PublicKeyword);
102+
return EnsureVisibility(
103+
node,
104+
node.SubOrFunctionKeyword,
105+
node.Modifiers,
106+
(x, k) => x.WithSubOrFunctionKeyword(k),
107+
(x, l) => x.WithModifiers(l),
108+
() => SyntaxKind.PublicKeyword);
73109
}
74110

75111
public override SyntaxNode VisitSubNewStatement(SubNewStatementSyntax node)
@@ -79,12 +115,22 @@ public override SyntaxNode VisitSubNewStatement(SubNewStatementSyntax node)
79115
return node;
80116
}
81117

82-
return EnsureVisibility(node, node.Modifiers, (x, l) => x.WithModifiers(l), () => SyntaxKind.PublicKeyword);
118+
return EnsureVisibility(
119+
node,
120+
node.SubKeyword,
121+
node.Modifiers,
122+
(x, k) => x.WithSubKeyword(k),
123+
(x, l) => x.WithModifiers(l),
124+
() => SyntaxKind.PublicKeyword);
83125
}
84126

85127
public override SyntaxNode VisitFieldDeclaration(FieldDeclarationSyntax node)
86128
{
87-
node = (FieldDeclarationSyntax)EnsureVisibility(node, node.Modifiers, (x, l) => x.WithModifiers(l), () => SyntaxKind.PrivateKeyword);
129+
node = (FieldDeclarationSyntax)EnsureVisibility(
130+
node,
131+
node.Modifiers,
132+
(x, l) => x.WithModifiers(l),
133+
() => SyntaxKind.PrivateKeyword);
88134

89135
// Now that the field has an explicit modifier remove any Dim modifiers on it
90136
// as it is now redundant
@@ -204,11 +250,59 @@ private static bool IsNestedDeclaration(SyntaxNode node)
204250
return false;
205251
}
206252

253+
private static SyntaxNode EnsureVisibility<T>(T node, SyntaxToken keyword, SyntaxTokenList originalModifiers, Func<T, SyntaxToken, T> withKeyword, Func<T, SyntaxTokenList, T> withModifiers, Func<SyntaxKind> getDefaultVisibility) where T : SyntaxNode
254+
{
255+
Func<SyntaxKind, T> withFirstModifier = (visibilityKind) =>
256+
{
257+
var leadingTrivia = keyword.LeadingTrivia;
258+
node = withKeyword(node, keyword.WithLeadingTrivia());
259+
260+
var visibilityToken = SyntaxFactory.Token(
261+
leadingTrivia,
262+
visibilityKind,
263+
SyntaxFactory.TriviaList(SyntaxFactory.SyntaxTrivia(SyntaxKind.WhitespaceTrivia, " ")));
264+
265+
var modifierList = SyntaxFactory.TokenList(visibilityToken);
266+
return withModifiers(node, modifierList);
267+
};
268+
269+
return EnsureVisibilityCore(
270+
node,
271+
originalModifiers,
272+
withFirstModifier,
273+
withModifiers,
274+
getDefaultVisibility);
275+
}
276+
277+
private static SyntaxNode EnsureVisibility<T>(T node, SyntaxTokenList originalModifiers, Func<T, SyntaxTokenList, T> withModifiers, Func<SyntaxKind> getDefaultVisibility) where T : SyntaxNode
278+
{
279+
Func<SyntaxKind, T> withFirstModifier = (visibilityKind) =>
280+
{
281+
var leadingTrivia = node.GetLeadingTrivia();
282+
node = node.WithLeadingTrivia();
283+
284+
var visibilityToken = SyntaxFactory.Token(
285+
leadingTrivia,
286+
visibilityKind,
287+
SyntaxFactory.TriviaList(SyntaxFactory.SyntaxTrivia(SyntaxKind.WhitespaceTrivia, " ")));
288+
289+
var modifierList = SyntaxFactory.TokenList(visibilityToken);
290+
return withModifiers(node, modifierList);
291+
};
292+
293+
return EnsureVisibilityCore(
294+
node,
295+
originalModifiers,
296+
withFirstModifier,
297+
withModifiers,
298+
getDefaultVisibility);
299+
}
300+
207301
/// <summary>
208302
/// Return a node declaration that has a visibility modifier. If one isn't present it will be added as the
209303
/// first modifier. Any trivia before the node will be added as leading trivia to the added <see cref="SyntaxToken"/>.
210304
/// </summary>
211-
private static SyntaxNode EnsureVisibility<T>(T node, SyntaxTokenList originalModifiers, Func<T, SyntaxTokenList, T> withModifiers, Func<SyntaxKind> getDefaultVisibility) where T : SyntaxNode
305+
private static SyntaxNode EnsureVisibilityCore<T>(T node, SyntaxTokenList originalModifiers, Func<SyntaxKind, T> withFirstModifier, Func<T, SyntaxTokenList, T> withModifiers, Func<SyntaxKind> getDefaultVisibility) where T : SyntaxNode
212306
{
213307
if (originalModifiers.Any(x => SyntaxFacts.IsAccessibilityModifier(x.Kind())))
214308
{
@@ -221,15 +315,7 @@ private static SyntaxNode EnsureVisibility<T>(T node, SyntaxTokenList originalMo
221315
SyntaxTokenList modifierList;
222316
if (originalModifiers.Count == 0)
223317
{
224-
var leadingTrivia = node.GetLeadingTrivia();
225-
node = node.WithLeadingTrivia();
226-
227-
var visibilityToken = SyntaxFactory.Token(
228-
leadingTrivia,
229-
visibilityKind,
230-
SyntaxFactory.TriviaList(SyntaxFactory.SyntaxTrivia(SyntaxKind.WhitespaceTrivia, " ")));
231-
232-
modifierList = SyntaxFactory.TokenList(visibilityToken);
318+
return withFirstModifier(visibilityKind);
233319
}
234320
else
235321
{
@@ -248,9 +334,9 @@ private static SyntaxNode EnsureVisibility<T>(T node, SyntaxTokenList originalMo
248334
}
249335

250336
modifierList = SyntaxFactory.TokenList(list);
337+
return withModifiers(node, modifierList);
251338
}
252339

253-
return withModifiers(node, modifierList);
254340
}
255341
}
256342
}

0 commit comments

Comments
 (0)