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/compile: //go:build file version ignored when using generic function from package "slices" in Go 1.21 #66064

Closed
corhere opened this issue Mar 1, 2024 · 21 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@corhere
Copy link

corhere commented Mar 1, 2024

Go version

go version go1.21.7 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/root/.cache/go-build'
GOENV='/root/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.7'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/go/src/repro/go.mod'
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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build936666295=/tmp/go-build -gno-record-gcc-switches'

What did you do?

https://go.dev/play/p/-yiceeveEDz?v=goprev

Details

//go:build go1.21

package main

import "slices"

func main() {
	_ = slices.Clone([]string{})
}
-- go.mod --
module play.ground

go 1.16

What did you see happen?

Go 1.21:

embedding interface element ~[]string requires go1.18 or later (-lang was set to go1.16; check go.mod)

Go build failed.

Go 1.22:


Program exited.

What did you expect to see?

I expect the program to type-check successfully on Go 1.21, same as it does with Go 1.22.

This issue is making it impossible to use packages in non-module repositories which use the slices or maps packages with Go modules as the synthesized go.mod file has an implied language version of go1.16.

@corhere corhere changed the title cmd/compile(?): //go:build file version ignored when calling package "slices" functions (reproduces with 1.21, not 1.22) cmd/compile: //go:build file version ignored when calling package "slices" functions (reproduces with 1.21, not 1.22) Mar 1, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 1, 2024
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 1, 2024
@Jorropo
Copy link
Member

Jorropo commented Mar 2, 2024

Does this has anything to do with slices ?
I mean, in general I don't think we adapt -lang based on build tags and anything trying to use newer language features behind build tags wouldn't work.

That looks like a Go 2 proposal to me.

@zigo101
Copy link

zigo101 commented Mar 2, 2024

The priority of //go:build should be higher than the go directive in go,mod file. So both should compile and run okay.

It is true that this is a "slices" unrelated problem.

@thediveo
Copy link

thediveo commented Mar 3, 2024

That looks like a Go 2 proposal to me.

Why? I lack the insight, so please help me out.

In any case, I've run into this because of Docker dependencies. What basically happens is that there are always non-mod indirect dependencies. If the Go 1 toolchain breaks than this is a severe issue when not being able to use the existing code.

You can't update the whole world ... or can you?

@mknyszek mknyszek changed the title cmd/compile: //go:build file version ignored when calling package "slices" functions (reproduces with 1.21, not 1.22) cmd/go: //go:build file version ignored when calling package "slices" functions (reproduces with 1.21, not 1.22) Mar 6, 2024
@mknyszek mknyszek added GoCommand cmd/go and removed compiler/runtime Issues related to the Go compiler and/or runtime. labels Mar 6, 2024
@mknyszek mknyszek added this to the Backlog milestone Mar 6, 2024
@mknyszek
Copy link
Contributor

mknyszek commented Mar 6, 2024

CC @bcmills and @matloob maybe?

@bcmills
Copy link
Contributor

bcmills commented Mar 7, 2024

This looks like a cmd/compile issue to me: it's not clear to me whether this is working as designed, but at the very least the error message should be clearer. The user code isn't explicitly embedding the type anywhere.

@bcmills bcmills added compiler/runtime Issues related to the Go compiler and/or runtime. and removed GoCommand cmd/go labels Mar 7, 2024
@bcmills bcmills changed the title cmd/go: //go:build file version ignored when calling package "slices" functions (reproduces with 1.21, not 1.22) cmd/compile: //go:build file version ignored when using generic function from package "slices" in Go 1.21 Mar 7, 2024
@bcmills
Copy link
Contributor

bcmills commented Mar 7, 2024

Actually, was respecting per-file Go versions a feature itself added in 1.22?

@mknyszek
Copy link
Contributor

mknyszek commented Mar 7, 2024

CC @golang/compiler

@griesemer
Copy link
Contributor

The error is coming from the type checker. Go 1.21 did introduce the ability to upgrade the language version per file, so I would expect this to work. Investigating.

@griesemer griesemer self-assigned this Mar 8, 2024
@griesemer griesemer modified the milestones: Backlog, Go1.23 Mar 8, 2024
@griesemer
Copy link
Contributor

griesemer commented Mar 11, 2024

Related to #64759 which addressed another aspect of the same problem. In that issue, we had missing position information for embedded elements of interfaces in the same package. In this issue we have missing position information for an imported type.

@griesemer
Copy link
Contributor

griesemer commented Mar 11, 2024

Analysis: During type-checking, the method

func (t *Interface) Empty() bool { return t.typeSet().IsAll() }

is invoked which calls Interface.typeSet. That function determines the type set and in order to do so, checks language version compatibility in some cases (here for the implicitly embedded interface element ~[]string). To check the version correctly, we need to know the respective file, and in order to know the file, we need to know the position of the embedding. There's no explicit position provided to the type set computation, and we don't appear to have a (stored) embedding position in this case and then the version check fails in Go 1.21. The same problem exists in Go 1.22, but the version check has changed (Checker.allowVersion) and in fact may have a separate bug (it simply ignores the file information if there's no position), which hides this issue in Go 1.22.

Furthermore, because the type in question here is imported, we don't have position information in the first place for embedded elements. In fact we shouldn't do a version check at all in this case.

@griesemer
Copy link
Contributor

cc: @findleyr for visibility

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/571075 mentions this issue: [release-branch.go1.21] go/types, types2: don't do version checks for embedded types of imported interfaces

gopherbot pushed a commit that referenced this issue Mar 12, 2024
… embedded types of imported interfaces

Imported interfaces don't have position information for embedded types.
When computing the type set of such interfaces, doing a version check
may fail because it will rely on the Go version of the current package.

We must not do a version check for features of types from imported
packages - those types have already been typechecked and are "correct".
The version check code does look at packages to avoid such incorrect
version checks, but we don't have the package information available
in an interface type (divorced from its object).

Instead, for now rely on the fact that imported interfaces don't have
position information for embedded types: if the position is unknown,
don't do a version check.

We may want to assert that positions are known in all other cases,
but since this is an older release, don't add such additional changes
to avoid introducing other bugs.

Fixes #66064.

Change-Id: I773d57e5410c3d4a911ab3e018b3233c2972b3c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/571075
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Griesemer <gri@google.com>
@griesemer
Copy link
Contributor

Closing as fixed in the 1.21 branch.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/571137 mentions this issue: go/types, types2: don't do version checks for embedded types of imported interfaces

@cherrymui
Copy link
Member

Reopened. CL reverted. Will re-submit later. Thanks.

@cherrymui cherrymui reopened this Mar 13, 2024
@mknyszek mknyszek moved this to In Progress in Go Compiler / Runtime Mar 13, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime Mar 14, 2024
@cherrymui
Copy link
Member

@gopherbot please backport this to Go 1.21 release. This causes build failure for valid Go code. We'll re-apply CL 571075.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #66326 (for 1.21).

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

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 20, 2024
@thanm
Copy link
Contributor

thanm commented Mar 26, 2024

Hi @griesemer what is the status of this bug with respect to fixes on the release branches? Are there any CLs that need to be submitted? Saw comment by @cherrymui about how there was a revert, so wanted to check (wearing my release interrupts hat). Thanks.

@griesemer
Copy link
Contributor

griesemer commented Mar 26, 2024

I don't know what the status is at the moment. @cherrymui mentioned that she will re-submit.

@thanm
Copy link
Contributor

thanm commented Mar 26, 2024

Thanks. I will look into it then...

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/574736 mentions this issue: [release-branch.go1.21] go/types, types2: don't do version checks for embedded types of imported interfaces

gopherbot pushed a commit that referenced this issue Mar 27, 2024
… embedded types of imported interfaces

[This is a re-apply of CL 571075]

Imported interfaces don't have position information for embedded types.
When computing the type set of such interfaces, doing a version check
may fail because it will rely on the Go version of the current package.

We must not do a version check for features of types from imported
packages - those types have already been typechecked and are "correct".
The version check code does look at packages to avoid such incorrect
version checks, but we don't have the package information available
in an interface type (divorced from its object).

Instead, for now rely on the fact that imported interfaces don't have
position information for embedded types: if the position is unknown,
don't do a version check.

We may want to assert that positions are known in all other cases,
but since this is an older release, don't add such additional changes
to avoid introducing other bugs.

Fixes #66326.
Updates #66064.

Change-Id: I158cf51aa382f85d612ab958ba4b591de1c5fdb2
Reviewed-on: https://go-review.googlesource.com/c/go/+/574736
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Development

No branches or pull requests