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

Remove LocateOwner in formatting #9237

Merged
merged 6 commits into from
Sep 5, 2023
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 @@ -311,29 +311,15 @@ private static bool ShouldFormatCore(FormattingContext context, TextSpan mapping
return true;
}

var sourceText = context.SourceText;
var absoluteIndex = mappingSpan.Start;

if (mappingSpan.Length > 0)
{
// Slightly ugly hack to get around the behavior of LocateOwner.
// In some cases, using the start of a mapping doesn't work well
// because LocateOwner returns the previous node due to it owning the edge.
// So, if we can try to find the owner using a position that fully belongs to the current mapping.
absoluteIndex = mappingSpan.Start + 1;
}

var change = new SourceChange(absoluteIndex, 0, string.Empty);
var syntaxTree = context.CodeDocument.GetSyntaxTree();
var owner = syntaxTree.Root.LocateOwner(change);
var owner = syntaxTree.Root.FindInnermostNode(mappingSpan.Start, includeWhitespace: true);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ryzngard I changed this to the overload that just takes a position, to match the old API. You had it as the one that takes a span, and I don't trust that the formatting engine can handle that sort of change, even if it makes sense with the parameters being passed in here.

if (owner is null)
{
// Can't determine owner of this position. Optimistically allow formatting.
foundOwner = null;
return true;
}

owner = FixOwnerToWorkaroundCompilerQuirks(owner);
foundOwner = owner;

// Special case: If we're formatting implicit statements, we want to treat the `@attribute` directive and
Expand All @@ -347,23 +333,58 @@ private static bool ShouldFormatCore(FormattingContext context, TextSpan mapping
return true;
}

if (IsRazorComment() ||
IsInBoundComponentAttributeName() ||
IsInHtmlAttributeValue() ||
IsInDirectiveWithNoKind() ||
IsInSingleLineDirective() ||
IsImplicitExpression() ||
IsInSectionDirectiveCloseBrace() ||
(!allowImplicitStatements && IsImplicitStatementStart()))
if (IsInsideRazorComment())
Copy link
Contributor

Choose a reason for hiding this comment

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

Just spread this out to aid debugging as it was impossible to tell which one of these methods was the thing that returned early. One of these "Should have done it years ago" changes.

{
return false;
}

if (IsInBoundComponentAttributeName())
{
return false;
}

if (IsInHtmlAttributeValue())
{
return false;
}

if (IsInDirectiveWithNoKind())
{
return false;
}

if (IsInSingleLineDirective())
{
return false;
}

if (!allowImplicitStatements && IsImplicitExpression())
{
return false;
}

if (IsInSectionDirectiveCloseBrace())
{
return false;
}

if (!allowImplicitStatements && IsImplicitStatementStart())
{
return false;
}

if (IsInTemplateBlock())
{
return false;
}

return true;

bool IsRazorComment()
bool IsInsideRazorComment()
{
if (owner.IsCommentSpanKind())
// We don't want to format _in_ comments, but we do want to move the start `@*` to the right position
if (owner is RazorCommentBlockSyntax &&
mappingSpan.Start != owner.SpanStart)
{
return true;
}
Expand Down Expand Up @@ -486,6 +507,15 @@ bool IsImplicitExpression()
return owner.AncestorsAndSelf().Any(n => n is CSharpImplicitExpressionSyntax);
}

bool IsInTemplateBlock()
{
// E.g, (| is position)
//
// `RenderFragment(|@<Component>);` - true
//
return owner.AncestorsAndSelf().Any(n => n is CSharpTemplateBlockSyntax);
}

bool IsInSectionDirectiveCloseBrace()
{
// @section Scripts {
Expand All @@ -510,26 +540,6 @@ owner.Parent is MarkupBlockSyntax block &&
}
}

private static SyntaxNode FixOwnerToWorkaroundCompilerQuirks(SyntaxNode owner)
{
// Workaround for https://github.com/dotnet/aspnetcore/issues/36689
// A tags owner comes back as itself if it is preceeded by a HTML comment,
// because the whitespace between the comment and the tag is reported as not editable

// Get to the outermost node first. eg in "<span" we might be on the node for the "<", which is parented
// by some other intermediate node, which is parented by the actual start tag node. We need to get out to
// the start tag node, in order to reason about its siblings. The siblings of the "<" are not helpful :)
var outerNode = owner.GetOutermostNode();
if (outerNode is not null &&
outerNode.TryGetPreviousSibling(out var whiteSpace) && whiteSpace.ContainsOnlyWhitespace() &&
whiteSpace.TryGetPreviousSibling(out var comment) && comment is MarkupCommentBlockSyntax)
{
return whiteSpace;
}

return owner;
}

private class IndentationData
{
private readonly int _offset;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.Language.Legacy;
using Microsoft.AspNetCore.Razor.Language.Syntax;
using Microsoft.AspNetCore.Razor.LanguageServer.Extensions;
using Microsoft.AspNetCore.Razor.LanguageServer.Protocol;
Expand Down Expand Up @@ -224,8 +223,7 @@ private static List<TextChange> AdjustRazorIndentation(FormattingContext context
private static bool IsPartOfHtmlTag(FormattingContext context, int position)
{
var syntaxTree = context.CodeDocument.GetSyntaxTree();
var change = new SourceChange(position, 0, string.Empty);
var owner = syntaxTree.Root.LocateOwner(change);
var owner = syntaxTree.Root.FindInnermostNode(position, includeWhitespace: true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Include whitespace may not be needed here...

Copy link
Contributor

Choose a reason for hiding this comment

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

If the tests all pass with it as false then no objection here, but I didn't check

if (owner is null)
{
// Can't determine owner of this position.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.Language.Legacy;
using Microsoft.AspNetCore.Razor.Language.Syntax;
using Microsoft.AspNetCore.Razor.LanguageServer.EndpointContracts;
using Microsoft.AspNetCore.Razor.LanguageServer.Extensions;
Expand Down Expand Up @@ -111,8 +110,7 @@ static bool TryGetNearestMarkupNameTokens(
[NotNullWhen(true)] out SyntaxToken? startTagNameToken,
[NotNullWhen(true)] out SyntaxToken? endTagNameToken)
{
var change = new SourceChange(location.AbsoluteIndex, length: 0, newText: "");
var owner = syntaxTree.Root.LocateOwner(change);
var owner = syntaxTree.Root.FindInnermostNode(location.AbsoluteIndex);
var element = owner?.FirstAncestorOrSelf<MarkupSyntaxNode>(
a => a.Kind is SyntaxKind.MarkupTagHelperElement || a.Kind is SyntaxKind.MarkupElement);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
<div class="navbar-collapse collapse">
<ul class="nav ">
<li>@Html.ActionLink("Home", "Index", "Home")</li>
<li>@Html.ActionLink( "About", "About", "Home")</li>
<li>@Html.ActionLink("About", "About", "Home")</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a surprise, but a pretty good change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🥳

<li>@Html.ActionLink("Contact", "Contact", "Home")</li>
</ul>
</div>
Expand Down