golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
120.67k stars 17.33k forks source link

cmd/compile: package-level variable initialization with constant dependencies doesn't match order specified in Go spec #66575

Closed bigwhite closed 1 week ago

bigwhite commented 2 months ago

Go version

go 1.22.0

Output of go env in your module/workspace:

$go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/Users/tonybai/Library/Caches/go-build'
GOENV='/Users/tonybai/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/tonybai/Go/pkg/mod'
GONOPROXY='bitbucket.org/bigwhite/t'
GOOS='darwin'
GOPATH='/Users/tonybai/Go'
GOPROXY='https://goproxy.cn'
GOROOT='/Users/tonybai/.bin/go1.22.0'
GOSUMDB='off'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/tonybai/.bin/go1.22.0/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='clang'
CXX='clang++'
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 -fdebug-prefix-map=/var/folders/cz/sbj5kg2d3m3c6j650z0qfm800000gn/T/go-build1615674647=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

I have example below:

// main.go

package main

import (
    "fmt"
)

var (
    v0 = constInitCheck()
    v1 = variableInit("v1")
    v2 = variableInit("v2")
)

const (
    c1 = "c1"
    c2 = "c2"
)

func constInitCheck() string {
    if c1 != "" {
        fmt.Println("main: const c1 has been initialized")
    }
    if c1 != "" {
        fmt.Println("main: const c2 has been initialized")
    }
    return ""
}

func variableInit(name string) string {
    fmt.Printf("main: var %s has been initialized\n", name)
    return name
}

func init() {
    fmt.Println("main: first init func invoked")
}

func init() {
    fmt.Println("main: second init func invoked")
}

func main() {
    // do nothing
}

What did you see happen?

Compile and run the code in go 1.22.0,I got this output:

$go run main.go
main: var v1 has been initialized
main: var v2 has been initialized
main: const c1 has been initialized
main: const c2 has been initialized
main: first init func invoked
main: second init func invoked

What did you expect to see?

According to the latest go spec:

Within a package, package-level variable initialization proceeds stepwise, with each step selecting the variable earliest in declaration order which has no dependencies on uninitialized variables.

More precisely, a package-level variable is considered ready for initialization if it is not yet initialized and either has no [initialization expression](https://go.dev/ref/spec#Variable_declarations) or its initialization expression has no dependencies on uninitialized variables. Initialization proceeds by repeatedly initializing the next package-level variable that is earliest in declaration order and ready for initialization, until there are no variables ready for initialization.

v0、v1 and v2 all have initializaiton expression , and should be considered as "not ready for initialization"。their init order should be v0 -> v1 -> v2。but the real init order is v1 -> v2 -> v0。

bigwhite commented 2 months ago

If we place const declaration block before var declaration block, the init order will become v0 -> v1 -> v2:

$go run main.go // go1.22.0
main: const c1 has been initialized
main: const c2 has been initialized
main: var v1 has been initialized
main: var v2 has been initialized
main: first init func invoked
main: second init func invoked
seankhliao commented 2 months ago

constInitCheck() has a dependency on c1 and c2, which are uninitialized at that point. variableInit has no dependency. this all appears consistent with the current spec.

bigwhite commented 2 months ago

@seankhliao thanks for instant reply. Go spec says "no dependencies on uninitialized variables" , but c1 and c2 are uninitialized constants. there is a little bit of inconsistency.

randall77 commented 2 months ago

This looks like it changed between 1.21 and 1.22. I agree with the OP, constants are not variables. @gri

randall77 commented 2 months ago

@griesemer

gopherbot commented 2 months ago

Change https://go.dev/cl/575075 mentions this issue: cmd/compile: put constants before variables in initialization order

randall77 commented 2 months ago

Bisect points to https://go-review.googlesource.com/c/go/+/517617, switching to use types2 for init order, which makes sense. types2 always had this bug. FYI @mdempsky

zigo101 commented 2 months ago

[edit]: BTW, should this be backported? As it may cause some wrong initial values: https://go.dev/play/p/2HA4DR3_cjD


Just a curious BTW question: why is a initialized after b? By my understanding of the spec, the order should be inverted.

package main

var x = 0
var a = foo()
var b = x

func foo() int {
    x++
    return x
}

func main() {
    println(a, b) // 1 0
}
ianlancetaylor commented 2 months ago

@go101 Thanks. Would you mind opening a separate issue for that? The gc compiler prints 1, 0 while gccgo prints 1, 1.

randall77 commented 2 months ago

Reopening for @griesemer to decide if anything needs to change in the spec.

@go101 I don't think this issue warrants backporting. The workaround is pretty easy.

griesemer commented 1 week ago

@go101 Re: your question about this code:

package main

var x = 0
var a = foo()
var b = x

func foo() int {
    x++
    return x
}

func main() {
    println(a, b) // 1 1 for Go 1.23
}

The result should be 1 1: x is initialized first as it doesn't depend on any other variables and thus is ready for initialization and it's first in the source. Then a is initialized: it depends on f which depends on x but x is initialized, so f can proceed and the result is 1 for a. Then b is set to the value of a which is 1. This matches gccgo.

With respect to the original issue, constants are never "initialized", they have their values already at compile time. The bug was fixed with CL 575075.

(Technical aside: constants appear in the implementation code determining initialization order only because that code is also used to detect invalid cycles among constant declarations; constants don't have an impact on initialization order. The bug was that constants were treated like variables w/o dependencies and thus their source order influenced the initialization order. The fix was to prioritize a constant always before any variable which effectively removes them from the initialization order - they are "pre-initialized" if you will.)

With respect to the spec: I don't think anything needs to change. The spec explicitly talks about variables, not constants in this context.

With respect to backporting: I think we should backport this. It's easy enough and it is a bug with respect to the spec.

Closing this issue as fixed but will create a backport issue.

griesemer commented 1 week ago

@gopherbot please consider this for backport to 1.22, it's a bug with respect to the spec.

gopherbot commented 1 week ago

Backport issue(s) opened: #67820 (for 1.22).

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