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

cmd/go: go directive is not only added during go mod init, but also under other conditions that are hard to deduce #30790

Open
dmitshur opened this Issue Mar 12, 2019 · 8 comments

Comments

Projects
None yet
4 participants
@dmitshur
Copy link
Member

dmitshur commented Mar 12, 2019

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

$ go version
go version go1.12 darwin/amd64

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=""
GOCACHE="/Users/dmitshur/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/dmitshur/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/3m/rg2zm24d1jg40wb48wr0hdjw00jwcj/T/go-build766457690=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Run the following commands in one module that has no module requirements:

$ cat go.mod 
module m1

$ go mod tidy

$ cat go.mod 
module m1

go 1.12

Now run the same commands in another module that has 1 or more module requirements:

$ cat go.mod
module m2

require rsc.io/quote v1.0.0

$ go mod tidy

$ cat go.mod
module m2

require rsc.io/quote v1.0.0

Additionally, if the source code in module m2 is modified to no longer import anything from the rsc.io/quote module, and go mod tidy is run, the first time it removes the require rsc.io/quote v1.0.0 line, leaving just module m2. The second time it adds a go directive.

What did you expect to see?

For modules that already exist, and do not already contain a go directive in their go.mod file, the go directive should either be always added automatically (i.e., during any of go build, go test, go mod tidy, etc., operations), or it should never be added.

What did you see instead?

Sometimes it's added, sometimes it isn't added.

Based on investigating the code with @julieqiu and @heschik, we've found that the go directive is always added to go.mod on any go build, go test, go mod tidy, etc., operation whenever the go.mod file has exactly one statement: the module statement. If the go.mod file has at least 1 require, or replace, or exclude directive, in addition to the module statement, then various go operations do not add a go directive.

This behavior is surprising and hard to predict (without looking into the source code to figure it out). I suspect it's an unintentional behavior. The commit message of CL 147281 that implemented this behavior says:

cmd/go: add go statement when initializing go.mod

When creating a go.mod file, add a go statement mentioning the current Go version.

Based on that, I suspect the originally intended behavior was to make it so that a go directive is automatically inserted only when creating a new Go module (via go mod init), but not when working with an existing module that has a go.mod file. I will assign this to @ianlancetaylor to confirm if this is a bug, and perhaps make the decision on how and whether this issue should be resolved.

I think we should fix this and even consider backporting to Go 1.12.1. Doing so should would make the logic of when a go directive is added easier to predict and understand, improving the user experience when in module mode.

/cc @bcmills @matloob

@thepudds

This comment has been minimized.

Copy link

thepudds commented Mar 12, 2019

Likely related: #30043 (comment)

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Mar 12, 2019

Before thinking about what should happen, why is the unpredictable go directive "quite disruptive to the user experience when in module mode"? Where does the disruption occur?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Mar 12, 2019

The intent of the CL was to add a "go" statement to a go.mod file, if one did not already exist, when running either go mod init or go mod tidy. Adding a "go" statement when running go mod tidy was suggested by @bcmills during CL review.

So it sounds like there is a bug: if the existing go.mod file contains anything other than just a "module" statement, then running go mod tidy does not add a "go" directive.

@dmitshur

This comment has been minimized.

Copy link
Member Author

dmitshur commented Mar 12, 2019

why is the unpredictable go directive "quite disruptive to the user experience when in module mode"?

The behavior I've observed is that various people are either uncertain about when the go directive is actually added (e.g., they describe it as "sometimes"), or they have an incorrect idea of when it's added (e.g., "always added by go mod tidy in Go 1.12" or "only added by go mod init").

If people choose to not add the go directive until figuring out what value it should be (related to #30791), it means they sometimes have to remove the go directive added (e.g., after running go test on a module with 0 required modules) but not other times (e.g., after running go test on a module with 1+ required modules). This encourages people to add the directive with an arbitrary value before understanding the short-term and long-term consequences of doing so.

Where does the disruption occur?

I've observed it occur during code review, and while people are running go commands inside various modules.

@dmitshur

This comment has been minimized.

Copy link
Member Author

dmitshur commented Mar 12, 2019

The intent of the CL was to add a "go" statement to a go.mod file, if one did not already exist, when running either go mod init or go mod tidy. Adding a "go" statement when running go mod tidy was suggested by @bcmills during CL review.

Currently, it also gets added when running go build or go test in a module with a go.mod file that has zero module requirements (and no exclude, replace statements):

$ cat go.mod
module m3
$ go test
PASS
ok  	m3	0.041s
$ cat go.mod
module m3

go 1.12

Is that a bug or intended behavior?

So it sounds like there is a bug: if the existing go.mod file contains anything other than just a "module" statement, then running go mod tidy does not add a "go" directive.

Making both go mod init and go mod tidy add the go directive, if it's not already present, regardless of whether the go.mod file contains just a module statement or more statements, sounds like a valid resolution to this issue.

I've considered suggesting that we change the behavior such that go directive is added only during go mod init and none of the other commands, so that existing modules made in Go 1.11 (without the go directive) don't suddenly start gaining a go directive after users upgrade to Go 1.12. However, if the go directive will ever become mandatory rather than optional, then some version of Go will have to start adding it to existing modules in addition to new modules. So it might as well be Go 1.12.

@dmitshur dmitshur changed the title cmd/go: go directive is not only added during go mod init, but also under specific and surprising conditions cmd/go: go directive is not only added during go mod init, but also under other conditions that are hard to deduce Mar 13, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Mar 13, 2019

Currently, it also gets added when running go build or go test in a module with a go.mod file that has zero module requirements (and no exclude, replace statements):
Is that a bug or intended behavior?

I don't know. I don't really understand how go build and go test are supposed to modify the go.mod file. This likely shouldn't happen. Clearly the inconsistency shouldn't happen, and likely the "go" directive should not be added at all in this case. But I could easily be convinced otherwise.

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Mar 13, 2019

The empty-requirements behavior is probably a side effect of this case:

if len(f.Syntax.Stmt) == 1 && f.Module != nil {
// Entire file is just a module statement.
// Populate require if possible.
legacyModInit()
}

The go directive is added here:

Perhaps that call should be moved inside the modFile == nil condition.

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Mar 13, 2019

Or, going in the other direction, perhaps we should always add the go directive if it is missing. That ensures that a module that was successfully built and tested with, say, Go 1.13, continues to build (with the same meaning) with Go 1.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.