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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Jul 13, 2022
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f87d263
fix issue with indentation of code/markup following directive blocks.…
tuespetre Jun 23, 2022
08d085e
changes for review
tuespetre Jun 23, 2022
48f2b9f
more changes
tuespetre Jun 23, 2022
4c0ba21
perf changes
tuespetre Jun 24, 2022
e2a7844
Add a fake source mapping indentation record for the end of @section …
davidwengier Jul 3, 2022
27ab312
Use a data type to store lazy indentation info
davidwengier Jul 13, 2022
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
53 changes: 41 additions & 12 deletions ...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormattingPassBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ protected async Task<List<TextChange>> AdjustIndentationAsync(FormattingContext

// Build source mapping indentation scopes.
var sourceMappingIndentations = new SortedDictionary<int, int>();
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 •
edited
Loading

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;
}

var scopeOwner = syntaxTreeRoot.LocateOwner(new SourceChange(originalLocation, 0, string.Empty));
sourceMappingIndentations[originalLocation] = 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] = (originalLocation - 1) * -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 @@ -207,6 +212,30 @@ protected async Task<List<TextChange>> AdjustIndentationAsync(FormattingContext
var absoluteIndex = sourceMappingIndentationScopes[index];
csharpDesiredIndentation = sourceMappingIndentations[absoluteIndex];

// If the indentation is negative, then its out sign that we are at the end of a @section block
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we only do negative indentation in this one scenario? Seeing negative indentation and then immediately assuming "must be a directive" is a bit scary to me. Only reason is that even if it's true that we only do it in one place today if / when we make changes to this in the future will we get into a false-positive situation?

I don't have enough background here so apologize if this is a silly question but if we're using negative numbers to indicate "need to do more" why are we unable to pre-emptively do the work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we only do negative indentation in this one scenario?

Yes, this is new. If you think it's risky, we can probably change this to store a record that is a bit more expressive, but I verified under the debugger that none of our existing tests fall into this branch (of having a negative indentation)

if we're using negative numbers to indicate "need to do more" why are we unable to pre-emptively do the work?

We're using negative numbers to avoid pre-emptive work, because pre-emptive work is harder/slower. The issue is that when we are at the end of a directive, we want to find the indentation that was correct at the point before the start of the directive. That is painful for two reasons: Firstly, source mappings could be out of order (which is why we store them in a SortedDictionary when building up the info), so we might not even have the before info yet, and secondly, because we use that SortedDictionary, we can't just arbitrarily ask for "the one before this one", we have to go searching for it. I tried to maintain a running list as we build the map to short cut this, but it was very difficult (eg SortedList is not a list, its more like a dictionary 🤦‍♂️)

We could do this as a post processing step for the map, to fill in these negative values, but I decided to take it a step further and essentially make that process lazy, because if we never actually need the indentation after the directive, then we never do the work to calculate it. That is what this is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification! Ya unfortunately I think having these negative values is definitely risky. It'll also add a few bits of "tribal knowledge" into the data storage type which scares me from a debugging perspective. I'd much rather store a more expressive record to ensure readability / understandability stay intact if that's possible

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated to use an IndentationData class which is capable of lazy-loading its indentation info.

// and what we've actually stored in the negative value of the location of the start of the @section
// block. We use this to find the indentation that was present before the @section block and revert
// to it.
if (csharpDesiredIndentation < 0)
{
var originalStart = csharpDesiredIndentation * -1;
index = Array.BinarySearch(sourceMappingIndentationScopes, originalStart);
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
csharpDesiredIndentation = index < 0
? minCSharpIndentation
: sourceMappingIndentations[sourceMappingIndentationScopes[index]];

// Now that we found the right indentation, we can write it back into the map, to save doing this binary
// search every time.
sourceMappingIndentations[absoluteIndex] = csharpDesiredIndentation;
}

// 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.
if (context.TryGetFormattingSpan(absoluteIndex, out var span))
Expand Down
48 changes: 48 additions & 0 deletions .../Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/CodeDirectiveFormattingTest.cs
View file
Open in desktop
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