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

Remove more LocateOwner usage #9132

Merged
merged 7 commits into from
Aug 30, 2023
Merged

Remove more LocateOwner usage #9132

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8e2f7a7
Update code actions and completion
ryzngard Aug 16, 2023
304bf37
Update diagnostics to get off LocateOwner
ryzngard Aug 16, 2023
9fdffc1
Use more explicit check for CSharpImplicitExpressionSyntax
ryzngard Aug 18, 2023
c3afa5c
formatting
ryzngard Aug 18, 2023
fae78d4
Merge branch 'main' into LocateOwner/CodeActions_Diagnostics_Completion
ryzngard Aug 23, 2023
e875dab
Add generated test. Fix formatting
ryzngard Aug 23, 2023
3ee49ec
Fix tests
ryzngard Aug 24, 2023
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
2 changes: 1 addition & 1 deletion ...oft.AspNetCore.Razor.LanguageServer/CodeActions/CSharp/DefaultCSharpCodeActionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public DefaultCSharpCodeActionProvider(LanguageServerFeatureOptions languageServ
}

var tree = context.CodeDocument.GetSyntaxTree();
var node = tree.GetOwner(context.Location.AbsoluteIndex);
var node = tree.Root.FindInnermostNode(context.Location.AbsoluteIndex);
var isInImplicitExpression = node?.AncestorsAndSelf().Any(n => n is CSharpImplicitExpressionSyntax) ?? false;

var allowList = isInImplicitExpression
Expand Down
2 changes: 1 addition & 1 deletion ...AspNetCore.Razor.LanguageServer/Completion/Delegation/DesignTimeHelperResponseRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public override async Task<VSInternalCompletionList> RewriteAsync(
}

var syntaxTree = await hostDocumentContext.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false);
var owner = syntaxTree.GetOwner(hostDocumentIndex);
var owner = syntaxTree.Root.FindInnermostNode(hostDocumentIndex);
if (owner is null)
{
Debug.Fail("Owner should never be null.");
Expand Down
45 changes: 29 additions & 16 deletions ...Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/RazorTranslateDiagnosticsService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Microsoft.AspNetCore.Razor.LanguageServer.Extensions;
using Microsoft.AspNetCore.Razor.LanguageServer.Protocol;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Razor.Workspaces.Extensions;
using Microsoft.Extensions.Logging;
using Diagnostic = Microsoft.VisualStudio.LanguageServer.Protocol.Diagnostic;
using DiagnosticSeverity = Microsoft.VisualStudio.LanguageServer.Protocol.DiagnosticSeverity;
Expand Down Expand Up @@ -120,7 +121,7 @@ private static Diagnostic[] FilterHTMLDiagnostics(

var filteredDiagnostics = unmappedDiagnostics
.Where(d =>
!InCSharpLiteral(d, sourceText, syntaxTree, logger) &&
!InCSharpLiteral(d, sourceText, syntaxTree) &&
!InAttributeContainingCSharp(d, sourceText, syntaxTree, processedAttributes, logger) &&
!AppliesToTagHelperTagName(d, sourceText, syntaxTree, logger) &&
!ShouldFilterHtmlDiagnosticBasedOnErrorCode(d, sourceText, syntaxTree, logger))
Expand Down Expand Up @@ -162,23 +163,34 @@ private Diagnostic[] MapDiagnostics(
private static bool InCSharpLiteral(
Diagnostic d,
SourceText sourceText,
RazorSyntaxTree syntaxTree,
ILogger logger)
RazorSyntaxTree syntaxTree)
{
if (d.Range is null)
{
return false;
}

var owner = syntaxTree.GetOwner(sourceText, d.Range.End, logger);
if (owner is null)

var owner = syntaxTree.Root.FindNode(d.Range.AsRazorTextSpan(sourceText), getInnermostNodeForTie: 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.

Hey, I added another FindNode usage!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, I can't explain why it doesn't find the CSharpCodeBlockSyntax with the same span. This could be a difference in how Roslyn and Razor make trees/how FindToken works.

My lack of understanding also means I don't think I can make a good comment. I just know it works and all the tests are passing

if (IsCsharpKind(owner))
{
return false;
return true;
}

var isCSharp = owner.Kind is SyntaxKind.CSharpExpressionLiteral or SyntaxKind.CSharpStatementLiteral or SyntaxKind.CSharpEphemeralTextLiteral;
if (owner is CSharpImplicitExpressionSyntax implicitExpressionSyntax
&& implicitExpressionSyntax.Body is CSharpImplicitExpressionBodySyntax bodySyntax
&& bodySyntax.CSharpCode is CSharpCodeBlockSyntax codeBlock)
{
return codeBlock.Children.Count == 1
&& IsCsharpKind(codeBlock.Children[0]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this whole thing is possibly:

        return (owner is CSharpImplicitExpressionSyntax 
            {
                Body: CSharpImplicitExpressionBodySyntax
                {
                    CSharpCode: CSharpCodeBlockSyntax
                    {
                        Children: [var firstChild]
                    }
                }
            } &&
            IsCsharpKind(firstChild);

though I'm not sure thats actually readable in any way, so this is not a suggestion to change it necessarily, I just enjoy these trivia challenges.

Also since when did the && go at the start of the line? I always put them at the end, so the conditions all line up neatly under the if (

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also since when did the && go at the start of the line?

This was from using the Wrap Expression code fix. I'm happy with either way though.

Benefit of this style is for diffing, so you only see the line being added/removed, and don't mess with others. Similar to wrapping arguments with

static void Method(
	arg1
	, arg2
	, arg3

Although for methods and enums I'd prefer to just leave a trailing comma if possible

Copy link
Contributor

Choose a reason for hiding this comment

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

"Similar to " is not the argument you think it is 😛

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I prefer && at the start of the line because I find it more readable. I'm only wrapping when the expression is long, and the && is more visible at the start of the line, which (imo) makes the precedence stand out more.

Copy link
Contributor

Choose a reason for hiding this comment

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

WannaFightJackReacherGIF


return false;

return isCSharp;
static bool IsCsharpKind([NotNullWhen(true)] SyntaxNode? node)
=> node?.Kind is SyntaxKind.CSharpExpressionLiteral
or SyntaxKind.CSharpStatementLiteral
or SyntaxKind.CSharpEphemeralTextLiteral;
}

private static bool AppliesToTagHelperTagName(
Expand All @@ -201,7 +213,7 @@ private static bool AppliesToTagHelperTagName(
return false;
}

var owner = syntaxTree.GetOwner(sourceText, diagnostic.Range.End, logger);
var owner = syntaxTree.FindInnermostNode(sourceText, diagnostic.Range.End, logger);

var startOrEndTag = owner?.FirstAncestorOrSelf<RazorSyntaxNode>(n => n is MarkupTagHelperStartTagSyntax || n is MarkupTagHelperEndTagSyntax);
if (startOrEndTag is null)
Expand Down Expand Up @@ -246,7 +258,7 @@ private static bool ShouldFilterHtmlDiagnosticBasedOnErrorCode(Diagnostic diagno
static bool IsCSharpInStyleBlock(Diagnostic diagnostic, SourceText sourceText, RazorSyntaxTree syntaxTree, ILogger logger)
{
// C# in a style block causes diagnostics because the HTML background document replaces C# with "~"
var owner = syntaxTree.GetOwner(sourceText, diagnostic.Range.Start, logger);
var owner = syntaxTree.FindInnermostNode(sourceText, diagnostic.Range.Start, logger);
if (owner is null)
{
return false;
Expand All @@ -262,7 +274,7 @@ static bool IsCSharpInStyleBlock(Diagnostic diagnostic, SourceText sourceText, R
// but we don't currently have a system to accomplish that
static bool IsAnyFilteredTooFewElementsError(Diagnostic diagnostic, SourceText sourceText, RazorSyntaxTree syntaxTree, ILogger logger)
{
var owner = syntaxTree.GetOwner(sourceText, diagnostic.Range.Start, logger);
var owner = syntaxTree.FindInnermostNode(sourceText, diagnostic.Range.Start, logger);
if (owner is null)
{
return false;
Expand Down Expand Up @@ -291,7 +303,7 @@ static bool IsAnyFilteredTooFewElementsError(Diagnostic diagnostic, SourceText s
// but we don't currently have a system to accomplish that
static bool IsHtmlWithBangAndMatchingTags(Diagnostic diagnostic, SourceText sourceText, RazorSyntaxTree syntaxTree, ILogger logger)
{
var owner = syntaxTree.GetOwner(sourceText, diagnostic.Range.Start, logger);
var owner = syntaxTree.FindInnermostNode(sourceText, diagnostic.Range.Start, logger);
if (owner is null)
{
return false;
Expand Down Expand Up @@ -319,7 +331,7 @@ static bool IsAnyFilteredInvalidNestingError(Diagnostic diagnostic, SourceText s

static bool IsInvalidNestingWarningWithinComponent(Diagnostic diagnostic, SourceText sourceText, RazorSyntaxTree syntaxTree, ILogger logger)
{
var owner = syntaxTree.GetOwner(sourceText, diagnostic.Range.Start, logger);
var owner = syntaxTree.FindInnermostNode(sourceText, diagnostic.Range.Start, logger);
if (owner is null)
{
return false;
Expand All @@ -334,7 +346,7 @@ static bool IsInvalidNestingWarningWithinComponent(Diagnostic diagnostic, Source
// but we don't currently have a system to accomplish that
static bool IsInvalidNestingFromBody(Diagnostic diagnostic, SourceText sourceText, RazorSyntaxTree syntaxTree, ILogger logger)
{
var owner = syntaxTree.GetOwner(sourceText, diagnostic.Range.Start, logger);
var owner = syntaxTree.FindInnermostNode(sourceText, diagnostic.Range.Start, logger);
if (owner is null)
{
return false;
Expand Down Expand Up @@ -371,7 +383,7 @@ private static bool InAttributeContainingCSharp(
return false;
}

var owner = syntaxTree.GetOwner(sourceText, diagnostic.Range.End, logger);
var owner = syntaxTree.FindInnermostNode(sourceText, diagnostic.Range.End, logger);
if (owner is null)
{
return false;
Expand Down Expand Up @@ -497,7 +509,8 @@ private bool TryRemapRudeEditRange(Range diagnosticRange, RazorCodeDocument code
// semi-intelligent way.

var syntaxTree = codeDocument.GetSyntaxTree();
var owner = syntaxTree.GetOwner(sourceText, diagnosticRange, logger: _logger);
var span = diagnosticRange.AsRazorTextSpan(codeDocument.GetSourceText());
var owner = syntaxTree.Root.FindNode(span, getInnermostNodeForTie: true);

switch (owner?.Kind)
{
Expand Down
54 changes: 4 additions & 50 deletions ...zor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/RazorSyntaxTreeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System.Collections.Generic;
using System.Linq;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.Language.Legacy;
using Microsoft.AspNetCore.Razor.Language.Syntax;
using Microsoft.AspNetCore.Razor.LanguageServer.Formatting;
using Microsoft.CodeAnalysis.Text;
Expand Down Expand Up @@ -59,23 +58,12 @@ public static IReadOnlyList<CSharpStatementSyntax> GetCSharpStatements(this Razo
return statements;
}

public static SyntaxNode? GetOwner(this RazorSyntaxTree syntaxTree, int absoluteIndex)
{
if (syntaxTree is null)
{
throw new ArgumentNullException(nameof(syntaxTree));
}

var change = new SourceChange(absoluteIndex, 0, string.Empty);
var owner = syntaxTree.Root.LocateOwner(change);
return owner;
}

public static SyntaxNode? GetOwner(
public static SyntaxNode? FindInnermostNode(
this RazorSyntaxTree syntaxTree,
SourceText sourceText,
Position position,
ILogger logger)
ILogger logger,
bool includeWhitespace = false)
{
if (syntaxTree is null)
{
Expand All @@ -102,40 +90,6 @@ public static IReadOnlyList<CSharpStatementSyntax> GetCSharpStatements(this Razo
return default;
}

return GetOwner(syntaxTree, absoluteIndex);
}

public static SyntaxNode? GetOwner(
this RazorSyntaxTree syntaxTree,
SourceText sourceText,
Range range,
ILogger logger)
{
if (syntaxTree is null)
{
throw new ArgumentNullException(nameof(syntaxTree));
}

if (sourceText is null)
{
throw new ArgumentNullException(nameof(sourceText));
}

if (range is null)
{
throw new ArgumentNullException(nameof(range));
}

var startInSync = range.Start.TryGetAbsoluteIndex(sourceText, logger, out var absoluteStartIndex);
var endInSync = range.End.TryGetAbsoluteIndex(sourceText, logger, out var absoluteEndIndex);
if (startInSync is false || endInSync is false)
{
return default;
}

var length = absoluteEndIndex - absoluteStartIndex;
var change = new SourceChange(absoluteStartIndex, length, string.Empty);
var owner = syntaxTree.Root.LocateOwner(change);
return owner;
return syntaxTree.Root.FindInnermostNode(absoluteIndex, includeWhitespace);
}
}
58 changes: 57 additions & 1 deletion src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/SyntaxNodeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.Language.Syntax;
using Microsoft.AspNetCore.Razor.LanguageServer.Formatting;
Expand Down Expand Up @@ -328,5 +329,60 @@ public static int GetTrailingWhitespaceLength(this SyntaxNode node, FormattingCo
}

public static SyntaxNode? FindInnermostNode(this SyntaxNode node, int index, bool includeWhitespace = false)
=> node.FindToken(index, includeWhitespace)?.Parent;
{
var token = node.FindToken(index, includeWhitespace);

// If the index is EOF but the node has index-1,
// then try to get a token to the left of the index.
// patterns like
// <button></button>$$
// should get the button node instead of the razor document (which is the parent
// of the EOF token)
if (token.Kind == SyntaxKind.EndOfFile && node.Span.Contains(index - 1))
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'm not sure this is the right answer. Would love thoughts on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

i guess this makes sense. if there were whitespace at the end, i think the compiler method would do the same thing. Maybe @333fred has opinions though

Copy link
Member

Choose a reason for hiding this comment

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

if there were whitespace at the end, i think the compiler method would do the same thing.

Yes. This is the type of heuristic that I would expect the IDE to need to do, but with FindToken you have a predictable set of rules to base that heuristic on.

{
token = node.FindToken(index - 1, includeWhitespace);
}

return token.Parent;
}

public static SyntaxNode? FindNode(this SyntaxNode @this, Language.Syntax.TextSpan span, bool includeWhitespace = false, bool getInnermostNodeForTie = false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shamelessly copied from Roslyn.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks scary, and seems like it only has one caller, which is surprising and doesn't fill me with joy, but the fact that its copied from Roslyn means I'm going to assume its fine :)

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 should probably add a lot of tests

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering about that, and also wondering if this should move down to the compiler, next to FindToken. I think @333fred has mentioned thinking about adding something that dealt with ties, but couldn't see any usage that required spans, so left it out.

Copy link
Member

Choose a reason for hiding this comment

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

We can certainly move this down to the compiler, but yes the initial FindToken method was all we thought we needed in the design meeting a few months ago. If we find that need FindNode as well, I'm perfectly fine with adding it.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, FindToken was considered table stakes and that we'd build whatever helpers we needed based on that and then push APIs down to the compiler that were particularly useful. In Roslyn, SyntaxNode didn't have a FindNode API for many years (or Ancestors, Descendants, FirstAncestorOrSelf`, etc.) and were only added when it was recognized that they were essential. I'm happy to not move this to the compiler unless it's super useful for other purposes.

{
if (!@this.FullSpan.Contains(span))
{
throw new ArgumentOutOfRangeException(nameof(span));
}

var node = @this.FindToken(span.Start, includeWhitespace)
.Parent
!.FirstAncestorOrSelf<SyntaxNode>(a => a.FullSpan.Contains(span));

node.AssumeNotNull();

// Tie-breaking.
if (!getInnermostNodeForTie)
{
var cuRoot = node.Ancestors().Last();
Copy link
Member

Choose a reason for hiding this comment

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

@333fred: Should there be a cheaper way to retrieve the RazorSyntaxTree for a given red node?

Copy link
Member

Choose a reason for hiding this comment

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

I was surprised there wasn't as well when I implemented FindToken. It's one of the things missing from our copy of Roslyn's nodes.


while (true)
{
var parent = node.Parent;
// NOTE: We care about FullSpan equality, but FullWidth is cheaper and equivalent.
if (parent == null || parent.FullWidth != node.FullWidth)
{
break;
}

// prefer child over compilation unit
if (parent == cuRoot)
{
break;
}

node = parent;
}
}

return node;
}
}
4 changes: 2 additions & 2 deletions src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorSyntaxFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public static bool TryGetAttributeNameAbsoluteIndex(RazorCodeDocument codeDocume
attributeNameAbsoluteIndex = 0;

var tree = codeDocument.GetSyntaxTree();
var owner = tree.GetOwner(absoluteIndex);
var owner = tree.Root.FindInnermostNode(absoluteIndex);

var attributeName = owner?.Parent switch
{
Expand Down Expand Up @@ -72,7 +72,7 @@ public static bool TryGetAttributeNameAbsoluteIndex(RazorCodeDocument codeDocume
public static bool TryGetFullAttributeNameSpan(RazorCodeDocument codeDocument, int absoluteIndex, out TextSpan attributeNameSpan)
{
var tree = codeDocument.GetSyntaxTree();
var owner = tree.GetOwner(absoluteIndex);
var owner = tree.Root.FindInnermostNode(absoluteIndex);

attributeNameSpan = GetFullAttributeNameSpan(owner?.Parent);

Expand Down
Loading