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: prevent interface conversion panic at compile time #31585

Open
gertcuykens opened this issue Apr 20, 2019 · 8 comments

Comments

@gertcuykens
Copy link
Contributor

commented Apr 20, 2019

I think the compiler at compile time should stop building to prevent this panic at runtime ?

https://play.golang.org/p/ffP9vSHdHof

package main

import (
	"fmt"
)

type Test4 interface {
	test(string) string
}

type Test2 interface {
	test(string) string
}

type Test1 func(string) string

func (t Test1) test(str string) string {
	return t(str)
}

type Test3 func(string) string

func (t Test3) test(str string) string {
	return t(str)
}

func main() {
	test1 := Test1(func(str string) string {
		return str + " world"
	})
	fmt.Println(test1.test("hello"))

	test2 := Test2(test1)
	fmt.Println(test2.test("hello"))

	test3 := test2.(Test2)
	fmt.Println(test3.test("hello"))

	test4 := test3.(Test4)
	fmt.Println(test4.test("hello"))

	test5 := test4.(Test4)
	fmt.Println(test5.test("hello"))

	test6 := test4.(Test2)
	fmt.Println(test6.test("hello"))

	test7 := test4.(Test1)
	fmt.Println(test7.test("hello"))

	test8 := test4.(Test3)
	fmt.Println(test8.test("hello"))
}

panic: interface conversion: main.Test4 is main.Test1, not main.Test3

@go101

This comment has been minimized.

Copy link

commented Apr 21, 2019

It is a panic at run time, not compile time. The output clearly states the reason: the dynamic type of test4 in main.Test1, not main.Test3.

A failed type assertion without the second optional result will panic. You can write the last assertion as the following to avoid panic.

	test8, _ := test4.(Test3) // present the second optional result
	if test8 != nil {
		fmt.Println(test8.test("hello"))
	}
@gertcuykens

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

Thanks, but what I ment was that I think the compiler should pick it up and prevent it from building in the first place. So what I mean is, it should be a compile error instead of a runtime error.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

The spec requires that this code compile successfully. We'd have to adjust the spec in order to do what you suggest, which is a very high bar.
We could possibly add a check to vet, but generally we don't do so when the runtime error is obvious. It's more commonly used to detect at compile time something that would be hard to debug at runtime.

If you want to move forward with a vet strategy, we'd need to know what such a detector would look like. How would the analysis happen? What would be the error rate (both positive and negative)? Are there actual instances of this bug in the wild?

@gertcuykens

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

go vet is also fine for this. I think it's in the same category as the a = append(b, 2, 3, 4) gotcha bug but arguably less common. Maybe this is not so important. As for analysing interfaces I think first step would be listing all interfaces of a program by line snipit. It would be a nice overview of your go program I think. go vet interfaces which can then also be used to highlight interfaces in ide's like microsoft/vscode-go#486 Second step would be to figure out if a cast make sense.

@ALTree ALTree added this to the Unplanned milestone Apr 21, 2019

@beoran

This comment has been minimized.

Copy link

commented Apr 22, 2019

I think the gopls tool which is currently being developed here will do what you want.

https://github.com/golang/go/wiki/gopls

@gertcuykens

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

As far as I can tell gopls doesn't do that by design https://microsoft.github.io/language-server-protocol/specification you going to need go vet always for detecting stuff like shadow variables and specific golang things like interfaces. LSP only fits generic program language things like defenition look up etc.

@stamblerre

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@gertcuykens

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

ok so the best place to implement a list of all interfaces to begin with is still go vet right? Maybe step two to compare it with other types to see if a cast makes sense could be implemented in gopls directly then. But step one is still a grouped by interface list of all interfaces variables (go vet interfaces) and it associated code line so you can follow the life of each interface in your progamer like for this example something like

line: xx type Test4 interface{}
line: xx test4 := test3.(Test4)
line: xx test5 := test4.(Test4)
line: xx test6 := test4.(Test2)

line:xx type Test2 interface{}
...

@dmitshur dmitshur changed the title cmd/compiler: prevent interface conversion panic at compile time cmd/compile: prevent interface conversion panic at compile time Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.