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

[DRAFT] Add Code Action Provider Name to CodeAction.Data #48951

Closed
wants to merge 11 commits into from
Closed

[DRAFT] Add Code Action Provider Name to CodeAction.Data #48951

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0512fd5
WIP
TanayParikh Oct 9, 2020
02a8da8
Merge branch 'master' into dev/taparik/addCodeActionProviderName
TanayParikh Oct 27, 2020
a637d17
Provider, Implementation
TanayParikh Oct 27, 2020
b9cdf56
Merge branch 'master' into dev/taparik/addCodeActionProviderName
TanayParikh Oct 27, 2020
3a09488
Merge branch 'master' into dev/taparik/addCodeActionProviderName
TanayParikh Nov 12, 2020
a52b3c0
Feedback
TanayParikh Nov 12, 2020
f477ac0
Fix missing func
TanayParikh Nov 12, 2020
337d594
Merge branch 'master' into dev/taparik/addCodeActionProviderName
TanayParikh Nov 13, 2020
a16be50
WIP
TanayParikh Nov 16, 2020
d47cd0b
Merge branch 'master' into dev/taparik/addCodeActionProviderName
TanayParikh Nov 16, 2020
c109013
External Access
TanayParikh Nov 16, 2020
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 src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,8 @@ protected static LSP.VSCompletionItem CreateCompletionItem(
return item;
}

private protected static CodeActionResolveData CreateCodeActionResolveData(string uniqueIdentifier, LSP.Location location)
=> new CodeActionResolveData(uniqueIdentifier, location.Range, CreateTextDocumentIdentifier(location.Uri));
private protected static CodeActionResolveData CreateCodeActionResolveData(string uniqueIdentifier, LSP.Location location, string providerName = null)
=> new CodeActionResolveData(uniqueIdentifier, providerName ?? string.Empty, location.Range, CreateTextDocumentIdentifier(location.Uri));

/// <summary>
/// Creates a solution with a document.
Expand Down
10 changes: 6 additions & 4 deletions ...atures/Core/Portable/CodeRefactorings/AddAwait/AbstractAddAwaitCodeRefactoringProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,15 @@ public sealed override async Task ComputeRefactoringsAsync(CodeRefactoringContex
context.RegisterRefactoring(
new MyCodeAction(
GetTitle(),
c => AddAwaitAsync(document, awaitable, withConfigureAwait: false, c)),
c => AddAwaitAsync(document, awaitable, withConfigureAwait: false, c),
CodeAction.CreateProviderName(PredefinedCodeRefactoringProviderNames.AddAwait, nameof(GetTitle))),
awaitable.Span);

context.RegisterRefactoring(
new MyCodeAction(
GetTitleWithConfigureAwait(),
c => AddAwaitAsync(document, awaitable, withConfigureAwait: true, c)),
c => AddAwaitAsync(document, awaitable, withConfigureAwait: true, c),
CodeAction.CreateProviderName(PredefinedCodeRefactoringProviderNames.AddAwait, nameof(GetTitleWithConfigureAwait))),
awaitable.Span);
}

Expand Down Expand Up @@ -106,8 +108,8 @@ private static async Task<Document> AddAwaitAsync(

private class MyCodeAction : CodeAction.DocumentChangeAction
{
public MyCodeAction(string title, Func<CancellationToken, Task<Document>> createChangedDocument)
: base(title, createChangedDocument)
public MyCodeAction(string title, Func<CancellationToken, Task<Document>> createChangedDocument, string providerName)
: base(title, createChangedDocument, providerName: providerName)
{
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ private static VSCodeAction GenerateVSCodeAction(
Kind = codeActionKind,
Diagnostics = request.Context.Diagnostics,
Children = nestedActions.ToArray(),
Data = new CodeActionResolveData(currentTitle, request.Range, request.TextDocument)
Data = new CodeActionResolveData(currentTitle, codeAction.ProviderName, request.Range, request.TextDocument)
};
}

Expand Down
13 changes: 12 additions & 1 deletion src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,24 @@ internal class CodeActionResolveData
/// </remarks>
public string UniqueIdentifier { get; }

/// <summary>
/// Identifies the Code Action Provider which produced this Code Action.
/// </summary>
/// <remarks>
/// The unique identifier is currently set as:
/// name of top level code action provider + '|' + name of code action provider implementation
/// e.g. 'Add Await Code Action Provider | GetTitleWithConfigureAwait'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mirroring format of above UniqueIdentifier

Copy link
Contributor

Choose a reason for hiding this comment

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

GetTitleWithConfigureAwait is localized, which I believe would likely be problematic if Razor is trying to avoid localization issues.

I'm wondering if ProviderName can simply be made up of PredefinedCodeFixProviderNames and PredefinedCodeRefactoringProviderNames?
This would mean that you wouldn't be able to filter out specific code actions from a provider, but it would be simpler and would avoid localization issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note; we're doing nameof(GetTitleWithConfigureAwait) not using the result of the call.

It appears the method name isn't localized, however the resource value is. In that case would we still run into localization issues?

With regard to making things simpler, I agree removing it would reduce the complexity. Would you prefer that approach?

My reasoning for specifying the implementation was, there may be cases where Razor may support one implementation of a provider, but not another. Granted, no specific example jumps out at me right now so this may be premature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh whoops, my bad, I totally missed that. 😅 Yeah, I don't think using nameof should have any localization issues.

In general, I do think the simpler approach would definitely be more ideal if that works for Razor. I don't think every code action provider will have something similar to GetTitleWithConfigureAwait, for example the Add Parameter code fix provider seems to have different logic for displaying their code actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks for the clarification! Will remove the implementation from the ProviderName.

Copy link
Contributor

Choose a reason for hiding this comment

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

You will need to define helper types say, RazorPredefinedCodeRefactoringProviderNames and RazorPredefinedCodeFixProviderNames with constant string fields that forward to the internal types PredefinedCodeRefactoringProviderNames and PredefinedCodeFixProviderNames respectively. Something similar to https://github.com/dotnet/roslyn/blob/master/src/Tools/ExternalAccess/FSharp/Diagnostics/FSharpIDEDiagnosticIds.cs. I presume the new files will go https://github.com/dotnet/roslyn/tree/master/src/Tools/ExternalAccess/Razor?

Copy link
Contributor Author

@TanayParikh TanayParikh Nov 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What's the difference in the code (refactoring/fix) providers in the PredefinedCodeRefactoringProviderNames and PredefinedCodeFixProviderNames and something like the Add Null Checks code action we have both the
AddNullChecksId (private field) as well as the Add Null Checks Feature Resource?

Ie. For the Razor external access, should the private AddNullChecksId be used or the FeatureResources?

@mavasani

Copy link
Contributor

@allisonchou allisonchou Nov 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For Razor external access, can just the PredefinedCodeRefactoringProviderNames and PredefinedCodeFixProviderNames be exposed? I guess I'm not understanding why AddNullChecksId or FeatureResources might be necessary to expose. For example, for AddNullChecksId, is Razor able to simply use the overarching PredefinedCodeRefactoringProviderNames.GenerateConstructorFromMembers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, for AddNullChecksId, is Razor able to simply use the overarching PredefinedCodeRefactoringProviderNames.GenerateConstructorFromMembers

Ah, I wasn't aware that AddNullChecks would be a part of the GenerateConstructorFromMembers. I tried annotating the associated abstract classes of GenerateConstructorFromMembers but that didn't actually annotate the Add Null Check CodeAction.Data payload with the expected provider name. Is there a specific (inherited) code action I may have missed annotating?

Copy link
Contributor

@allisonchou allisonchou Nov 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, wait. The "Add null checks" feature resource you linked isn't actually its own separate code action, I believe it's just part of the Generate constructor dialog (see number 4 image at https://docs.microsoft.com/en-us/visualstudio/ide/reference/generate-constructor?view=vs-2019).
Sorry, didn't catch that before. It might also be important to note that Roslyn currently filters out any code actions with dialogs/options in LSP, since LSP currently doesn't support them.

When you say you want "Add null check" code action, are you possibly thinking of the action associated with the AbstractAddParameterCheckCodeRefactoringProvider? http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs,96

/// </remarks>
public string ProviderName { get; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to be added to published API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I don't think we need the ProviderName property in this file actually. The properties in CodeActionResolveData are just used to carry over necessary information from our CodeActionHandler class to our CodeActionResolveHandler class. Since CodeActionHandler is the class that tells LSP what code actions are available, I don't think we need to carry over the ProviderName information to the resolve handler, since all the resolve handler does is populate the code action edits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't we need this property here to ensure the CodeAction.Data payload actually carries the provider name?

The properties in CodeActionResolveData are just used to carry over necessary information from our CodeActionHandler class to our CodeActionResolveHandler class.

In-between Razor get's the CodeActionResolveData and that's where I was intending to inspect the payload to consume the ProviderName. Is there a better way to ensure razor get's the provider name?

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be misunderstanding how things work here. Could you explain what Razor is doing in between Roslyn's CodeActionsHandler and CodeActionResolveHandler?

My initial impression was that per our conversation with Miguel, Razor would provide Roslyn with a list ProviderNames through a custom property to Roslyn's CodeActionsHandler. Roslyn would then filter code actions based on these provider names, and only return back a filtered set of code actions. Later, Roslyn would get code action resolve requests for each of the filtered code actions, and then would populate these code actions with the edit data.
^Is this correct? Please feel free to correct me if not.

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 might be misunderstanding how things work here. Could you explain what Razor is doing in between Roslyn's CodeActionsHandler and CodeActionResolveHandler?

  1. Razor sends a textDocument/codeAction request to the Roslyn CodeActionsHandler.
  2. Roslyn runs its analyzers & [NEW in this PR] annotates the response CodeAction.CodeActionResolveData with the ProviderName property.
  3. Razor receives the response from Update links in README.md with ported wiki content #2, inspects the CodeAction.CodeActionResolveData.ProviderName to identify the code action, determine whether or not it is supported, and take the appropriate action to display it to the user in a razor compatible manner.
  4. [Optional]: Should it be necessary to resolve the code action (ex. user selects it), then razor sends a textDocument/codeActionResolve request to Roslyn's CodeActionResolveHandler with the initial CodeAction.CodeActionResolveData.
  5. Roslyn returns resolved edits, razor remaps those edits & displays them to the user.

My initial impression was that per our conversation with Miguel, Razor would provide Roslyn with a list ProviderNames through a custom property to Roslyn's CodeActionsHandler. Roslyn would then filter code actions based on these provider names, and only return back a filtered set of code actions. Later, Roslyn would get code action resolve requests for each of the filtered code actions, and then would populate these code actions with the edit data.

This was definitely one of the proposals we were discussing, but we ended up deciding against it for now in the interest of simplicity and reduce coupling. This also required potential LSP Spec changes, whereas annotating the CodeActionResolveData payload does not as the spec just specifies an object Data;. The goal of this PR was to take the approach steps detailed above. I hope that helps clarify things, let me know if there are any other questions :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That was a great explanation, thank you! In that case, yeah, I definitely think the data payload needs the provider name.


public LSP.Range Range { get; }

public LSP.TextDocumentIdentifier TextDocument { get; }

public CodeActionResolveData(string uniqueIdentifier, LSP.Range range, LSP.TextDocumentIdentifier textDocument)
public CodeActionResolveData(string uniqueIdentifier, string providerName, LSP.Range range, LSP.TextDocumentIdentifier textDocument)
{
UniqueIdentifier = uniqueIdentifier;
ProviderName = providerName;
Range = range;
TextDocument = textDocument;
}
Expand Down
24 changes: 21 additions & 3 deletions src/Workspaces/Core/Portable/CodeActions/CodeAction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ public abstract class CodeAction
/// </remarks>
public virtual string? EquivalenceKey => null;

/// <summary>
/// Identifies the Code Action Provider which produced this Code Action.
/// </summary>
/// <remarks>
/// e.g. 'Add Await Code Action Provider'
/// </remarks>
public virtual string? ProviderName => null;

internal virtual bool IsInlinable => false;

internal virtual CodeActionPriority Priority => CodeActionPriority.Medium;
Expand Down Expand Up @@ -356,16 +364,26 @@ public static CodeAction Create(string title, ImmutableArray<CodeAction> nestedA
return new CodeActionWithNestedActions(title, nestedActions, isInlinable);
}

/// <summary>
/// Creates a code action provider name.
/// </summary>
/// <param name="provider">Specific Code Action Provider (<see cref="CodeRefactoringProvider"/> or <see cref="CodeRefactoringProvider"/> implementation) of the <see cref="CodeAction"/> group.</param>
/// <param name="implementation">Specific Code Action implementation from the Code Action Provider.</param>
public static string CreateProviderName(string provider, string implementation)
=> string.Join("|", provider, implementation);

internal abstract class SimpleCodeAction : CodeAction
{
public SimpleCodeAction(string title, string? equivalenceKey)
public SimpleCodeAction(string title, string? equivalenceKey, string? providerName = null)
{
Title = title;
EquivalenceKey = equivalenceKey;
ProviderName = providerName;
}

public sealed override string Title { get; }
public sealed override string? EquivalenceKey { get; }
public sealed override string? ProviderName { get; }
}

internal class CodeActionWithNestedActions : SimpleCodeAction
Expand Down Expand Up @@ -410,8 +428,8 @@ internal class DocumentChangeAction : SimpleCodeAction
{
private readonly Func<CancellationToken, Task<Document>> _createChangedDocument;

public DocumentChangeAction(string title, Func<CancellationToken, Task<Document>> createChangedDocument, string? equivalenceKey = null)
: base(title, equivalenceKey)
public DocumentChangeAction(string title, Func<CancellationToken, Task<Document>> createChangedDocument, string? equivalenceKey = null, string? providerName = null)
: base(title, equivalenceKey, providerName)
{
_createChangedDocument = createChangedDocument;
}
Expand Down