[release/dev17.14] Fix formatting issues (#11913)

Backport of #11908 and #11911 to release/dev17.14

Fixes
[developercommunity.visualstudio.com/t/Format-Document-in-Razor-pages-no-longer/10903159](https://developercommunity.visualstudio.com/t/Format-Document-in-Razor-pages-no-longer/10903159)
Fixes
[devdiv.visualstudio.com/DevDiv/_workitems/edit/2471065](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2471065)

/cc @davidwengier

## Customer Impact

## Regression

- [ ] Yes
- [ ] No

[If yes, specify when the regression was introduced. Provide the PR or
commit if known.]

## Testing

[How was the fix verified? How was the issue missed previously? What
tests were added?]

## Risk

[High/Medium/Low. Justify the indication by mentioning how risks were
measured and addressed.]

Author: phil-allen-msft

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/TextLineExtensions.cs

@@ -1,16 +1,30 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the MIT license. See License.txt in the project root for license information.
 
+using System.Diagnostics;
 using Microsoft.AspNetCore.Razor;
-using Microsoft.CodeAnalysis.Razor;
 
 namespace Microsoft.CodeAnalysis.Text;
 
 internal static class TextLineExtensions
 {
+    public static char CharAt(this TextLine line, int offset)
+    {
+        ArgHelper.ThrowIfNegative(offset);
+        ArgHelper.ThrowIfGreaterThanOrEqual(offset, line.SpanIncludingLineBreak.Length);
+
+        var text = line.Text.AssumeNotNull();
+        var index = line.Start + offset;
+        Debug.Assert(index < text.Length, "This should be impossible as we validated offset against the line length above.");
+
+        return text[index];
+    }
+
     public static string GetLeadingWhitespace(this TextLine line)
     {
-        return line.ToString().GetLeadingWhitespace();
+        return line.GetFirstNonWhitespaceOffset() is int offset
+            ? line.Text.AssumeNotNull().ToString(TextSpan.FromBounds(line.Start, line.Start + offset))
+            : string.Empty;
     }
 
     public static int GetIndentationSize(this TextLine line, long tabSize)

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

@@ -651,7 +651,8 @@ private LineInfo VisitCodeOrFunctionsDirective(RazorSyntaxNode openBrace)
                     _builder.AppendLine("class F");
                     _builder.AppendLine("{");
 
-                    return CreateLineInfo(skipNextLine: true);
+                    // Roslyn might move our brace to the previous line, so we might _not_ need to skip it 🤦‍
+                    return CreateLineInfo(skipNextLineIfBrace: true);
                 }
 
                 // If the braces are on different lines, then we can do nothing, unless its an @code or @functions
@@ -733,6 +734,7 @@ private LineInfo CreateLineInfo(
                 bool checkForNewLines = false,
                 bool skipPreviousLine = false,
                 bool skipNextLine = false,
+                bool skipNextLineIfBrace = false,
                 int htmlIndentLevel = 0,
                 int originOffset = 0,
                 int formattedLength = 0,
@@ -759,6 +761,7 @@ _elementEndLine is { } endLine &&
                     CheckForNewLines: checkForNewLines,
                     SkipPreviousLine: skipPreviousLine,
                     SkipNextLine: skipNextLine,
+                    SkipNextLineIfBrace: skipNextLineIfBrace,
                     HtmlIndentLevel: htmlIndentLevel,
                     OriginOffset: originOffset,
                     FormattedLength: formattedLength,

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

@@ -13,6 +13,7 @@ internal partial class CSharpFormattingPass
     /// <param name="CheckForNewLines">Whether the origin document text could have overflowed to multiple lines in the formatted document</param>
     /// <param name="SkipPreviousLine">Whether to skip the previous line in the formatted document, since it doesn't represent anything in the origin document</param>
     /// <param name="SkipNextLine">Whether to skip the next line in the formatted document, since it doesn't represent anything in the origin document</param>
+    /// <param name="SkipNextLineIfBrace">Whether to skip the next line in the formatted document, like <see cref="SkipNextLine" />, but only skips if the next line is a brace</param>
     /// <param name="HtmlIndentLevel">The indent level that the Html formatter applied to this line</param>
     /// <param name="OriginOffset">How many characters after the first non-whitespace character of the origin line should be skipped before applying formatting</param>
     /// <param name="FormattedLength">How many characters of the origin line the formatted line represents</param>
@@ -25,6 +26,7 @@ private readonly record struct LineInfo(
         bool CheckForNewLines,
         bool SkipPreviousLine,
         bool SkipNextLine,
+        bool SkipNextLineIfBrace,
         int HtmlIndentLevel,
         int OriginOffset,
         int FormattedLength,

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

@@ -21,6 +21,8 @@ internal sealed partial class CSharpFormattingPass(IHostServicesProvider hostSer
     private readonly ILogger _logger = loggerFactory.GetOrCreateLogger<CSharpFormattingPass>();
     private readonly IHostServicesProvider _hostServicesProvider = hostServicesProvider;
 
+    private Func<SourceText, SourceText>? _formattedCSharpDocumentModifierFunc = null;
+
     public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext context, ImmutableArray<TextChange> changes, CancellationToken cancellationToken)
     {
         // Process changes from previous passes
@@ -36,6 +38,12 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
         var formattedCSharpText = await FormatCSharpAsync(generatedCSharpText, context.Options.ToIndentationOptions(), cancellationToken).ConfigureAwait(false);
         _logger.LogTestOnly($"Formatted generated C# document:\r\n{formattedCSharpText}");
 
+        if (_formattedCSharpDocumentModifierFunc is { } func)
+        {
+            formattedCSharpText = func(formattedCSharpText);
+            _logger.LogTestOnly($"Formatted generated C# document (after func):\r\n{formattedCSharpText}");
+        }
+
         // We now have a formatted C# document, and an original document, but we can't just apply the changes to the original
         // document as they come from very different places. What we want to do is go through each line of the generated document,
         // take the indentation that is in it, and apply it to the original document, and then take any formatting changes
@@ -94,16 +102,37 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
                         // making it run over two lines, or even "string Prop { get" and making it span three lines.
                         // Since we assume Roslyn won't change anything non-whitespace, we just keep inserting the formatted lines
                         // of C# until we match the original line contents.
+                        // Of course, Roslyn could just as easily remove whitespace, eg making a "class Goo {" into "class Goo\n{",
+                        // so whilst the same theory applies, instead of inserting formatted lines, we eat the original lines.
                         while (!changedText.NonWhitespaceContentEquals(formattedCSharpText, originalStart, originalLine.End, formattedStart, formattedLine.End))
                         {
-                            // Sanity check: Because we're looking ahead through lines until the original line content is fully matches, we could loop forever if there is a bug somewhere
-                            Debug.Assert(
-                                FormattingUtilities.CountNonWhitespaceChars(changedText, originalStart, originalLine.End) >= FormattingUtilities.CountNonWhitespaceChars(formattedCSharpText, formattedStart, formattedLine.End),
-                                "Infinite loop in formatting! A bug in our visitor, or has Roslyn changed a non-whitespace char?");
-
-                            iFormatted++;
-                            formattedLine = formattedCSharpText.Lines[iFormatted];
-                            formattingChanges.Add(new TextChange(new(originalLine.EndIncludingLineBreak, 0), htmlIndentString + formattedCSharpText.ToString(formattedLine.SpanIncludingLineBreak)));
+                            // If there are more non-whitespace chars in the original line, then its something like "if (true) {" to "if (true)", so keep inserting formatted lines until we're past the brace.
+                            if (FormattingUtilities.CountNonWhitespaceChars(changedText, originalStart, originalLine.End) >= FormattingUtilities.CountNonWhitespaceChars(formattedCSharpText, formattedStart, formattedLine.End))
+                            {
+                                iFormatted++;
+                                if (iFormatted >= formattedCSharpText.Lines.Count)
+                                {
+                                    _logger.LogError($"Ran out of formatted lines while trying to process formatted changes after {iOriginal} lines. Abandoning further formatting to not corrupt the source file, please report this issue.");
+                                    break;
+                                }
+
+                                formattedLine = formattedCSharpText.Lines[iFormatted];
+                                formattingChanges.Add(new TextChange(new(originalLine.EndIncludingLineBreak, 0), htmlIndentString + formattedCSharpText.ToString(formattedLine.SpanIncludingLineBreak)));
+                            }
+                            else
+                            {
+                                // Otherwise, there are more whitespace chars in the formatted line, so "if (true)" to "if (true) {", so we need to remove the original lines until we're past the brace.
+                                var oldEnd = originalLine.End;
+                                iOriginal++;
+                                if (iOriginal >= changedText.Lines.Count)
+                                {
+                                    _logger.LogError("Ran out of lines while trying to process formatted changes. Abandoning further formatting to not corrupt the source file, please report this issue.");
+                                    break;
+                                }
+
+                                originalLine = changedText.Lines[iOriginal];
+                                formattingChanges.Add(new TextChange(TextSpan.FromBounds(oldEnd, originalLine.End), ""));
+                            }
                         }
                     }
                 }
@@ -113,6 +142,17 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
             {
                 iFormatted++;
             }
+            else if (lineInfo.SkipNextLineIfBrace)
+            {
+                // 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
+                // that we insert, but Roslyn settings might place on the same like as the class declaration.
+                if (iFormatted + 1 < formattedCSharpText.Lines.Count &&
+                    formattedCSharpText.Lines[iFormatted + 1] is { Span.Length: > 0 } nextLine &&
+                    nextLine.CharAt(0) == '{')
+                {
+                    iFormatted++;
+                }
+            }
         }
 
         // We're finished processing the original file, which means we've done all of the indentation for the file, and we've done
@@ -182,4 +222,14 @@ private async Task<SourceText> FormatCSharpAsync(SourceText generatedCSharpText,
     [Obsolete("Only for the syntax visualizer, do not call")]
     internal static string GetFormattingDocumentContentsForSyntaxVisualizer(RazorCodeDocument codeDocument)
         => CSharpDocumentGenerator.Generate(codeDocument, new()).SourceText.ToString();
+
+    internal TestAccessor GetTestAccessor() => new TestAccessor(this);
+
+    internal readonly struct TestAccessor(CSharpFormattingPass instance)
+    {
+        public void SetFormattedCSharpDocumentModifierFunc(Func<SourceText, SourceText> func)
+        {
+            instance._formattedCSharpDocumentModifierFunc = func;
+        }
+    }
 }

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/RazorFormattingPass.cs

@@ -3,7 +3,6 @@
 
 using System.Collections.Generic;
 using System.Collections.Immutable;
-using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Threading;
@@ -404,8 +403,15 @@ private bool FormatBlock(FormattingContext context, RazorSourceDocument source,
             {
                 var openBraceLineNumber = openBraceNode.GetLinePositionSpan(source).Start.Line;
                 var openBraceLine = source.Text.Lines[openBraceLineNumber];
-                Debug.Assert(openBraceLine.GetFirstNonWhitespacePosition().HasValue);
-                additionalIndentation = source.Text.GetSubTextString(TextSpan.FromBounds(openBraceLine.Start, openBraceLine.GetFirstNonWhitespacePosition().GetValueOrDefault()));
+
+                // The open brace node might actually start with a newline on the line before, which could be blank,
+                // so make sure we find some actual content.
+                while (!openBraceLine.GetFirstNonWhitespacePosition().HasValue)
+                {
+                    openBraceLine = source.Text.Lines[++openBraceLineNumber];
+                }
+
+                additionalIndentation = openBraceLine.GetLeadingWhitespace();
             }
         }
 

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/RazorFormattingService.cs

@@ -5,6 +5,7 @@
 using System.Collections.Frozen;
 using System.Collections.Generic;
 using System.Collections.Immutable;
+using System.Diagnostics;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
@@ -321,8 +322,8 @@ private static ImmutableArray<TextChange> UnwrapCSharpSnippets(ImmutableArray<Te
     }
 
     /// <summary>
-    /// This method counts the occurrences of CRLF and LF line endings in the original text. 
-    /// If LF line endings are more prevalent, it removes any CR characters from the text changes 
+    /// This method counts the occurrences of CRLF and LF line endings in the original text.
+    /// If LF line endings are more prevalent, it removes any CR characters from the text changes
     /// to ensure consistency with the LF style.
     /// </summary>
     private static ImmutableArray<TextChange> NormalizeLineEndings(SourceText originalText, ImmutableArray<TextChange> changes)
@@ -355,9 +356,20 @@ private static ImmutableArray<TextChange> ReplaceInChanges(ImmutableArray<TextCh
         return changes.DrainToImmutable();
     }
 
-    internal static class TestAccessor
+    internal TestAccessor GetTestAccessor() => new TestAccessor(this);
+
+    internal class TestAccessor(RazorFormattingService service)
     {
         public static FrozenSet<string> GetCSharpTriggerCharacterSet() => s_csharpTriggerCharacterSet;
         public static FrozenSet<string> GetHtmlTriggerCharacterSet() => s_htmlTriggerCharacterSet;
+
+        public void SetFormattedCSharpDocumentModifierFunc(Func<SourceText, SourceText> func)
+        {
+            // This is only valid for the new formatting engine, so a test that sets it for the old formatter is probably written incorrectly.
+            Debug.Assert(service._languageServerFeatureOptions.UseNewFormattingEngine);
+
+            var pass = service._documentFormattingPasses.OfType<New.CSharpFormattingPass>().Single();
+            pass.GetTestAccessor().SetFormattedCSharpDocumentModifierFunc(func);
+        }
     }
 }