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: add support for 'refactor.extract.constant' code actions #37170

Open
stamblerre opened this issue Feb 11, 2020 · 15 comments
Open

x/tools/gopls: add support for 'refactor.extract.constant' code actions #37170

stamblerre opened this issue Feb 11, 2020 · 15 comments
Labels
FeatureRequest gopls help wanted Tools

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Feb 11, 2020

See microsoft/vscode-go#3040.

https://microsoft.github.io/language-server-protocol/specifications/specification-3-15/#textDocument_codeAction has more details on the possibilities here.

The function refactoring can be implemented by making use of guru's freevars code.

@stamblerre stamblerre added this to the gopls/v1.0.0 milestone Feb 11, 2020
@gopherbot gopherbot added Tools gopls labels Feb 11, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 3, 2020

Change https://golang.org/cl/221917 mentions this issue: internal/lsp: add code action to extract local variable

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 20, 2020

Change https://golang.org/cl/241957 mentions this issue: internal/lsp: support extract function

@joshbaum
Copy link

@joshbaum joshbaum commented Jul 20, 2020

https://golang.org/cl/241957 and https://golang.org/cl/240182 support extracting a function and variable, respectively, as a code action. The following are open issues with function extraction:

  • Extraction does not include comments. Comments are replaced with newlines in the extracted function.
  • You cannot extract a selection that begins with or ends with a comment.
  • Control flow statements (break, continue, goto, etc.) are not handled specially. This can produce bad code if extracted poorly.
  • Defer statements are not handled specially. This can also produce bad code if extracted poorly.
  • Identifiers can be on the left-hand side of an assignment statement even if they are never used again. This produces a yellow line with the message "variable is never used." This occurs when a variable name is used later in scope, but the first time it is used after the selection, it is defined again (using :=) rather than assigned. This does not produce incorrect code, but should be fixed.

gopherbot pushed a commit to golang/tools that referenced this issue Jul 20, 2020
Extract function is a code action, similar to extract variable. After
highlighting a selection, if valid, the lightbulb appears to trigger
extraction. The current implementation does not allow users to
extract selections with a return statement.

Updates golang/go#37170

Change-Id: I5fc3b19cf7dbca4407ecf0cc37017661223614d1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/241957
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Josh Baum <joshbaum@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 20, 2020

Change https://golang.org/cl/243650 mentions this issue: internal/lsp/source: support return statements in extract function

@stamblerre stamblerre removed this from the Unreleased milestone Jul 23, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Jul 28, 2020
Previously, users could not extract code that contained a return
statement. Now, users can extract code with return statements, as long
as the statements are nested within an if, case, or other control
flow statement.

Updates golang/go#37170

Change-Id: I2df52d0241517472decabce3666a32392ff257bd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/243650
Run-TryBot: Josh Baum <joshbaum@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre changed the title x/tools/gopls: add support for extracting a function/variable as a code action x/tools/gopls: improve support for extracting a function/variable as a code action Aug 28, 2020
@stamblerre stamblerre changed the title x/tools/gopls: improve support for extracting a function/variable as a code action x/tools/gopls: improve support for extracting a function/variable Aug 28, 2020
@stamblerre stamblerre added this to the gopls/unplanned milestone Oct 21, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 21, 2021

Change https://golang.org/cl/312469 mentions this issue: internal/lsp: add support for extracting non-nested returns

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 27, 2021

Change https://golang.org/cl/313211 mentions this issue: internal/lsp: print comments that would be lost during extract func

gopherbot pushed a commit to golang/tools that referenced this issue Apr 27, 2021
If there is a return statement that is guaranteed to execute in
the selection to extract to function, then the result of calling
the extracted function can be directly returned.

Updates golang/go#37170

Change-Id: I6454e4107d670e4a1bc9048b2e1073fc80fc78ab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/312469
Trust: Suzy Mueller <suzmue@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Apr 28, 2021
Due to the limitations of comments in ast, it is difficult to move
comments. The extract function feature currently does not handle
comments at all. This change instead prints the comments that would
have been lost above the call to the function, so that the user can
easily recover them. Otherwise, it was possible for users to lose
comments and not notice.

Updates golang/go#37170

Change-Id: I1e2d865f5deddefbb0417732490decbdfcde5f3d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/313211
Trust: Suzy Mueller <suzmue@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 24, 2021

Change https://golang.org/cl/351989 mentions this issue: internal/lsp: allow extract func ranges to begin/end with comments

@suerta-git
Copy link

@suerta-git suerta-git commented Oct 11, 2021

Hi there, have these code actions been released or not?

Currently when I do the refactoring there is:
Screen Shot 2021-10-11 at 10 51 59 PM
Is this normal behavior?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 14, 2021

@suerta-git: What refactoring would you like to see there?

@suerta-git
Copy link

@suerta-git suerta-git commented Oct 14, 2021

@stamblerre I am expecting to see extract variable.
It works in quick fix like this:
Screen Shot 2021-10-14 at 2 21 06 PM

But not works on my hotkey:
Screen Shot 2021-10-14 at 2 20 22 PM

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 14, 2021

Ok, if you are seeing a refactoring there but it doesn't work with the hotkey, then this is probably a VS Code bug, not a bug in the extension. Please file an issue with https://github.com/microsoft/vscode.

@suerta-git
Copy link

@suerta-git suerta-git commented Oct 15, 2021

@stamblerre so you mean this extension already provides the funtionality for 'refactor.extract.constant/variable', but the hotkey doesn't work due to some reason like a vscode bug, right?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Oct 15, 2021

Oh actually- now I see the issue. We don't actually offer refactor.extract.constant, but we do offer refactor.extract.variable. We can make this fix on our side.

@stamblerre stamblerre changed the title x/tools/gopls: improve support for extracting a function/variable x/tools/gopls: add support for 'refactor.extract.constant' code actions Oct 15, 2021
@suerta-git
Copy link

@suerta-git suerta-git commented Oct 17, 2021

Hi @stamblerre thanks for your reply. I tried to reinstall my vscode and restore to the initial state. Then set one customized shortcut like this in keybindings.json (refer to official doc):

// Place your key bindings in this file to override the defaults
[
    {
        "key": "alt+cmd+v",
        "command": "editor.action.codeAction",
        "args": {
          "kind": "refactor.extract.variable"
        }
    }
]

The issue still exists:

Using quick fix (cmd + . in mac os):

Screen Shot 2021-10-17 at 5 48 44 PM

Using shortcut (alt+cmd+v defined above):

Screen Shot 2021-10-17 at 5 37 57 PM

As a reference, for other languages,refactor.extract.constant shortcut is avaiable on TypeScript (doesn't support refactor.extract.variable), but both of two shortcuts aren't avaiable on Python and C#.

Seems even though vscode provides the ability for customizing shortcuts, this feature is not well supported by language extensions. Not sure if it is due to some bugs in vscode.

@suzmue
Copy link
Contributor

@suzmue suzmue commented Dec 23, 2021

gopls sets the codeActionKind for Extract variable to be refactor.extract. So the keybinding would need to be

{
        "key": "alt+cmd+v",
        "command": "editor.action.codeAction",
        "args": {
          "kind": "refactor.extract"
        }
    }

We should definitely consider being more specific for the code action kinds to enable keybindings like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest gopls help wanted Tools
Projects
None yet
Development

No branches or pull requests

6 participants