-
Notifications
You must be signed in to change notification settings - Fork 18k
cmd/compile: internal compiler error: incomplete itab #50002
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
Comments
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:
I don't think this is a beta-1 release blocker, but we may be able to fix it in time. |
Why would there be a runtime error at line 14? Surely it should just fail to select the case? |
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. |
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 I think there are two consistent alternatives on how to resolve this: (1) We only allow 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 |
Change https://golang.org/cl/369774 mentions this issue: |
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. |
I'm kinda leaning toward number 2. The reason you can't do
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
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. |
Are you saying that you'd reject the instantiation of
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. |
@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 |
@mdempsky Why would we want to reject the declaration of
|
@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., |
@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. |
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 For example: https://gotipplay.golang.org/p/6blhiU8j_ZO |
I think this is perhaps analogous to the way that |
My expectation is that even among users who learn about using But we can always review user reports in the future and relax the rules. |
Change https://golang.org/cl/369894 mentions this issue: |
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. |
To be a bit clearer about that, the following code is not valid for all instantiations of
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. |
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. |
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. |
If folks are leaning towards option 2, that SGTM too then. |
I am fine with option 2 as well. Thanks @rogpeppe and @norunners for raising.
@griesemer I think in that case you meant for one of the |
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. |
commit 0985990
The following code panics: https://gotipplay.golang.org/p/FR0-a4NF08e
The panic that I see is:
Thanks to @norunners on the Gophers Slack for reporting this.
The text was updated successfully, but these errors were encountered: