Drop Html edits that would split a C# literal across multiple lines (#12396)

Fixes #11846
Also happened in dotnet/vscode-csharp#7264

The VS Code html formatter loves to break long C# strings, which at
worst means we abandon the formatting operation, or at worst Roslyn
throws.

Also some stuff in here to use the new formatting logging, which is how
I repro'd the issue. Yay!

Author: davidwengier

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

@@ -28,6 +28,7 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
         // Process changes from previous passes
         var changedText = context.SourceText.WithChanges(changes);
         var changedContext = await context.WithTextAsync(changedText, cancellationToken).ConfigureAwait(false);
+        context.Logger?.LogObject("SourceMappings", changedContext.CodeDocument.GetRequiredCSharpDocument().SourceMappings);
 
         // To format C# code we generate a C# document that represents the indentation semantics the user would be
         // expecting in their Razor file. See the doc comments on CSharpDocumentGenerator for more info

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

@@ -7,45 +7,52 @@
 using Microsoft.AspNetCore.Razor.Language;
 using Microsoft.AspNetCore.Razor.Language.Syntax;
 using Microsoft.AspNetCore.Razor.PooledObjects;
+using Microsoft.CodeAnalysis.Razor.DocumentMapping;
 using Microsoft.CodeAnalysis.Razor.TextDifferencing;
 using Microsoft.CodeAnalysis.Text;
 
 namespace Microsoft.CodeAnalysis.Razor.Formatting;
 
-internal sealed class HtmlFormattingPass : IFormattingPass
+internal sealed class HtmlFormattingPass(IDocumentMappingService documentMappingService) : IFormattingPass
 {
-    public Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext context, ImmutableArray<TextChange> changes, CancellationToken cancellationToken)
+    private readonly IDocumentMappingService _documentMappingService = documentMappingService;
+
+    public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext context, ImmutableArray<TextChange> changes, CancellationToken cancellationToken)
     {
         var changedText = context.SourceText;
 
         if (changes.Length > 0)
         {
-            var filteredChanges = FilterIncomingChanges(context.CodeDocument.GetRequiredSyntaxTree(), changes);
+            var filteredChanges = await FilterIncomingChangesAsync(context, changes, cancellationToken).ConfigureAwait(false);
             changedText = changedText.WithChanges(filteredChanges);
 
             context.Logger?.LogSourceText("AfterHtmlFormatter", changedText);
         }
 
-        return Task.FromResult(SourceTextDiffer.GetMinimalTextChanges(context.SourceText, changedText, DiffKind.Char));
+        return SourceTextDiffer.GetMinimalTextChanges(context.SourceText, changedText, DiffKind.Char);
     }
 
-    private static ImmutableArray<TextChange> FilterIncomingChanges(RazorSyntaxTree syntaxTree, ImmutableArray<TextChange> changes)
+    private async Task<ImmutableArray<TextChange>> FilterIncomingChangesAsync(FormattingContext context, ImmutableArray<TextChange> changes, CancellationToken cancellationToken)
     {
-        var sourceText = syntaxTree.Source.Text;
+        var codeDocument = context.CodeDocument;
+        var csharpDocument = codeDocument.GetRequiredCSharpDocument();
+        var syntaxRoot = codeDocument.GetRequiredSyntaxRoot();
+        var sourceText = codeDocument.Source.Text;
+        SyntaxNode? csharpSyntaxRoot = null;
 
         using var changesToKeep = new PooledArrayBuilder<TextChange>(capacity: changes.Length);
 
         foreach (var change in changes)
         {
-            // Don't keep changes that start inside of a razor comment block.
-            var comment = syntaxTree.Root.FindInnermostNode(change.Span.Start)?.FirstAncestorOrSelf<RazorCommentBlockSyntax>();
+            // We don't keep changes that start inside of a razor comment block.
+            var node = syntaxRoot.FindInnermostNode(change.Span.Start);
+            var comment = node?.FirstAncestorOrSelf<RazorCommentBlockSyntax>();
             if (comment is not null && change.Span.Start > comment.SpanStart)
             {
                 continue;
             }
 
-            // Normally we don't touch Html changes much but there is one
-            // edge case when including render fragments in a C# code block, eg:
+            // When render fragments are inside a C# code block, eg:
             //
             // @code {
             //      void Foo()
@@ -56,22 +63,68 @@ private static ImmutableArray<TextChange> FilterIncomingChanges(RazorSyntaxTree
             //
             // This is popular in some libraries, like bUnit. The issue here is that
             // the Html formatter sees ~~~~~<SurveyPrompt /> and puts a newline before
-            // the tag, but obviously that breaks things.
+            // the tag, but obviously that breaks things by separating the transition and the tag.
             //
             // It's straight forward enough to just check for this situation and ignore the change.
 
             // There needs to be a newline being inserted between an '@' and a '<'.
-            if (change.NewText is ['\r' or '\n', ..] &&
-                sourceText.Length > 1 &&
-                sourceText[change.Span.Start - 1] == '@' &&
-                sourceText[change.Span.Start] == '<')
+            if (change.NewText is ['\r' or '\n', ..])
             {
-                continue;
+                if (change.Span.Start > 0 &&
+                    sourceText.Length > 1 &&
+                    sourceText[change.Span.Start - 1] == '@' &&
+                    sourceText[change.Span.Start] == '<')
+                {
+                    continue;
+                }
+
+                // The Html formatter in VS Code wraps long lines, based on a user setting, but when there
+                // are long C# string literals that ends up breaking the code. For example:
+                //
+                // @("this is a long string that spans past some user set maximum limit")
+                //
+                // could become
+                //
+                // @("this is a long string that spans past
+                // some user set maximum limit")
+                //
+                // That doesn't compile, and depending on the scenario, can even cause a crash inside the
+                // Roslyn formatter.
+                //
+                // Strictly speaking if literal is a verbatim string, or multiline raw string literal, then
+                // it would compile, but it would also change the value of the string, and since these edits
+                // come from the Html formatter which clearly has no idea it's doing that, it is safer to
+                // disregard them all equally, and let the user make the final decision.
+                //
+                // In order to avoid hard coding all of the various string syntax kinds here, we can just check
+                // for any literal, as the only literals that can contain spaces, which is what the Html formatter
+                // will wrap on, are strings. And if it did decide to insert a newline into a number, or the 'null'
+                // keyword, that would be pretty bad too.
+                if (csharpSyntaxRoot is null)
+                {
+                    var csharpSyntaxTree = await context.OriginalSnapshot.GetCSharpSyntaxTreeAsync(cancellationToken).ConfigureAwait(false);
+                    csharpSyntaxRoot = await csharpSyntaxTree.GetRootAsync(cancellationToken).ConfigureAwait(false);
+                }
+
+                if (_documentMappingService.TryMapToCSharpDocumentPosition(csharpDocument, change.Span.Start, out _, out var csharpIndex) &&
+                    csharpSyntaxRoot.FindNode(new TextSpan(csharpIndex, 0), getInnermostNodeForTie: true) is { } csharpNode &&
+                    csharpNode is CSharp.Syntax.LiteralExpressionSyntax or CSharp.Syntax.InterpolatedStringTextSyntax)
+                {
+                    continue;
+                }
             }
 
             changesToKeep.Add(change);
         }
 
         return changesToKeep.ToImmutableAndClear();
     }
+
+    internal TestAccessor GetTestAccessor() => new(this);
+
+    internal readonly struct TestAccessor(HtmlFormattingPass pass)
+    {
+        public Task<ImmutableArray<TextChange>> FilterIncomingChangesAsync(FormattingContext context, ImmutableArray<TextChange> changes, CancellationToken cancellationToken)
+            => pass.FilterIncomingChangesAsync(context, changes, cancellationToken);
+    }
 }

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

@@ -51,7 +51,7 @@ public RazorFormattingService(
         ];
 
         _documentFormattingPasses = [
-                new HtmlFormattingPass(),
+                new HtmlFormattingPass(documentMappingService),
                 new RazorFormattingPass(),
                 new CSharpFormattingPass(hostServicesProvider, loggerFactory),
             ];
@@ -92,7 +92,7 @@ public async Task<ImmutableArray<TextChange>> GetDocumentFormattingChangesAsync(
 
         var logger = _formattingLoggerFactory.CreateLogger(documentContext.FilePath, range is null ? "Full" : "Range");
         logger?.LogObject("Options", options);
-        logger?.LogObject("HtmlChanges", htmlChanges);
+        logger?.LogObject("HtmlChanges", htmlChanges.SelectAsArray(e => e.ToRazorTextChange()));
         logger?.LogObject("Range", range);
         logger?.LogSourceText("InitialDocument", sourceText);
 

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/FormattingTestBase.cs

@@ -347,6 +347,9 @@ internal static IDocumentSnapshot CreateDocumentSnapshot(
                 return CreateDocumentSnapshot(
                     path, fileKind, codeDocument, projectEngine, imports, importDocuments, tagHelpers, inGlobalNamespace);
             });
+        snapshotMock
+            .Setup(d => d.GetCSharpSyntaxTreeAsync(It.IsAny<CancellationToken>()))
+            .ReturnsAsync(codeDocument.GetOrParseCSharpSyntaxTree(CancellationToken.None));
 
         return snapshotMock.Object;
     }

src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.Test/Cohost/Formatting/FormattingLogTest.cs

@@ -0,0 +1,123 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Linq;
+using System.Runtime.CompilerServices;
+using System.Runtime.Serialization;
+using System.Text.Json;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Razor;
+using Microsoft.AspNetCore.Razor.Language;
+using Microsoft.AspNetCore.Razor.Test.Common;
+using Microsoft.CodeAnalysis.ExternalAccess.Razor.Features;
+using Microsoft.CodeAnalysis.Razor.Formatting;
+using Microsoft.CodeAnalysis.Razor.Protocol;
+using Microsoft.VisualStudio.Razor.LanguageClient.Cohost;
+using Microsoft.VisualStudio.Razor.LanguageClient.Cohost.Formatting;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace Microsoft.VisualStudio.LanguageServices.Razor.Test.Cohost.Formatting;
+
+/// <summary>
+/// Not tests of the formatting log, but tests that use formatting logs sent in
+/// by users reporting issues.
+/// </summary>
+[Collection(HtmlFormattingCollection.Name)]
+public class FormattingLogTest(FormattingTestContext context, HtmlFormattingFixture fixture, ITestOutputHelper testOutput)
+    : FormattingTestBase(context, fixture.Service, testOutput), IClassFixture<FormattingTestContext>
+{
+    [Fact]
+    [WorkItem("https://github.com/dotnet/vscode-csharp/issues/7264")]
+    public async Task UnexpectedFalseInIndentBlockOperation()
+    {
+        var contents = GetResource("InitialDocument.txt");
+        var document = CreateProjectAndRazorDocument(contents);
+
+        var optionsFile = GetResource("Options.json");
+        var options = (TempRazorFormattingOptions)JsonSerializer.Deserialize(optionsFile, typeof(TempRazorFormattingOptions), JsonHelpers.JsonSerializerOptions).AssumeNotNull();
+
+        var formattingService = (RazorFormattingService)OOPExportProvider.GetExportedValue<IRazorFormattingService>();
+        formattingService.GetTestAccessor().SetFormattingLoggerFactory(new TestFormattingLoggerFactory(TestOutputHelper));
+
+        var htmlChangesFile = GetResource("HtmlChanges.json");
+        var htmlChanges = JsonSerializer.Deserialize<RazorTextChange[]>(htmlChangesFile, JsonHelpers.JsonSerializerOptions);
+        var sourceText = await document.GetTextAsync();
+        var htmlEdits = htmlChanges.Select(c => sourceText.GetTextEdit(c.ToTextChange())).ToArray();
+
+        await GetFormattingEditsAsync(document, htmlEdits, span: default, options.CodeBlockBraceOnNextLine, options.InsertSpaces, options.TabSize, options.ToRazorFormattingOptions().CSharpSyntaxFormattingOptions.AssumeNotNull());
+    }
+
+    private string GetResource(string name, [CallerMemberName] string? testName = null)
+    {
+        var baselineFileName = $@"TestFiles\FormattingLog\{testName}\{name}";
+
+        var testFile = TestFile.Create(baselineFileName, GetType().Assembly);
+        Assert.True(testFile.Exists());
+
+        return testFile.ReadAllText();
+    }
+
+    // HACK: Temporary types for deserializing because RazorCSharpSyntaxFormattingOptions doesn't have a parameterless constructor.
+    internal class TempRazorFormattingOptions()
+    {
+        [DataMember(Order = 0)]
+        public bool InsertSpaces { get; init; } = true;
+        [DataMember(Order = 1)]
+        public int TabSize { get; init; } = 4;
+        [DataMember(Order = 2)]
+        public bool CodeBlockBraceOnNextLine { get; init; } = false;
+        [DataMember(Order = 3)]
+        public TempRazorCSharpSyntaxFormattingOptions? CSharpSyntaxFormattingOptions { get; init; }
+
+        public RazorFormattingOptions ToRazorFormattingOptions()
+            => new()
+            {
+                InsertSpaces = InsertSpaces,
+                TabSize = TabSize,
+                CodeBlockBraceOnNextLine = CodeBlockBraceOnNextLine,
+                CSharpSyntaxFormattingOptions = CSharpSyntaxFormattingOptions is not null
+                    ? new RazorCSharpSyntaxFormattingOptions(
+                        CSharpSyntaxFormattingOptions.Spacing,
+                        CSharpSyntaxFormattingOptions.SpacingAroundBinaryOperator,
+                        CSharpSyntaxFormattingOptions.NewLines,
+                        CSharpSyntaxFormattingOptions.LabelPositioning,
+                        CSharpSyntaxFormattingOptions.Indentation,
+                        CSharpSyntaxFormattingOptions.WrappingKeepStatementsOnSingleLine,
+                        CSharpSyntaxFormattingOptions.WrappingPreserveSingleLine,
+                        CSharpSyntaxFormattingOptions.NamespaceDeclarations,
+                        CSharpSyntaxFormattingOptions.PreferTopLevelStatements,
+                        CSharpSyntaxFormattingOptions.CollectionExpressionWrappingLength)
+                    : RazorCSharpSyntaxFormattingOptions.Default
+            };
+    }
+
+    [DataContract]
+    internal sealed record class TempRazorCSharpSyntaxFormattingOptions(
+        [property: DataMember] RazorSpacePlacement Spacing,
+        [property: DataMember] RazorBinaryOperatorSpacingOptions SpacingAroundBinaryOperator,
+        [property: DataMember] RazorNewLinePlacement NewLines,
+        [property: DataMember] RazorLabelPositionOptions LabelPositioning,
+        [property: DataMember] RazorIndentationPlacement Indentation,
+        [property: DataMember] bool WrappingKeepStatementsOnSingleLine,
+        [property: DataMember] bool WrappingPreserveSingleLine,
+        [property: DataMember] RazorNamespaceDeclarationPreference NamespaceDeclarations,
+        [property: DataMember] bool PreferTopLevelStatements,
+        [property: DataMember] int CollectionExpressionWrappingLength)
+    {
+        public TempRazorCSharpSyntaxFormattingOptions()
+            : this(
+                  default,
+                  default,
+                  default,
+                  default,
+                  default,
+                  default,
+                  default,
+                  default,
+                  true,
+                  default)
+        {
+        }
+    }
+}

src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.Test/Cohost/Formatting/FormattingTestBase.cs

@@ -76,15 +76,11 @@ private protected async Task RunFormattingTestAsync(
         var uri = new Uri(document.CreateUri(), $"{document.FilePath}{FeatureOptions.HtmlVirtualDocumentSuffix}");
         var htmlEdits = await _htmlFormattingService.GetDocumentFormattingEditsAsync(LoggerFactory, uri, generatedHtml, insertSpaces, tabSize);
 
-        var requestInvoker = new TestHtmlRequestInvoker([(Methods.TextDocumentFormattingName, htmlEdits)]);
-
-        var clientSettingsManager = new ClientSettingsManager(changeTriggers: []);
-        clientSettingsManager.Update(clientSettingsManager.GetClientSettings().AdvancedSettings with { CodeBlockBraceOnNextLine = codeBlockBraceOnNextLine });
-
         var span = input.TryGetNamedSpans(string.Empty, out var spans)
             ? spans.First()
             : default;
-        var edits = await GetFormattingEditsAsync(span, insertSpaces, tabSize, document, requestInvoker, clientSettingsManager, csharpSyntaxFormattingOptions);
+
+        var edits = await GetFormattingEditsAsync(document, htmlEdits, span, codeBlockBraceOnNextLine, insertSpaces, tabSize, csharpSyntaxFormattingOptions);
 
         if (edits is null)
         {
@@ -99,6 +95,17 @@ private protected async Task RunFormattingTestAsync(
         AssertEx.EqualOrDiff(expected, finalText.ToString());
     }
 
+    private protected async Task<TextEdit[]?> GetFormattingEditsAsync(TextDocument document, TextEdit[]? htmlEdits, TextSpan span, bool codeBlockBraceOnNextLine, bool insertSpaces, int tabSize, RazorCSharpSyntaxFormattingOptions csharpSyntaxFormattingOptions)
+    {
+        var requestInvoker = new TestHtmlRequestInvoker([(Methods.TextDocumentFormattingName, htmlEdits)]);
+
+        var clientSettingsManager = new ClientSettingsManager(changeTriggers: []);
+        clientSettingsManager.Update(clientSettingsManager.GetClientSettings().AdvancedSettings with { CodeBlockBraceOnNextLine = codeBlockBraceOnNextLine });
+
+        var edits = await GetFormattingEditsAsync(span, insertSpaces, tabSize, document, requestInvoker, clientSettingsManager, csharpSyntaxFormattingOptions);
+        return edits;
+    }
+
     private protected async Task RunOnTypeFormattingTestAsync(
         TestCode input,
         string expected,