[-v] cmd/compile: incorrect package initialization order for spec example

[chadwhitacre]

https://github.com/golang/go/issues/22326

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.

[chadwhitacre]

Seems like this is the thing to understand:

https://github.com/whit537/go/blob/2c1d2e06afee98d0770427d8b6c29bd9971a0999/src/cmd/compile/internal/gc/sinit.go#L243-L254

[chadwhitacre]

And are there tests for this? What was the test mentioned on the main ticket? How do I run it?

[chadwhitacre]

go/types also agrees with the spec, and there is a test case for this exact example here).

What is go/types? How do I run that test?

[chadwhitacre]

https://golang.org/cmd/go/#hdr-Test_packages

[chadwhitacre]

$ go test
main.go:8:2: use of internal package not allowed
main.go:9:2: use of internal package not allowed
main.go:10:2: use of internal package not allowed
main.go:11:2: use of internal package not allowed
main.go:12:2: use of internal package not allowed
main.go:13:2: use of internal package not allowed
main.go:14:2: use of internal package not allowed
main.go:15:2: use of internal package not allowed
main.go:16:2: use of internal package not allowed
main.go:17:2: use of internal package not allowed
$ p
/Users/whit537/personal/golang/go/src/cmd/compile
$

[chadwhitacre]

$ go test src/cmd/compile/internal/gc/
can't load package: package src/cmd/compile/internal/gc: cannot find package "src/cmd/compile/internal/gc" in any of:
        /usr/local/Cellar/go/1.9.1/libexec/src/src/cmd/compile/internal/gc (from $GOROOT)
        /Users/whit537/go/src/src/cmd/compile/internal/gc (from $GOPATH)
$

[chadwhitacre]

Let's get that api_test.go running ...

$ go test api_test.go 
# command-line-arguments
api_test.go:14:2: use of internal package not allowed
FAIL    command-line-arguments [setup failed]
$

[griesemer]

FYI: In go/types directory: go test -run InitOrder.

[chadwhitacre]

@griesemer Yeah, just got there. You do your own work, I'll ping you on https://github.com/golang/go/issues/22326 if I get stuck. :)

[chadwhitacre]

Interesting that https://github.com/golang/go/issues/22326#issuecomment-337677612 links to https://golang.org/src/go/types/api_test.go?#L542 but in local checkout and fork here the line is 609:

https://github.com/whit537/go/blob/2c1d2e06afee98d0770427d8b6c29bd9971a0999/src/go/types/api_test.go#L609

¯\_(ツ)_/¯

[chadwhitacre]

🤔

$ go test -run TestInitOrderInfo
# _/Users/whit537/personal/golang/go/src/go/types
api_test.go:14:2: use of internal package not allowed
FAIL    _/Users/whit537/personal/golang/go/src/go/types [setup failed]
$ p
/Users/whit537/personal/golang/go/src/go/types
$ go test -run InitOrder
# _/Users/whit537/personal/golang/go/src/go/types
api_test.go:14:2: use of internal package not allowed
FAIL    _/Users/whit537/personal/golang/go/src/go/types [setup failed]
$

[chadwhitacre]

https://github.com/whit537/go/blob/2c1d2e06afee98d0770427d8b6c29bd9971a0999/src/go/types/api_test.go#L14

[chadwhitacre]

Uuuuuuuuhhhhh

https://stackoverflow.com/questions/41060764/golang-disable-use-of-internal-package-not-allowed

P.S. https://stackoverflow.com/questions/16935965/how-to-run-test-cases-in-a-specified-file → https://golang.org/pkg/testing/#hdr-Subtests_and_Sub_benchmarks

[chadwhitacre]

Okay, first gear ...

An import of a path containing the element “internal” is disallowed if the importing code is outside the tree rooted at the parent of the “internal” directory.

Wuuuuh?

[chadwhitacre]

if the importing code is outside the tree rooted at the parent of the “internal” directory

[chadwhitacre]

https://docs.google.com/document/d/1e8kOo3r51b2BWtTs_1uADIA5djfXhPT36s6eHVRIvaU/edit

[chadwhitacre]

https://golang.org/cmd/go/#hdr-Internal_Directories

[chadwhitacre]

Maybe something about envvars? GOROOT jumping out at me in that gdoc ...

[chadwhitacre]

importable only by code in the directory tree rooted at the parent of "internal"

Soooooo we are trying to import api_test.go from code outside the directory tree rooted at the parent of api_test.go.

[chadwhitacre]

What is "tree rooted at the parent of the “internal” directory"?

[chadwhitacre]

/Users/whit537/personal/golang/go/src/go
total 0
drwxr-xr-x  14 whit537  staff   476 Oct 18 12:06 ./
drwxr-xr-x  68 whit537  staff  2312 Oct 18 12:06 ../
drwxr-xr-x  15 whit537  staff   510 Oct 18 12:06 ast/
drwxr-xr-x  12 whit537  staff   408 Oct 18 15:23 build/
drwxr-xr-x   4 whit537  staff   136 Oct 18 12:06 constant/
drwxr-xr-x  16 whit537  staff   544 Oct 18 12:06 doc/
drwxr-xr-x   5 whit537  staff   170 Oct 18 12:06 format/
drwxr-xr-x   3 whit537  staff   102 Oct 18 12:06 importer/
drwxr-xr-x   5 whit537  staff   170 Oct 18 12:06 internal/
drwxr-xr-x  10 whit537  staff   340 Oct 18 12:06 parser/
drwxr-xr-x   8 whit537  staff   272 Oct 18 12:06 printer/
drwxr-xr-x   6 whit537  staff   204 Oct 18 12:06 scanner/
drwxr-xr-x   7 whit537  staff   238 Oct 18 12:06 token/
drwxr-xr-x  50 whit537  staff  1700 Oct 18 12:06 types/
$

[chadwhitacre]

But:

$ ls -l types/|grep api_test
-rw-r--r--   1 whit537  staff  41010 Oct 18 12:06 api_test.go
$

🤔

[chadwhitacre]

$ echo $GOROOT

$ echo $GOPATH

$

Soooooo what r those about?

[chadwhitacre]

https://stackoverflow.com/questions/7970390/what-should-be-the-values-of-gopath-and-goroot

[chadwhitacre]

https://golang.org/cmd/go/#hdr-GOPATH_environment_variable, GOROOT mentioned in there as well but not given its own section afaict.

[chadwhitacre]

Note: GOROOT must be set only when installing to a custom location.

https://golang.org/doc/install#tarball_non_standard ... well, we're running from source, so I guess that probably counts as non-standard?

[chadwhitacre]

Progress! 💃

$ export GOROOT=/Users/whit537/personal/golang/go
$ go test -run InitOrder
can't load package: package go: no Go files in /Users/whit537/personal/golang/go/src/go
$

[chadwhitacre]

Ooooor not! 🙈

$ go test -run InitOrder
can't load package: package .: no Go files in /Users/whit537/personal/golang/go/src
$ echo $GOROOT

$ p
/Users/whit537/personal/golang/go/src
$

[chadwhitacre]

Start over:

$ p
/Users/whit537/personal/golang/go/src/go/types
$ echo $GOROOT

$ go test -run InitOrder
# _/Users/whit537/personal/golang/go/src/go/types
api_test.go:14:2: use of internal package not allowed
FAIL    _/Users/whit537/personal/golang/go/src/go/types [setup failed]
$

[chadwhitacre]

Yeah okay progress:

$ export GOROOT=/Users/whit537/personal/golang/go
$ go test -run InitOrder
# runtime/internal/sys
compile: version "devel +3ba818c Wed Oct 18 18:46:04 2017 +0000" does not match go tool version "go1.9.1"
FAIL    go/types [build failed]
$

[chadwhitacre]

So I need GOROOT/bin on PATH ya?

[chadwhitacre]

$ ./bin/go version
go version devel +3ba818c Wed Oct 18 18:46:04 2017 +0000 darwin/amd64
$

[chadwhitacre]

$ go version
go version devel +3ba818c Wed Oct 18 18:46:04 2017 +0000 darwin/amd64
$

[chadwhitacre]

💃

$ p
/Users/whit537/personal/golang/go/src/go/types
$ echo $GOROOT
/Users/whit537/personal/golang/go
$ go test -run InitOrder
PASS
ok      go/types        0.015s
$

[chadwhitacre]

Okay! Now why the heck do these tests pass?

[chadwhitacre]

Hmm ... looks like maybe it's only evaluating one level of ordering? Not recursing once d is declared?

https://github.com/whit537/go/blob/2c1d2e06afee98d0770427d8b6c29bd9971a0999/src/go/types/api_test.go#L622-L624

[chadwhitacre]

So this is setting up test cases, how are the cases tested?

https://github.com/whit537/go/blob/2c1d2e06afee98d0770427d8b6c29bd9971a0999/src/go/types/api_test.go#L700-L718

[chadwhitacre]

What is the info? It has InitOrders?

https://github.com/whit537/go/blob/2c1d2e06afee98d0770427d8b6c29bd9971a0999/src/go/types/api_test.go#L701

[chadwhitacre]

https://github.com/whit537/go/blob/2c1d2e06afee98d0770427d8b6c29bd9971a0999/src/go/types/api.go#L134-L141

[chadwhitacre]

https://github.com/whit537/go/blob/2c1d2e06afee98d0770427d8b6c29bd9971a0999/src/go/types/api.go#L216-L221

[chadwhitacre]

So InitOrder needs to be mutated and it's not? Or ... ?

[chadwhitacre]

https://github.com/whit537/go/blob/2c1d2e06afee98d0770427d8b6c29bd9971a0999/src/go/types/initorder.go#L12-L21

[chadwhitacre]

For the record, gccgo gets this right.

---

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

[chadwhitacre]

Okay, so that sounds more like:

• write a failing test for cmd/compile
• fix it

[chadwhitacre]

Actually tho I don't understand the relationship between go/types and cmd/compile. Apparently the compiler doesn't use go/types, but isn't that where the ordering code is?

https://github.com/whit537/go/issues/1#issuecomment-337730313

[chadwhitacre]

Notes while offline:

---

Why are there two kinds of init?

go/types/initorder.go cmd/compile/internal/go/sinit.go

---

Why doesn't the compiler use go/types? Should it?

---

InitOrder tests under go/types are under api_test.go. Are there tests for sinit functions somewhere other than sinit_test.go (which doesn't exist)?

---

No.

---

So I guess we make a new sinit_test.go and come up with a failing test case for the example?

---

How does sinit work? What are the functions and what calls what?

---

initplan is interesting

---

Yeah looks like some sort of top-level? initlist, initplans hmm ...

---

Actually no ... what is?

---

What in here is called from somewhere else outside this file?

---

Ah, yes! initfix :)

---

What's this "Node"?

---

Does it recurse?

// initfix computes initialization order for a list l of top-level // declarations and outputs the corresponding list of statements // to include in the init() function body.

---

Node comes from syntax.go.

---

It has a nodeInitorder field.

    nodeInitorder, _                   // tracks state during init1; two bits
    _, _                               // second nodeInitorder bit

---

So for this test I want to take the input from the example and end up with a node to feed to initfix.

---

work plan:

• [x] get a basic new test passing
• [ ] figure out how to generate a node from the example text
• [ ] pass it in to initfix(?) and figure out what to assert about it
• [ ] whittle down to a minimal case
• [ ] identify bug
• [ ] fix bug

---

Do we need to test initfix or fninit?

---

pkgFor over in api_test.go seems to be where the string is hydrated to an AST over there.

---

So do I use api:Check for to hydrate or stay out of go/types?

---

$ go test internal/gc/init_test.go 
--- FAIL: TestInitOrder (0.00s)
        init_test.go:20: 0: y u no greet? :(
FAIL
FAIL    command-line-arguments  0.008s
$

---

$ go test internal/gc/init_test.go 
ok      command-line-arguments  0.008s
$

---

Doesn't seem to be a fantastic testing story for Main.

---

I can parse syntax into a file. But if the hypothesis is that fninit isn't called enough by Main, then Main is what needs testing.

---

Let's take it that the problem is under fninit. That should be easier to isolate.

---

So we need to convert a text string into a list of Nodes for fninit to consume. Is that what noder is about?

---

Sort of. noder.parseFiles returns a list of lines, but then a lot happens in Main between the call to parseFiles and the call to fninit.

---

And the call to parseFiles itself is between initUniverse and finishUniverse. O.O

---

Then Phases 1 through 7, then fninit.

---

Can we hook lower? In init2? Can we find the bug by visual inspection?

---

Is initfix supposed

[chadwhitacre]

Alright so I have a parsed ... thing? *Node?

[chadwhitacre]

eeb72c38f716790ed8f495eb328670cf3bababf4

[chadwhitacre]

$ go test sinit_test.go
# command-line-arguments
./sinit_test.go:8:2: imported and not used: "cmd/compile/internal/gc"
./sinit_test.go:22:3: undefined: initfix
FAIL    command-line-arguments [build failed]
$

diff --git a/src/cmd/compile/internal/gc/sinit_test.go b/src/cmd/compile/internal/gc/sinit_test.go
index b6a7bf2..c07d9a8 100644
--- a/src/cmd/compile/internal/gc/sinit_test.go
+++ b/src/cmd/compile/internal/gc/sinit_test.go
@@ -5,6 +5,7 @@
 package gc

 import (
+   "cmd/compile/internal/gc"
    "cmd/compile/internal/syntax"
    "testing"
 )
@@ -18,6 +19,7 @@ func TestInitOrder(t *testing.T) {
    for i, test := range tests {

        parsed, err := syntax.ParseBytes(nil, []byte(test.src), nil, nil, nil, 0)
+       initfix(parsed)

        if test.src != `Greetings, program!` {
            t.Errorf("%d: y u no greet? :(", i)

[chadwhitacre]

$ go test sinit_test.go 
# command-line-arguments
./sinit_test.go:22:3: cannot refer to unexported name gc.initfix
./sinit_test.go:22:3: undefined: gc.initfix
FAIL    command-line-arguments [build failed]
$

diff --git a/src/cmd/compile/internal/gc/sinit_test.go b/src/cmd/compile/internal/gc/sinit_test.go
index b6a7bf2..370cef1 100644
--- a/src/cmd/compile/internal/gc/sinit_test.go
+++ b/src/cmd/compile/internal/gc/sinit_test.go
@@ -5,6 +5,7 @@
 package gc

 import (
+   "cmd/compile/internal/gc"
    "cmd/compile/internal/syntax"
    "testing"
 )
@@ -18,6 +19,7 @@ func TestInitOrder(t *testing.T) {
    for i, test := range tests {

        parsed, err := syntax.ParseBytes(nil, []byte(test.src), nil, nil, nil, 0)
+       gc.initfix(parsed)

        if test.src != `Greetings, program!` {
            t.Errorf("%d: y u no greet? :(", i)