Remove more LocateOwner usage #9132

ryzngard · 2023-08-16T22:51:35Z

Update code actions and completion
Update diagnostics to get off LocateOwner

ryzngard · 2023-08-16T22:53:03Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/SyntaxNodeExtensions.cs

+        // <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))


I'm not sure this is the right answer. Would love thoughts on this.

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

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.

ryzngard · 2023-08-16T22:53:48Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/SyntaxNodeExtensions.cs

+        return token.Parent;
+    }
+
+    public static SyntaxNode? FindNode(this SyntaxNode @this, Language.Syntax.TextSpan span, bool includeWhitespace = false, bool getInnermostNodeForTie = false)


shamelessly copied from Roslyn.

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 :)

I should probably add a lot of tests

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.

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.

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.

davidwengier

It is surprising to me that there are two pieces of code here that deal with diagnostic spans/ranges in quite different ways, so my comments are all just around that. It's hard to know if I'm completely off base though without seeing what the syntax tree looks like.

...rc/Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/RazorTranslateDiagnosticsService.cs

davidwengier · 2023-08-16T23:46:12Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/SyntaxNodeExtensions.cs

+        // <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))


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

davidwengier · 2023-08-16T23:48:22Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/SyntaxNodeExtensions.cs

+        return token.Parent;
+    }
+
+    public static SyntaxNode? FindNode(this SyntaxNode @this, Language.Syntax.TextSpan span, bool includeWhitespace = false, bool getInnermostNodeForTie = false)


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 :)

ryzngard · 2023-08-18T00:16:49Z

...rc/Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/RazorTranslateDiagnosticsService.cs

-        var owner = syntaxTree.GetOwner(sourceText, d.Range.End, logger);
-        if (owner is null)
+
+        var owner = syntaxTree.Root.FindNode(d.Range.AsRazorTextSpan(sourceText), getInnermostNodeForTie: true);


Hey, I added another FindNode usage!

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

ryzngard · 2023-08-18T00:18:51Z

...rc/Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/RazorTranslateDiagnosticsService.cs

        }

-        var isCSharp = owner.Kind is SyntaxKind.CSharpExpressionLiteral or SyntaxKind.CSharpStatementLiteral or SyntaxKind.CSharpEphemeralTextLiteral;
+        if (owner is CSharpImplicitExpressionSyntax implicitExpressionSyntax &&


I'm sure this could have been written with some fancy matching, but I couldn't figure it out for the 2 minutes I tried.

I can't actually comment on the entire range, this replaces from here to return false;

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

Oh, and I see that David had a separate comment with exactly that below.

davidwengier

Love the new code that traverses the tree. I think FindNode tests would be good, if for no other reason than to demonstrate how it works, but could be a follow up too. The existing tests are presumably good enough for this usage.

davidwengier · 2023-08-18T02:37:30Z

...rc/Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/RazorTranslateDiagnosticsService.cs

+        {
+            return codeBlock.Children.Count == 1
+                && IsCsharpKind(codeBlock.Children[0]);
+        }


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 (

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

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

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.

DustinCampbell · 2023-08-21T18:34:33Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/SyntaxNodeExtensions.cs

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


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

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.

ryzngard · 2023-08-23T23:08:19Z

@davidwengier I added some generated tests for the time being, I think some hand written ones will also be useful. This at least asserts current behavior with a bunch of different spans

ryzngard added 2 commits August 16, 2023 15:50

Update code actions and completion

8e2f7a7

Update diagnostics to get off LocateOwner

304bf37

ryzngard requested a review from a team as a code owner August 16, 2023 22:51

ryzngard commented Aug 16, 2023

View reviewed changes

davidwengier approved these changes Aug 16, 2023

View reviewed changes

Use more explicit check for CSharpImplicitExpressionSyntax

9fdffc1

ryzngard commented Aug 18, 2023

View reviewed changes

formatting

c3afa5c

davidwengier approved these changes Aug 18, 2023

View reviewed changes

DustinCampbell reviewed Aug 21, 2023

View reviewed changes

ryzngard added 2 commits August 23, 2023 14:45

Merge branch 'main' into LocateOwner/CodeActions_Diagnostics_Completion

fae78d4

Add generated test. Fix formatting

e875dab

Fix tests

3ee49ec

ryzngard merged commit 598c7d5 into dotnet:main Aug 30, 2023

ryzngard deleted the LocateOwner/CodeActions_Diagnostics_Completion branch August 30, 2023 19:46

ghost added this to the Next milestone Aug 30, 2023

allisonchou mentioned this pull request Sep 5, 2023

[Automated] PRs inserted in VS build main-34105.97 #9242

Closed

Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove more LocateOwner usage #9132

Remove more LocateOwner usage #9132

ryzngard commented Aug 16, 2023

ryzngard Aug 16, 2023

davidwengier Aug 16, 2023

333fred Aug 21, 2023

ryzngard Aug 16, 2023

davidwengier Aug 16, 2023

ryzngard Aug 17, 2023

davidwengier Aug 17, 2023

333fred Aug 21, 2023

DustinCampbell Aug 21, 2023

davidwengier left a comment

davidwengier Aug 16, 2023

davidwengier Aug 16, 2023

ryzngard Aug 18, 2023

ryzngard Aug 18, 2023

ryzngard Aug 18, 2023

333fred Aug 24, 2023

333fred Aug 24, 2023

davidwengier left a comment

davidwengier Aug 18, 2023

ryzngard Aug 18, 2023

davidwengier Aug 18, 2023

333fred Aug 24, 2023

davidwengier Aug 24, 2023

DustinCampbell Aug 21, 2023

333fred Aug 21, 2023

ryzngard commented Aug 23, 2023

Remove more LocateOwner usage #9132

Remove more LocateOwner usage #9132

Conversation

ryzngard commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzngard commented Aug 23, 2023