Skip to content

spec: initialization ordering does not consider references in interface methods #68568

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

Closed
karalabe opened this issue Jul 24, 2024 · 16 comments
Closed
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.

Comments

@karalabe
Copy link
Contributor

Go version

go version go1.22.5 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/karalabe/Library/Caches/go-build'
GOENV='/Users/karalabe/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/karalabe/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/karalabe/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.22.5/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.22.5/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.5'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/karalabe/work/sources/github.com/karalabe/go-init-bug/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 -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/bc/q6rslcrn5tngrfvr9w8s45vr0000gn/T/go-build2401450664=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

I hit a global variable init order that depends on the file build order (i.e. lexicographic order). I haven not used init() methods that could be problematic according to the spec, rather only defined variables, types and called methods on the types.

Repro needs multiple files, please see: https://github.com/karalabe/go-init-bug

What did you see happen?

Depending on the file order, the Go compiler accesses not-yet-initialized variables.

What did you expect to see?

I expect the Go compiler to correctly order my type definitions and variable declarations.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 24, 2024
@karalabe
Copy link
Contributor Author

karalabe commented Jul 24, 2024

Been digging into this a bit more, I can trigger it within a single file too:

https://go.dev/play/p/ArZIYeg1g-L

package main

import (
	"fmt"
)

type X struct{}
type Y struct{}

type Sizer interface {
	Size() int
}
func Size(obj Sizer) int { return obj.Size() }

var sizeBad = 2 * Size(new(Y)) // Change this to new(Y).Size() and it works correctly

func (*X) Size() int { return 100 }

var sizeY = Size(new(X)) + 10
func (*Y) Size() int { return sizeY }

func main() {
	fmt.Println(sizeBad)
}

Seems the root cause is that the interface call is not detected as an initialization dependency.

Edit: Which is still wrong, because nowhere in the code can any of the sizes be 0 since they are all initialized on declaration.

@karalabe
Copy link
Contributor Author

karalabe commented Jul 24, 2024

Further simplified: https://go.dev/play/p/uGhQfR0tUKH

package main

import (
	"fmt"
)

// A boring interface and a method operating on it
// -----------------------------------------------
type Sizer interface {
	Size() int
}

func Size(obj Sizer) int {
	return obj.Size()
}
// -----------------------------------------------

var sizeBad = 2 * Size(new(S))

func Size1() int { return 1 }
var size2 = Size1() + 1

type S struct{}
func (*S) Size() int { return size2 }

func main() {
	fmt.Println(sizeBad)
}

Outputs 0, expected 4.

@ianlancetaylor
Copy link
Contributor

CC @griesemer

This suggests that when initialization code refers to a type, we should look through all methods of the type for any initialization. However, I would not be surprised if introducing that introduces initialization loop errors for some existing code.

Rearranging the code does change the behavior, which is unfortunate.

@ianlancetaylor ianlancetaylor changed the title cmd/compile: invalid variable init order, succeeded in using uninitialized globals spec: initialization ordering does not consider references in methods Jul 24, 2024
@karalabe
Copy link
Contributor Author

@ianlancetaylor The interface part in there is important too for some reason. If I were to replace Size(new(S)) with new(S).Size() then it works correctly, so it does consider references in methods, just it seems to break when crossing an interface boundary.

@cuonglm
Copy link
Member

cuonglm commented Jul 24, 2024

I think this behavior is allowed by spec, since interface methods are not considered for dependency references.

Dependency analysis is performed per package; only references referring to variables, functions, and (non-interface) methods declared in the current package are considered. If other, hidden, data dependencies exists between variables, the initialization order between those variables is unspecified.

@cuonglm cuonglm changed the title spec: initialization ordering does not consider references in methods spec: initialization ordering does not consider references in interface methods Jul 24, 2024
@karalabe
Copy link
Contributor Author

FWIW, the example from the spec does indeed produce the same behavior https://go.dev/play/p/yVeZx2xrsGO

That said, seems like something that would be better off fixed then left as a footgun?

@karalabe
Copy link
Contributor Author

Perhaps just a description of my use case.

I have a code generator that can generate encoder/decoders between Go types and a specific binary format. As part of that format, static objects have fixed sizes and I'd like to precompute and store these sizes in some global package-private variable for each type so that I don't have to iterate over the fields of the types over and over and over all the time.

Since I can have nested objects, the size of an outer object naturally depends on the size of the internal ones (i.e. fields), so when pre-computing the outer size, I need the ensure the inner size is already pre-computed. Now, since this is a code generator that users can just run on their own types, it's out of my control as to what file order they'll end up having, and thus what init order they'll end up with.

But if this bug persists, then my outer type's size calculation might precede an inner type's size calculation, thus breaking everything. As this is not even a compilation error, the user will be none the wiser and this will result in a runtime error (or rather panic in my case).

Unfortunately I haven't figured out a way to force Go to actually forcefully run a size calculation on the type before calling my interface method. This could be highlighted in https://go.dev/play/p/-QEeLdw5kxx with the example from the specs, where I added a y = a declaration as the first one, but the compiler still delays evaluating this and rather does the evaluation of the line afterwards it which uses an uninitialised a. This kind of highlights that I can't even generate some trick into my code that would ensure proper ordering of events.

@cuonglm
Copy link
Member

cuonglm commented Jul 24, 2024

How about:

  • Always generate code that involve interface methods call at the end of your file?
  • Or do not use interface methods call at all?

@karalabe
Copy link
Contributor Author

Always generate code that involve interface methods call at the end of your file?

I am generating one file per type, so unfortunately end or beginning is the same because the ordering issue arises across files. Sure, I could merge everything together an reorder them but then it becomes an unmaintainable mess that would be very hard to find issues in.

Or do not use interface methods call at all?

Unfortunately my size calculation is non trivial, so inlining all that code for every type just feels the wrong solution.

@cuonglm
Copy link
Member

cuonglm commented Jul 24, 2024

Unfortunately my size calculation is non trivial, so inlining all that code for every type just feels the wrong solution.

What do you mean when saying "inlining all that code for every type"?

I mean instead of generating Size(new(Y)), you could just generate new(Y).Size() which will work.

You can still do var _ Sizer = (*Y)(nil) to ensure Y satisfy your Sizer.

@ianlancetaylor
Copy link
Contributor

Apologies for missing that interface methods are involved.

I don't see how we can handle initialization ordering correctly (or perhaps I should say "correctly") when interface methods are involved. In the general case we aren't going to be able to figure out the dynamic type of an interface value, so we aren't going to be able to know which method was called, so we aren't going to be able to ensure that all references from that method was fully initialized before the interface conversion occurs.

So I don't see what change we can make here that will keep initialization simple and reliable.

@zigo101
Copy link

zigo101 commented Jul 24, 2024

This is known for a long time, and for some reason, it is deliberately unspecified:

@griesemer
Copy link
Contributor

griesemer commented Jul 24, 2024

What @iant and @cuonglm said.

Since initialization only considers the package at hand, maybe one could statically determine the set of all possible interfaces and thus methods and take them into account somehow. But even if that's possible the behavior would be difficult to understand. In any case, if such an approach exists, it would need to be discussed via a proposal.

Generally, the reason for the package initialization mechanism in Go is to provide sensible values for global variables with initialization expressions and to make simple cases straight-forward w/o requiring much extra work.

If a program requires a very specific or complex initialization order, that program should use explicitly written code that handles initialization as that will be much clearer than relying on an inferred complex initialization order that may be hard to follow by a reader.

Closing this as working as intended.

@karalabe
Copy link
Contributor Author

karalabe commented Jul 25, 2024

I'm a bit torn on this. Whilst I can accept that it's hard to do and possibly not worth it, behaving surprisingly depending on type/file ordering seems like a footgun to have in the code. Perhaps a middleground could be to have the Go linter detect such cases and warn about it?

By such cases I mean in general having interface calls in global initers.

@griesemer
Copy link
Contributor

Perhaps a middleground could be to have the Go linter detect such cases and warn about it?

Feel free to give it a shot.

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.
Projects
Development

No branches or pull requests

7 participants