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

fix issue with indentation of code/markup following directive blocks ... #6531

Merged
merged 6 commits into from
Jul 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ protected async Task<List<TextChange>> AdjustIndentationAsync(FormattingContext
var significantLocationIndentation = await CSharpFormatter.GetCSharpIndentationAsync(context, significantLocations, cancellationToken);

// Build source mapping indentation scopes.
var sourceMappingIndentations = new SortedDictionary<int, int>();
var sourceMappingIndentations = new SortedDictionary<int, IndentationData>();
var syntaxTreeRoot = context.CodeDocument.GetSyntaxTree().Root;
foreach (var originalLocation in sourceMappingMap.Keys)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to mention, the really exciting thing to do here would be to artificially insert something in the sourceMappingMap, so that we create a source mapping point at the end of the @section to get things back on track. I think that would require compiler changes though, and so haven't looked too deeply into it, because we're currently working through plans to bring the compiler into this repo, which would make the work easy. If you wanted to try that approach though, I think it would be the ideal solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree, this would be ideal.

Copy link
Contributor

@davidwengier davidwengier Jul 3, 2022

Choose a reason for hiding this comment

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

@tuespetre I'm not sure what your plans were for this PR, but this idea was nagging at me, so I took the liberty of pushing a commit to your branch that does something very similar. Let me know what you think.

It adds a source mapping record to our map, to represent the end of the section block, so that if that is ever found to be the correct mapping point to use, we end up using the indentation from just before the start of the @section.

I didn't touch the test you added, so assuming it covered the extra cases you were talking about, I think this is a nice balance between your awesome idea, to not get hampered by bad source mappings of section blocks, but with better performance characteristics than always traversing the tree would give.

{
var significantLocation = sourceMappingMap[originalLocation];
Expand All @@ -118,7 +119,23 @@ protected async Task<List<TextChange>> AdjustIndentationAsync(FormattingContext
continue;
}

sourceMappingIndentations[originalLocation] = indentation;
var scopeOwner = syntaxTreeRoot.LocateOwner(new SourceChange(originalLocation, 0, string.Empty));
sourceMappingIndentations[originalLocation] = new IndentationData(indentation);

// For @section blocks we have special handling to add a fake source mapping/significant location at the end of the
// section, to return the indentation back to before the start of the section block.
if (scopeOwner?.Parent?.Parent?.Parent is RazorDirectiveSyntax containingDirective &&
containingDirective.DirectiveDescriptor.Directive == SectionDirective.Directive.Directive &&
!sourceMappingIndentations.ContainsKey(containingDirective.EndPosition - 1))
{
// We want the indentation for the end point to be whatever the indentation was before the start point. For
// performance reasons, and because source mappings could be un-ordered, we defer that calculation until
// later, when we have all of the information in place. We use a negative number to indicate that there is
// more processing to do.
// This is saving repeatedly realising the source mapping indentations keys, then converting them to an array,
// and then doing binary search here, before we've processed all of the mappings
sourceMappingIndentations[containingDirective.EndPosition - 1] = new IndentationData(lazyLoad: true, offset: originalLocation - 1);
}
}

var sourceMappingIndentationScopes = sourceMappingIndentations.Keys.ToArray();
Expand Down Expand Up @@ -175,18 +192,6 @@ protected async Task<List<TextChange>> AdjustIndentationAsync(FormattingContext

var index = Array.BinarySearch(sourceMappingIndentationScopes, lineStart);

if (index < 0 && context.SourceText[lineStart] == '@')
{
// Sometimes we are only off by one in finding a source mapping, for example with a simple if statement:
//
// @|if (true)
//
// The sourceMappingIndentationScopes knows about where the pipe is (ie, after the "@") but we're asking
// for indentation at the line start. In these cases we are better off using the real indentation scope,
// than hoping the one before it is correct.
index = Array.BinarySearch(sourceMappingIndentationScopes, lineStart + 1);
}

if (index < 0)
{
// Couldn't find the exact value. Find the index of the element to the left of the searched value.
Expand All @@ -205,7 +210,7 @@ protected async Task<List<TextChange>> AdjustIndentationAsync(FormattingContext
{
// index will now be set to the same value as the end of the closest source mapping.
var absoluteIndex = sourceMappingIndentationScopes[index];
csharpDesiredIndentation = sourceMappingIndentations[absoluteIndex];
csharpDesiredIndentation = sourceMappingIndentations[absoluteIndex].GetIndentation(sourceMappingIndentations, sourceMappingIndentationScopes, minCSharpIndentation);

// This means we didn't find an exact match and so we used the indentation of the end of a previous mapping.
// So let's use the MinCSharpIndentation of that same location if possible.
Expand Down Expand Up @@ -454,5 +459,47 @@ private static SyntaxNode FixOwnerToWorkaroundCompilerQuirks(SyntaxNode owner)

return owner;
}

private class IndentationData
{
private readonly int _offset;
private int _indentation;
private bool _lazyLoad;

public IndentationData(int indentation)
{
_indentation = indentation;
}

public IndentationData(bool lazyLoad, int offset)
{
_lazyLoad = lazyLoad;
_offset = offset;
}

public int GetIndentation(SortedDictionary<int, IndentationData> sourceMappingIndentations, int[] indentationScopes, int minCSharpIndentation)
{
// If we're lazy loading, then we need to find the indentation from the source mappings, at the offset,
// which for whatever reason may not have been available when creating this class.
if (_lazyLoad)
{
_lazyLoad = false;

var index = Array.BinarySearch(indentationScopes, _offset);
if (index < 0)
{
index = (~index) - 1;
}

// If there is a source mapping to the left of the original start point, then we use its indentation
// otherwise use the minimum
_indentation = index < 0
? minCSharpIndentation
: sourceMappingIndentations[indentationScopes[index]]._indentation;
}

return _indentation;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,54 @@ @section Scripts {
fileKind: FileKinds.Legacy);
}

[Fact]
public async Task Format_SectionDirectiveBlock5()
{
await RunFormattingTestAsync(
input: """
@functions {
public class Foo{
void Method() { }
}
}

@section Foo {
@{ var test = 1; }
}

<p></p>

@section Scripts {
<script></script>
}

<p></p>
""",
expected: """
@functions {
public class Foo
{
void Method() { }
}
}

@section Foo {
@{
var test = 1;
}
}

<p></p>

@section Scripts {
<script></script>
}

<p></p>
""",
fileKind: FileKinds.Legacy);
}

[Fact]
public async Task Formats_CodeBlockDirectiveWithRazorComments()
{
Expand Down