cmd/compile: internal compiler error: incomplete itab

[rogpeppe]

commit 098599003ba78225152d22984f82f78892221dad

The following code panics: https://gotipplay.golang.org/p/FR0-a4NF08e

package main

type S struct{}

func (S) M() byte {
    return 0
}

type I[T any] interface {
    M() T
}

func F[T, A any](x I[T]) {
    _ = x.(A)
}

func main() {
    F[byte, string](S{})
}

The panic that I see is:

goroutine 1 [running]:
runtime/debug.Stack()
    /home/rogpeppe/go/src/runtime/debug/stack.go:24 +0x65
cmd/compile/internal/base.FatalfAt({0x3f0460?, 0xc0?}, {0xd20742, 0xf}, {0x0, 0x0, 0x0})
    /home/rogpeppe/go/src/cmd/compile/internal/base/print.go:227 +0x1ca
cmd/compile/internal/base.Fatalf(...)
    /home/rogpeppe/go/src/cmd/compile/internal/base/print.go:196
cmd/compile/internal/reflectdata.writeITab(0xc000498700, 0xc0003f0460, 0xc00049f960)
    /home/rogpeppe/go/src/cmd/compile/internal/reflectdata/reflect.go:1310 +0x42b
cmd/compile/internal/reflectdata.ITabLsym(0x138cd80?, 0xc00049d520?)
    /home/rogpeppe/go/src/cmd/compile/internal/reflectdata/reflect.go:856 +0x19c
cmd/compile/internal/noder.(*genInst).finalizeSyms(0x138cd80)
    /home/rogpeppe/go/src/cmd/compile/internal/noder/stencil.go:1727 +0x64d
cmd/compile/internal/noder.(*genInst).buildInstantiations(0x138cd80, 0x1)
    /home/rogpeppe/go/src/cmd/compile/internal/noder/stencil.go:76 +0xb0
cmd/compile/internal/noder.BuildInstantiations(...)
    /home/rogpeppe/go/src/cmd/compile/internal/noder/stencil.go:47
cmd/compile/internal/noder.(*irgen).generate(0xc0004c2000, {0xc00011eb90, 0x2, 0x203000?})
    /home/rogpeppe/go/src/cmd/compile/internal/noder/irgen.go:320 +0x3db
cmd/compile/internal/noder.check2({0xc00011eb90, 0x2, 0x2})
    /home/rogpeppe/go/src/cmd/compile/internal/noder/irgen.go:92 +0x16d
cmd/compile/internal/noder.LoadPackage({0xc00013a0f0, 0x2, 0x0?})
    /home/rogpeppe/go/src/cmd/compile/internal/noder/noder.go:90 +0x335
cmd/compile/internal/gc.Main(0xd497d8)
    /home/rogpeppe/go/src/cmd/compile/internal/gc/main.go:191 +0xb13
main.main()
    /home/rogpeppe/go/src/cmd/compile/main.go:55 +0xdd

Thanks to @norunners on the Gophers Slack for reporting this.

[danscales]

Interesting. This error is because in the instantiation of F[byte, string], we are guaranteed to get a run-time error at line 14, because an I[byte] value cannot possibly be converted to string, since string has no M method. We need to catch this while generating dictionary, and not try to generate the I[byte] -> string itab, but instead just force an error at runtime at line 14 if F[byte,string] is called.

Same compiler crash happens for a type switch:

package main

type S struct{}

func (S) M() byte {
    return 0
}

type I[T any] interface {
    M() T
}

func F[T, A any](x I[T]) {
    switch x.(type) {
    case A:
        println("hi")
    }
}

func main() {
    F[byte, string](S{})
}

I don't think this is a beta-1 release blocker, but we may be able to fix it in time.

@randall77

[rogpeppe]

just force an error at runtime at line 14

Why would there be a runtime error at line 14? Surely it should just fail to select the case?

[danscales]

Yes, for the 2nd example that I added (type-switch), there would be no runtime error. The case would just never match. The run-time error was in reference to the original example that does a type assert at line 14.

[mdempsky]

As discussed with @danscales earlier, this is an interesting case because it's code where the simple textual expansion of the code would be invalid normally. E.g., we don't allow interface{M()}(nil).(string) because string doesn't have an M method; but we currently allow interface{M()}(nil).(T) where T is a type parameter any, which can be instantiated as string.

I think there are two consistent alternatives on how to resolve this:

(1) We only allow interface{M()}(nil).(T) if T is constrained to have method M. That is, reject the above code during type checking. (2) We always allow interface{M()}(nil).(T), even if T is instantiated as a type that doesn't have method M. (It would still runtime panic though.)

I think choice 1 is more conservative, but more work to implement. Choice 2 is more consistent with how we handle duplicate cases in generic type switches though (i.e., normally switch interface{}(nil).(type) { case int, int: } is invalid, but switch interface{}(nil).(type) { case int, T: } is okay even if T is instantiated as int).

@griesemer @ianlancetaylor @findleyr

[gopherbot]

Change https://golang.org/cl/369774 mentions this issue: go/types: type assertions to type parameters should be static

[findleyr]

I think (1) is probably not that hard, maybe https://golang.org/cl/369774 suffices? Also, @griesemer it looks like this is fall-out from the recent change to the underlying of a type parameter to be its constraint interface.

That CL is just a proof of concept; I'm still not sure what's correct here. However, we have thus far erred on the side of disallowing features that we're unsure about.

[randall77]

I'm kinda leaning toward number 2. The reason you can't do

var x interface{m()} = ...
x.(string)

in regular code isn't that we don't know what the answer should be. It clearly should be panic (and unconditionally, at that). It's just that why would someone write that? Pretty clearly not intended. So we issue a compile-time error.

Whereas if T is a type parameter constrained by any, writing

var x interface{m()} = ...
x.(T)

in generic code clearly makes sense and isn't obviously unintended. It will not always panic (even though we could statically determine instantiations which would unconditionally panic). And the answer isn't ambiguous or anything, we know what the answer should be.

[rogpeppe]

(1) We only allow interface{M()}(nil).(T) if T is constrained to have method M. That is, reject the above code during type checking.

Are you saying that you'd reject the instantiation of F in main? Or that you'd reject the definition of F in main because it's known that it's only instantiated with types that can't satisfy the type switch? (presumably you could only do that if we're in the main package, otherwise it could be instantiated with some type A that does actually have an M method.

We always allow interface{M()}(nil).(T), even if T is instantiated as a type that doesn't have method M. (It would still runtime panic though.)

This is what I'd expect (it wouldn't runtime panic in the type switch or comma-ok form, presumably).

To my mind, the "can never happen" type conversion check is a convenience because it's easy to check in the static case. This case isn't static, so I don't think the compiler error is appropriate.

[mdempsky]

@findleyr Nice, that seems simple enough.

@randall77 I think the situation of how to compile either of those is equally clear. We could easily make x.(string) just unconditionally panic. But even if we disallow x.(T) in the second case, users will still be able to write interface{}(x).(T). So we're not fundamentally taking away any power from them.

[mdempsky]

@rogpeppe My suggestion (1) is that we reject the declaration of F, independent of any instantiations. This is also what @findleyr's CL above implements.

[rogpeppe]

@mdempsky Why would we want to reject the declaration of F when it can manifestly be instantiated correctly? https://gotipplay.golang.org/p/3ytXbPMMR7h

    F[byte, S](S{})

[mdempsky]

@rogpeppe A design principle of generics is that if a generic type/function declaration is valid, then all instantiations should be valid too. There shouldn't be cases where changing the implementation detail of a generic function could suddenly result in some downstream uses to start failing to compile.

That design isn't perfectly achieved today (e.g., chan T will fail to instantiate for T larger than 64KiB, but we could fix this by autoboxing large element types), but it's something we've strived for in other areas.

[findleyr]

@mdempsky Why would we want to reject the declaration of F when it can manifestly be instantiated correctly?

@rogpeppe independent of what we decide here, I don't think the rule of thumb is "anything that can be instantiated correctly should be instantiated correctly". We are allowing a lot of new programs in 1.18, and are extending the compatibility promise to those programs (modulo bugs). I think it's reasonable to disallow something like this (perhaps temporarily) because we don't fully understand its consequences, or because the implementation is too complicated, or we're unsure.

[rogpeppe]

A design principle of generics is that if a generic type/function declaration is valid, then all instantiations should be valid too.

I'd say that was true in this case. All instantiations are indeed valid, because the static "cannot happen" compiler check that you can have in a regular function isn't appropriate in this case.

My concern here is that by getting people into the habit of writing any(v).(T) rather than just v.(T), we will be losing opportunities for the compiler to point out genuinely impossible type conversions.

For example: https://gotipplay.golang.org/p/6blhiU8j_ZO

[rogpeppe]

I think this is perhaps analogous to the way that unsafe.Sizeof is not a constant in generic functions, although it is in non-generic functions.

[mdempsky]

My concern here is that by getting people into the habit of writing any(v).(T) rather than just v.(T), we will be losing opportunities for the compiler to point out genuinely impossible type conversions.

My expectation is that even among users who learn about using any(v).(T) as an idiom, will still first attempt to write v.(T). In your example, I expect this to still produce the "conflicting types for M method" error message. If users see that and still think to write any(v).(T), I'm inclined to think that's on them.

But we can always review user reports in the future and relax the rules.

[gopherbot]

Change https://golang.org/cl/369894 mentions this issue: cmd/compile: deal with unsatisfiable type assertion in some instantiations

[danscales]

I guess I would lean toward disallowing for now, if it seems easy and solid to do disallow in types2, since this is coming up so late before beta.

But, as an extra piece of information, I believe we can implement (2) quite easily in the current compiler. I think we can do it with some extra checks in the compiler (so as to avoid trying to create the problematic itab) and some extra generated code in some cases for type asserts. However, I've uploaded a prototype change https://golang.org/cl/369894 that is even simpler, where we allow creating the problematic itab as a dummy itab. Since that dummy itab can't actually be created for a real value at runtime, all the expect type assertion failures and mismatches on type cases just naturally happen correctly.

[rogpeppe]

I think this is perhaps analogous to the way that unsafe.Sizeof is not a constant in generic functions, although it is in non-generic functions.

To be a bit clearer about that, the following code is not valid for all instantiations of S and T:

func CheckSizes[S, T any](n uintptr) string {
    switch n {
    case unsafe.Sizeof(*new(S)):
        return "S"
    case unsafe.Sizeof(*new(T)):
        return "T"
    }
    return "none"
}

but it runs ok anyway even though it can fail for some instantiations in the non-generic case.

ISTM that this is quite a similar situation to the dynamic type conversion case.

[ianlancetaylor]

Of the choices in https://github.com/golang/go/issues/50002#issuecomment-987146868 I think that we should implement choice 2. Choice 1 is defensible but I don't see the need for that restriction. It would seem to make some code harder to write, and I don't think it makes things easier for anybody.

[griesemer]

I'm also leaning towards choice 2 at the moment. It seems to me that we should report a compile-time error only if there's no possible instantiation for which the type assertion can succeed; but clearly there are instantiations for which this code can succeed.

For instance, in this case

func f[P interface{ m() int }](x interface{ m() string }) {
    _ = x.(P)
}

we correctly report a compile-time error, even though we don't in this case:

func g(x interface{ m() string }) {
    _ = x.(interface{ m() string })
}

for historic reasons (and we can't fix that for backward-compatibility reasons). In both these cases the type assertion cannot succeed, no matter the dynamic types.

[mdempsky]

If folks are leaning towards option 2, that SGTM too then.

[findleyr]

I am fine with option 2 as well. Thanks @rogpeppe and @norunners for raising.

we correctly report a compile-time error, even though we don't in this case:

@griesemer I think in that case you meant for one of the m methods to return int (FWIW this is a vet warning).

[danscales]

OK, I have updated my change to the compiler and Keith has reviewed it, so I will check it in. We both agree that it is minimal risk for beta-1, since it could only possibly introduce bugs in programs that have the specific case we are discussing/fixing i.e. generic functions for which some instantiations lead to impossible type assertions or type switch cases that can never match.