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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: prevent interface conversion panic at compile time #31585

Closed
gertcuykens opened this issue Apr 20, 2019 · 9 comments
Closed

cmd/compile: prevent interface conversion panic at compile time #31585

gertcuykens opened this issue Apr 20, 2019 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unplanned

Comments

@gertcuykens
Copy link
Contributor

gertcuykens commented Apr 20, 2019 •
edited

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

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

gertcuykens commented Apr 21, 2019 •
edited

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

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

gertcuykens commented Apr 21, 2019 •
edited

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 the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 21, 2019
@ALTree ALTree added this to the Unplanned milestone Apr 21, 2019
@beoran
Copy link

beoran commented Apr 22, 2019 •
edited

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

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

@gertcuykens
Copy link
Contributor Author

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

gopls includes go vet checks as part of textDocument/publishDiagnostics (see https://github.com/golang/tools/blob/master/internal/lsp/source/diagnostics.go).

@gertcuykens
Copy link
Contributor Author

gertcuykens commented Apr 22, 2019 •
edited

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

Thank you @gertcuykens for filing this issue and everyone for chiming in. As @go101 and @randall77 have mentioned, this in deed is a runtime error and we can't change this, but as @beoran and @stamblerre have noted, you can add vet rules for such.

I shall also kindly cc @johanbrandhorst @dominikh who might be interested in these vet rules, but for now I shall close this issue. Please feel free to reopen if I've missed something.

@golang golang locked and limited conversation to collaborators Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
8 participants
@beoran @gertcuykens @ALTree @odeke-em @stamblerre @randall77 @gopherbot @go101