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: declaring a variable before a particular type definition causes compiler crash #50276

Closed
jhbrown94 opened this issue Dec 20, 2021 · 11 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jhbrown94
Copy link

jhbrown94 commented Dec 20, 2021

What version of Go are you using (go version)?

go version go1.18beta1 darwin/amd64

Does this issue reproduce with the latest release?

N/A - uses generics.

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

Go Playground, or on my Mac OS X 10.14.6 (intel)

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/jhbrown/Library/Caches/go-build"
GOENV="/Users/jhbrown/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/jhbrown/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/jhbrown/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/d6/kh18314s5wj315ls2klq51nr0000gn/T/go-build2660062083=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have a (legal) recursive type. Declaring a variable of one of those types before the type definition causes a compiler crash in a minimal example, see https://go.dev/play/p/BbenDNoSDAu?v=gotip

In my original codebase, it causes an incorrect compiler error (rather than a crash) when I declare the variable in a different file than the types are declared in, but I haven't been able to preserve that behavior in reducing it to a self-contained example.

This is blocking development.

What did you expect to see?

The code should compile.

What did you see instead?

The compiler crashes. As the example shows, moving the variable declaration to a different spot in the file is sufficient to make the code compile.

@jhbrown94
Copy link
Author

jhbrown94 commented Dec 20, 2021

This may be a duplicate of #50259 but in the example in that bug report, the types are incorrect and should not compile, whereas I believe the types in this report are correct and should compile.

@odeke-em odeke-em added this to the Go1.18 milestone Dec 20, 2021
@thanm
Copy link
Contributor

thanm commented Dec 20, 2021

This is blocking development.

Just curious as to why the alias is required? When I take it out (e.g. replace line 21 with "type Step Pair[Box, interface{}]" your code seems to compile ok.

Aliases are a pretty boutique/obscure Go feature -- they are (in my experience) really only needed when moving types around between packages.

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Dec 20, 2021
@jhbrown94
Copy link
Author

jhbrown94 commented Dec 20, 2021

This is blocking development.

Just curious as to why the alias is required? When I take it out (e.g. replace line 21 with "type Step Pair[Box, interface{}]" your code seems to compile ok.

That's a fair point, this is work-aroundable in that way.

Aliases are a pretty boutique/obscure Go feature -- they are (in my experience) really only needed when moving types around between packages.

I suspect that you'll find aliases come into heavy use now that Go has generic types -- it's suddenly an important form of abstraction. You're right that I can replace every instance of Step with Pair[Box, interface{}] but that is generally burdensome if I'm using Step in a lot of places, and in particular makes a refactoring (say, to replace interface{} with a more specific type) more painful than it ought to be.

@thanm
Copy link
Contributor

thanm commented Dec 20, 2021

You're right that I can replace every instance of Step with Pair[Box, interface{}]

Just to be clear, I never made that suggestion. I just suggested that you use

type Step Pair[Box, interface{}]

instead of

type Step = Pair[Box, interface{}]

Thanks.

@jhbrown94
Copy link
Author

jhbrown94 commented Dec 20, 2021

You're right that I can replace every instance of Step with Pair[Box, interface{}]

Just to be clear, I never made that suggestion. I just suggested that you use

type Step Pair[Box, interface{}]

instead of

type Step = Pair[Box, interface{}]

Thanks.

Ugh, indeed you did. Total failure of reading comprehension on my part. My apologies. That also works, including in the original codebase. It feels slightly dirty as it creates a new type where I don't really intend to, but the semantics works out in the end.

@griesemer
Copy link
Contributor

griesemer commented Jan 4, 2022

Simpler reproducer:

package p

type Transform[T any] struct{}
type Pair[S any] struct {}

// Comment this line out to make this compile
var first Transform[Step]

type Box Transform[Step]
type Step = Pair[Box]

The type checker passes this code (though I have not yet verified that it's doing the correct thing).

@griesemer
Copy link
Contributor

griesemer commented Jan 5, 2022

I agree that this is likely a duplicate of #50259. Leaving open for now until confirmed.

@griesemer
Copy link
Contributor

griesemer commented Jan 12, 2022

Removing release-blocker label as there are work-arounds for this code.

@griesemer
Copy link
Contributor

griesemer commented Jan 21, 2022

Confirming that this is a duplicate of #50259. Will be closed with forthcoming fix.

@gopherbot
Copy link

gopherbot commented Jan 21, 2022

Change https://golang.org/cl/379916 mentions this issue: cmd/compile/internal/types2: report an error when using a broken alias

@gopherbot
Copy link

gopherbot commented Jan 21, 2022

Change https://golang.org/cl/380056 mentions this issue: cmd/compile/internal/types2: reorder object processing to avoid broken aliases

gopherbot pushed a commit that referenced this issue Jan 24, 2022
By processing non-alias type declarations before alias type declaration,
and those before everything else we can avoid some of the remaining
errors which are due to alias types not being available.

For #25838.
For #50259.
For #50276.
For #50729.

Change-Id: I233da2899a6d4954c239638624dfa8c08662e6b9
Reviewed-on: https://go-review.googlesource.com/c/go/+/380056
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
The type checker doesn't have a general mechanism to "use" the type
of a type alias whose type depends on a recursive type declaration
which is not yet completely type-checked. In some cases, the type of
a type alias is needed before it is determined; the type is incorrect
(invalid) in that case but no error is reported. The type-checker is
happy with this (incorrect type), but the compiler may crash under
some circumstances.

A correct fix will likely require some form of forwarding type which
is a fairly pervasive change and may also affect the type checker API.

This CL introduces a simple side table, a map of broken type aliases,
which is consulted before the type associated with a type alias is
used. If the type alias is broken, an error is reported.

This is a stop-gap solution that prevents the compiler from crashing.
The reported error refers to the corresponding issue which suggests
a work-around that may be applicable in some cases.

Also fix a minor error related to type cycles: If we have a cycle
that doesn't start with a type, don't use a compiler error message
that explicitly mentions "type".

Fixes golang#50259.
Fixes golang#50276.
Fixes golang#50779.

For golang#50729.

Change-Id: Ie8e38f49ef724e742e8e78625e6d4f3d4014a52c
Reviewed-on: https://go-review.googlesource.com/c/go/+/379916
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
By processing non-alias type declarations before alias type declaration,
and those before everything else we can avoid some of the remaining
errors which are due to alias types not being available.

For golang#25838.
For golang#50259.
For golang#50276.
For golang#50729.

Change-Id: I233da2899a6d4954c239638624dfa8c08662e6b9
Reviewed-on: https://go-review.googlesource.com/c/go/+/380056
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants