Skip to content

Commit 97fff7c

Browse files
davidwengierdavidwengier
authored
Fix formatting issue with code block opening braces (#11969)
Fixes #11970 (which is really https://developercommunity.visualstudio.com/t/Razor-file-formatting-throws-ran-out-of/10924272) ((which is really https://dev.azure.com/devdiv/DevDiv/_workitems/edit/2508457)) Seems I missed a case in my previous fix, which is where Roslyn is configured for K&R braces, but the Razor file is not. In order to track this down, I experimented with automatically generating tests to provide cover for the C# syntax formatting options. I referred to that coverage as "exhaustive", but quickly met with this thing called "Math" which said that would be too many tests, so I toned it down. The last two commits are the generator and the tests. I'm not convinced they're adding any value, now that I've found the issue and created real targeted tests for the same thing, in our normal formatting test class, but I left them anyway. Copilot wrote most of the generator anyway, so its not a big deal to recreate it again, if anyone thinks it shouldn't be there. Let me know.
2 parents 00adc9e + aafa1e7 commit 97fff7c

File tree

12 files changed

+2289
-41
lines changed

12 files changed

+2289
-41
lines changed

‎src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxNodeExtensions.cs‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -85,31 +85,27 @@ internal static bool IsAttributeDirective(this SyntaxNode node, [NotNullWhen(tru
8585
return false;
8686
}
8787

88-
internal static bool IsCodeDirective(this SyntaxNode node, out SyntaxToken openBraceToken)
88+
internal static bool IsCodeDirective(this SyntaxNode node)
8989
{
9090
if (IsDirective(node, ComponentCodeDirective.Directive, out var body) &&
9191
body.CSharpCode is { Children: { Count: > 0 } children } &&
92-
children.TryGetOpenBraceToken(out var openBrace))
92+
children.TryGetOpenBraceToken(out _))
9393
{
94-
openBraceToken = openBrace;
9594
return true;
9695
}
9796

98-
openBraceToken = default;
9997
return false;
10098
}
10199

102-
internal static bool IsFunctionsDirective(this SyntaxNode node, out SyntaxToken openBraceToken)
100+
internal static bool IsFunctionsDirective(this SyntaxNode node)
103101
{
104102
if (IsDirective(node, FunctionsDirective.Directive, out var body) &&
105103
body.CSharpCode is { Children: { Count: > 0 } children } &&
106-
children.TryGetOpenBraceToken(out var openBrace))
104+
children.TryGetOpenBraceToken(out _))
107105
{
108-
openBraceToken = openBrace;
109106
return true;
110107
}
111108

112-
openBraceToken = default;
113109
return false;
114110
}
115111

‎src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/New/CSharpFormattingPass.CSharpDocumentGenerator.cs‎

Lines changed: 12 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -695,14 +695,10 @@ public override LineInfo VisitRazorDirective(RazorDirectiveSyntax node)
695695
return VisitTypeParamDirective(typeParam, conditions);
696696
}
697697

698-
if (node.IsCodeDirective(out var openBrace))
698+
if (node.IsCodeDirective() ||
699+
node.IsFunctionsDirective())
699700
{
700-
return VisitCodeOrFunctionsDirective(openBrace);
701-
}
702-
703-
if (node.IsFunctionsDirective(out var functionsOpenBrace))
704-
{
705-
return VisitCodeOrFunctionsDirective(functionsOpenBrace);
701+
return VisitCodeOrFunctionsDirective();
706702
}
707703

708704
// All other directives that have braces are handled here
@@ -721,25 +717,17 @@ body.CSharpCode is CSharpCodeBlockSyntax code &&
721717
return EmitCurrentLineAsComment();
722718
}
723719

724-
private LineInfo VisitCodeOrFunctionsDirective(RazorSyntaxToken openBrace)
720+
private LineInfo VisitCodeOrFunctionsDirective()
725721
{
726-
// If the open brace is on the same line as the directive, then we need to ensure the contents are indented
727-
if (GetLineNumber(openBrace) == GetLineNumber(_currentToken))
728-
{
729-
// If its an @code or @functions we want to wrap the contents in a class
730-
// so that access modifiers are valid, and will be formatted as appropriate.
731-
_builder.AppendLine("class F");
732-
_builder.AppendLine("{");
733-
734-
// Roslyn might move our brace to the previous line, so we might _not_ need to skip it 🤦‍
735-
return CreateLineInfo(skipNextLineIfBrace: true);
736-
}
722+
// If its an @code or @functions we want to wrap the contents in a class so that access modifiers
723+
// on any members declared within it are valid, and will be formatted as appropriate.
724+
// We let the users content be the class name, as it will either be "@code" or "@functions", which
725+
// are both valid, and it might have an open brace after it, or that might be on the next line,
726+
// but if we just let that flow to the generated document, we don't need to do any fancy checking.
727+
_builder.Append("class ");
728+
_builder.AppendLine(_currentLine.ToString());
737729

738-
// If the braces are on different lines, then we can do nothing, unless its an @code or @functions
739-
// in which case we need to use a class. Note we don't output an open brace, as the next line of
740-
// the original file will have one.
741-
_builder.AppendLine("class F");
742-
return CreateLineInfo();
730+
return CreateLineInfo(skipNextLineIfBrace: true);
743731
}
744732

745733
private LineInfo VisitUsingDirective()

‎src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/New/CSharpFormattingPass.cs‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,11 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
5858
iFormatted++;
5959
}
6060

61+
if (iFormatted >= formattedCSharpText.Lines.Count)
62+
{
63+
break;
64+
}
65+
6166
var formattedLine = formattedCSharpText.Lines[iFormatted];
6267
if (lineInfo.ProcessIndentation &&
6368
formattedLine.GetFirstNonWhitespaceOffset() is { } formattedIndentation)
@@ -139,14 +144,26 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
139144
}
140145
else if (lineInfo.SkipNextLineIfBrace)
141146
{
142-
// If the next line is a brace, we skip it, otherwise we don't. This is used to skip the opening brace of a class
147+
// If the next line is a brace, we skip it. This is used to skip the opening brace of a class
143148
// that we insert, but Roslyn settings might place on the same like as the class declaration.
144149
if (iFormatted + 1 < formattedCSharpText.Lines.Count &&
145150
formattedCSharpText.Lines[iFormatted + 1] is { Span.Length: > 0 } nextLine &&
146151
nextLine.CharAt(0) == '{')
147152
{
148153
iFormatted++;
149154
}
155+
156+
// On the other hand, we might insert the opening brace of a class, and Roslyn might collapse
157+
// it up to the previous line, so we would want to skip the next line in the original document
158+
// in that case. Fortunately its illegal to have `@code {\r\n {` in a Razor file, so there can't
159+
// be false positives here.
160+
if (iOriginal + 1 < changedText.Lines.Count &&
161+
changedText.Lines[iOriginal + 1] is { } nextOriginalLine &&
162+
nextOriginalLine.GetFirstNonWhitespaceOffset() is { } firstChar &&
163+
nextOriginalLine.CharAt(firstChar) == '{')
164+
{
165+
iOriginal++;
166+
}
150167
}
151168
}
152169

‎src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/DocumentFormattingTest.cs‎

Lines changed: 166 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
using Xunit.Abstractions;
1212

1313
#if COHOSTING
14-
namespace Microsoft.VisualStudio.Razor.LanguageClient.Cohost;
14+
namespace Microsoft.VisualStudio.Razor.LanguageClient.Cohost.Formatting;
1515
#else
1616
namespace Microsoft.AspNetCore.Razor.LanguageServer.Formatting;
1717
#endif
@@ -72,6 +72,168 @@ public void Bar() {
7272
});
7373
}
7474

75+
[FormattingTestFact]
76+
public async Task RoslynFormatBracesAsKandR_CodeBlockBraceOnNextLine()
77+
{
78+
await RunFormattingTestAsync(
79+
input: """
80+
<h1>count is @counter</h1>
81+
82+
@code
83+
{
84+
private int counter;
85+
86+
class Goo
87+
{
88+
public void Bar()
89+
{
90+
counter++;
91+
}
92+
}
93+
}
94+
""",
95+
expected: """
96+
<h1>count is @counter</h1>
97+
98+
@code
99+
{
100+
private int counter;
101+
102+
class Goo {
103+
public void Bar() {
104+
counter++;
105+
}
106+
}
107+
}
108+
""",
109+
formattingOptionsOverride: RazorCSharpSyntaxFormattingOptions.Default with
110+
{
111+
NewLines = RazorNewLinePlacement.None
112+
});
113+
}
114+
115+
[FormattingTestFact]
116+
public async Task RoslynFormatBracesAsKandR_NoRazorOrHtml()
117+
{
118+
await RunFormattingTestAsync(
119+
input: """
120+
@code {
121+
private bool IconMenuActive { get; set; } = false;
122+
protected void ToggleIconMenu(bool iconMenuActive)
123+
{
124+
IconMenuActive = iconMenuActive;
125+
}
126+
}
127+
""",
128+
expected: """
129+
@code {
130+
private bool IconMenuActive { get; set; } = false;
131+
protected void ToggleIconMenu(bool iconMenuActive)
132+
{
133+
IconMenuActive = iconMenuActive;
134+
}
135+
}
136+
""",
137+
formattingOptionsOverride: RazorCSharpSyntaxFormattingOptions.Default with
138+
{
139+
NewLines = RazorNewLinePlacement.BeforeOpenBraceInMethods
140+
});
141+
}
142+
143+
[FormattingTestFact]
144+
public async Task RoslynFormatBracesAsKandR_CodeBlockBraceOnNextLine_NoRazorOrHtml()
145+
{
146+
await RunFormattingTestAsync(
147+
input: """
148+
@code
149+
{
150+
private bool IconMenuActive { get; set; } = false;
151+
protected void ToggleIconMenu(bool iconMenuActive)
152+
{
153+
IconMenuActive = iconMenuActive;
154+
}
155+
}
156+
""",
157+
expected: """
158+
@code
159+
{
160+
private bool IconMenuActive { get; set; } = false;
161+
protected void ToggleIconMenu(bool iconMenuActive)
162+
{
163+
IconMenuActive = iconMenuActive;
164+
}
165+
}
166+
""",
167+
formattingOptionsOverride: RazorCSharpSyntaxFormattingOptions.Default with
168+
{
169+
NewLines = RazorNewLinePlacement.BeforeOpenBraceInMethods
170+
});
171+
}
172+
173+
[FormattingTestFact]
174+
public async Task RoslynFormatBracesAsKandR_CodeBlockBraceIndented_NoRazorOrHtml()
175+
{
176+
await RunFormattingTestAsync(
177+
input: """
178+
@code
179+
{
180+
private bool IconMenuActive { get; set; } = false;
181+
protected void ToggleIconMenu(bool iconMenuActive)
182+
{
183+
IconMenuActive = iconMenuActive;
184+
}
185+
}
186+
""",
187+
expected: """
188+
@code
189+
{
190+
private bool IconMenuActive { get; set; } = false;
191+
protected void ToggleIconMenu(bool iconMenuActive)
192+
{
193+
IconMenuActive = iconMenuActive;
194+
}
195+
}
196+
""",
197+
formattingOptionsOverride: RazorCSharpSyntaxFormattingOptions.Default with
198+
{
199+
NewLines = RazorNewLinePlacement.BeforeOpenBraceInMethods
200+
});
201+
}
202+
203+
[FormattingTestFact]
204+
public async Task RoslynFormatBracesAsKandR_CodeBlockBraceIndented_InsideHtml()
205+
{
206+
await RunFormattingTestAsync(
207+
input: """
208+
<div>
209+
@code
210+
{
211+
private bool IconMenuActive { get; set; } = false;
212+
protected void ToggleIconMenu(bool iconMenuActive)
213+
{
214+
IconMenuActive = iconMenuActive;
215+
}
216+
}
217+
</div>
218+
""",
219+
expected: """
220+
<div>
221+
@code
222+
{
223+
private bool IconMenuActive { get; set; } = false;
224+
protected void ToggleIconMenu(bool iconMenuActive)
225+
{
226+
IconMenuActive = iconMenuActive;
227+
}
228+
}
229+
</div>
230+
""",
231+
formattingOptionsOverride: RazorCSharpSyntaxFormattingOptions.Default with
232+
{
233+
NewLines = RazorNewLinePlacement.BeforeOpenBraceInMethods
234+
});
235+
}
236+
75237
[FormattingTestFact]
76238
public async Task PropertyShrunkToOneLine()
77239
{
@@ -6422,8 +6584,8 @@ public Task NestedExplicitExpression3()
64226584
[FormattingTestFact]
64236585
[WorkItem("https://github.com/dotnet/razor/issues/11873")]
64246586
public Task NestedExplicitExpression4()
6425-
=> RunFormattingTestAsync(
6426-
input: """
6587+
=> RunFormattingTestAsync(
6588+
input: """
64276589
@if (true)
64286590
{
64296591
<span>
@@ -6442,7 +6604,7 @@ public Task NestedExplicitExpression4()
64426604
</span>
64436605
}
64446606
""",
6445-
expected: """
6607+
expected: """
64466608
@if (true)
64476609
{
64486610
<span>

‎src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/OnTypeFormattingTest.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
using Xunit.Abstractions;
1010

1111
#if COHOSTING
12-
namespace Microsoft.VisualStudio.Razor.LanguageClient.Cohost;
12+
namespace Microsoft.VisualStudio.Razor.LanguageClient.Cohost.Formatting;
1313
#else
1414
namespace Microsoft.AspNetCore.Razor.LanguageServer.Formatting;
1515
#endif

‎src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.Test/Cohost/CohostEndpointTestBase.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
using Microsoft.CodeAnalysis.Razor.Remote;
2020
using Microsoft.CodeAnalysis.Razor.Workspaces;
2121
using Microsoft.CodeAnalysis.Remote.Razor;
22+
using Microsoft.CodeAnalysis.Remote.Razor.Logging;
2223
using Microsoft.CodeAnalysis.Remote.Razor.SemanticTokens;
2324
using Microsoft.CodeAnalysis.Text;
2425
using Microsoft.NET.Sdk.Razor.SourceGenerators;
@@ -69,6 +70,9 @@ protected override async Task InitializeAsync()
6970

7071
AddDisposable(_exportProvider);
7172

73+
var remoteLogger = _exportProvider.GetExportedValue<RemoteLoggerFactory>();
74+
remoteLogger.SetTargetLoggerFactory(LoggerFactory);
75+
7276
_remoteServiceInvoker = new TestRemoteServiceInvoker(JoinableTaskContext, _exportProvider, LoggerFactory);
7377
AddDisposable(_remoteServiceInvoker);
7478

0 commit comments

Comments
 (0)