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

x/tools/gopls: provide a way to run ExecuteCommand features in one step #40438

Open
leitzler opened this issue Jul 27, 2020 · 9 comments
Open

x/tools/gopls: provide a way to run ExecuteCommand features in one step #40438

leitzler opened this issue Jul 27, 2020 · 9 comments

Comments

@leitzler
Copy link
Contributor

@leitzler leitzler commented Jul 27, 2020

What version of Go are you using (go version)?

golang.org/x/tools v0.0.0-20200721032237-77f530d86f9a
golang.org/x/tools/gopls v0.0.0-20200721032237-77f530d86f9a

I was adding fillstruct support to govim and they way gopls currently works is that it require a CodeAction/ExecuteCommand/ApplyEdit dance.

From a user point of view I see fillstruct as a direct request, initiated when the user already have the cursor placed on a non-filled struct identifier and the sole intention is to fill it.

It would be nice to be able to call ExecuteCommand from the client w/o a CodeAction request in before, but as discussed on Slack the different commands (and their arguments) aren't stable enough to be made public.
Example of a fillstruct command as returned by CodeAction:

Command:     &protocol.Command{
    Title:     "Fill foo with default values",
    Command:   "fill_struct",
    Arguments: {
        "{\"uri\":\"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.lmXRD9mP/main.go\",\"range\":{\"start\":{\"line\":7,\"character\":6},\"end\":{\"line\":7,\"character\":6}}}",
    },
},

As long as there is only one single (fillstruct) Command in the response, govim can automatically do another roundtrip and call ExecuteCommand with that Command. But if there are more than one Command in the CodaAction response (or the command isn't a fillstruct) there is currently no good way for the client to tell them apart.

Neither Command name (e.g. "fill_struct") or Arguments are specified anywhere so the client cannot know if/which command that belong to the request. Title is a dynamic string that contain the struct identifier. Using that field would require string matching (and unique struct identifiers on each line).

AFAIK other commands, such as "Extract variable" or "Extract function", are falls into the same category of user-invoked commands with a clear intent.

From my point of view gopls needs the following to be able to provide one step commands (I could obviously have missed other/better ways).

  • Use explicit kinds for all CodeActions with Commands, like refactor.rewrite.fillstruct instead of just the base kind refactor.rewrite so that it is possible to request a single kind of code action commands.
  • Allow a more narrow range as part of the CodeAction request (see CL244519). fillstruct operates on the whole line regardless of range specified in the CodeAction parameters.
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 27, 2020

Change https://golang.org/cl/244519 mentions this issue: internal/lsp: allow narrower scope for convenience CodeActions

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jul 27, 2020

Use explicit kinds for all CodeActions with Commands, like refactor.rewrite.fillstruct instead of just the base kind refactor.rewrite so that it is possible to request a single kind of code action commands.

We do still need to look into how these code action kinds work, and maybe we will use them. Still, I think that a client executing commands directly will always be prone to errors and incompatibilities, at least until the protocol has a way to tell the client which arguments should be provided to which commands.

I'm not sure if this is something that the protocol will support, but microsoft/language-server-protocol#642 and microsoft/language-server-protocol#430 are relevant. It may be worth filing an issue for this specifically, as I'm sure that every language has to deal with this issue.

Unless something changes, the only "correct" way to call fillstruct will be to first call codeAction, as that's the only correct way of getting the command and checking if it's available to call.

gopherbot pushed a commit to golang/tools that referenced this issue Jul 27, 2020
Code actions that apply convenience fixes were filtered by the start
line so it wasn't possible to narrow the scope to a specific range.

This change allows clients to send a specific range (or cursor position)
to filter all fixes where the range doesn't intersect with the provided
range. It also widens the diagnostic returned by fillstruct analysis.

The idea is to provide a way to narrow the scope without breaking
clients that do want to ask for code actions using the entire line.

Updates golang/go#40438

Change-Id: Ifd984a092a4a3bf0b3a2a5426d3e65023ba4eebc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244519
Run-TryBot: Pontus Leitzler <leitzler@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@leitzler
Copy link
Contributor Author

@leitzler leitzler commented Jul 28, 2020

We do still need to look into how these code action kinds work, and maybe we will use them. Still, I think that a client executing commands directly will always be prone to errors and incompatibilities, at least until the protocol has a way to tell the client which arguments should be provided to which commands.

Yes, the purpose here would be to provide a way for the client to ask for fillstruct (or any of the others) explicitly in the CodeAction request.

I think that would be needed in the case of refactor.extract already, and refactor.rewrite if/when other rewrites than fillstruct is added.

If the user want to extract to function they call CodeAction with only set to refactor.extract today and gets two CodeAction responses, one for "Extract to variable" and one for "Extract to function". They can't be told apart w/o looking at the Title.

That's why I suggest adding refactor.extract.function in the CodeAction request so the response can be used to call ExecuteCommand.

@leitzler
Copy link
Contributor Author

@leitzler leitzler commented Aug 25, 2020

To clarify the intent here; It doesn't have to be client crafted calls to ExecuteCommand. Requiring another LSP roundtrip (like CodeLens or CodeAction) is also a way to implement it in a way so it acts as one step for the user.

Heschi had a suggestion on the latest tools call, for a potential way forward. That would be to add existing CodeLenses as CodeActions as well (opt-in, disabled by default). It would require each command to have a unique kind that can be sent from the client asOnly: field, e.g. refactor.rewrite.fillstruct instead of just the base kind refactor.rewrite.

Adding unique kinds to gopls might not be as intrusive as it can seem:

  • The LSP specification about how the server announce the capabilities has a comment about it. All individual kinds don't have to be announced separately, it can continue to just announce base kinds:

/**
* CodeActionKinds that this server may return.
*
* The list of kinds may be generic, such as CodeActionKind.Refactor, or the server
* may list out every specific kind they provide.
*/
codeActionKinds?: CodeActionKind[];

It could be tricky to fit some of the existing CodeLenses into a CodeAction basekind. I read somewhere (but lost the link) that the source base kind doesn't show up as a lightbulb in VSCode (since it "applies to the entire file") for example, but things like "go generate" isn't a perfect fit for that description.

According to LSP specification the entire set of kinds ".. is open and client needs to announce the kinds it support..." so it doesn't really prevent gopls from using a new base kind.

Thoughts?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 26, 2020

That sounds reasonable enough to me, but it's definitely not a priority for us, so this would have to be contributor-driven. We're certainly open to adding more granular code action kinds.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 3, 2020

Change https://golang.org/cl/259377 mentions this issue: gopls: add "go test" code action

gopherbot pushed a commit to golang/tools that referenced this issue Oct 13, 2020
This change adds "go test" as a code action and also introduce the
concept of explicit code actions, i.e. code actions that aren't returned
to the client unless it explicitly ask for it.

The purpose is to be able to have a mechanism that allows users to
execute a specific command in one shot, and future CL:s will add more of
the existing code lenses as explicit code actions. Code lenses can't be
used directly since they lack the range/kind combo to make them unique.

Updates golang/go#40438

Change-Id: I245df4e26c9e02e529662181e2c1bc44f999e101
Reviewed-on: https://go-review.googlesource.com/c/tools/+/259377
Trust: Rebecca Stambler <rstambler@golang.org>
Trust: Peter Weinberger <pjw@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre added this to the gopls/unplanned milestone Oct 21, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 8, 2021

Change https://golang.org/cl/290111 mentions this issue: internal/lsp: switch to the new command API

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 8, 2021

Change https://golang.org/cl/290190 mentions this issue: gopls/doc: add argument documentation for commands

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 8, 2021

Change https://golang.org/cl/289489 mentions this issue: internal/lsp/command: add an interface for workspace/executeCommand

gopherbot pushed a commit to golang/tools that referenced this issue Feb 9, 2021
This CL lays the groundwork for future refactoring, by defining a formal
(Go) interface for the set of commands provided by gopls in the
workspace/executeCommand RPC. It then creates some boilerplate bindings
via code generation.

The intent is to, first of all, clean up our current usage of commands.
Currently the 'specification' of a command is really split across
internal/lsp/command.go, internal/lsp/source/command.go, and
internal/lsp/source/code_lens.go. Changing a command signature might
require altering all three of those files, and it's easy to get wrong.

But also, we'd like to eventually be able to tell plugin authors that
they can call our commands in an ad-hoc manner (meaning with arguments
that they assign, rather than extract from a code lens). In order to do
that, we need to be able to generate documentation for the command
signature, and should also stop using positional arguments.  This CL
aims to solve that as well, by providing a commandmeta package that can
be used for document generation.

For golang/go#40438

Change-Id: I0d29de044e107d6e7b267f340879a5282f0b4944
Reviewed-on: https://go-review.googlesource.com/c/tools/+/289489
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 9, 2021
Fully switch to the new generated command API, and remove the old
dynamic command configuration.

This involved several steps:
 + Switch the command dispatch in internal/lsp/command.go to go through
   the command package. This means that all commands must now use the new
   signature.
 + Update commandHandler to use the new command signatures.
 + Fix some errors discovered in the command interface now that we're
   actually using it.
 + Regenerate bindings.
 + Update all code lens and suggested fixes to new the new command
   constructors.
 + Generate values in the command package to hold command names and the
   full set of commands, so that they may be referenced by name.
 + Update any references to command names to use the command package.
 + Delete command metadata from the source package. Rename command.go to
   fix.go.
 + Update lsp tests to execute commands directly rather than use an
   internal API. This involved a bit of hackery to collect the edits.
 + Update document generation to use command metadata. Documenting the
   arguments is left to a later CL.
 + Various small fixes related to the above.

This change is intended to be invisible to users. We have changed the
command signatures, but have not (previously) committed to backwards
compatibility for commands. Notably, the gopls.test and gopls.gc_details
signatures are preserved, as these are the two cases where we are aware
of LSP clients calling them directly, not from a code lens or
diagnostic.

For golang/go#40438

Change-Id: Ie1b92c95d6ce7e2fc25fc029d1f85b942f40e851
Reviewed-on: https://go-review.googlesource.com/c/tools/+/290111
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 9, 2021
Update our docgen to include documentation for commands. This is done in
an ad-hoc manner. We'll probably need to iterate on this as we go.

For golang/go#40438

Change-Id: I0a6922aa2f5e7dc4c8a43e0f843ddb54971cbe44
Reviewed-on: https://go-review.googlesource.com/c/tools/+/290190
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants