Fix formatting of mixed indentation in VS Code

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

@@ -1,6 +1,7 @@
 // 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.Collections.Immutable;
 using System.Threading;
 using System.Threading.Tasks;
@@ -23,7 +24,52 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
 
         if (changes.Length > 0)
         {
+            // There is a lot of uncertainty when we're dealing with edits that come from the Html formatter
+            // because we are not responsible for it. It could make all sorts of strange edits, and it could
+            // structure those edits is all sorts of ways. eg, it could have individual character edits, or
+            // it could have a single edit that replaces a whole section of text, or the whole document.
+            // Since the Html formatter doesn't understand Razor, and in fact doesn't even format the actual
+            // Razor document directly (all C# is replaced), we have to be selective about what edits we will
+            // actually use, but being selective is tricky because we might be missing some intentional edits
+            // that the formatter made.
+            //
+            // To solve this, and work around various issues due to the Html formatter seeing a much simpler
+            // document that we are actually dealing with, the first thing we do is take the changes it suggests
+            // and apply them to the document it saw, then use our own algorithm to produce a set of changes
+            // that more closely match what we want to get out of it. Specifically, we only want to see changes
+            // to whitespace, or Html, not changes that include C#. Fortunately since we encode all C# as tildes
+            // it means we can do a word-based diff, and all C# will essentially be equal to all other C#, so
+            // won't appear in the diff.
+            //
+            // So we end up with a set of changes that are only ever to whitespace, or legitimate Html (though
+            // in reality the formatter doesn't change that anyway).
+
+            // Avoid computing a minimal diff if we don't need to. Slightly wasteful if we've come from one
+            // of the other overloads, but worth it if we haven't (and worth it for them to validate before
+            // doing the work to convert edits to changes).
+            if (changes.Any(static e => e.NewText?.Contains('~') ?? false))
+            {
+                var htmlSourceText = context.CodeDocument.GetHtmlSourceText();
+                context.Logger?.LogSourceText("HtmlSourceText", htmlSourceText);
+                var htmlWithChanges = htmlSourceText.WithChanges(changes);
+
+                changes = SourceTextDiffer.GetMinimalTextChanges(htmlSourceText, htmlWithChanges, DiffKind.Word);
+                if (changes.Length == 0)
+                {
+                    return [];
+                }
+            }
+
+            // Now that the changes are on our terms, we can apply our own filtering without having to worry
+            // that we're missing something important. We could still, in theory, be missing something the Html
+            // formatter intentionally did, but we also know the Html formatter made its decisions without an
+            // awareness of Razor anyway, so it's not a reliable source.
             var filteredChanges = await FilterIncomingChangesAsync(context, changes, cancellationToken).ConfigureAwait(false);
+            if (filteredChanges.Length == 0)
+            {
+                return [];
+            }
+
             changedText = changedText.WithChanges(filteredChanges);
 
             context.Logger?.LogSourceText("AfterHtmlFormatter", changedText);
@@ -49,6 +95,7 @@ private async Task<ImmutableArray<TextChange>> FilterIncomingChangesAsync(Format
             var comment = node?.FirstAncestorOrSelf<RazorCommentBlockSyntax>();
             if (comment is not null && change.Span.Start > comment.SpanStart)
             {
+                context.Logger?.LogMessage($"Dropping change {change} because it's in a Razor comment");
                 continue;
             }
 
@@ -75,6 +122,7 @@ private async Task<ImmutableArray<TextChange>> FilterIncomingChangesAsync(Format
                     sourceText[change.Span.Start - 1] == '@' &&
                     sourceText[change.Span.Start] == '<')
                 {
+                    context.Logger?.LogMessage($"Dropping change {change} because it breaks a C# template");
                     continue;
                 }
 
@@ -110,6 +158,7 @@ private async Task<ImmutableArray<TextChange>> FilterIncomingChangesAsync(Format
                     csharpSyntaxRoot.FindNode(new TextSpan(csharpIndex, 0), getInnermostNodeForTie: true) is { } csharpNode &&
                     csharpNode is CSharp.Syntax.LiteralExpressionSyntax or CSharp.Syntax.InterpolatedStringTextSyntax)
                 {
+                    context.Logger?.LogMessage($"Dropping change {change} because it breaks a C# string literal");
                     continue;
                 }
             }

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/TextDifferencing/DiffKind.cs

@@ -5,6 +5,19 @@ namespace Microsoft.CodeAnalysis.Razor.TextDifferencing;
 
 internal enum DiffKind : byte
 {
+    /// <summary>
+    /// Diff by character.
+    /// </summary>
     Char,
+    /// <summary>
+    /// Diff by line
+    /// </summary>
     Line,
+    /// <summary>
+    /// Diff by word
+    /// </summary>
+    /// <remarks>
+    /// Word break characters are: whitespace, '/' and '"'. Contiguous word breaks are treated as a single word.
+    /// </remarks>
+    Word,
 }

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/TextDifferencing/SourceTextDiffer.WordDiffer.cs

@@ -0,0 +1,150 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Collections.Immutable;
+using System.Diagnostics;
+using System.Text;
+using Microsoft.AspNetCore.Razor.PooledObjects;
+using Microsoft.CodeAnalysis.Text;
+
+namespace Microsoft.CodeAnalysis.Razor.TextDifferencing;
+
+internal partial class SourceTextDiffer
+{
+    private sealed class WordDiffer : SourceTextDiffer
+    {
+        private readonly ImmutableArray<TextSpan> _oldWords;
+        private readonly ImmutableArray<TextSpan> _newWords;
+
+        private char[] _oldBuffer;
+        private char[] _newBuffer;
+        private char[] _appendBuffer;
+
+        protected override int OldSourceLength { get; }
+        protected override int NewSourceLength { get; }
+
+        public WordDiffer(SourceText oldText, SourceText newText) : base(oldText, newText)
+        {
+            _oldBuffer = RentArray(1024);
+            _newBuffer = RentArray(1024);
+            _appendBuffer = RentArray(1024);
+
+            _oldWords = TokenizeWords(oldText);
+            _newWords = TokenizeWords(newText);
+
+            OldSourceLength = _oldWords.Length;
+            NewSourceLength = _newWords.Length;
+        }
+
+        public override void Dispose()
+        {
+            ReturnArray(_oldBuffer);
+            ReturnArray(_newBuffer);
+            ReturnArray(_appendBuffer);
+        }
+
+        private static ImmutableArray<TextSpan> TokenizeWords(SourceText text)
+        {
+            if (text.Length == 0)
+            {
+                return [];
+            }
+
+            using var builder = new PooledArrayBuilder<TextSpan>();
+
+            var currentSpanStart = 0;
+            var currentClassification = Classify(text[0]);
+
+            // This algorithm is simpler than a normal tokenizer might be because we want to keep contiguous
+            // whitespace characters in the same "word", and we don't really care about contiguous quotes
+            // or slashes, so we can keep it simple and just capture a "word" when the classification of
+            // the current character changes.
+            var index = 1;
+            while (index < text.Length)
+            {
+                var classification = Classify(text[index]);
+                if (classification != currentClassification)
+                {
+                    // We've hit a word boundary, so store this and move on
+                    builder.Add(TextSpan.FromBounds(currentSpanStart, index));
+                    currentSpanStart = index;
+                    currentClassification = classification;
+                }
+
+                index++;
+            }
+
+            // It's impossible for the loop to capture the last word
+            Debug.Assert(currentSpanStart < text.Length);
+            builder.Add(TextSpan.FromBounds(currentSpanStart, text.Length));
+
+            return builder.ToImmutableAndClear();
+
+            // The type of classification doesn't matter as long as its unique and equatible
+            static int Classify(char c)
+                => c switch
+                {
+                    '/' => 0,
+                    '"' => 1,
+                    _ when char.IsWhiteSpace(c) => 2,
+                    _ => 3,
+                };
+        }
+
+        protected override bool SourceEqual(int oldSourceIndex, int newSourceIndex)
+        {
+            var oldWord = _oldWords[oldSourceIndex];
+            var newWord = _newWords[newSourceIndex];
+            if (oldWord.Length != newWord.Length)
+            {
+                return false;
+            }
+
+            var length = oldWord.Length;
+
+            // Copy the text into char arrays for comparison. Note: To avoid allocation,
+            // we try to reuse the same char buffers and only grow them when a longer
+            // line is encountered.
+            var oldChars = EnsureBuffer(ref _oldBuffer, oldWord.Length);
+            var newChars = EnsureBuffer(ref _newBuffer, newWord.Length);
+
+            OldText.CopyTo(oldWord.Start, oldChars, 0, length);
+            NewText.CopyTo(newWord.Start, newChars, 0, length);
+
+            for (var i = 0; i < length; i++)
+            {
+                if (oldChars[i] != newChars[i])
+                {
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
+        protected override int GetEditPosition(DiffEdit edit)
+            => _oldWords[edit.Position].Start;
+
+        protected override int AppendEdit(DiffEdit edit, StringBuilder builder)
+        {
+            if (edit.Kind == DiffEditKind.Insert)
+            {
+                Assumes.NotNull(edit.NewTextPosition);
+                var newWordIndex = edit.NewTextPosition.GetValueOrDefault();
+
+                for (var i = 0; i < edit.Length; i++)
+                {
+                    var word = _newWords[newWordIndex + i];
+                    var buffer = EnsureBuffer(ref _appendBuffer, word.Length);
+                    NewText.CopyTo(word.Start, buffer, 0, word.Length);
+
+                    builder.Append(buffer, 0, word.Length);
+                }
+
+                return _oldWords[edit.Position].Start;
+            }
+
+            return _oldWords[edit.Position + edit.Length - 1].End;
+        }
+    }
+}

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/TextDifferencing/SourceTextDiffer.cs

@@ -13,38 +13,32 @@
 
 namespace Microsoft.CodeAnalysis.Razor.TextDifferencing;
 
-internal abstract partial class SourceTextDiffer : TextDiffer, IDisposable
+internal abstract partial class SourceTextDiffer(SourceText oldText, SourceText newText) : TextDiffer, IDisposable
 {
-    protected readonly SourceText OldText;
-    protected readonly SourceText NewText;
-
-    protected SourceTextDiffer(SourceText oldText, SourceText newText)
-    {
-        OldText = oldText ?? throw new ArgumentNullException(nameof(oldText));
-        NewText = newText ?? throw new ArgumentNullException(nameof(newText));
-    }
+    protected readonly SourceText OldText = oldText;
+    protected readonly SourceText NewText = newText;
 
     public abstract void Dispose();
 
     protected abstract int GetEditPosition(DiffEdit edit);
     protected abstract int AppendEdit(DiffEdit edit, StringBuilder builder);
 
     /// <summary>
-    ///  Rents a char array of at least <paramref name="minimumLength"/> from the shared array pool.
+    /// Rents a char array of at least <paramref name="minimumLength"/> from the shared array pool.
     /// </summary>
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     protected static char[] RentArray(int minimumLength)
         => ArrayPool<char>.Shared.Rent(minimumLength);
 
     /// <summary>
-    ///  Returns a char array to the shared array pool.
+    /// Returns a char array to the shared array pool.
     /// </summary>
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     protected static void ReturnArray(char[] array, bool clearArray = false)
         => ArrayPool<char>.Shared.Return(array, clearArray);
 
     /// <summary>
-    ///  Ensures that <paramref name="array"/> references a char array of at least <paramref name="minimumLength"/>.
+    /// Ensures that <paramref name="array"/> references a char array of at least <paramref name="minimumLength"/>.
     /// </summary>
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     protected static char[] EnsureBuffer(ref char[] array, int minimumLength)
@@ -114,9 +108,13 @@ public static ImmutableArray<TextChange> GetMinimalTextChanges(SourceText oldTex
             return newText.GetTextChangesArray(oldText);
         }
 
-        using SourceTextDiffer differ = kind == DiffKind.Line
-            ? new LineDiffer(oldText, newText)
-            : new CharDiffer(oldText, newText);
+        using SourceTextDiffer differ = kind switch
+        {
+            DiffKind.Line => new LineDiffer(oldText, newText),
+            DiffKind.Char => new CharDiffer(oldText, newText),
+            DiffKind.Word => new WordDiffer(oldText, newText),
+            _ => throw new ArgumentOutOfRangeException(nameof(kind)),
+        };
 
         var edits = differ.ComputeDiff();
 

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/CodeActions/Html/HtmlCodeActionProviderTest.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Immutable;
+using System.Linq;
 using System.Text.Json;
 using System.Threading;
 using System.Threading.Tasks;
@@ -17,6 +18,7 @@
 using Microsoft.CodeAnalysis.Razor.Protocol;
 using Microsoft.CodeAnalysis.Razor.Protocol.CodeActions;
 using Microsoft.CodeAnalysis.Testing;
+using Microsoft.CodeAnalysis.Text;
 using Moq;
 using Xunit;
 using Xunit.Abstractions;
@@ -123,16 +125,10 @@ public async Task ProvideAsync_RemapsAndFixesEdits()
         Assert.NotNull(action.Edit);
         Assert.True(action.Edit.TryGetTextDocumentEdits(out var documentEdits));
         Assert.Equal(documentPath, documentEdits[0].TextDocument.DocumentUri.GetRequiredParsedUri().AbsolutePath);
-        // Edit should be converted to 2 edits, to remove the tags
-        Assert.Collection(documentEdits[0].Edits,
-            e =>
-            {
-                Assert.Equal("", ((TextEdit)e).NewText);
-            },
-            e =>
-            {
-                Assert.Equal("", ((TextEdit)e).NewText);
-            });
+
+        var text = SourceText.From(contents);
+        var changed = text.WithChanges(documentEdits[0].Edits.Select(e => text.GetTextChange((TextEdit)e)));
+        Assert.Equal("Goo @(DateTime.Now) Bar", changed.ToString());
     }
 
     private static RazorCodeActionContext CreateRazorCodeActionContext(

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/CodeActions/Html/HtmlCodeActionResolverTest.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Razor.LanguageServer.ProjectSystem;
@@ -13,6 +14,7 @@
 using Microsoft.CodeAnalysis.Razor.DocumentMapping;
 using Microsoft.CodeAnalysis.Razor.ProjectSystem;
 using Microsoft.CodeAnalysis.Testing;
+using Microsoft.CodeAnalysis.Text;
 using Moq;
 using Xunit;
 using Xunit.Abstractions;
@@ -85,15 +87,9 @@ public async Task ResolveAsync_RemapsAndFixesEdits()
         Assert.NotNull(action.Edit);
         Assert.True(action.Edit.TryGetTextDocumentEdits(out var documentEdits));
         Assert.Equal(documentPath, documentEdits[0].TextDocument.DocumentUri.GetRequiredParsedUri().AbsolutePath);
-        // Edit should be converted to 2 edits, to remove the tags
-        Assert.Collection(documentEdits[0].Edits,
-            e =>
-            {
-                Assert.Equal("", ((TextEdit)e).NewText);
-            },
-            e =>
-            {
-                Assert.Equal("", ((TextEdit)e).NewText);
-            });
+
+        var text = SourceText.From(contents);
+        var changed = text.WithChanges(documentEdits[0].Edits.Select(e => text.GetTextChange((TextEdit)e)));
+        Assert.Equal("Goo @(DateTime.Now) Bar", changed.ToString());
     }
 }