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: internal compiler error: incomplete itab #50002

Closed
rogpeppe opened this issue Dec 6, 2021 · 24 comments
Closed

cmd/compile: internal compiler error: incomplete itab #50002

rogpeppe opened this issue Dec 6, 2021 · 24 comments
Labels
NeedsInvestigation okay-after-beta1 release-blocker
Milestone

Comments

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Dec 6, 2021

commit 0985990

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
Copy link
Contributor

@danscales danscales commented Dec 6, 2021

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
Copy link
Contributor Author

@rogpeppe rogpeppe commented Dec 6, 2021

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
Copy link
Contributor

@danscales danscales commented Dec 6, 2021

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
Copy link
Member

@mdempsky mdempsky commented Dec 6, 2021

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

@toothrot toothrot added the NeedsInvestigation label Dec 6, 2021
@toothrot toothrot added this to the Go1.18 milestone Dec 6, 2021
@toothrot toothrot added okay-after-beta1 release-blocker labels Dec 6, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 6, 2021

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

@findleyr
Copy link
Contributor

@findleyr findleyr commented Dec 6, 2021

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
Copy link
Contributor

@randall77 randall77 commented Dec 6, 2021

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
Copy link
Contributor Author

@rogpeppe rogpeppe commented Dec 6, 2021

(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
Copy link
Member

@mdempsky mdempsky commented Dec 6, 2021

@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
Copy link
Member

@mdempsky mdempsky commented Dec 6, 2021

@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
Copy link
Contributor Author

@rogpeppe rogpeppe commented Dec 6, 2021

@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
Copy link
Member

@mdempsky mdempsky commented Dec 6, 2021

@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
Copy link
Contributor

@findleyr findleyr commented Dec 6, 2021

@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
Copy link
Contributor Author

@rogpeppe rogpeppe commented Dec 6, 2021

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
Copy link
Contributor Author

@rogpeppe rogpeppe commented Dec 6, 2021

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
Copy link
Member

@mdempsky mdempsky commented Dec 6, 2021

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
Copy link

@gopherbot gopherbot commented Dec 7, 2021

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

@danscales
Copy link
Contributor

@danscales danscales commented Dec 7, 2021

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
Copy link
Contributor Author

@rogpeppe rogpeppe commented Dec 7, 2021

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 7, 2021

Of the choices in #50002 (comment) 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
Copy link
Contributor

@griesemer griesemer commented Dec 7, 2021

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
Copy link
Member

@mdempsky mdempsky commented Dec 7, 2021

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

@findleyr
Copy link
Contributor

@findleyr findleyr commented Dec 7, 2021

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
Copy link
Contributor

@danscales danscales commented Dec 7, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation okay-after-beta1 release-blocker
Projects
None yet
Development

No branches or pull requests

9 participants