Move code actions to LSP by allisonchou · Pull Request #45572 · dotnet/roslyn

allisonchou · 2020-06-30T22:57:05Z

Moves code actions over to LSP.

How it works:
There are three main handlers involved in processing LSP code actions: CodeActionsHandler, CodeActionResolveHandler, and RunCodeActionsHandler.

CodeActionsHandler: Deals with the initial code actions request from LSP. Returns an array of VSCodeActions generated from Roslyn code actions. The Edit and Command properties in the VSCodeAction are left blank, since those are populated in the CodeActionResolveHandler. We avoid filling out those two properties so we don't need to a bunch of work unless the user actually requests it.
CodeActionResolveHandler: Invoked when a user hovers over a code action. We leave the bulk of the processing to this handler. Fills out either the Edit or Command properties (can be both as well, but due to the LSP bug below this isn't currently possible). If we send back an Edit, the edit is processed and executed on the client/LSP side. If we send back a Command, the command will eventually make its way to the RunCodeActionsHandler and run from there.
RunCodeActionsHandler: Invoked when Command property is filled out. We run actions as commands if they can't be executed as WorkspaceEdits for some reason (i.e. if not an ApplyChangesOperation, or due to the LSP bug below). These commands are executed on the local Roslyn side. The general ExecuteWorkspaceCommandHandler is called first. The general handler then calls into our code-action specific RunCodeActionsHandler.

Code actions that add a document must currently be executed on the server side due to this LSP bug: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1147293/

eta: thanks to David for all the help on this PR!

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

…eActionSupport

dibarbet

Have some high level comments I want to get out of the way before diving too deep.

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

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

dibarbet · 2020-07-01T00:13:07Z

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

+            var runAsCommand = false;
+
+            var applyChangesOperations = operations.Where(operation => operation is ApplyChangesOperation);
+            if (applyChangesOperations.Any())


I wonder if this should be all, or more specific filtering.

If we have an ApplyChangesOperation + DocumentNavigation operation, then it should be safe to do this (we can't tell the client to navigate anywhere anyway, so we can't do anything but drop the document navigation operation). But if there is an ApplyChangesOperation + some other actual modification operation, then we might get into a corrupt state b/c we applied half of the code action.

Not sure the best way to do this, thoughts @CyrusNajmabadi ?

dibarbet · 2020-07-01T00:15:05Z

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

+            }
+
+            // Set up to run as command on the server instead of using WorkspaceEdits.
+            var commandOperations = operations.All(operation => !(operation is ApplyChangesOperation));


Ah I see, we apply any non ApplyChangesOperation as a command. Then I think I'm only worried about the order. The LSP spec states that commands are applied after workspace edits. But if the code action specifies the non-ApplyChangesOperation to run before the ApplyChangesOperation, the code action could get corrupted if order matters.

This also could be problematic - I see in the execute code actions request, we just find the one with the right title. It will end up executing all of the operations, so if there are workspace edits + a command, it will execute the edits twice (once on client workspace edits, once on server during RunCodeActions ApplyChangesOperation)

Do you think it would be best if we were to just file a Roslyn bug on this?
I think ideally, the solution would be to include a check in RunCodeActionsHandler to see whether we are executing an ApplyChangesOperation so that we don't execute those kinds of operations twice. But we can't currently do that since some ApplyChangesOperations are applied as commands due to this bug on the LSP side. Once the LSP bug is fixed, I think we can address the concerns you mentioned.

The current approach is: if we have a mix of ApplyChangesOperations and other operations, only the ApplyChangesOperations are executed. But if we have only non-ApplyChangesOperations, then we can execute them (as a command). This isn't ideal, but it prevents workspace edits from potentially executing twice.

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

dibarbet · 2020-07-01T00:38:40Z

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

+            return codeAction;
+
+            // Local functions
+            static async Task<TextDocumentEdit[]> ComputeWorkspaceEdits(


I think it would be cleaner for this to be something like TryComputeWorkspaceEdits that returns false when unable to compute them and a command has to be used instead.

Can that be done when the function is async? If we return a bool, then we would need to have an out parameter for the workspace edits, but I don't think we can use out parameters in async methods. I guess we could also return a tuple of (bool, WorkspaceEdits) but that seems a bit messy.

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

dibarbet · 2020-07-01T00:46:12Z

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

+                    Kind = codeActionKind,
+                    Diagnostics = request.Context.Diagnostics,
+                    Children = nestedActions.ToArray(),
+                    Data = new CodeActionResolveData { CodeActionParams = request, DistinctTitle = parentTitle + codeAction.Title }


are there cases where the nested code action titles conflict between two different nested groups? If so, I wonder if we could use the codeactionprovider type name + title instead of having to specify the entire tree (unless the title can be the same within a nested group)

we should also doc on the CodeActionResolveData what goes inside the DistinctTitle

Hmm, I think this would fail with 'Suppress or Configure issues' if we had two diagnostics that we could suppress/configure, e.g. we would have "ConfigureSeverityCodeFixProvider" + "None" twice in this case.

allisonchou · 2020-07-02T20:04:14Z

@dibarbet I think I've addressed all your concerns via code changes or by responding to the initial feedback.
Just want to confirm with Manish that the resource string can be moved. Also, I'm not completely sure but I think a bug may need to be filed to address your feedback here: #45572 (comment).

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

CyrusNajmabadi · 2020-07-02T20:07:47Z

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

+        }
+
+        public override async Task<LSP.VSCodeAction> HandleRequestAsync(
+            LSP.VSCodeAction codeAction,


i'm a little confused as to the semantics here. it takes in a code action... and returns a code action? can you clarify the concept here?

Per your suggestion I've added some details in the original post of this PR, as well as added more comments in the code.
The CodeActionsResolveHandler fills out the VSCodeAction's Edit and/or Command properties. The handler is only invoked when the user requests the code action (e.g. by hovering), so it saves unnecessary computation.

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

CyrusNajmabadi · 2020-07-02T20:08:42Z

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

+                return codeAction;
+            }
+
+            var codeActionToResolve = GetCodeActionToResolve(data.DistinctTitle, codeActions);


so to resolve a code action, we have to compute all the code actions again? seems costly. can/should we store/save things locally across calls to speed this up?

One suggestion I would have is to do the caching in a follow-up pr. This matches the current behavior in nexus (always recompute) so we're not regressing anything.

This pr is already fairly big, and caching I think will be a little complex and would prefer to review separately.

i am ok with that decision for this PR. :)

Opened #45727 to track this.

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

CyrusNajmabadi · 2020-07-06T08:15:32Z

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

-                                codeRefactorings.SelectMany(r => r.CodeActions.Select(ca => ca.action)));
+            // Special case (also dealt with specially in local Roslyn): 
+            // If we have configure/suppress code actions, combine them under one top-level code action.
+            var configureSuppressActions = codeFixes.Where(a => a.Action is AbstractConfigurationActionWithNestedActions);


Suggested change

var configureSuppressActions = codeFixes.Where(a => a.Action is AbstractConfigurationActionWithNestedActions);

var configureSuppressActions = codeFixes.OfType<AbstractConfigurationActionWithNestedActions>();

Will this work since we wanted to check the type of a.Action and not just a?

CyrusNajmabadi · 2020-07-06T08:16:12Z

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

-            // Flatten out the nested codeactions.
-            var nestedCodeActions = codeActions.Where(c => c is CodeAction.CodeActionWithNestedActions nc && nc.IsInlinable).SelectMany(nc => nc.NestedCodeActions);
-            codeActions = codeActions.Where(c => !(c is CodeAction.CodeActionWithNestedActions)).Concat(nestedCodeActions);
+            return results.ToArray();


this also feels like it' reimplementing logic that must exist elsewhere. that doesn't seem desirable. i would far prefer to have a single location where we sort/order these things, and then use it from VS and LSP.

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

CyrusNajmabadi · 2020-07-06T08:20:27Z

Done with pass. I have some moderate concerns, but nothing critically blocking. We could create work items to track some things if they are too large for this PR. i have a general deep concern that we're being too loosy goosy with just papering over things that seem like they would be bugs, and that that could lead to a very frustrating user experience where things don't work and users don't know why.

allisonchou · 2020-07-07T03:29:38Z

Opened #45726, #45727, and #45623 to track future work items for LSP code actions on the Roslyn side.

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

dibarbet · 2020-07-08T20:05:51Z

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

+                () => codeRefactoringService.GetRefactoringsAsync(document, textSpan, cancellationToken));
+
+            await Task.WhenAll(codeFixCollectionsTask, codeRefactoringsTask).ConfigureAwait(false);
+            return (await codeFixCollectionsTask.ConfigureAwait(false), await codeRefactoringsTask.ConfigureAwait(false));


I think you don't need WhenAll here since you await the completion of the tasks before returning.

I discussed this with Cyrus recently and was told that it would probably be good to keep the WhenAll in here. I think the logic was that if both codeFixCollectionsTask and codeRefactoringsTask error or cancel, then using WhenAll allows us to retrieve both messages, while if we awaited them separately then we might only get one.

Allison is correct. It's an extremely subtle thing and easy to make a mistake around. However, we can lose exceptions if we kick off tasks and then await each one serially. then WhenAll ensures we properly see all of this, and doesn't hurt anything in terms of perf. i.e. the later awaits will be no-ops because they will precheck if the tasks are complete and will grab the values if so.

Ah interesting, that behavior makes sense now that I think about it. Thanks for the explanation!

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

dibarbet · 2020-07-08T20:24:50Z

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

+
+                    var changedDocuments = projectChanges.SelectMany(pc => pc.GetChangedDocuments());
+                    var changedAnalyzerConfigDocuments = projectChanges.SelectMany(pc => pc.GetChangedAnalyzerConfigDocuments());
+                    var changedAdditionalDocuments = projectChanges.SelectMany(pc => pc.GetChangedAdditionalDocuments());


I think adding / removing documents via LSP workspace edits should be supported, but there are bugs in the LSP client preventing it from working

@allisonchou I think right now if we have to execute these actions on the server, let's not even implement the bit turning them into LSP edits. Just do a simple check to see if there are any changes at all outside the current document and return a command to execute on the server.

Once they fix the bugs we can followup and do the actual implementation to ensure add / remove documents works correctly.

dibarbet · 2020-07-08T20:26:41Z

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

+                    // Changed analyzer config documents
+                    // We won't get any results until https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1147293/
+                    // is fixed.
+                    await AddTextDocumentEdits(


See above, lets not do this in this PR. Can followup once the LSP client bug is fixed.

The issue with changed documents should now be fixed due to a change on the Liveshare side (hooray).
There's a bug on the Roslyn side when modifying editorconfig documents (#44331 is the PR fix), but it should hopefully be merged in soon.
Still pending is the work to support adding documents via code actions in LSP.

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

src/Features/LanguageServer/ProtocolUnitTests/CodeActions/CodeActionResolveTests.cs

src/Features/LanguageServer/ProtocolUnitTests/CodeActions/CodeActionsTests.cs

src/VisualStudio/LiveShare/Impl/Microsoft.VisualStudio.LanguageServices.LiveShare.csproj

allisonchou · 2020-07-10T00:58:12Z

It looks like modifying existing documents is now supported after a bug fix on the Liveshare side. Adding new documents is still not supported.
There's currently a bug that affects modifying .editorconfig documents on the Roslyn side, but this should be fixed once #44331 is merged in.

allisonchou · 2020-07-10T20:24:19Z

@CyrusNajmabadi @dibarbet Would it be possible to get another look at this PR? I believe all feedback has been addressed.

Outstanding bugs:

Adding/removing documents is not supported: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1147293/
Modifying an editorconfig document will auto-generate two unnecessary files on the client side. I talked to Manish, and this bug should be fixed once this PR is merged in: Move C# editorconfig diagnostic support API to CompilationOptions, instead of SyntaxTree #44331
Code actions with underscores in their title are not formatted correctly: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1150812/

Currently, the future work items on the Roslyn side are:

Make code action ordering logic uniform between local and LSP experiences #45726 (high-pri): Make the logic between LSP/local Roslyn uniform - specifically, extract out code in SuggestedActionSource to be used in both
[LSP] Enable caching in code action logic #45727: Enable caching in code action logic

dibarbet

Looks mostly good to me - let's definitely get #45726 done though. Ideally there shouldn't really be much logic in these methods other than converting roslyn code actions to LSP code actions / edits.

allisonchou added 11 commits June 17, 2020 12:55

Update package versions

d04f555

Preliminary implementation - need to figure out exception

7c90bdb

Add new files

ac2d87d

Nested action support

b7ddd77

Suppress or configure action support

086573d

Reverting code

45ef245

Partial added/analzyer config implementation

8ca1743

Further changes

8b715bc

Attempting to fix command bug

8e7c2ba

Partial command changes

b0fe7be

Add tests and resource files

92a5fb7

allisonchou added the Area-IDE label Jun 30, 2020

allisonchou requested review from CyrusNajmabadi and dibarbet June 30, 2020 22:57

allisonchou requested review from a team as code owners June 30, 2020 22:57

allisonchou commented Jun 30, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionsHandler.cs Outdated Show resolved Hide resolved

Merge branch 'master-vs-deps' of github.com:dotnet/roslyn into LSPCod…

ac1771d

…eActionSupport

dibarbet reviewed Jul 1, 2020

View reviewed changes

Code review feedback

a9e73e5

allisonchou requested a review from dibarbet July 2, 2020 19:59

CyrusNajmabadi reviewed Jul 2, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveData.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 2, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveData.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 2, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveData.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 2, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveData.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 2, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveHandler.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 2, 2020

View reviewed changes

allisonchou mentioned this pull request Jul 2, 2020

[LSP] Allow more code actions to execute on client via WorkspaceEdit once LSP bug is fixed #45623

Closed

CyrusNajmabadi reviewed Jul 6, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveHandler.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 6, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveHandler.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 6, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveHandler.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 6, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveHandler.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 6, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveHandler.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 6, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/RunCodeActionsHandler.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 6, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/RunCodeActionsHandler.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 6, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/RunCodeActionsHandler.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 6, 2020

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/CodeActions/RunCodeActionsHandler.cs Outdated Show resolved Hide resolved

allisonchou added 2 commits July 6, 2020 15:31

Partial code review feedback

3e8ab95

Code review feedback pt2

e91a566

This was referenced Jul 7, 2020

Make code action ordering logic uniform between local and LSP experiences #45726

Closed

[LSP] Enable caching in code action logic #45727

Closed

allisonchou requested a review from CyrusNajmabadi July 8, 2020 00:05

dibarbet reviewed Jul 8, 2020

View reviewed changes

Code review feedback

e7c3dcd

If document is removed, execute as command temporarily

458cce6

Remove unnecessary Task.Run

2d9547d

dibarbet approved these changes Jul 10, 2020

View reviewed changes

Resolve merge conflicts

f723cfc

allisonchou merged commit e0604a4 into dotnet:master-vs-deps Jul 13, 2020

ghost added this to the Next milestone Jul 13, 2020

JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020

	var configureSuppressActions = codeFixes.Where(a => a.Action is AbstractConfigurationActionWithNestedActions);
	var configureSuppressActions = codeFixes.OfType<AbstractConfigurationActionWithNestedActions>();

Conversation

allisonchou commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dibarbet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allisonchou commented Jul 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Jul 6, 2020

Uh oh!

allisonchou commented Jul 7, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allisonchou commented Jun 30, 2020 •

edited

Loading