x/tools/gopls: add mode/option/mechanism to allow automatic updates to go.mod

[myitcv]

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

$ go version
go version devel +dfdc3880b0 Thu Sep 10 02:22:19 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200915031644-64986481280e
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20200915031644-64986481280e

Does this issue reproduce with the latest release?

n/a

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-build336384284=/tmp/go-build -gno-record-gcc-switches"

What did you do?

By default gopls does not not automatically update a user's go.mod file, a setting established via the TempModfile approach:

https://github.com/golang/tools/blob/64986481280e06c512dcd7674532f4f7f4cfc0c6/internal/lsp/source/options.go#L101

In this mode, in order to make changes to their go.mod file, a user either needs to:

1. accept the suggestions from gopls
2. run go mod tidy
3. run go test or similar

Alternatively, with TempModfile turned off, go.mod files are updated as a side effect of cmd/go's various actions, and both gopls and cmd/go use and see the same source of truth.

#40728 however moves us to a situation where -mod=readonly effectively becomes the default for many cmd/go commands which appears to rule out us turning off TempModfile. Therefore the only option is 1 or 2 from above, i.e. manually intervening.

This makes the following workflow quite painful:

• accept unimported completion candidate
• run go test which now fails under cmd/go: default to & improve -mod=readonly #40728
• have to manually accept change to go.mod because gopls is maintaining a shadow go.mod where the change has been made but the real go.mod is as yet unaltered (this is effectively a variant of the concern raised in x/tools/gopls: support multi-module workspaces #32394 (comment)) (edit: see x/tools/gopls: add mode/option/mechanism to allow automatic updates to go.mod #41397 (comment))

The first step, is, to my mind an implicit acceptance of the fact that a dependency has been added. Indeed, given the import statement is added to the file in question I would suggest the requirement should also also be added. But I can see that some people might want/prefer the explicit step.

Hence, this issue is a request that some mode/option/mechanism be maintained whereby the user can continue to opt in to automatically having changes applied to their go.mod file. The example presented involves accepting unimported completions, but generally speaking I do want/need gopls to make changes to my go.mod file as a side effect of what I do via gopls.

---

cc @stamblerre @heschik @findleyr

FYI @leitzler

[stamblerre]

I'm not sure if this will be necessary because the Go command's behavior is changing too: #40728.

The -mod=mod flag that gopls is currently relying on will likely be deprecated in 1.16, so I think that the only mechanism gopls would have of providing these updates would be something like automatically running go mod tidy on every Go file save (which would be too expensive IMO).

BTW, regarding:

gopls is maintaining a shadow go.mod where the change has been made but the real go.mod is as yet unaltered (this is effectively a variant of the concern raised in #32394 (comment))

gopls doesn't actually maintain a temporary go.mod file that it updates and reuses. Any call to the go command gets its own new temporary go.mod file that is a copy of whatever is currently in the existing go.mod file.

/cc @jayconrod

[bcmills]

We have previously discussed modifying goimports to provide the specific module paths and versions for the imports it suggests (particularly if they are not already in the build list). Does gopls currently modify go.mod when it runs goimports?

[myitcv]

gopls doesn't actually maintain a temporary go.mod file that it updates and reuses.

Perhaps I'm misunderstanding something then. With TempModfile: true, what happens when I accept an unimported completion that requires a new dependency? The real go.mod file is not changed (by definition), but a change must be made/maintained somewhere, not least because it gives rise to the suggested edit to the real go.mod file?

[stamblerre]

Does gopls currently modify go.mod when it runs goimports?

It uses -modfile with a temporary go.mod so that any go list commands it runs will modify that.

but a change must be made/maintained somewhere, not least because it gives rise to the suggested edit to the real go.mod file?

The two are separate actually. Like @bcmills said, unimported completions don't have module paths and versions associated with them, so it only add imports to Go files. gopls also runs go mod tidy any time an import in any Go file is changed (on disk), which is where the go mod tidy diagnostics come from.

[myitcv]

Thanks for the clarification on the implementation details, @stamblerre. I've edited my misunderstanding in the description, linking to your explanation in the previous comment.

gopls also runs go mod tidy any time an import in any Go file is changed (on disk), which is where the go mod tidy diagnostics come from.

So this issue is ultimately about auto-applying these diagnostics. I guess options here range from a gopls mode/option, to the client being able to precisely identify them as "go mod tidy diagnostics" and auto-apply them.

But it sounds like we also need the functionality @bcmills describes. Is that covered elsewhere?

[stamblerre]

to the client being able to precisely identify them as "go mod tidy diagnostics" and auto-apply them.

This is what VS Code does. Any go mod tidy diagnostics are classified as source.organizeImports, so VS Code can apply them on save.

But it sounds like we also need the functionality @bcmills describes. Is that covered elsewhere?

I'm not sure about this. @heschik would probably know though.

[myitcv]

This is what VS Code does. Any go mod tidy diagnostics are classified as source.organizeImports, so VS Code can apply them on save.

Interesting. So is there a manual step involved at all in VSCode? Are these diagnostics automatically applied, or is there some config option that enables/disables this?

[stamblerre]

This is the configuration in the VS Code:

"[go.mod]": {
	"editor.codeActionsOnSave": {
		"source.organizeImports": true,
	},		
}

Note that I actually have it disabled because I find it to be too slow, especially when you're typing in the go.mod file, and it might be in a bad state.

[myitcv]

Interesting, thank you very much.

So it looks like exactly the same could/should be achieved in govim. i.e. we switch to TempModfile: true and then use an option/mode equivalent to the above to enable/disable this.

Note that I actually have it disabled because I find it to be too slow, especially when you're typing in the go.mod file, and it might be in a bad state.

Can you expand on this? Is gopls too slow when enabled? How does typing in the go.mod affect something that happens on save?

[stamblerre]

So it looks like exactly the same could/should be achieved in govim. i.e. we switch to TempModfile: true and then use an option/mode equivalent to the above to enable/disable this.

Sure, but I just want to reiterate the default behavior of the Go command will be changing. So it may be worth matching that because otherwise you'll have the reverse issue--gopls will introduce edits, but go test, etc. won't.

Can you expand on this? Is gopls too slow when enabled? How does typing in the go.mod affect something that happens on save?

I tend to save as I type a lot, which is why I was running into this. Probably much less of an issue with Vim. VS Code also has removed the codeActionOnSaveTimeout feature which used to prevent slow saves.

[myitcv]

So it may be worth matching that because otherwise you'll have the reverse issue--gopls will introduce edits, but go test, etc. won't.

For a default I think you're probably right. As I said above, I fall into the "using my editor means I'm implicitly giving the LSP the right to make changes to files I'm working with" camp. But I can quite see why others would prefer the more explicit behaviour this new default introduces, with manual accepts of suggested edits.

I tend to save as I type a lot, which is why I was running into this. Probably much less of an issue with Vim. VS Code also has removed the codeActionOnSaveTimeout feature which used to prevent slow saves.

Thanks, that's useful context.

And thanks again for taking the time to explain this.

So, on the basis I think we have a solution here, I'm tempted to suggest we close this?

Unless you want to leave it open, @stamblerre, perhaps to track your question to @heschik in #41397 (comment)?

[stamblerre]

I think closing is fine--if there was additional work needed to improve goimports in some way, I'm sure that Heschi would open an issue, so I'm guessing this isn't actually necessary.

[myitcv]

I just tried this within govim against 6498648 (Go version eaa97fb). The setup was as follows:

-- main.go --
package main

func main() {
}
-- go.mod --
module mod.com

go 1.16

I then changed main.go as follows (simulating an unimported completion) and saved that file:

package main

import (
	"fmt"

	"github.com/kr/pretty"
)

func main() {
	fmt.Printf("%v", pretty.Sprint(5))
}

With TempModfile: false

• go.mod changes happen as a side effect of gopls' normal operation
• no diagnostics received for go.mod
• diagnostics received for main.go in the period before the file save, after which they disappear

All as expected.

With TempModfile: true

• no automatic changes to go.mod
• diagnostics received for main.go in the period before the file save, after which they resort to github.com/kr/pretty is not in your go.mod file
• diagnostics received for go.mod: github.com/kr/pretty is not in your go.mod file

It seems I misunderstood/interpreted something in our exchange above. Because saving main.go does not trigger gopls to send the client edits to go.mod.

With TempModfile: true the only means by which I can make changes to my go.mod is by triggering a save of go.mod itself. As I outlined in the description, that is the kind of break in workflow I'm looking to avoid because I've already accepted that import and by definition the requirement it implies.

What I was expecting is that instead of the diagnostic github.com/kr/pretty is not in your go.mod file being sent for the go.mod file we have a workspace edit to apply. The beauty of this approach being that it is async from the original save of the main.go file, hence you don't end up blocking on a go mod edit on each save.

Re-opening because it doesn't appear the current setup supports the workflow detailed in the description.

[stamblerre]

What I was expecting is that instead of the diagnostic github.com/kr/pretty is not in your go.mod file being sent for the go.mod file we have a workspace edit to apply.

You need to request code actions for the diagnostic in the go.mod file in order to get the associated suggested fix. There is also the go mod tidy code lens which appears whenever your module is not tidy, and you could use that to always apply edits.

[myitcv]

So it sounds like to automate this we (govim) need to either:

1. match diagnostics for a go.mod file against "is not in your go.mod file", then request code actions for those diagnostics, then filter the code actions that are returned (the quickfix kind? Anything more specific), and then apply them all
2. repeatedly ask for the go mod tidy code lens

Option 2 doesn't seem feasible because the go.mod file might not be open, and in any case govim doesn't know which go.mod file it should use in the general case.

Option 1 sounds a bit brittle, but better in that we are using the diagnostic edge as a trigger point for working out whether there is something to do. Can this option be refined in some way?

[leitzler]

We could use the diagnostic from option 1 to know where the go.mod is located that we want to ask for in a CodeLens call.

It would still be fragile to do string matching on diagnostics. Even if we should go for string matching, the response from the CodeLens call contains several commands and we wouldn't know which one to pick (since the command names aren't public/stable).

The last part could be solved by adding tidy as an explicit CodeAction, so we can request it with a known kind (i.e. tidy/go.tidy/etc). That would align with the suggestion Heschi had at the previous tools call.

[stamblerre]

match diagnostics for a go.mod file against "is not in your go.mod file", then request code actions for those diagnostics, then filter the code actions that are returned (the quickfix kind? Anything more specific), and then apply them all

They will be the "source.organizeImports" kind - all of the go mod tidy diagnostics are categorized this way. We are considering making them errors rather than warnings in the future, so you could also get code actions for all of the error level diagnostics.

The VS Code config I shared earlier is doing effectively this - basically saying apply any organize import code actions to the go.mod file on save (though not on ay Go file save).

[leitzler]

Oh, sorry I missed that part. A source.organizeImports request would do it.

I don't quite follow the part about diagnostics being of a certain kind, are you referring to the Source in the diagnostic?

e.g.

PublishDiagnostics callback: &protocol.PublishDiagnosticsParams{
    URI:         "file:///private/var/folders/j4/l2j99h6d5qd6knjlllql0bb80000gn/T/tmp.H5PUVQQb/go.mod",
    Version:     0,
    Diagnostics: {
        {
            Range: protocol.Range{
                Start: protocol.Position{},
                End:   protocol.Position{Line:0, Character:8},
            },
            Severity:           2,
            Code:               nil,
            Source:             "go mod tidy",
            Message:            "github.com/kr/pretty is not in your go.mod file",
            Tags:               nil,
            RelatedInformation: nil,
        },
    },
}

[stamblerre]

Ah sorry, for kind, I was referring to the CodeActionKind that is part of the Only field in the request.
For error vs. warning, I was referring to the Severity field.

[leitzler]

I see. Then I can't really see a way to automatically update go.mod as things look today since:

• The client doesn't know where/what go.mod is (unless string-matching URI)
• The client doesn't know if a diagnostic is about missing deps (unless string-matching message/source)

Apologies if I missed why earlier in the conversation, but why can't the code action response for my .go-file not only have an edit that adds the import, but also updates go.mod? Is it because -mod=readonly will be default?

In that case, should it do things automatically if the user specifies -mod=autoupdate? (or was that just a part of Russ original post that won't happen in reality?)

[stamblerre]

Apologies if I missed why earlier in the conversation, but why can't the code action response for my .go-file not only have an edit that adds the import, but also updates go.mod?

This should be the case. When a dependency is missing from the go.mod, you get a diagnostic for both the Go file and the go.mod file. You can then send those diagnostics back to the server as part of a code action, which will return, in this case, the same fix - adding the dependency to the go.mod file. You can specify a CodeActionKind of source.organizeImports to make sure that these are only go mod tidy diagnostics (or match on the go mod tidy source) and then apply these fixes.

In that case, should it do things automatically if the user specifies -mod=autoupdate? (or was that just a part of Russ original post that won't happen in reality?)

I don't think that gopls will obey -mod flags provided by the user, but will rather obey the TempModfile configuration. With TempModfile: false, changes will be applied directly to the files.

[myitcv]

I don't think that gopls will obey -mod flags provided by the user, but will rather obey the TempModfile configuration. With TempModfile: false, changes will be applied directly to the files.

My assumption on opening this issue was that, even with TempModfile: false, this wouldn't be the case, precisely because of the changes being made to cmd/go in 1.16. Is gopls doing something special that causes the side effects of go list etc to result in go.{mod,sum} changes? Or have I totally missed something?

[stamblerre]

Sorry, I was speaking about the state pre-1.16 and as of right now. With gopls and Go at tip, you will actually still get the previous -mod=mod behavior, but we are working on switching to use -mod=readonly by default.

[myitcv]

Ok, thanks for clarifying

I don't think that gopls will obey -mod flags provided by the user, but will rather obey the TempModfile configuration. With TempModfile: false, changes will be applied directly to the files.

...

but we are working on switching to use -mod=readonly by default.

At which point I think the TempModfile solution becomes redundant, correct?

I ask that only as an observation to clarify, because I think the better solution here is effectively the one we've arrived at in the course of this discussion (thanks for your patience). Namely: match diagnostics for a go.mod file against "is not in your go.mod file", then request code actions of the source.organizeImports kind for those diagnostics on that file, and then apply them all. Arguably, the matching against "is not in your go.mod file" diagnostics is not necessary, we could simply request source.organizeImports code actions whenever there are diagnostics for any go.mod file.

This has the added benefit of changes to a go.mod being initiated by gopls as opposed to happening as a side effect of a cmd/go command.

[myitcv]

Thanks again for helping to talk this through, @stamblerre. I'll close this for now because I think we've reached agreement on a way forward. I've raised govim/govim#956 to capture actually implementing this change.