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 calling generic fn which has related type params (reproduces with 1.21 and 1.20, not at tip) #64759

Closed
corhere opened this issue Dec 15, 2023 · 15 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 Dec 15, 2023

Go version

go version go1.21.5 darwin/amd64

What operating system and processor architecture are you using (go env)?

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/Users/corysnider/Library/Caches/go-build'
GOENV='/Users/corysnider/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Volumes/Workspace/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Volumes/Workspace'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/Cellar/go/1.21.5/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/Cellar/go/1.21.5/libexec/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.21.5'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/dev/null'
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/8p/tzn5bkn967vfgttpr2d2kpjm0000gq/T/go-build2320458225=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

$ cat repro.go
//go:build go1.18

package repro

func genericfn[S ~[]E, E any](s S) S {
	return s
}

func do() []string {
	return genericfn([]string{})
}
$ go tool compile -lang=go1.16 repro.go

What did you expect to see?

Compilation succeeds. It succeeds with go version devel go1.22-b60bf8f8e1 Fri Dec 15 19:35:21 2023 +0000 darwin/amd64.

What did you see instead?

embedding interface element ~[]string requires go1.18 or later (-lang was set to go1.16; check go.mod)
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 15, 2023
@mdempsky
Copy link
Contributor

This looks like a typechecker issue. My understanding of 804d786's commit message is that this should be valid. (Not sure if there's more permanent documentation describing the expected behavior here; for example, I don't see any mention of the backwards compatibility logic for M >= 21 at https://pkg.go.dev/cmd/go. /cc @rsc)

/cc @griesemer @findleyr

@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 15, 2023
@griesemer
Copy link
Contributor

I cannot reproduce this. With

$ go version
go version go1.22rc1 darwin/amd64

and x.go:

$ cat x.go
//go:build go1.18

package repro

func genericfn[S ~[]E, E any](s S) S {
	return s
}

func do() []string {
	return genericfn([]string{})
}

compilation succeeds:

$ go tool compile -lang=go1.16 x.go
$

To double-check, I added a print statement in the relevant code section, and it does indeed select go1.18 for that file.

Please verify that you are indeed using a new compiler version.
I'm going to close this for now. Feel free to re-open if you have additional evidence, thanks.

@griesemer griesemer removed the NeedsFix The path to resolution is known, but the work has not been done. label Dec 19, 2023
@corhere corhere changed the title cmd/compile: //go:build file version ignored when calling generic fn which has related type params [go1.21] cmd/compile: //go:build file version ignored when calling generic fn which has related type params [1.21 only] Dec 20, 2023
@corhere
Copy link
Author

corhere commented Dec 20, 2023

@griesemer I know that Go 1.22 is not affected. I said as much in the issue report. The issue is reproducible on Go 1.21.5.

@corhere corhere changed the title [go1.21] cmd/compile: //go:build file version ignored when calling generic fn which has related type params [1.21 only] cmd/compile: //go:build file version ignored when calling generic fn which has related type params [1.21 only] Dec 20, 2023
@griesemer
Copy link
Contributor

@corhere Ah. Apologies for not reading more carefully.

Hm, the relevant code didn't exist for 1.21.5 I believe. Not sure if we can easily/should backport.

Reopening, to determine next steps.

@griesemer griesemer reopened this Dec 20, 2023
@griesemer griesemer self-assigned this Dec 20, 2023
@griesemer griesemer added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 20, 2023
@griesemer griesemer added this to the Go1.22 milestone Dec 20, 2023
@griesemer
Copy link
Contributor

griesemer commented Dec 21, 2023

Confirming that this issue exists for go1.21.5.
It appears that we don't have correct position information when computing a type set and then the type checker doesn't use the file-specific version information. This is definitely a bug.

Smaller reproducer source code:

//go:build go1.18
package p

func f[S ~[]E, E any](S) {}

func _() {
	f([]string{})
}

@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 21, 2023
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 21, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/555275 mentions this issue: [release-branch.go1.21] go/types, types2: don't lose position info of interface embeddings

@dmitshur
Copy link
Contributor

@griesemer This appears to reproduce with Go 1.20.13 as well, is that right?

If so, our backport policy is to support last two Go versions equally (see here). Will the backport CL you've prepared work on release-branch.go1.20 as well? Please see https://go.dev/wiki/MinorReleases for information on the backport process. Specifically, this issue is in the Go1.22 milestone, but you should ask gopherbot to create tracking backport issues, which will then be reviewed. Thanks.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/555295 mentions this issue: go/types, types2: don't lose position info of interface embeddings

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/555296 mentions this issue: go/types, types2: don't lose position info of interface embeddings

@dmitshur dmitshur changed the title cmd/compile: //go:build file version ignored when calling generic fn which has related type params [1.21 only] cmd/compile: //go:build file version ignored when calling generic fn which has related type params (reproduces with 1.21 and 1.20, not at tip) Jan 10, 2024
@griesemer
Copy link
Contributor

We will fix this issue in the 1.22 branch (the code is incorrect there as well, though the issue seems to be masked) and then backport.

@dmitshur dmitshur moved this from Todo to In Progress in Go Compiler / Runtime Jan 10, 2024
@griesemer
Copy link
Contributor

@gopherbot Please consider this for backport to 1.21 and 1.20, this causes a compilation failure for code that chooses to select a future Go version in one of its source files.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #65052 (for 1.20), #65053 (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.

gopherbot pushed a commit that referenced this issue Jan 11, 2024
Accurate position information for embedded types in interfaces is
crucial to identify the corresponding source file, and with that
the Go language version associated with that file. (The position
information is also important for proper error messages.)

Before this CL, the position information for embedded types was
discarded after type set computation, in the assumption that it
was not needed anymore. However, substitutions that update the
interface may lead to repeated type set computations which then
won't have the correct position information.

This CL does preserve the position information for embedded
types until the end of type checking (cleanup phase), and also
copy the position information during a substitution of the
interface.

The respective bug (#64759) doesn't seem to appear in 1.22 (most
likely because it's hidden by some of the changes made with respect
to the file version logic), but the existing code is still wrong.
The backport of this code to 1.21 and 1.20 fixes the issue in those
releases.

For #64759.

Change-Id: I80f4004c9d79cb02eac6739c324c477706615102
Reviewed-on: https://go-review.googlesource.com/c/go/+/555296
Run-TryBot: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/555415 mentions this issue: [release-branch.go1.21] go/types, types2: don't lose position info of interface embeddings

@griesemer
Copy link
Contributor

Forward-compatibility (specifically, the ability to have a go:build line indicate a Go version that is younger than the Go version used in the .mod file) was introduced with Go 1.21.

The fix for this issue does not need to (and cannot) be backported to 1.20.

@griesemer
Copy link
Contributor

This was fixed at tip (1.22).

@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime Jan 11, 2024
gopherbot pushed a commit that referenced this issue Jan 25, 2024
… interface embeddings

Accurate position information for embedded types in interfaces is
crucial to identify the corresponding source file, and with that
the Go language version associated with that file. (The position
information is also important for proper error messages.)

Before this CL, the position information for embedded types was
discarded after type set computation, in the assumption that it
was not needed anymore. However, substitutions that update the
interface may lead to repeated type set computations which then
won't have the correct position information.

This CL does preserve the position information for embedded
types until the end of type checking (cleanup phase), and also
copy the position information during a substitution of the
interface.

The respective bug (#64759) doesn't seem to appear in 1.22 (most
likely because it's hidden by some of the changes made with respect
to the file version logic), but the existing code is still wrong.
The backport of this code to 1.21 and 1.20 fixes the issue in those
releases.

For #64759.
Fixes #65053.

Change-Id: I80f4004c9d79cb02eac6739c324c477706615102
Reviewed-on: https://go-review.googlesource.com/c/go/+/555296
Run-TryBot: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/555415
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Accurate position information for embedded types in interfaces is
crucial to identify the corresponding source file, and with that
the Go language version associated with that file. (The position
information is also important for proper error messages.)

Before this CL, the position information for embedded types was
discarded after type set computation, in the assumption that it
was not needed anymore. However, substitutions that update the
interface may lead to repeated type set computations which then
won't have the correct position information.

This CL does preserve the position information for embedded
types until the end of type checking (cleanup phase), and also
copy the position information during a substitution of the
interface.

The respective bug (golang#64759) doesn't seem to appear in 1.22 (most
likely because it's hidden by some of the changes made with respect
to the file version logic), but the existing code is still wrong.
The backport of this code to 1.21 and 1.20 fixes the issue in those
releases.

For golang#64759.

Change-Id: I80f4004c9d79cb02eac6739c324c477706615102
Reviewed-on: https://go-review.googlesource.com/c/go/+/555296
Run-TryBot: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
robmry added a commit to robmry/moby that referenced this issue Feb 29, 2024
Avoid golang/go#64759

Signed-off-by: Rob Murray <rob.murray@docker.com>
robmry added a commit to robmry/moby that referenced this issue Mar 1, 2024
Avoid golang/go#64759

Co-authored-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
robmry added a commit to robmry/moby that referenced this issue Mar 1, 2024
Avoid golang/go#64759

Co-authored-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
robmry added a commit to robmry/moby that referenced this issue Mar 1, 2024
Avoid golang/go#64759

Co-authored-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
Dzejrou pushed a commit to Dzejrou/moby that referenced this issue Mar 5, 2024
Avoid golang/go#64759

Co-authored-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
Dzejrou pushed a commit to Dzejrou/moby that referenced this issue Mar 5, 2024
Avoid golang/go#64759

Co-authored-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.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
None yet
Development

No branches or pull requests

5 participants