-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
go/types, x/tools/gopls: crash when editing gioui.org/app/os_x11.go #59944
Comments
CC @mdempsky @griesemer @adonovan Somehow we have a method on a cgo type that is not recorded on its receiver. This looks like a go/types bug. Note that gopls uses the hidden I'll avoid the panic for now. Haven't yet found the bug. |
Oh! I think I found it. We need to consider the Ctype prefix here: Otherwise, we will not find the root object to add the Binding0 method to: |
Change https://go.dev/cl/492315 mentions this issue: |
Due to a long-standing bug in go/types, the objectpath package cannot find methods declared on aliases of cgo types. We should fix the bug in go/types, but also must avoid the panic in gopls as this fix will not be back-ported to all supported Go versions. Updates golang/go#59944 Change-Id: I42d94e610ad92d258c8034d59ea3b0ef312ddebb Reviewed-on: https://go-review.googlesource.com/c/tools/+/492315 Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Robert Findley <rfindley@google.com> Auto-Submit: Robert Findley <rfindley@google.com>
Change https://go.dev/cl/493877 mentions this issue: |
While gopls probably shouldn't crash, note that it's not intended that users are allowed to declare methods on C types anyway. E.g., see #57926. |
@mdempsky thanks. I was unaware of that discussion. CC @adonovan Unfortunate timing; I wonder if we should support this in go/types anyway. But now that I look at it, I'm not sure why gopls even needs the go115UsesCgo field on Matthew: do you think we could eliminate this configuration? |
@mdempsky I took a look at the code base that raised this and could not find any instance of either a direct method declaration on a C type, or via a type alias, though this #57926 (comment) suggests that it's not an alien concept in gio. |
@kortschak the reference is in the gio-shader dependency here: The panic occured while recording analysis facts for this package. |
Thanks. |
@findleyr is this a Go release blocker for 1.21? Or only a gopls release blocker, which I think is now fixed? |
Not a release blocker, sorry (though it will be fixed before the freeze). |
When associating methods with their receiver base, we need to implement the same indirection through Cgo types as is done for selector expressions. This fixes a bug where methods declared on aliases of Cgo types were not associated with their receiver. While porting to types2, align the types2 testFiles helper with the go/types implementation. In order to avoid call-site bloat, switch to an options pattern for configuring the Config used to type-check. Fixes golang/go#59944 Change-Id: Id14101f01c122b6c856ae5453bd00ec07e83f414 Reviewed-on: https://go-review.googlesource.com/c/go/+/493877 Reviewed-by: Robert Griesemer <gri@google.com> Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
gopls version
2563079
go env
What did you do?
What did you expect to see?
No crash.
What did you see instead?
Editor and settings
Sublime Text.
Logs
Above.
The text was updated successfully, but these errors were encountered: