go: cmd/compile: incorrect package initialization order for spec example

From https://golang.org/ref/spec#Package_initialization:

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.

[…]

For example, given the declarations
var (
	a = c + b
	b = f()
	c = f()
	d = 3
)

func f() int {
	d++
	return d
}
the initialization order is d, b, c, a.

If Go initializes b before c, then after initialization I’d expect the value of b to be 4 and c to be 5. However, this test outputs b as 5 and c as 4. Swapping the b and c declarations doesn’t change the output, but swapping the order in the addition in the declaration of a does change the output. Does this mean that the initialization order in the example is actually d, c, b, a? And that both LHS and RHS are in scope in the phrase “earliest in declaration order”? Or (more likely) am I missing something about what it means to declare and initialize a variable?

P.S. Location in current master:

https://github.com/golang/go/blob/a9afa4e933f3eff131f12e24bb0f5b9f3168ca14/doc/go_spec.html#L6341

About this issue

Original URL
State: closed
Created 7 years ago
Reactions: 3
Comments: 42 (18 by maintainers)

Commits related to this issue

cmd/compile: initialize variables in the right order The spec advertises the sort order for variable initialization as dependency and then declaration, but the compiler currently orders initializatio... — committed to chadwhitacre/go by chadwhitacre 6 years ago
cmd/compile: initialize variables in the right order The spec advertises the sort order for variable initialization as dependency and then declaration, but the compiler currently orders initializatio... — committed to chadwhitacre/go by chadwhitacre 5 years ago
cmd/compile: initialize variables in the right order The spec advertises the sort order for variable initialization as declaration (i.e., lhs of an assignment) and then dependency, but the compiler c... — committed to chadwhitacre/go by chadwhitacre 5 years ago
cmd/compile: change sinit.go functions into methods This will make it easier for subsequent CLs to track additional state during package initialization scheduling. Passes toolstash-check. Updates #... — committed to golang/go by mdempsky 5 years ago
cmd/compile: move sinit.go globals into InitSchedule Eliminates global state from sinit.go. Passes toolstash-check. Updates #22326. Change-Id: Ie3cb14bff625baa20134d1488962ab02d24f0c15 Reviewed-on... — committed to golang/go by mdempsky 5 years ago
spec: clarify language on package-level variable initialization The very first paragraph on "Package initialization" stated that "variables are initialized in declaration order, but after any variabl... — committed to golang/go by griesemer 5 years ago
cmd/compile: fix fmt_test.go after CL 170062 Updates #22326. Change-Id: Ia9173b6eb29b2a4f90f4ba39bf53b6e9b7a6d6bf Reviewed-on: https://go-review.googlesource.com/c/go/+/179398 Run-TryBot: Matthew De... — committed to golang/go by mdempsky 5 years ago

Most upvoted comments

Ftr, my first patch was buggy. I submitted a second patch, which passes the test suite.

chadwhitacre on Jan 6, 2019

For the record, gccgo gets this right.

ianlancetaylor on Oct 18, 2017

Yes, aware of it. Thanks.

On Mon, May 20, 2019, 5:33 PM Matthew Dempsky notifications@github.com wrote:

CL 170062 is ready for review, if it’s not too late in the cycle.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/22326?email_source=notifications&email_token=ACBCIT2IELKMAVQMTCWBWWLPWMKKPA5CNFSM4D7X4AKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV2ECVQ#issuecomment-494158166, or mute the thread https://github.com/notifications/unsubscribe-auth/ACBCIT3VMZEZYB6SWOUXDWTPWMKKPANCNFSM4D7X4AKA .

griesemer on May 21, 2019

Yes please.

josharian on Jan 4, 2019

Awesome. Lucky for you (?) we are in the freeze part of the release cycle (https://github.com/golang/go/wiki/Go-Release-Cycle), so thorough code review and iteration has to wait for a while anyway. Do go ahead and mail your change soon, just anticipate delays. (Sorry.)

josharian on Dec 27, 2018

sinit.go could use a substantial rewrite. However, that’s pretty non-trivial.

Last time I looked at adding state to some sinit.go methods, I found it pretty hard to work with, because walk and sinit call each other recursively, and it gets gnarly quickly. Maybe your needs will be more easily met.

Also, long term, we’d like to get rid of the current Node AST entirely, so any major refactoring to sinit.go would hopefully move in the direction of simplification and elimination of code, e.g. by shifting it into the SSA backend.

That’s not really a precise answer to your implicit question, but I hope it helps a little.

josharian on Jun 1, 2018

@whit537 For historical reasons (the compiler used to be written in C, so it couldn’t use Go’s testing package), the compiler is mostly tested through tests in $GOROOT/test. You can run those tests by cd’ing into $GOROOT/test and running go run run.go to run them all or something like go run run.go -- fixedbugs/bug345.go to run a single one.

It looks like there are some initialization related tests in there (e.g., $GOROOT/test/*init*.go).

mdempsky on Oct 25, 2017

@whit537 I’m not sure what you mean with the test passes. If you’re referring to go/types’s test then of course it passes. go/types does this correctly as I mentioned above. The bug is in the compiler (which doesn’t use go/types).

griesemer on Oct 18, 2017

Thanks for the issue. The spec example is correct but cmd/compile is wrong; and I’m pretty sure we have an issue for it, but I can’t find it at the moment. go/types also agrees with the spec, and there is a test case for this exact example here).

The spec says:

...by repeatedly initializing the next package-level variable that is earliest in declaration order and ready for initialization...

At first, a is dependent on c and b, so it must wait. Both b and c depend on d so they also must wait. Once d is initialized, both c and b have no (uninitialized) dependency and thus can be initialized. They must be initialized in declaration order; since b is declared before c, b must be initialized first. Hence you get the order d, b, c, a.

By introducing a little helper function, we can have the code print out the initialization order explicitly: https://play.golang.org/p/yiajBYfoSG . As you can see, the order is clearly wrong.

The reason it is wrong for cmd/compile is that the compiler doesn’t sort the “ready to initialize” variables in declaration order for some reason.

griesemer on Oct 18, 2017