Skip to content

Commit fd9e1dc

Browse files
davidwengierdavidwengier
authored
Fix formatting of mixed indentation in VS Code (#12418)
Fixes #12416, reported by @Peter-Juhasz on Discord When a file has mixed indentation, VS Code sends across edits that include changes to content, presumably for brevity, but the Html formatter doesn't know about C#, and so changing content breaks it. To fix it, we run the edits through our source text differ, doing a diff at the word level so movements of C# (which Html sees as either tildes or a comment) end up being identical, and therefore the edits can be safely discarded.
2 parents 4282a2e + edabdc6 commit fd9e1dc

File tree

15 files changed

+759
-144
lines changed

15 files changed

+759
-144
lines changed

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Linq;
45
using System.Collections.Immutable;
56
using System.Threading;
67
using System.Threading.Tasks;
@@ -23,7 +24,52 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
2324

2425
if (changes.Length > 0)
2526
{
27+
// There is a lot of uncertainty when we're dealing with edits that come from the Html formatter
28+
// because we are not responsible for it. It could make all sorts of strange edits, and it could
29+
// structure those edits is all sorts of ways. eg, it could have individual character edits, or
30+
// it could have a single edit that replaces a whole section of text, or the whole document.
31+
// Since the Html formatter doesn't understand Razor, and in fact doesn't even format the actual
32+
// Razor document directly (all C# is replaced), we have to be selective about what edits we will
33+
// actually use, but being selective is tricky because we might be missing some intentional edits
34+
// that the formatter made.
35+
//
36+
// To solve this, and work around various issues due to the Html formatter seeing a much simpler
37+
// document that we are actually dealing with, the first thing we do is take the changes it suggests
38+
// and apply them to the document it saw, then use our own algorithm to produce a set of changes
39+
// that more closely match what we want to get out of it. Specifically, we only want to see changes
40+
// to whitespace, or Html, not changes that include C#. Fortunately since we encode all C# as tildes
41+
// it means we can do a word-based diff, and all C# will essentially be equal to all other C#, so
42+
// won't appear in the diff.
43+
//
44+
// So we end up with a set of changes that are only ever to whitespace, or legitimate Html (though
45+
// in reality the formatter doesn't change that anyway).
46+
47+
// Avoid computing a minimal diff if we don't need to. Slightly wasteful if we've come from one
48+
// of the other overloads, but worth it if we haven't (and worth it for them to validate before
49+
// doing the work to convert edits to changes).
50+
if (changes.Any(static e => e.NewText?.Contains('~') ?? false))
51+
{
52+
var htmlSourceText = context.CodeDocument.GetHtmlSourceText();
53+
context.Logger?.LogSourceText("HtmlSourceText", htmlSourceText);
54+
var htmlWithChanges = htmlSourceText.WithChanges(changes);
55+
56+
changes = SourceTextDiffer.GetMinimalTextChanges(htmlSourceText, htmlWithChanges, DiffKind.Word);
57+
if (changes.Length == 0)
58+
{
59+
return [];
60+
}
61+
}
62+
63+
// Now that the changes are on our terms, we can apply our own filtering without having to worry
64+
// that we're missing something important. We could still, in theory, be missing something the Html
65+
// formatter intentionally did, but we also know the Html formatter made its decisions without an
66+
// awareness of Razor anyway, so it's not a reliable source.
2667
var filteredChanges = await FilterIncomingChangesAsync(context, changes, cancellationToken).ConfigureAwait(false);
68+
if (filteredChanges.Length == 0)
69+
{
70+
return [];
71+
}
72+
2773
changedText = changedText.WithChanges(filteredChanges);
2874

2975
context.Logger?.LogSourceText("AfterHtmlFormatter", changedText);
@@ -49,6 +95,7 @@ private async Task<ImmutableArray<TextChange>> FilterIncomingChangesAsync(Format
4995
var comment = node?.FirstAncestorOrSelf<RazorCommentBlockSyntax>();
5096
if (comment is not null && change.Span.Start > comment.SpanStart)
5197
{
98+
context.Logger?.LogMessage($"Dropping change {change} because it's in a Razor comment");
5299
continue;
53100
}
54101

@@ -75,6 +122,7 @@ private async Task<ImmutableArray<TextChange>> FilterIncomingChangesAsync(Format
75122
sourceText[change.Span.Start - 1] == '@' &&
76123
sourceText[change.Span.Start] == '<')
77124
{
125+
context.Logger?.LogMessage($"Dropping change {change} because it breaks a C# template");
78126
continue;
79127
}
80128

@@ -110,6 +158,7 @@ private async Task<ImmutableArray<TextChange>> FilterIncomingChangesAsync(Format
110158
csharpSyntaxRoot.FindNode(new TextSpan(csharpIndex, 0), getInnermostNodeForTie: true) is { } csharpNode &&
111159
csharpNode is CSharp.Syntax.LiteralExpressionSyntax or CSharp.Syntax.InterpolatedStringTextSyntax)
112160
{
161+
context.Logger?.LogMessage($"Dropping change {change} because it breaks a C# string literal");
113162
continue;
114163
}
115164
}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,19 @@ namespace Microsoft.CodeAnalysis.Razor.TextDifferencing;
55

66
internal enum DiffKind : byte
77
{
8+
/// <summary>
9+
/// Diff by character.
10+
/// </summary>
811
Char,
12+
/// <summary>
13+
/// Diff by line
14+
/// </summary>
915
Line,
16+
/// <summary>
17+
/// Diff by word
18+
/// </summary>
19+
/// <remarks>
20+
/// Word break characters are: whitespace, '/' and '"'. Contiguous word breaks are treated as a single word.
21+
/// </remarks>
22+
Word,
1023
}

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

Lines changed: 9 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -1,115 +1,27 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
using System.Text;
4+
using System.Collections.Immutable;
5+
using Microsoft.AspNetCore.Razor.PooledObjects;
56
using Microsoft.CodeAnalysis.Text;
67

78
namespace Microsoft.CodeAnalysis.Razor.TextDifferencing;
89

910
internal partial class SourceTextDiffer
1011
{
11-
private class LineDiffer : SourceTextDiffer
12+
private sealed class LineDiffer(SourceText oldText, SourceText newText)
13+
: TextSpanDiffer(oldText, newText)
1214
{
13-
private readonly TextLineCollection _oldLines;
14-
private readonly TextLineCollection _newLines;
15-
16-
private char[] _oldLineBuffer;
17-
private char[] _newLineBuffer;
18-
private char[] _appendBuffer;
19-
20-
protected override int OldSourceLength { get; }
21-
protected override int NewSourceLength { get; }
22-
23-
public LineDiffer(SourceText oldText, SourceText newText)
24-
: base(oldText, newText)
25-
{
26-
_oldLineBuffer = RentArray(1024);
27-
_newLineBuffer = RentArray(1024);
28-
_appendBuffer = RentArray(1024);
29-
30-
_oldLines = oldText.Lines;
31-
_newLines = newText.Lines;
32-
33-
OldSourceLength = _oldLines.Count;
34-
NewSourceLength = _newLines.Count;
35-
}
36-
37-
public override void Dispose()
15+
protected override ImmutableArray<TextSpan> Tokenize(SourceText text)
3816
{
39-
ReturnArray(_oldLineBuffer);
40-
ReturnArray(_newLineBuffer);
41-
ReturnArray(_appendBuffer);
42-
}
43-
44-
protected override bool SourceEqual(int oldSourceIndex, int newSourceIndex)
45-
{
46-
var oldLine = _oldLines[oldSourceIndex];
47-
var newLine = _newLines[newSourceIndex];
48-
49-
var oldSpan = oldLine.SpanIncludingLineBreak;
50-
var newSpan = newLine.SpanIncludingLineBreak;
51-
52-
if (oldSpan.Length != newSpan.Length)
53-
{
54-
return false;
55-
}
56-
57-
var length = oldSpan.Length;
17+
using var builder = new PooledArrayBuilder<TextSpan>();
5818

59-
// Simple case: Both lines are empty.
60-
if (length == 0)
19+
foreach (var line in text.Lines)
6120
{
62-
return true;
63-
}
64-
65-
// Copy the text into char arrays for comparison. Note: To avoid allocation,
66-
// we try to reuse the same char buffers and only grow them when a longer
67-
// line is encountered.
68-
var oldChars = EnsureBuffer(ref _oldLineBuffer, length);
69-
var newChars = EnsureBuffer(ref _newLineBuffer, length);
70-
71-
OldText.CopyTo(oldSpan.Start, oldChars, 0, length);
72-
NewText.CopyTo(newSpan.Start, newChars, 0, length);
73-
74-
for (var i = 0; i < length; i++)
75-
{
76-
if (oldChars[i] != newChars[i])
77-
{
78-
return false;
79-
}
80-
}
81-
82-
return true;
83-
}
84-
85-
protected override int GetEditPosition(DiffEdit edit)
86-
=> _oldLines[edit.Position].Start;
87-
88-
protected override int AppendEdit(DiffEdit edit, StringBuilder builder)
89-
{
90-
if (edit.Kind == DiffEditKind.Insert)
91-
{
92-
Assumes.NotNull(edit.NewTextPosition);
93-
var newTextPosition = edit.NewTextPosition.GetValueOrDefault();
94-
95-
for (var i = 0; i < edit.Length; i++)
96-
{
97-
var newLine = _newLines[newTextPosition + i];
98-
99-
var newSpan = newLine.SpanIncludingLineBreak;
100-
if (newSpan.Length > 0)
101-
{
102-
var buffer = EnsureBuffer(ref _appendBuffer, newSpan.Length);
103-
NewText.CopyTo(newSpan.Start, buffer, 0, newSpan.Length);
104-
105-
builder.Append(buffer, 0, newSpan.Length);
106-
}
107-
}
108-
109-
return _oldLines[edit.Position].Start;
21+
builder.Add(line.SpanIncludingLineBreak);
11022
}
11123

112-
return _oldLines[edit.Position + edit.Length - 1].EndIncludingLineBreak;
24+
return builder.ToImmutableAndClear();
11325
}
11426
}
11527
}

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

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System.Collections.Immutable;
5+
using System.Text;
6+
using Microsoft.CodeAnalysis.Text;
7+
8+
namespace Microsoft.CodeAnalysis.Razor.TextDifferencing;
9+
10+
internal partial class SourceTextDiffer
11+
{
12+
private abstract class TextSpanDiffer : SourceTextDiffer
13+
{
14+
private readonly ImmutableArray<TextSpan> _oldSpans = [];
15+
private readonly ImmutableArray<TextSpan> _newSpans = [];
16+
17+
private char[] _oldBuffer;
18+
private char[] _newBuffer;
19+
private char[] _appendBuffer;
20+
21+
protected override int OldSourceLength { get; }
22+
protected override int NewSourceLength { get; }
23+
24+
public TextSpanDiffer(SourceText oldText, SourceText newText)
25+
: base(oldText, newText)
26+
{
27+
_oldBuffer = RentArray(1024);
28+
_newBuffer = RentArray(1024);
29+
_appendBuffer = RentArray(1024);
30+
31+
if (oldText.Length > 0)
32+
{
33+
_oldSpans = Tokenize(oldText);
34+
}
35+
36+
if (newText.Length > 0)
37+
{
38+
_newSpans = Tokenize(newText);
39+
}
40+
41+
OldSourceLength = _oldSpans.Length;
42+
NewSourceLength = _newSpans.Length;
43+
}
44+
45+
protected abstract ImmutableArray<TextSpan> Tokenize(SourceText text);
46+
47+
public override void Dispose()
48+
{
49+
ReturnArray(_oldBuffer);
50+
ReturnArray(_newBuffer);
51+
ReturnArray(_appendBuffer);
52+
}
53+
54+
protected override bool SourceEqual(int oldSourceIndex, int newSourceIndex)
55+
{
56+
var oldSpan = _oldSpans[oldSourceIndex];
57+
var newSpan = _newSpans[newSourceIndex];
58+
59+
if (oldSpan.Length != newSpan.Length)
60+
{
61+
return false;
62+
}
63+
64+
var length = oldSpan.Length;
65+
66+
// Simple case: Both lines are empty.
67+
if (length == 0)
68+
{
69+
return true;
70+
}
71+
72+
// Copy the text into char arrays for comparison. Note: To avoid allocation,
73+
// we try to reuse the same char buffers and only grow them when a longer
74+
// line is encountered.
75+
var oldChars = EnsureBuffer(ref _oldBuffer, length);
76+
var newChars = EnsureBuffer(ref _newBuffer, length);
77+
78+
OldText.CopyTo(oldSpan.Start, oldChars, 0, length);
79+
NewText.CopyTo(newSpan.Start, newChars, 0, length);
80+
81+
for (var i = 0; i < length; i++)
82+
{
83+
if (oldChars[i] != newChars[i])
84+
{
85+
return false;
86+
}
87+
}
88+
89+
return true;
90+
}
91+
92+
protected override int GetEditPosition(DiffEdit edit)
93+
=> _oldSpans[edit.Position].Start;
94+
95+
protected override int AppendEdit(DiffEdit edit, StringBuilder builder)
96+
{
97+
if (edit.Kind == DiffEditKind.Insert)
98+
{
99+
Assumes.NotNull(edit.NewTextPosition);
100+
var newTextPosition = edit.NewTextPosition.GetValueOrDefault();
101+
102+
for (var i = 0; i < edit.Length; i++)
103+
{
104+
var newSpan = _newSpans[newTextPosition + i];
105+
106+
if (newSpan.Length > 0)
107+
{
108+
var buffer = EnsureBuffer(ref _appendBuffer, newSpan.Length);
109+
NewText.CopyTo(newSpan.Start, buffer, 0, newSpan.Length);
110+
111+
builder.Append(buffer, 0, newSpan.Length);
112+
}
113+
}
114+
115+
return _oldSpans[edit.Position].Start;
116+
}
117+
118+
return _oldSpans[edit.Position + edit.Length - 1].End;
119+
}
120+
}
121+
}

0 commit comments

Comments
 (0)