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: gopls.add_dependency command relies on side effects rather than sending workspace edits #44035

Closed
myitcv opened this issue Feb 1, 2021 · 2 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@myitcv
Copy link
Member

myitcv commented Feb 1, 2021

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

$ go version
go version go1.15.7 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.1.1-0.20210128154556-db4c57db23ae
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20210128154556-db4c57db23ae

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
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/myitcv/gostuff/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
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-build717042068=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Started with:

-- go.mod --
module example.com/repro

go 1.16
-- main.go --
package main

import (
	"fmt"

	_ "golang.org/x/tools/imports"
)

func main() {
	fmt.Println("hello, world!")
}

i.e. the requirement on golang.org/x/tools/imports is missing from go.{mod,sum}.

Loading main.go in Vim (using govim) gives error diagnostics as expected:

main.go|6 col 4| could not import golang.org/x/tools/imports (no required module provides package "golang.org/x/tools/imports")
main.go|6 col 4| golang.org/x/tools is not in your go.mod file

If we ask for suggested fixes at main.go:6 we get a code action response with the gopls.add_dependency command.

If we execute the gopls.add_dependency command, go.{mod,sum} change beneath the editor and gopls.

Log files:

I haven't kept up to speed with all the conversations on this topic (and might well have forgotten key conversations, for which I apologise), but this is surprising. My expectation was that, because tempModfile=true, the required changes to go.{mod,sum} would be sent as edits to those files, rather than cmd/go being executed and gopls relying on the editor to "see" those changes and relay them.

Relying on side effects in this way is problematic:

  • file watching is notoriously flakey
  • if the editor has either/both of go.{mod,sum} open, then it should not send any changes to gopls until that buffer (to use Vim terminology) has been updated by reloading from disk. Vim at least does not automatically reload a buffer where the underlying file on disk has changed: it would be incredibly bad UX to do this by default in any case, not to mention problematic if the user has pending changes

This issue is related to #44008 in that it had been my assumption to this point that changes to go.{mod,sum} files were being sent as workspace edits. Hence, offering a mode of automatically adding module requirements in response an unimported completion would be trivial. If, however, our analysis is correct and instead we are relying on side effects of commands then I can see how we were very likely talking past each other.

That said, I think we should be moving to a model where gopls sends all edits to the editor, regardless of whether a file is open or not, rather than relying on side effects, file watching etc.

@stamblerre @heschik would very much welcome your thoughts.


FYI @leitzler

@myitcv myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Feb 1, 2021
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 1, 2021
@gopherbot gopherbot added this to the Unreleased milestone Feb 1, 2021
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Feb 2, 2021
@heschi
Copy link
Contributor

heschi commented Feb 2, 2021

This is fixable. We'll need to be able to run multiple Go invocations in the same -modfile tempdir (e.g. go list, go get for go_get_package) and then extract the resulting go.mod and go.sum files. To send the edits we'll have to use either codeAction/resolve (new in LSP 3.16) or workspace/applyEdit, which is a little jankier.

@gopherbot
Copy link

Change https://golang.org/cl/290189 mentions this issue: internal/lsp: apply go.mod/sum changes via workspace edits

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants