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

rittneje commented 1 month ago

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.)

gabyhelp commented 1 month ago

Related Issues and Documentation

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

ianlancetaylor commented 1 month ago

CC @griesemer

griesemer commented 1 month ago

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.

rittneje commented 1 month ago

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.

griesemer commented 1 month ago

@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.

matloob commented 1 month ago

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).

rittneje commented 1 month ago

@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).

griesemer commented 1 month ago

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

matloob commented 1 month ago

@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.

rittneje commented 1 month ago

@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.)

matloob commented 1 month ago

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.

rittneje commented 1 month ago

@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.

gopherbot commented 1 month ago

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

matloob commented 1 month ago

@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.

rsc commented 1 month ago

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?

rittneje commented 1 month ago

@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.

qiulaidongfeng commented 1 month ago

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.

griesemer commented 1 month ago

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

rsc commented 1 month ago

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 commented 1 month ago

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.

gopherbot commented 1 month ago

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

griesemer commented 1 month ago

@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.

rsc commented 1 month ago

@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.

matloob commented 1 month ago

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.

gopherbot commented 1 month ago

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

gopherbot commented 1 month ago

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

gopherbot commented 4 weeks ago

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 commented 4 weeks ago

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

gopherbot commented 4 weeks ago

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

gopherbot commented 4 weeks ago

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

gopherbot commented 4 weeks ago

Closed by merging CL 604935 (commit 63b0f805cd83f97c43a45e9558d00513c2399fbf) to release-branch.go1.23.

matloob commented 4 weeks ago

@aclements @cherrymui There was a question about the behavior for downgrades with -lang >= 1.21. The behavior we submitted is to avoid downgrades for language versions earlier than go1.21. As @cherrymui pointed out in a comment on golang.org/cl/603895 , this introduces a discontinuity in behavior:

-lang	version in //go:build directive	determined version
1.23	1.19	1.23
1.23	1.20	1.23
1.23	1.21	1.21
1.23	1.22	1.22
1.23	1.23	1.23

An alternative to avoid this discontinuity is to have //go:build directives with go versions earlier than go1.21 to downgrade to go 1.21 instead of avoiding the downgrade completely. That would give us the following behavior:

-lang	version in //go:build directive	determined version
1.23	1.19	1.21
1.23	1.20	1.21
1.23	1.21	1.21
1.23	1.22	1.22
1.23	1.23	1.23

This alternative is a bit more complex, but it still allows us to avoid the GOPATH breakages with downgrading past 1.21.

timothy-king commented 4 weeks ago

@cherrymui pointed out that the go version determines the lifetimes of variables declared by for/range loops when going from 1.21 to 1.22. It could be quite confusing [and theoretically buggy] that a 1.19 //go:build directive does not pin to the <=1.21 semantics.

gopherbot commented 2 weeks ago

Change https://go.dev/cl/607955 mentions this issue: go/types, types2: use max(fileVersion, go1.21) if fileVersion present

gopherbot commented 2 weeks ago

Change https://go.dev/cl/608797 mentions this issue: internal/versions: disable a test case incompatible with CL 607955

matloob commented 2 weeks ago

@gopherbot please backport this to 1.23. It's necessary to unbreak GOPATH mode with certain build tags

gopherbot commented 2 weeks ago

Backport issue(s) opened: #69094 (for 1.23).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

gopherbot commented 2 weeks ago

Change https://go.dev/cl/608935 mentions this issue: [release-branch.go1.23] go/types, types2: use max(fileVersion, go1.21) if fileVersion present

gopherbot commented 1 week ago

Change https://go.dev/cl/609415 mentions this issue: [gopls-release-branch.0.16] internal/versions: disable a test case incompatible with CL 607955

gopherbot commented 1 week ago

Change https://go.dev/cl/609416 mentions this issue: [gopls-release-branch.0.16] all: disable tests incompatible with CL 603895

gopherbot commented 1 week ago

Change https://go.dev/cl/609436 mentions this issue: cmd/go/testdata/script: add a test case for issue #68658

golang / go