x/tools/gopls: GOFLAGS=-mod=readonly is no longer honoured

[myitcv]

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

$ go version
go version devel +c8bd8010ff Thu Jan 28 16:45:43 2021 +0000 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"
GOVCS=""
GOVERSION="devel +c8bd8010ff Thu Jan 28 16:45:43 2021 +0000"
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-build599539801=/tmp/go-build -gno-record-gcc-switches"

What did you do?

-- go.mod --
module mod.com

go 1.13
-- main.go --
package main

import (
        "fmt"

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

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

First:

export GOFLAGS=-mod=readonly

Then open main.go

What did you expect to see?

No changes to go.mod

What did you see instead?

module mod.com

go 1.13

require golang.org/x/tools v0.1.0

Bisected down to 9a0e0bb, which looks unrelated.

---

cc @stamblerre

FYI @leitzler

[myitcv]

Note the Go version here is significant. This is not reproducible with go1.15

[leitzler]

Just a note, govim do set allowModfileModifications to true since we want users to be able to let gopls add imports without the steps Diagnostic -> Open suggested fixes -> Apply suggested fix.

The bisected commit is actually related since it enabled https://go-review.googlesource.com/c/tools/+/274532 - internal/lsp/source: allow opt-out from -mod=readonly, GOPROXY=off.

Should allowModfileModifications allow modifications as in look at what the user provides via -mod= instead of (as it looks now) "force modfile modifications" ?

[myitcv]

Ah, thanks @leitzler - I'd forgotten about allowModfileModifications.

The bisected commit is actually https://go-review.googlesource.com/c/tools/+/275442, which confuses me.

In any case, I'm very likely behind so would appreciate help understanding why allowModfileModifications is necessary here, given that we already have a mechanism via GOFLAGS=-mod=XYZ to control this behaviour.

Perhaps one answer is that it gives a more natively gopls means of changing the behaviour directly via a well-named, specific config option.

Assuming allowModfileModifications is necessary for this or some other reason, I guess this issue boils down to trying to understand why there is a difference in behaviour between Go 1.15 and Go 1.16. Because this test passes for Go 1.15, but fails for Go 1.16: yet in both cases, allowModfileModifications=true and GOFLAGS=-mod=readonly. And as part of that understanding what the precedence is between the two controls.

[heschi]

CL 275442 fixed a bug that broke the setting.

gopls needs to control the -mod flag very explicitly -- we can't allow the user to override it broadly. When we changed to -mod=readonly as the default, I added allowModfileModifications as an opt-out, but we don't test it and it will eventually be removed. If you want the default behavior you should remove the opt-out, not add additional configuration.

The 1.15/1.16 difference is an implementation detail; in 1.15 auto is the default, so with allowModfileModifications enabled we pass no -mod flag, which happens to allow the GOFLAGS value through. In 1.16 we have to pass -mod=mod, which blocks GOFLAGS.

[myitcv]

CL 275442 fixed a bug that broke the setting.

Thanks for clarifying.

I added allowModfileModifications as an opt-out, but we don't test it and it will eventually be removed

Can I please ask that we don't remove this setting until we have a clear answer to the question of how people who want the semantics of -mod=mod can proceed. Our (govim's) argument is that folks who want the equivalent behaviour of cmd/go side effects should be supported, the primary use case being unimported completions - when I select such a completion, I am by definition saying that I want to add the module requirement and import, so requiring an additional step is frustrating the user's workflow.

Another use case @leitzler points out is the case of manually typed imports (as opposed to via unimported completions). In this case too, the user has intentionally added an import that needs to be satisfied by a module requirement.

It's entirely possibly we can deal with specific cases on a case-by-case basis, i.e. we add some mechanism to deal with unimported completions independent of generally "supporting" -mod=mod. But hopefully the reasoning behind this is clear. The conflating this with -mod=mod is intentional, because its semantics are very close to what such a user wants in this case.

If you want the default behavior you should remove the opt-out, not add additional configuration. ... The 1.15/1.16 difference is an implementation detail; in 1.15 auto is the default, so with allowModfileModifications enabled we pass no -mod flag, which happens to allow the GOFLAGS value through. In 1.16 we have to pass -mod=mod, which blocks GOFLAGS.

Thanks - your explanation makes it clear we should remove this setting for Go 1.16 if what we are looking to test is the behaviour of -mod=readonly (which is the default with Go 1.16).

[myitcv]

It's just occurred to me that up until I just raised #44035 I've been working on the assumption that gopls sends workspace edits to update go.{mod,sum} files in response to commands/etc. Turns out this is not the case. Hence, we've very likely been talking past each other when I've been touting this approach by which gopls could/should provide edits for go.{mod,sum} in response to unimported completions.

[heschi]

#44035 has nothing to do with this issue. It would be entirely possible to send the modifications as edit commands, it just hasn't ever been necessary.

When we made the switch to manually-managed go.mod files, we explicitly decided that adding a dependency is much more significant than adding an import and should not happen automatically. As far as I know, you and Pontus are the only people who have ever complained about the change to us, and at this point it's been over two months. I don't think we should be supporting an entire extra mode on evidence that thin. If you want to argue otherwise, that's fine, but it would be much more effective to have it in one issue that actually addresses that question than in this issue and #44035, which address implementation details.

[myitcv]

#44035 has nothing to do with this issue.

I wasn't intimating it is related, simply pointing out what my understanding was prior to noticing this, and explaining how (from my perspective at least) I've potentially been talking past you/others.

It would be entirely possible to send the modifications as edit commands, it just hasn't ever been necessary.

As I mention in that issue, it would certainly make working in Vim (and potentially other buffer-based editors) more reliable and less surprising. Right now the behaviour is no worse that Go 1.15 on the basis the side effects of cmd/go are seen in just the same way.

As far as I know, you and Pontus are the only people who have ever complained about the change to us, and at this point it's been over two months

Just so that I'm clear, my understanding is that the following conditions need to be satisfied in order for a user to run into this change in behaviour:

1. using Go 1.16 (which was released in beta1 back in December), now rc1
2. using a sufficiently up-to-date version of gopls (does gopls auto-update?)
3. adding an import to a file (either by unimported completions or manually) that requires a new module requirement be added to go.{mod,sum}. Note I have not included goimports-like import handling here because that is less precise than either of the aforementioned methods.

I suspect you and the vscode-go team have a much better handle numbers-wise on how many people have run into this scenario.

That said, it's highly likely we are in the minority, not least because we're talking from a Vim perspective.

Whilst we might be in the minority, what we are trying to solve for is the best experience for govim users. As we have encountered in the past, that doesn't always align with what works best for VSCode users. We understand and appreciate that. But I hope you'll agree it's reasonable for us to make our case, a case we always look to do with well reasoned arguments backed by a clear explanation of how it affects user workflow.

If you want to argue otherwise, that's fine, but it would be much more effective to have it in one issue that actually addresses that question than in this issue and #44035, which address implementation details.

I raised #44035 because I wasn't aware of another issue that's tracking the change to send go.{mod,sum} modifications as edit commands. It's quite possible I've either forgotten an old issue or not turned up anything in my searches.

That change should, I believe, be made regardless of whether (per this discussion) we retain a mode that emulates -mod=mod. So I think they are two separate issues. I'll defer to you however on how and where you would like to track them.

[heschi]

-mod=readonly (manual go.mod control) has been the default for all users since https://github.com/golang/tools/releases/tag/gopls%2Fv0.6.0, and yes, gopls does auto-update for vscode users. The change in behavior was inspired by go 1.16, but is not coupled to it. I'm not sure what you mean by goimports-like handling, but the rule is simple: gopls never modifies a user's go.mod without them explicitly requesting it by accepting a quick fix or code lens.

I would therefore expect that many or most gopls users have at some point added an import that they do not have a matching dependency for.

Again, the issue you are reporting here is an implementation detail of an opt-out that I intended to be a last resort. If you want to discuss the -mod=readonly change, I strongly suggest you open a new issue laying out your explanation, from first principles, of why you think Vim users would prefer to have their go.mod automatically managed, so that we can discuss it in one place.

[myitcv]

Thanks for clarifying. I'll open an issue tomorrow as you suggest.

[myitcv]

I opened #44765 so believe this issue can now be closed.