go icon indicating copy to clipboard operation
go copied to clipboard

cmd/go: breaking change in 1.23rc2 with version constraints in GOPATH mode

Open rittneje opened this issue 1 year ago • 38 comments

Go version

go version go1.23rc2 darwin/amd64

Output of go env in your module/workspace:

GO111MODULE='auto'
GOARCH='amd64'
GOBIN=''
GOCACHE='/tmp/.gocache'
GOENV='/Users/rittneje/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/rittneje/gomodtest/pkg/mod'
GONOPROXY='[REDACTED]'
GONOSUMDB='[REDACTED]'
GOOS='darwin'
GOPATH='/Users/rittneje/gomodtest'
GOPRIVATE='[REDACTED]'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/rittneje/sdk/go1.23rc2'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/rittneje/sdk/go1.23rc2/pkg/tool/darwin_amd64'
GOVCS='[REDACTED]'
GOVERSION='go1.23rc2'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/rittneje/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD=''
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/kf/kr7_s3xx0l12zbj3jrn082hmzy5gvy/T/go-build3078931624=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

As a minimal reproduction, we have a repository in GOPATH-mode, organized like so:

  • src
    • foo
      • main.go

main.go is defined as follows:

//go:build go1.10

package p

import "fmt"

func main() {
	var x any
	fmt.Println(x)
}

What did you see happen?

Compiling with 1.23rc2 fails:

src/foo/main.go:8:8: predeclared any requires go1.18 or later (file declares //go:build go1.10)

What did you expect to see?

It should still work, like it does in 1.22.

(Please note the above is just a minimal reproduction. In reality, the offending go:build directive is in a vendored dependency.)

rittneje avatar Jul 30 '24 19:07 rittneje

CC @griesemer

ianlancetaylor avatar Jul 30 '24 22:07 ianlancetaylor

I don't see what's wrong here: the file specifies the language version as go1.10 and any is not available in 1.10, so this error is legit. The code behaves the same for Go 1.21, Go 1.22 (not as claimed), and Go at tip (Go 1.23...).

If the issue exists in a larger code base, please provide a reproducer that accurately reflects the situation. Otherwise, please close this issue, thanks.

griesemer avatar Jul 30 '24 23:07 griesemer

The code behaves the same for Go 1.21, Go 1.22 (not as claimed), and Go at tip (Go 1.23...).

@griesemer False.

$ go version
go version go1.21.12 linux/amd64
$ go build ./src/...
$ echo $?
0
$ go version
go version go1.22.5 darwin/amd64
$ go build ./src/...
$ echo $?
0
$ ~/go/bin/go1.23rc2 version
go version go1.23rc2 darwin/amd64
$ ~/go/bin/go1.23rc2 build ./src/...
# _/Users/rittneje/gomodtest/src/foo
src/foo/main.go:8:8: predeclared any requires go1.18 or later (file declares //go:build go1.10)
$ echo $?
1

I'm going to guess that the playground compiles in "module mode" not "GOPATH mode" and thus is not a valid way to reproduce the issue I described.

rittneje avatar Jul 31 '24 03:07 rittneje

@rittneje I see. If the compiler is invoked without a valid -lang version, or a -lang version before go1.21, than file local versions are ignored and the code is compiled assuming the latest language version, despite the //go:build go1.10 line. Presumably that is what's happening in Go 1.22 and before.

I suspect a change in the way the go command invokes the compiler in GOPATH mode. @golang/tools-team for visibility.

griesemer avatar Jul 31 '24 03:07 griesemer

Yes, we changed how the go command invokes the compiler: https://go.dev/cl/593156 changes the go command to always pass in a language version (which in gopath mode it sets to the toolchain version, so as new a version as possible).

I think that the behavior of passing in the language version is the correct behavior. I also think that doing the downgrade is the correct behavior. But we can change the way the go command is invoked if it's decided that we don't want to do the downgrade in GOPATH mode (after all in the documentation of the buildconstraint downgrade it's mainly mentioned in the context of modules).

matloob avatar Jul 31 '24 15:07 matloob

@matloob I think it would be more helpful if there were a way for us to specify the language version to the compiler in GOPATH mode.

For example, right now any GOPATH-mode application compiled under 1.22+ will use the new loop semantics, without any ability to opt out (since they cannot use module directive).

rittneje avatar Jul 31 '24 16:07 rittneje

Perhaps GCFLAGS can be used to set -lang in GOPATH mode.

griesemer avatar Jul 31 '24 16:07 griesemer

@matloob I think it would be more helpful if there were a way for us to specify the language version to the compiler in GOPATH mode.

For example, right now any GOPATH-mode application compiled under 1.22+ will use the new loop semantics, without any ability to opt out (since they cannot use module directive).

@rittneje This is the sort of thing the //go:build downgrade behavior that you're seeing on your example is for. Just like adding //go:build go1.10 sets the go version to Go 1.10 (disabling the use of any), you could set //go:build go1.21 to revert to the old for loop semantics on a go file that relies on the old behavior. The rule is that if there's a //go:build go version constraint we us the oldest version implied by that constraint for that file.

matloob avatar Jul 31 '24 17:07 matloob

@matloob Yes, but that would mean that a GOPATH-mode repo has to add //go:build go1.21 to every single file when upgrading to 1.22, which isn't exactly developer-friendly to say the least. (Nor do I think it is particularly aligned with the spirit of Go's goals of backwards compatibility.)

rittneje avatar Jul 31 '24 18:07 rittneje

cc @samthanawalla

@rittneje I'm sorry, you're right. Looking at #60915, we decided to keep passing in language version go1.21 for GOPATH mode to avoid breaking compatibility. We'll fix this.

matloob avatar Jul 31 '24 18:07 matloob

@matloob It seems that with Go 1.22, a GOPATH-mode application will use the 1.22 loop semantics, even though it wasn't supposed to as per #60915. However, now that ship has sailed, and GOPATH-mode applications may well be relying on the 1.22 loop semantics, meaning that restoring 1.21 semantics as originally proposed is itself a breaking change.

rittneje avatar Jul 31 '24 18:07 rittneje

Change https://go.dev/cl/602216 mentions this issue: cmd/go: pin the language version passed to the compiler for GOPATH mode

gopherbot avatar Jul 31 '24 20:07 gopherbot

@rittneje That's a good point. My instinct is that we should still use "go1.21" as the language version passed to the compiler but I think there's no clear answer unfortunately.

I'll think about and discuss this with my colleagues to try to find the least negatively impactful solution.

matloob avatar Jul 31 '24 21:07 matloob

It would help a lot to decide what the right answer is here if we understood better what people still use GOPATH mode (non-module mode) for at all. What are the use cases for GOPATH these days?

rsc avatar Aug 01 '24 15:08 rsc

@rsc For us, we still use GOPATH mode because we have very large repositories with many packages right under src. Consequently, we'd have to rewrite every single import in order to make them compatible with module mode. We'd also have to track our dependencies differently. In short, it's a very large time sink.

I'll also note that for us, having GOPATH mode use 1.22 loop semantics is desirable.

rittneje avatar Aug 01 '24 21:08 rittneje

What are the use cases for GOPATH these days?

Write some simple code, such as benchmak's study of the performance gap between range-over-func and iter.Pull, such as gadgets for personal use cases (such as test AI that needs to merge multiple files into one file for easy upload). What I found in my GOPATH are these use cases.

qiulaidongfeng avatar Aug 02 '24 01:08 qiulaidongfeng

@aclements for visibility. We should try to resolve this one way or another for 1.23.

griesemer avatar Aug 06 '24 18:08 griesemer

GOPATH does not seem relevant here. //go:build lines that downgrade are not supposed to downgrade past the "event horizon" of Go 1.21, because in earlier versions of Go those lines did not have that effect. That is, this program compiled fine with Go 1.19 and it should continue to compile fine with newer versions of Go. The code that is handling the //go:build lines needs to treat versions less than 1.21 as meaning "Go 1.21".

(I may have it wrong and the event horizon is Go 1.20, but there's really no difference between those two.)

rsc avatar Aug 07 '24 16:08 rsc

I see what happened. The downgrade was disabled when the default Go version was old, but now that the default Go version in non-module code is "new", the downgrade is being enabled. The code is in cmd/compile/internal/types2/check.go:

	downgradeOk := check.version.cmp(go1_21) >= 0

	// determine Go version for each file
	for _, file := range check.files {
		// use unaltered Config.GoVersion by default
		// (This version string may contain dot-release numbers as in go1.20.1,
		// unlike file versions which are Go language versions only, if valid.)
		v := check.conf.GoVersion

		fileVersion := asGoVersion(file.GoVersion)
		if fileVersion.isValid() {
			// use the file version, if applicable
			// (file versions are either the empty string or of the form go1.dd)
			if pkgVersionOk {
				cmp := fileVersion.cmp(check.version)
				// Go 1.21 introduced the feature of setting the go.mod
				// go line to an early version of Go and allowing //go:build lines
				// to “upgrade” (cmp > 0) the Go version in a given file.
				// We can do that backwards compatibly.
				//
				// Go 1.21 also introduced the feature of allowing //go:build lines
				// to “downgrade” (cmp < 0) the Go version in a given file.
				// That can't be done compatibly in general, since before the
				// build lines were ignored and code got the module's Go version.
				// To work around this, downgrades are only allowed when the
				// module's Go version is Go 1.21 or later.
				//
				// If there is no valid check.version, then we don't really know what
				// Go version to apply.
				// Legacy tools may do this, and they historically have accepted everything.
				// Preserve that behavior by ignoring //go:build constraints entirely in that
				// case (!pkgVersionOk).
				if cmp > 0 || cmp < 0 && downgradeOk {    <<< THIS LINE
					v = file.GoVersion
				}
			}

			// Report a specific error for each tagged file that's too new.
			// (Normally the build system will have filtered files by version,
			// but clients can present arbitrary files to the type checker.)
			if fileVersion.cmp(go_current) > 0 {
				// Use position of 'package [p]' for types/types2 consistency.
				// (Ideally we would use the //build tag itself.)
				check.errorf(file.PkgName, TooNew, "file requires newer Go version %v", fileVersion)
			}
		}

Adding -lang= for GOPATH-compiled files introduced this problem because now this code can't tell the difference between a module with an explicit go line and the implicit GOPATH rule. I think this should be simplified by the one-line change:

-		if fileVersion.isValid() {
+		if fileVersion.isValid() && fileVersion.cmp(go1_21) >= 0 {

That is, ignore downgrades marked from before Go 1.21, because they weren't downgrades back then.

That may make some of the rest of the code null and void, but that's fine, it's the kind of very small, low-risk change we want for this stage in the release cycle.

rsc avatar Aug 07 '24 16:08 rsc

Change https://go.dev/cl/603895 mentions this issue: cmd/compile/internal/types2: only use fileVersion if 1.21 or greater

gopherbot avatar Aug 07 '24 17:08 gopherbot

@rsc Just to be clear, the suggested change will ignore any file-specific downgrade to a version lower than 1.21. Is that your suggestion?

With the proposed fix, with 1.23 it won't be possible to downgrade below 1.21 even if in module mode. This is something that is currently possible in 1.22 and we test for it explicitly. For instance, with -lang=go1.22, in module mode one can downgrade to 1.19. Perhaps that doesn't make any sense but it's a capability that existed for at least one version.

griesemer avatar Aug 07 '24 22:08 griesemer

@rsc Just to be clear, the suggested change will ignore any file-specific downgrade to a version lower than 1.21. Is that your suggestion?

Yes, that's right. There's no good reason to downgrade beyond 1.21 in module mode anyway, it just fell out from the heuristic we happened to choose, and I tested it. The only downgrade that matters is 1.22+ -> before 1.22, to get old for loop behavior. That one must be preserved. The others don't really matter.

rsc avatar Aug 09 '24 14:08 rsc

I made a change similar to the above: it changes the definition of downgradeOk

-			downgradeOk := check.version.cmp(go1_21) >= 0
+			downgradeOk := check.version.cmp(go1_21) >= 0 && fileVersion.cmp(go1_21) >= 0

(it also moves the definition inside the if where we check that fileVersion.isValid)

I think we need to do something like this so we can allow upgrades to versions below 1.21, which is needed to avoid breaking working files.

matloob avatar Aug 09 '24 19:08 matloob

Change https://go.dev/cl/604498 mentions this issue: all: update tests for disabling of file downgrades below go1.21

gopherbot avatar Aug 09 '24 20:08 gopherbot

Change https://go.dev/cl/604817 mentions this issue: all: disable tests incomatible with CL 603895

gopherbot avatar Aug 12 '24 17:08 gopherbot

Change https://go.dev/cl/604935 mentions this issue: [release-branch.go1.23] go/types, types2: only use fileVersion if 1.21 or greater

gopherbot avatar Aug 12 '24 18:08 gopherbot

Change https://go.dev/cl/604955 mentions this issue: internal/testfiles: adjust test so all modules are after 1.21

gopherbot avatar Aug 12 '24 20:08 gopherbot

Change https://go.dev/cl/604995 mentions this issue: go/analysis/passes/stdversion: adjust test for version downgrade

gopherbot avatar Aug 12 '24 20:08 gopherbot

Change https://go.dev/cl/604996 mentions this issue: go/ssa/interp: reenable tests

gopherbot avatar Aug 12 '24 20:08 gopherbot