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

TanayParikh · 2020-10-27T18:17:03Z

This is an extremely early PR. I just wanted to confirm the general methodology is sound for adding the provider/implementation names to the CodeAction.Data payload.

This payload will be used by razor, to determine the "type" of a code action in a language agnostic way. Currently we use the English CodeAction.Title which won't work for other languages.

If the approach is fine, I'll expand to a greater set of code action providers.

For: https://github.com/dotnet/aspnetcore/issues/25939

TanayParikh · 2020-10-27T18:22:57Z

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveData.cs

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


Mirroring format of above UniqueIdentifier

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.

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.

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.

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

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?

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

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?

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?

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

TanayParikh · 2020-10-27T19:24:08Z

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveData.cs

+        /// name of top level code action provider + '|' + name of code action provider implementation
+        /// e.g. 'Add Await Code Action Provider | GetTitleWithConfigureAwait'
+        /// </remarks>
+        public string ProviderName { get; }


Needs to be added to published API.

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.

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?

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.

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

Razor sends a textDocument/codeAction request to the Roslyn CodeActionsHandler.

Roslyn runs its analyzers & [NEW in this PR] annotates the response CodeAction.CodeActionResolveData with the ProviderName property.

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.

[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.

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

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

src/Workspaces/Core/Portable/CodeActions/CodeAction.cs

TanayParikh · 2020-11-13T04:58:41Z

Question regarding the general design.

For some code actions we "bubble up" data like the provider name => Provider => to SimpleCodeAction => to the base CodeAction.

For other code actions, we inherit directly from the base CodeAction (ex. ImplementInterfaceCodeAction). In this case we'd want to override the virtual ProviderName we've added.

I'm just wondering if we want to keep this distinction. Personally, I'd prefer to override the ProviderName property wherever we'd like to annotate a code action with the provider name, instead of having to bubble up through various constructors. I believe this would make for a neater "annotation" mechanism. Just wanted to get your thoughts.

@dibarbet @allisonchou

TanayParikh · 2020-11-13T22:22:27Z

/ping; hoping to get this unblocked and merged soon :)

allisonchou · 2020-11-14T02:36:50Z

Personally, I'd prefer to override the ProviderName property wherever we'd like to annotate a code action with the provider name, instead of having to bubble up through various constructors. I believe this would make for a neater "annotation" mechanism.

Hmm, I think either approach seems ok here from my perspective. I think bubbling up actually looks neater to me, but overriding would definitely be more consistent.
Not sure if @mavasani might have an opinion on this?

mavasani · 2020-11-16T20:59:38Z

Hmm, I think either approach seems ok here from my perspective. I think bubbling up actually looks neater to me, but overriding would definitely be more consistent.
Not sure if @mavasani might have an opinion on this?

Same here - I think all the properties on code action are similar - you can either define your own custom code action type and override the virtual properties OR pass them via the constructor.

TanayParikh · 2020-11-16T21:08:03Z

Hmm, I think either approach seems ok here from my perspective. I think bubbling up actually looks neater to me, but overriding would definitely be more consistent.
Not sure if @mavasani might have an opinion on this?

Same here - I think all the properties on code action are similar - you can either define your own custom code action type and override the virtual properties OR pass them via the constructor.

Thanks for clarifying!

mavasani · 2020-11-16T21:18:54Z

src/Workspaces/Core/Portable/CodeActions/CodeAction.cs

+        /// <remarks>
+        /// e.g. 'Add Await Code Action Provider'
+        /// </remarks>
+        internal virtual string? ProviderName => null;


We may want to consider exposing this property as a public API - this is something that can be made part of the code action telemetry and we can resolve the long outstanding issue #4919. @allisonchou do you mind bringing it to the design meeting?

ryanbrandenburg · 2020-11-30T22:05:28Z

Design Meeting Notes: We should have a presentation from a knowledgeable Razor person about why we need this and what the alternatives (if any) are. Hopefully we can make that happen at next weeks design meetings.

TanayParikh added 3 commits October 9, 2020 16:31

WIP

0512fd5

Merge branch 'master' into dev/taparik/addCodeActionProviderName

02a8da8

Provider, Implementation

a637d17

TanayParikh requested review from dibarbet and allisonchou October 27, 2020 18:17

Dotnet-GitSync-Bot added the Area-IDE label Oct 27, 2020

TanayParikh commented Oct 27, 2020

View reviewed changes

Merge branch 'master' into dev/taparik/addCodeActionProviderName

b9cdf56

TanayParikh commented Oct 27, 2020

View reviewed changes

src/Workspaces/Core/Portable/CodeActions/CodeAction.cs Outdated Show resolved Hide resolved

TanayParikh commented Oct 27, 2020

View reviewed changes

src/Workspaces/Core/Portable/CodeActions/CodeAction.cs Outdated Show resolved Hide resolved

runfoapp bot mentioned this pull request Oct 29, 2020

Analyzer diagnostics cause CI to fail #49021

Closed

TanayParikh added 4 commits November 12, 2020 12:41

Merge branch 'master' into dev/taparik/addCodeActionProviderName

3a09488

Feedback

a52b3c0

Fix missing func

f477ac0

Merge branch 'master' into dev/taparik/addCodeActionProviderName

337d594

runfoapp bot mentioned this pull request Nov 14, 2020

Compilation errors cause CI to fail #49020

Closed

TanayParikh added 3 commits November 16, 2020 13:31

WIP

a16be50

Merge branch 'master' into dev/taparik/addCodeActionProviderName

d47cd0b

External Access

c109013

mavasani reviewed Nov 16, 2020

View reviewed changes

TanayParikh closed this Feb 19, 2021

TanayParikh deleted the dev/taparik/addCodeActionProviderName branch February 19, 2021 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TanayParikh commented Oct 27, 2020

TanayParikh Oct 27, 2020

allisonchou Oct 27, 2020

TanayParikh Oct 28, 2020

allisonchou Oct 28, 2020

TanayParikh Oct 28, 2020

mavasani Oct 29, 2020

TanayParikh Nov 13, 2020 •

edited

Loading

allisonchou Nov 14, 2020 •

edited

Loading

TanayParikh Nov 16, 2020

allisonchou Nov 17, 2020 •

edited

Loading

TanayParikh Oct 27, 2020

allisonchou Oct 28, 2020

TanayParikh Nov 12, 2020

allisonchou Nov 14, 2020

TanayParikh Nov 16, 2020

allisonchou Nov 17, 2020

TanayParikh commented Nov 13, 2020 •

edited

Loading

TanayParikh commented Nov 13, 2020

allisonchou commented Nov 14, 2020

mavasani commented Nov 16, 2020

TanayParikh commented Nov 16, 2020

mavasani Nov 16, 2020

ryanbrandenburg commented Nov 30, 2020

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

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

Conversation

TanayParikh commented Oct 27, 2020

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

TanayParikh Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

allisonchou Nov 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonchou Nov 17, 2020 • edited Loading

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

TanayParikh commented Nov 13, 2020 • edited Loading

TanayParikh commented Nov 13, 2020

allisonchou commented Nov 14, 2020

mavasani commented Nov 16, 2020

TanayParikh commented Nov 16, 2020

Choose a reason for hiding this comment

ryanbrandenburg commented Nov 30, 2020

TanayParikh Nov 13, 2020 •

edited

Loading

allisonchou Nov 14, 2020 •

edited

Loading

allisonchou Nov 17, 2020 •

edited

Loading

TanayParikh commented Nov 13, 2020 •

edited

Loading