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.

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

Support to resolve code actions #106410

Closed
jrieken opened this issue Sep 10, 2020 · 20 comments · Fixed by #106856
Closed

Support to resolve code actions #106410

jrieken opened this issue Sep 10, 2020 · 20 comments · Fixed by #106856
Assignees
Labels
api-finalization api-proposal editor-code-actions Editor inplace actions (Ctrl + .) feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded

Comments

@jrieken
Copy link
Member

jrieken commented Sep 10, 2020

Todays API for code actions wants all actions to be returned fully resolved, e.g the command or edit must be set. For some code action providers that's not easy and the usual "workaround" is to move the heavy lifting into the command (instead of using workspace edits). However, with commands it's harder to control the overall experience, like undo-stack or previewing changes.

The proposed change below adds a resolveCodeAction function to code action provider which the editor would call before applying a code action.

export interface CodeActionProvider<T extends CodeAction> {
  provideCodeActions(document: TextDocument, range: Range | Selection, context: CodeActionContext, token: CancellationToken): ProviderResult<(Command | T)[]>;

  // NEW - allow to fill in things like edit or command
  resolveCodeAction(action: T, token: CancellationToken): ProviderResult<T>;
}

fyi @dbaeumer since this exists in LSP already

@jrieken jrieken added api-proposal editor-code-actions Editor inplace actions (Ctrl + .) labels Sep 10, 2020
@jrieken jrieken added this to the September 2020 milestone Sep 10, 2020
@jrieken jrieken added the feature-request Request for new features or functionality label Sep 10, 2020
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 10, 2020

Would resolve only be called when the user selects a code action? Or would it behave like for suggestions where we try resolving entries as you arrow through the code action list?

@jrieken
Copy link
Member Author

jrieken commented Sep 10, 2020

Right now I would say only when selecting a code action. We have all information to render a list of code actions but for the future. Who knows... VS has a feature where focusing a code action already shows a hover with the diff of the changes...

@mjbvz
Copy link
Collaborator

mjbvz commented Sep 10, 2020

That sounds good. The one complication is that we currently use native context menus for code actions. I'll have to check to see if we can detect when the user changes the active item in a native context menu

Related: I've wanted to switch to use UI for code actions for a long time but have never had the time to look into this

@dbaeumer
Copy link
Member

Will we always call resolve even if command or edit ?

@jrieken
Copy link
Member Author

jrieken commented Sep 11, 2020

No, I'd say only when needed, e.g when not having an edit or command. It's a little ambiguous because edit or command can be set...

@dbaeumer
Copy link
Member

I am in favor of this.

@jrieken
Copy link
Member Author

jrieken commented Sep 16, 2020

Work on this has started in #106856. I have implemented the following

  • old school providers that return commands, not code actions, cannot use resolve
  • resolve only allows to fill in the edit. A command is easy to provide upfront and we actually want authors to provide edits

@jrieken
Copy link
Member Author

jrieken commented Sep 18, 2020

@dbaeumer the proposal is now in master and ready to use

@testforstephen
Copy link

A good proposal. I see the Java language server can benefit from the lazy resolve idea.

Currently when editing a big Java file (e.g. 5k+ lines), it will cause the Java language server to take much CPU time to calculate the workspace edits for all code action proposals such as quickfix, refactoring, source action. For some resource-constrained machines, this affects the processing of other requests. In particular, it may cause the codeCompletion handler waiting for resources to run, and slow down code completion performance.

@jrieken when it's ready in insider, i'd like to have a try to see how it goes.

@mjbvz
Copy link
Collaborator

mjbvz commented Sep 19, 2020

@jrieken I adopted this for JS/TS refectorings in 5a7d0a1 (also made a small fix to the VS Code implementation)

We still need to use a command to trigger a rename after extracting a method or constant but this new API simplified the implementation.

@jrieken
Copy link
Member Author

jrieken commented Sep 21, 2020

Thanks @mjbvz

@dbaeumer
Copy link
Member

@jrieken thanks!

jrieken added a commit that referenced this issue Sep 25, 2020
@jrieken
Copy link
Member Author

jrieken commented Sep 25, 2020

This is now finalized

@jrieken jrieken closed this as completed Sep 25, 2020
@dbaeumer
Copy link
Member

@jrieken just to be sure. It is out of proposed state. Right ?

@jrieken
Copy link
Member Author

jrieken commented Sep 25, 2020

Yeah - as final as it gets

@dbaeumer
Copy link
Member

Great!

@jrieken jrieken added the verification-needed Verification of issue is requested label Sep 29, 2020
@testforstephen
Copy link

A quick question here.

I see codeAction/resolve is already added into lsp 3.16. Does VS Code client already adopt the spec? // cc: @jrieken @dbaeumer

@jrieken
Copy link
Member Author

jrieken commented Sep 30, 2020

yes. it is in current insiders and next stable

@testforstephen
Copy link

thank you @jrieken.

I tried the latest insider at 09-29, looks like the initialize request from vscode client doesn't carry the clientCapability for textDocument.codeAction.resolveSupport.

Below is the codeAction capability from LSP initialize params. It doesn't declare the codeAction/resolve support.

{
  "capabilities": {
   ...
   "textDocument": {
         "codeAction": {
                "dynamicRegistration": true,
                "isPreferredSupport": true,
                "codeActionLiteralSupport": {
                    "codeActionKind": {
                        "valueSet": [
                            "",
                            "quickfix",
                            "refactor",
                            "refactor.extract",
                            "refactor.inline",
                            "refactor.rewrite",
                            "source",
                            "source.organizeImports"
                        ]
                    }
                }
            },

@jrieken
Copy link
Member Author

jrieken commented Sep 30, 2020

🤷 idk - that's LSP, not VS Code. Ping @dbaeumer for what version of the LSP client supporting this

@mjbvz mjbvz added the verified Verification succeeded label Sep 30, 2020
mjbvz added a commit to mjbvz/vscode that referenced this issue Sep 30, 2020
@jrieken jrieken added the on-release-notes Issue/pull request mentioned in release notes label Oct 1, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-finalization api-proposal editor-code-actions Editor inplace actions (Ctrl + .) feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@jrieken @dbaeumer @mjbvz @testforstephen and others