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

Fix a couple of exceptions encountered when formatting documents with preprocessor directives #11373

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Fix a couple of exceptions encountered when formatting documents with preprocessor directives #11373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions ...Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxNodeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,12 @@ public static bool TryGetLinePositionSpanWithoutWhitespace(this SyntaxNode node,
var startPositionSpan = GetLinePositionSpan(firstToken, source, node.SpanStart);
var endPositionSpan = GetLinePositionSpan(lastToken, source, node.SpanStart);

if (endPositionSpan.End < startPositionSpan.Start)
{
linePositionSpan = default;
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

When does this happen?

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 think due to FUSE, there was a zero-width marker token at the end of the document which upset this algorithm. It's entirely possible this never happens in real life, but being defensive doesn't hurt.

linePositionSpan = new LinePositionSpan(startPositionSpan.Start, endPositionSpan.End);
return true;

Expand Down
12 changes: 9 additions & 3 deletions src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/CSharpFormatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,15 @@ static void ExtractTriviaAnnotations(
var formattedTriviaList = formattedRoot.GetAnnotatedTrivia(MarkerId);
foreach (var trivia in formattedTriviaList)
{
// We only expect one annotation because we built the entire trivia with a single annotation.
var annotation = trivia.GetAnnotations(MarkerId).Single();
if (!int.TryParse(annotation.Data, out var projectedIndex))
// We only expect one annotation because we built the entire trivia with a single annotation, but
// we need to be defensive here. Annotations are a little hard to work with though, so apologies for
// the slightly odd method of validation.
using var enumerator = trivia.GetAnnotations(MarkerId).GetEnumerator();
enumerator.MoveNext();
var annotation = enumerator.Current;
// We shouldn't be able to enumerate any more, and we should be able to parse our data out of the annotation.
if (enumerator.MoveNext() ||
!int.TryParse(annotation.Data, out var projectedIndex))
Comment on lines -110 to +117
Copy link
Member

Choose a reason for hiding this comment

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

The defensive code seems fine to me, but what are the conditions where multiple annotations are returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given this document with source mappings at each $$

<div>
@{$$
#if DEBUG
$$}

@{$$
#endif
$$}
</div>

If DEBUG is not defined, then the whole span between directives is a single "DisabledTextTrivia", but obviously it contains two mappings, so two points to track.

{
// This shouldn't happen realistically unless someone messed with the annotations we added.
// Let's ignore this annotation.
Expand Down
40 changes: 40 additions & 0 deletions ...est/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/HtmlFormattingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,46 @@ @typeparam TValue
}
}

[FormattingTestFact]
public async Task PreprocessorDirectives()
{
await RunFormattingTestAsync(
input: """
<div Model="SomeModel">
<div />
@{
#if DEBUG
}
<div />
@{
#endif
}
</div>

@code {
private object SomeModel {get;set;}
}
""",
expected: """
<div Model="SomeModel">
<div />
@{
#if DEBUG
}
Copy link
Contributor

Choose a reason for hiding this comment

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

huh... this looks wrong to me :/

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this either TBH. Is the preprocessor directive increasing the indent level? It doesn't seem to in standalone C# file. E.g.

if (true)
{
#if DEBUG
if (true)
{
#endif
}
}

formatted becomes

if (true)
{
#if DEBUG
    if (true)
    {
#endif
    }
}

which is what I would expect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be very clear, this PR doesn't attempt to actually adjust how this document is formatted, it just makes the formatting engine not crash. The {get;set;} being spaced out correctly here is providing value to the user that they didn't get before.

Having said that, in the tests there are no preprocessor defines (actually we have no way to pass them to the Razor compiler at all I don't think) so the code in the #if is not parsed or formatted by Roslyn. Pretend it doesn't exist, and the result looks right.

<div />
@{
#endif

}
</div>

@code {
private object SomeModel { get; set; }
}
""",
allowDiagnostics: true);
}

private ImmutableArray<TagHelperDescriptor> GetComponents()
{
AdditionalSyntaxTrees.Add(Parse("""
Expand Down