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/cmd/gopls: provide method to run goimports #31359

Closed
myitcv opened this issue Apr 9, 2019 · 10 comments
Closed

x/tools/cmd/gopls: provide method to run goimports #31359

myitcv opened this issue Apr 9, 2019 · 10 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Apr 9, 2019

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

$ go version
go version go1.12.2 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20190408220357-e5b8258f4918

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN="/home/myitcv/gostuff/src/github.com/myitcv/govim/cmd/govim/.bin"
GOCACHE="/home/myitcv/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPROXY=""
GORACE=""
GOROOT="/home/myitcv/gos"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build585627113=/tmp/go-build -gno-record-gcc-switches"

What did you do?

See #31359 (comment) for an updated description for this issue.

The change in https://go-review.googlesource.com/c/tools/+/170997 appears to have stopped CodeAction returning any results to the client, i.e. no edits are returned for a badly formatted document.

Test case is: https://github.com/myitcv/govim/blob/049c3d51c5b8bc80aea8d79516f4fc6faf728b4a/cmd/govim/testdata/format_on_save.txt

Which leads to the following sequence of events:

Sequence of events
2019-04-09T09:02:56.675940: gopls.Initialize() call; params:
2019-04-09T09:02:56.675940: &protocol.InitializeParams{
2019-04-09T09:02:56.675940:     InnerInitializeParams: protocol.InnerInitializeParams{
2019-04-09T09:02:56.675940:         ProcessID:             0,
2019-04-09T09:02:56.675940:         RootPath:              "",
2019-04-09T09:02:56.675940:         RootURI:               "file:///tmp/go-test-script082203924/script-format_on_save",
2019-04-09T09:02:56.675940:         Capabilities:          {},
2019-04-09T09:02:56.675940:         InitializationOptions: nil,
2019-04-09T09:02:56.675940:         Trace:                 "",
2019-04-09T09:02:56.675940:     },
2019-04-09T09:02:56.675940:     WorkspaceFoldersInitializeParams: protocol.WorkspaceFoldersInitializeParams{},
2019-04-09T09:02:56.675940: }
2019-04-09T09:02:56.680080: gopls.Initialize() return; err: ; res:
2019-04-09T09:02:56.680080: &protocol.InitializeResult{
2019-04-09T09:02:56.680080:     Capabilities: protocol.ServerCapabilities{
2019-04-09T09:02:56.680080:         InnerServerCapabilities: protocol.InnerServerCapabilities{
2019-04-09T09:02:56.680080:             TextDocumentSync: map[string]interface {}{
2019-04-09T09:02:56.680080:                 "openClose": bool(true),
2019-04-09T09:02:56.680080:                 "change":    float64(1),
2019-04-09T09:02:56.680080:             },
2019-04-09T09:02:56.680080:             HoverProvider:      true,
2019-04-09T09:02:56.680080:             CompletionProvider: &protocol.CompletionOptions{
2019-04-09T09:02:56.680080:                 TriggerCharacters:   {"."},
2019-04-09T09:02:56.680080:                 AllCommitCharacters: nil,
2019-04-09T09:02:56.680080:                 ResolveProvider:     false,
2019-04-09T09:02:56.680080:             },
2019-04-09T09:02:56.680080:             SignatureHelpProvider: &protocol.SignatureHelpOptions{
2019-04-09T09:02:56.680080:                 TriggerCharacters: {"(", ","},
2019-04-09T09:02:56.680080:             },
2019-04-09T09:02:56.680080:             DefinitionProvider:               true,
2019-04-09T09:02:56.680080:             ReferencesProvider:               false,
2019-04-09T09:02:56.680080:             DocumentHighlightProvider:        true,
2019-04-09T09:02:56.680080:             DocumentSymbolProvider:           true,
2019-04-09T09:02:56.680080:             WorkspaceSymbolProvider:          false,
2019-04-09T09:02:56.680080:             CodeActionProvider:               true,
2019-04-09T09:02:56.680080:             CodeLensProvider:                 (*protocol.CodeLensOptions)(nil),
2019-04-09T09:02:56.680080:             DocumentFormattingProvider:       true,
2019-04-09T09:02:56.680080:             DocumentRangeFormattingProvider:  true,
2019-04-09T09:02:56.680080:             DocumentOnTypeFormattingProvider: (*struct { FirstTriggerCharacter string "json:\"firstTriggerCharacter\""; MoreTriggerCharacter []string "json:\"moreTriggerCharacter,omitempty\"" })(nil),
2019-04-09T09:02:56.680080:             RenameProvider:                   false,
2019-04-09T09:02:56.680080:             DocumentLinkProvider:             (*protocol.DocumentLinkOptions)(nil),
2019-04-09T09:02:56.680080:             ExecuteCommandProvider:           (*protocol.ExecuteCommandOptions)(nil),
2019-04-09T09:02:56.680080:             Experimental:                     nil,
2019-04-09T09:02:56.680080:         },
2019-04-09T09:02:56.680080:         ImplementationServerCapabilities:   protocol.ImplementationServerCapabilities{},
2019-04-09T09:02:56.680080:         TypeDefinitionServerCapabilities:   protocol.TypeDefinitionServerCapabilities{TypeDefinitionProvider:true},
2019-04-09T09:02:56.680080:         WorkspaceFoldersServerCapabilities: protocol.WorkspaceFoldersServerCapabilities{},
2019-04-09T09:02:56.680080:         ColorServerCapabilities:            protocol.ColorServerCapabilities{},
2019-04-09T09:02:56.680080:         FoldingRangeServerCapabilities:     protocol.FoldingRangeServerCapabilities{},
2019-04-09T09:02:56.680080:         DeclarationServerCapabilities:      protocol.DeclarationServerCapabilities{},
2019-04-09T09:02:56.680080:         SelectionRangeServerCapabilities:   protocol.SelectionRangeServerCapabilities{},
2019-04-09T09:02:56.680080:     },
2019-04-09T09:02:56.680080:     Custom: {},
2019-04-09T09:02:56.680080: }
2019-04-09T09:02:56.698326: gopls.DidOpen() call; params:
2019-04-09T09:02:56.698326: &protocol.DidOpenTextDocumentParams{
2019-04-09T09:02:56.698326:     TextDocument: protocol.TextDocumentItem{URI:"file:///tmp/go-test-script082203924/script-format_on_save/file.go", LanguageID:"", Version:0, Text:"package blah\n\nconst ( x = 5\ny = os.PathSeparator\n )"},
2019-04-09T09:02:56.698326: }
2019-04-09T09:02:56.698421: gopls.DidOpen() return; err: 
2019-04-09T09:02:56.704242: gopls.CodeAction() call; params:
2019-04-09T09:02:56.704242: &protocol.CodeActionParams{
2019-04-09T09:02:56.704242:     TextDocument: protocol.TextDocumentIdentifier{URI:"file:///tmp/go-test-script082203924/script-format_on_save/file.go"},
2019-04-09T09:02:56.704242:     Range:        protocol.Range{},
2019-04-09T09:02:56.704242:     Context:      protocol.CodeActionContext{},
2019-04-09T09:02:56.704242: }
2019-04-09T09:02:56.750207: gopls.CodeAction() return; err: ; res:
2019-04-09T09:02:56.750207: []protocol.CodeAction(nil)

What did you expect to see?

The CodeAction return some edits.

What did you see instead?

No edits.

cc @stamblerre @ianthehat

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 13, 2019

The change that caused this was actually golang.org/cl/171019. People were having their files get rewritten if they were particularly broken, so I figured rather than allowing that to happen, we should temporarily stop formatting broken files. I'm going to investigate why that was happening, so hopefully I'll be able to revert this change soon.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 13, 2019

Given the test above, I bisected the issue down to https://go-review.googlesource.com/c/tools/+/170997. i.e. it broke before the changes in https://golang.org/cl/171019.

But it might be I'm seeing something else?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 15, 2019

You don't get any publish diagnostics requests from the server? That change was basically supposed to make codeActions depend on the diagnostics. Instead of assuming we need to run goimports every time, we first look at the diagnostics sent in the code action, and if there aren't any, we won't run goimports.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 15, 2019

Ah. But I think if I call the method I should be given the appropriate edits, no?

@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 16, 2019

Just checked: I don't appear to be getting any diagnostics for a file that is badly formatted either.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 16, 2019

A badly formatted file will not give you any diagnostics - just one with errors. In the case of the example you provided, you should definitely be getting sent a "undeclared name: os" diagnostic from the language server. Then, the codeAction would contain diagnostics in the context, which would then be used to return a response. Basically a codeAction should be associated with a diagnostic, which is why we made that change.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 16, 2019

Stepping back one second: this test is verifying the gopls equivalent of goimports works as the file is being saved. So yes, the "os" import is missing, but that's the point of the test!

But it seems with the changes I probably calling the wrong thing/looking in the wrong place for the edits that are "the result of goimports". Does that make sense? Can you help point me towards what I should be doing here?

@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 16, 2019

Just adding a couple more notes here from our Slack conversation.

What I'm trying to achieve here is effectively the equivalent of running goimports as a file is saved.

Prior to this change, I could achieve this by calling the codeAction and getting a synchronous response.

Now we need to wait for diagnostics to be received in the client before calling the codeAction.

The issue is:

  • saving a file is a user-directed command
  • I don't know whether/how long I need to wait

So I think we have a gap here as far as the equivalent of being able to synchronously call goimports is concerned.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 16, 2019

Thanks for adding this info. I just filed microsoft/language-server-protocol#726 to get more insight.

@myitcv myitcv changed the title x/tools/cmd/gopls: CodeAction returns no edits x/tools/cmd/gopls: provide method to run goimports Apr 16, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 16, 2019

Change https://golang.org/cl/172406 mentions this issue: internal/lsp: run source.organizeImports on all codeActions

@golang golang locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.