Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Razor generated documents to respect tabs/spaces settings #31211

Merged
merged 15 commits into from
Mar 30, 2021
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,15 @@ public sealed class CodeWriter
private int _currentLineIndex;
private int _currentLineCharacterIndex;

public CodeWriter()
public CodeWriter() : this(Environment.NewLine, RazorCodeGenerationOptions.CreateDefault())
{
NewLine = Environment.NewLine;
}

public CodeWriter(string newLine, RazorCodeGenerationOptions options)
{
NewLine = newLine;
IndentWithTabs = options.IndentWithTabs;
TabSize = options.IndentSize;
_builder = new StringBuilder();
}

Expand All @@ -47,6 +53,10 @@ public string NewLine
}
}

public bool IndentWithTabs { get; }

public int TabSize { get; }

public SourceLocation Location => new SourceLocation(_absoluteIndex, _currentLineIndex, _currentLineCharacterIndex);

public char this[int index]
Expand All @@ -62,16 +72,34 @@ public char this[int index]
}
}

// Internal for testing.
internal CodeWriter Indent(int size)
public CodeWriter Indent(int size)
{
if (Length == 0 || this[Length - 1] == '\n')
if (size == 0 || (Length != 0 && this[Length - 1] != '\n'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to your PR, but indexing in to StringBuilder (which is what this[..] is doing) is really slow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting. Maybe we can keep track of the last character somehow, possibly storing it in a field?
(Don't know if it's in the scope of this PR, but would be a good follow up later on.)

{
_builder.Append(' ', size);
return this;
}

_currentLineCharacterIndex += size;
_absoluteIndex += size;
var actualSize = 0;
if (IndentWithTabs)
{
// Avoid writing directly to the StringBuilder here, that will throw off the manual indexing
// done by the base class.
var tabs = size / TabSize;
actualSize += tabs;
_builder.Append('\t', tabs);

var spaces = size % TabSize;
actualSize += spaces;
_builder.Append(' ', spaces);
}
else
{
actualSize = size;
_builder.Append(' ', size);
}

_currentLineCharacterIndex += actualSize;
_absoluteIndex += actualSize;

return this;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -48,29 +48,7 @@ public static CodeWriter WritePadding(this CodeWriter writer, int offset, Source
var basePadding = CalculatePadding();
var resolvedPadding = Math.Max(basePadding - offset, 0);

if (context.Options.IndentWithTabs)
{
// Avoid writing directly to the StringBuilder here, that will throw off the manual indexing
// done by the base class.
var tabs = resolvedPadding / context.Options.IndentSize;
for (var i = 0; i < tabs; i++)
{
writer.Write("\t");
}

var spaces = resolvedPadding % context.Options.IndentSize;
for (var i = 0; i < spaces; i++)
{
writer.Write(" ");
}
}
else
{
for (var i = 0; i < resolvedPadding; i++)
{
writer.Write(" ");
}
}
writer.Indent(resolvedPadding);

return writer;

Expand All @@ -86,7 +64,7 @@ int CalculatePadding()
}
else if (@char == '\t')
{
spaceCount += context.Options.IndentSize;
spaceCount += writer.TabSize;
}
else
{
Expand Down Expand Up @@ -569,11 +547,11 @@ public struct CSharpCodeWritingScope : IDisposable
private int _tabSize;
private int _startIndent;

public CSharpCodeWritingScope(CodeWriter writer, int tabSize = 4, bool autoSpace = true)
public CSharpCodeWritingScope(CodeWriter writer, bool autoSpace = true)
{
_writer = writer;
_autoSpace = autoSpace;
_tabSize = tabSize;
_tabSize = writer.TabSize;
_startIndent = -1; // Set in WriteStartScope

WriteStartScope();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -32,7 +32,7 @@ public override RazorCSharpDocument WriteDocument(RazorCodeDocument codeDocument
}

var context = new DefaultCodeRenderingContext(
new CodeWriter(),
new CodeWriter(Environment.NewLine, _options),
_codeTarget.CreateNodeWriter(),
codeDocument,
documentNode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace Microsoft.AspNetCore.Razor.Language
internal class DefaultRazorCodeGenerationOptions : RazorCodeGenerationOptions
{
public DefaultRazorCodeGenerationOptions(
bool indentWithTabs,
bool indentWithTabs,
int indentSize,
bool designTime,
string rootNamespace,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
#nullable enable
Microsoft.AspNetCore.Razor.Language.CodeGeneration.CodeWriter.IndentWithTabs.get -> bool
Microsoft.AspNetCore.Razor.Language.CodeGeneration.CodeWriter.TabSize.get -> int
Microsoft.AspNetCore.Razor.Language.Intermediate.CascadingGenericTypeParameter
Microsoft.AspNetCore.Razor.Language.Intermediate.CascadingGenericTypeParameter.CascadingGenericTypeParameter() -> void
~Microsoft.AspNetCore.Razor.Language.CodeGeneration.CodeWriter.CodeWriter(string newLine, Microsoft.AspNetCore.Razor.Language.RazorCodeGenerationOptions options) -> void
~Microsoft.AspNetCore.Razor.Language.CodeGeneration.CodeWriter.Indent(int size) -> Microsoft.AspNetCore.Razor.Language.CodeGeneration.CodeWriter
~Microsoft.AspNetCore.Razor.Language.Intermediate.CascadingGenericTypeParameter.GenericTypeNames.get -> System.Collections.Generic.IReadOnlyCollection<string>
~Microsoft.AspNetCore.Razor.Language.Intermediate.CascadingGenericTypeParameter.GenericTypeNames.set -> void
~Microsoft.AspNetCore.Razor.Language.Intermediate.ComponentIntermediateNode.ProvidesCascadingGenericTypes.get -> System.Collections.Generic.Dictionary<string, Microsoft.AspNetCore.Razor.Language.Intermediate.CascadingGenericTypeParameter>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,5 +359,47 @@ public void WriteAutoPropertyDeclaration_WithModifiers_WritesPropertyDeclaration
var output = writer.GenerateCode();
Assert.Equal("public static global::System.String MyString { get; set; }" + Environment.NewLine, output);
}

[Fact]
public void CSharpCodeWriter_RespectTabSetting()
{
// Arrange
var options = RazorCodeGenerationOptions.Create(o =>
{
o.IndentWithTabs = true;
o.IndentSize = 4;
});

var writer = new CodeWriter(Environment.NewLine, options);

// Act
writer.BuildClassDeclaration(Array.Empty<string>(), "C", "", Array.Empty<string>(), Array.Empty<string>());
writer.WriteField(Array.Empty<string>(), Array.Empty<string>(), "int", "f");

// Assert
var output = writer.GenerateCode();
Assert.Equal("class C" + Environment.NewLine + "{" + Environment.NewLine + "\tint f;" + Environment.NewLine, output);
}

[Fact]
public void CSharpCodeWriter_RespectSpaceSetting()
{
// Arrange
var options = RazorCodeGenerationOptions.Create(o =>
{
o.IndentWithTabs = false;
o.IndentSize = 4;
});

var writer = new CodeWriter(Environment.NewLine, options);

// Act
writer.BuildClassDeclaration(Array.Empty<string>(), "C", "", Array.Empty<string>(), Array.Empty<string>());
writer.WriteField(Array.Empty<string>(), Array.Empty<string>(), "int", "f");

// Assert
var output = writer.GenerateCode();
Assert.Equal("class C" + Environment.NewLine + "{" + Environment.NewLine + " int f;" + Environment.NewLine, output);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ protected override void BuildRenderTree(Microsoft.AspNetCore.Components.Renderin
#line default
#line hidden
#nullable disable


__o = Microsoft.AspNetCore.Components.BindConverter.FormatValue(
#nullable restore
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,19 @@ Generated Location: (2126:73,38 [18] )
Source Location: (557:12,84 [6] x:\dir\subdir\Test\TestComponent.cshtml)
|
|
Generated Location: (2353:78,96 [6] )
Generated Location: (2341:78,84 [6] )
|
|

Source Location: (589:13,30 [10] x:\dir\subdir\Test\TestComponent.cshtml)
|myVariable|
Generated Location: (2540:83,30 [10] )
Generated Location: (2528:83,30 [10] )
|myVariable|

Source Location: (637:13,78 [3] x:\dir\subdir\Test\TestComponent.cshtml)
|
}|
Generated Location: (2905:92,78 [3] )
Generated Location: (2893:92,78 [3] )
|
}|

Expand All @@ -62,7 +62,7 @@ Source Location: (651:16,7 [245] x:\dir\subdir\Test\TestComponent.cshtml)
for (var i = 0; i < 10; i++)
{
|
Generated Location: (3087:102,7 [245] )
Generated Location: (3075:102,7 [245] )
|
ElementReference myElementReference;
TemplatedComponent myComponentReference;
Expand All @@ -76,12 +76,12 @@ Generated Location: (3087:102,7 [245] )

Source Location: (912:25,28 [1] x:\dir\subdir\Test\TestComponent.cshtml)
|i|
Generated Location: (3499:119,28 [1] )
Generated Location: (3487:119,28 [1] )
|i|

Source Location: (925:25,41 [1] x:\dir\subdir\Test\TestComponent.cshtml)
|i|
Generated Location: (3675:127,41 [1] )
Generated Location: (3663:127,41 [1] )
|i|

Source Location: (931:25,47 [166] x:\dir\subdir\Test\TestComponent.cshtml)
Expand All @@ -93,7 +93,7 @@ Source Location: (931:25,47 [166] x:\dir\subdir\Test\TestComponent.cshtml)
System.GC.KeepAlive(myVariable);
}
|
Generated Location: (3847:134,47 [166] )
Generated Location: (3835:134,47 [166] )
|
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ protected override void BuildRenderTree(Microsoft.AspNetCore.Components.Renderin
#line default
#line hidden
#nullable disable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TanayParikh It looks like Ajay was seeing the same issue in dotnet/razor#1606. It doesn't look too impactful, but just want to check this change is OK?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, so this wont have any actual impact on the runnability of the code since it's outside of the line pragma; however, it's concerning that this is being touched at all? Definitely need to look into what's happening here. Also, in the other files I wouldn't imagine that generated locations would change unless there was a mashup of tabs in the source files / interesting settings in the tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, @pranavkm does this not worry you?


}
#pragma warning restore 1998
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Generated Location: (1301:42,16 [6] )
Source Location: (216:6,26 [2] x:\dir\subdir\Test\TestComponent.cshtml)
|
|
Generated Location: (1398:47,38 [2] )
Generated Location: (1386:47,26 [2] )
|
|