Skip to content

Remove LocateOwner usage from CodeActions #9037

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

Merged
merged 9 commits into from
Jul 28, 2023
Merged

Remove LocateOwner usage from CodeActions #9037

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ddcee04
Remove LocateOwner usage from CodeActions
Jul 27, 2023
9b123af
Extension method
Jul 27, 2023
e28a3b8
Add include whitespace
Jul 28, 2023
0f8a378
Rename extension method
Jul 28, 2023
6edc7da
Merge branch 'main' into LocateOwner/code_action_providers
Jul 28, 2023
ef14e95
Use AncestorsAndSelf
Jul 28, 2023
811fa65
Fix broken behavior on whitespace before a component
Jul 28, 2023
82ebb8d
Update src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeAc…
ryzngard Jul 28, 2023
7008d16
Merge branch 'main' into LocateOwner/code_action_providers
Jul 28, 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
4 changes: 2 additions & 2 deletions ...AspNetCore.Razor.LanguageServer/CodeActions/CSharp/TypeAccessibilityCodeActionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,15 +217,15 @@ private static IEnumerable<RazorVSInternalCodeAction> ProcessCodeActionsVS(

static bool TryGetOwner(RazorCodeActionContext context, [NotNullWhen(true)] out SyntaxNode? owner)
{
var change = new SourceChange(context.Location.AbsoluteIndex, length: 0, newText: string.Empty);
var syntaxTree = context.CodeDocument.GetSyntaxTree();
if (syntaxTree?.Root is null)
{
owner = null;
return false;
}

owner = syntaxTree.Root.LocateOwner(change);
var token = syntaxTree.Root.FindToken(context.Location.AbsoluteIndex);
owner = token?.Parent;
if (owner is null)
{
Debug.Fail("Owner should never be null.");
Expand Down
4 changes: 2 additions & 2 deletions ...etCore.Razor.LanguageServer/CodeActions/Razor/ComponentAccessibilityCodeActionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ public ComponentAccessibilityCodeActionProvider(TagHelperFactsService tagHelperF
using var _ = ListPool<RazorVSInternalCodeAction>.GetPooledObject(out var codeActions);

// Locate cursor
var change = new SourceChange(context.Location.AbsoluteIndex, length: 0, newText: string.Empty);
var node = context.CodeDocument.GetSyntaxTree().Root.LocateOwner(change);
var token = context.CodeDocument.GetSyntaxTree().Root.FindToken(context.Location.AbsoluteIndex);
var node = token?.Parent;
if (node is null)
{
return s_emptyResult;
Expand Down
4 changes: 2 additions & 2 deletions ...spNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToCodeBehindCodeActionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,14 @@ public ExtractToCodeBehindCodeActionProvider(ILoggerFactory loggerFactory)
return s_emptyResult;
}

var change = new SourceChange(context.Location.AbsoluteIndex, length: 0, newText: string.Empty);
var syntaxTree = context.CodeDocument.GetSyntaxTree();
if (syntaxTree?.Root is null)
{
return s_emptyResult;
}

var owner = syntaxTree.Root.LocateOwner(change);
var token = syntaxTree.Root.FindToken(context.Location.AbsoluteIndex);
var owner = token?.Parent;
Copy link
Member

@davidwengier davidwengier Jul 27, 2023

Choose a reason for hiding this comment

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

Can you validate that the switch on line 69 below, is still needed? Some of those quirks in tree shape might have been fixed by the FindToken API.

Copy link
Contributor Author

@ryzngard ryzngard Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like it's still needed. Handle_AtEndOfCodeDirectiveWithNoSpace_ReturnsResult fails for

@code$${ private var x = 1; }

if I change just to look for a node.Parent is RazorDirectiveSyntax, along with the following other tests

image

Copy link
Member

@davidwengier davidwengier Jul 28, 2023

Choose a reason for hiding this comment

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

Thanks for checking. I suspect @code$$ { falls into the second arm now, not the first, but makes sense that the first one is needed with the current algorithm.

@333fred does Roslyn do any heuristics like "if the token in front of the caret is punctuation, use the token behind it"?

Copy link
Member

@333fred 333fred Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nope. The rule is very simple and straightforward: all trailing trivia, up to and not including the newline, is attached to the previous token. Any further trivia is attached to the next token (which is potentially the eof token at the end of the file). And, importantly, punctuation such as { is not trivia.

Copy link
Member

@davidwengier davidwengier Jul 28, 2023

Choose a reason for hiding this comment

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

Thanks for confirming. If this becomes important we can always create an extension method that deals with it. As it is the switch expression only deals with two cases, so its not quite at the level of "deal with compiler quirks" :)

if (owner is null)
{
_logger.LogWarning("Owner should never be null.");
Expand Down