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: breaking change in 1.23rc2 with version constraints in GOPATH mode #68658

Closed
rittneje opened this issue Jul 30, 2024 · 47 comments
Closed
Assignees
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rittneje
Copy link

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

@seankhliao seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Jul 30, 2024
@ianlancetaylor
Copy link
Contributor

CC @griesemer

@griesemer griesemer self-assigned this Jul 30, 2024
@griesemer
Copy link
Contributor

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 griesemer added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 30, 2024
@griesemer griesemer added this to the Go1.23 milestone Jul 30, 2024
@rittneje
Copy link
Author

rittneje commented Jul 31, 2024

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
Copy link
Contributor

@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 griesemer added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jul 31, 2024
@matloob
Copy link
Contributor

matloob commented Jul 31, 2024

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
Copy link
Author

@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 griesemer added NeedsFix The path to resolution is known, but the work has not been done. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 31, 2024
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Jul 31, 2024
@griesemer
Copy link
Contributor

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

@matloob
Copy link
Contributor

matloob commented Jul 31, 2024

@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
Copy link
Author

@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
Copy link
Contributor

matloob commented Jul 31, 2024

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 matloob added NeedsFix The path to resolution is known, but the work has not been done. release-blocker and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jul 31, 2024
@rittneje
Copy link
Author

@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
Copy link
Contributor

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

@matloob
Copy link
Contributor

matloob commented Jul 31, 2024

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/tools that referenced this issue Aug 27, 2024
CL 607955 is changing the behavior of "//go:build" file versions.
Before, file versions did not apply if go version was set for the
package, but after CL 6079055, the package's go version does not
influence whether the file version is applied: max(fileVersion, go1.21)
will always be applied.

Once CL 607955 is released in a go 1.23 minor release, we can update the
test to require go1.23 and test for the new behavior (though it would
fail for users building with a version of go older than the minor
release with the behavior update).

For golang/go#68658

Change-Id: I99d39ce108274edf401d861e553ad923b508f936
Reviewed-on: https://go-review.googlesource.com/c/tools/+/608797
Reviewed-by: Robert Griesemer <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@matloob
Copy link
Contributor

matloob commented Aug 27, 2024

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

@gopherbot
Copy link
Contributor

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 pushed a commit that referenced this issue Aug 27, 2024
Change the rules for how //go:build "file versions" are applied: instead
of considering whether a file version is an upgrade or downgrade from
the -lang version, always use max(fileVersion, go1.21). This prevents
file versions from downgrading the version below go1.21.  Before Go 1.21
the //go:build version did not have the meaning of setting the file's
langage version.

This fixes an issue that was appearing in GOPATH builds: Go 1.23.0
started providing -lang versions to the compiler in GOPATH mode (among
other places) which it wasn't doing before, and it set -lang to the
toolchain version (1.23). Because the -lang version was greater than
go1.21, language version used to compile the file would be set to the
//go:build file version. //go:build file versions below 1.21 could cause
files that could previously build to stop building.

For example, take a Go file with a //go:build line specifying go1.10.
If that file used a 1.18 feature, that use would compile fine with a Go
1.22 toolchain. But it would produce an error when compiling with the
1.23.0 toolchain because it set the language version to 1.10 and
disallowed the 1.18 feature. This breaks backwards compatibility: when
the build tag was added, it did not have the meaning of restricting the
language version.

For #68658

Change-Id: I6cedda81a55bcccffaa3501eef9e2be6541b6ece
Reviewed-on: https://go-review.googlesource.com/c/go/+/607955
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
@gopherbot
Copy link
Contributor

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 pushed a commit that referenced this issue Aug 28, 2024
…) if fileVersion present

Change the rules for how //go:build "file versions" are applied: instead
of considering whether a file version is an upgrade or downgrade from
the -lang version, always use max(fileVersion, go1.21). This prevents
file versions from downgrading the version below go1.21.  Before Go 1.21
the //go:build version did not have the meaning of setting the file's
langage version.

This fixes an issue that was appearing in GOPATH builds: Go 1.23.0
started providing -lang versions to the compiler in GOPATH mode (among
other places) which it wasn't doing before, and it set -lang to the
toolchain version (1.23). Because the -lang version was greater than
go1.21, language version used to compile the file would be set to the
//go:build file version. //go:build file versions below 1.21 could cause
files that could previously build to stop building.

For example, take a Go file with a //go:build line specifying go1.10.
If that file used a 1.18 feature, that use would compile fine with a Go
1.22 toolchain. But it would produce an error when compiling with the
1.23.0 toolchain because it set the language version to 1.10 and
disallowed the 1.18 feature. This breaks backwards compatibility: when
the build tag was added, it did not have the meaning of restricting the
language version.

For #68658
Fixes #69094

Change-Id: I6cedda81a55bcccffaa3501eef9e2be6541b6ece
Reviewed-on: https://go-review.googlesource.com/c/go/+/607955
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
(cherry picked from commit aeac0b6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/608935
@gopherbot
Copy link
Contributor

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
Copy link
Contributor

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

gopherbot pushed a commit to golang/tools that referenced this issue Aug 29, 2024
…03895

The tests disabled in this CL depend on being able to downgrade Go
versions to versions earlier than Go 1.21. This behavior will change
with CL 603895. Disable these tests for now and re-enable them with
fixes in a later CL.

For golang/go#68658

Change-Id: I13bdc03117989a128d90195ac90b2905102d293f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/604817
Commit-Queue: Michael Matloob <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Tim King <[email protected]>
Auto-Submit: Michael Matloob <[email protected]>
(cherry picked from commit 7f262d6)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/609416
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
TryBot-Bypass: Robert Findley <[email protected]>
gopherbot pushed a commit to golang/tools that referenced this issue Aug 29, 2024
…compatible with CL 607955

CL 607955 is changing the behavior of "//go:build" file versions.
Before, file versions did not apply if go version was set for the
package, but after CL 6079055, the package's go version does not
influence whether the file version is applied: max(fileVersion, go1.21)
will always be applied.

Once CL 607955 is released in a go 1.23 minor release, we can update the
test to require go1.23 and test for the new behavior (though it would
fail for users building with a version of go older than the minor
release with the behavior update).

For golang/go#68658

Change-Id: I99d39ce108274edf401d861e553ad923b508f936
Reviewed-on: https://go-review.googlesource.com/c/tools/+/608797
Reviewed-by: Robert Griesemer <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
(cherry picked from commit adb7301)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/609415
TryBot-Bypass: Robert Findley <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/tools that referenced this issue Sep 27, 2024
For golang/go#68658

Change-Id: I6e05e21f924cda18e368543e59071b2c79c16ef2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/615698
Commit-Queue: Tim King <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/615685 mentions this issue: go/analysis/passes/stdversion: reenable tests

@timothy-king
Copy link
Contributor

@gopherbot please backport this to 1.22. It's necessary to have consistent file versions with build tags with later versions.

@timothy-king timothy-king reopened this Sep 27, 2024
@dmitshur
Copy link
Contributor

dmitshur commented Oct 2, 2024

@timothy-king You're running into friction issue #25574 – there was already a backport request in #68658 (comment). As a workaround, you can create the 1.22 backport issue manually.

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/tools that referenced this issue Oct 2, 2024
Enable units tests after 1.23. Tests were rewritten for the changes
to the 1.21 file version event horizon.

The test is not enabled for 1.22. A backport for golang/go#68658 may be required.

For golang/go#68658

Change-Id: I15f46e3ff3de7c02592d5a357ae199116323c29f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/615685
Reviewed-by: Alan Donovan <[email protected]>
Commit-Queue: Tim King <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@timothy-king
Copy link
Contributor

The decision for the backport for 1.22 can be done on #69749. Closing this issue which is about 1.23 and 1.24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Done
Development

No branches or pull requests