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/cgo: compiler accepts invalid declaration of methods on C data types #57926

Closed
adonovan opened this issue Jan 19, 2023 · 10 comments
Closed
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Jan 19, 2023

$ cat a.go
package main

/*
typedef int foo;
*/
import "C"

func (C.foo) method() int { return 123 }

func main() {
        var x C.foo
        println(x.method()) // "123"
}
$ go run a.go
123

Is this program valid? I am surprised that one is allowed to declare Go methods on C datatypes because they are pretending to come from another package called "C". Superficially it looks like it should be a syntax error in the method's receiver declaration.

I understand that this is just an artifact of the translation to Go files, in which C.foo is replaced by something like __C_foo, which is a valid Go type capable of bearing methods, but the cgo build process should probably reject this.

@ianlancetaylor
Copy link
Member

It's probably fairly easy to reject this in cmd/cgo itself.

@beoran
Copy link

beoran commented Jan 20, 2023

Actually I used this in the past as a convenient feature. So I think it's not a bug...

@seankhliao seankhliao changed the title cmd/go: compiler accepts invalid declaration of methods on C data types cmd/cgo: compiler accepts invalid declaration of methods on C data types Jan 20, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 20, 2023
@bcmills bcmills removed the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 20, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 20, 2023
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 20, 2023
@dr2chase dr2chase assigned dr2chase and mdempsky and unassigned dr2chase Feb 1, 2023
@mknyszek mknyszek added this to the Backlog milestone Feb 8, 2023
@adonovan
Copy link
Member Author

adonovan commented Apr 20, 2023

We got another gopls bug report related to this. As Ian says, it seems like it should be technically easy to fix this in cgo, but I fear the fix may break existing programs that have exploited the (IMHO) buggy behavior. Does that constitute an incompatible change? Does this need a proposal?

@adonovan
Copy link
Member Author

adonovan commented Apr 20, 2023

Notice also this variant using a type alias. It may defeat a purely syntactic attempt to fix the problem by detecting method receiver types that are dotted names.

package main

/*
typedef int foo;
*/
import "C"

type T = C.foo

func (T) method() int { return 123 }

func main() {
	var x C.foo
	println(x.method()) // "123"
}

@ianlancetaylor
Copy link
Member

I'm inclined to think that it's OK to break program that rely on defining methods on types that are defined in C. To me that seems analogous to defining methods on types defined in a different package, which is forbidden. In particular, if we permitted methods defined on types defined in C, then we could never fix #13467 which I think everyone would like fixed if we can figure out how.

@adonovan
Copy link
Member Author

It's probably fairly easy to reject this in cmd/cgo itself.

I don't know how to robustly reject this in cgo itself since it needs the type checker to resolve the method type, which could be a type alias to C.foo, or an arbitrarily deep chain of type aliases. Nor do I know how to robustly reject it in the compiler, because the func (_Ctype_foo) method() declaration could appear in ordinary Go source. We could check for the presence of import _ "runtime/cgo" to reduce false positives, but it's just a heuristic.

Got any ideas?

@ianlancetaylor
Copy link
Member

I suppose my first idea is to not worry about type aliases. Because, who cares? We'll fix 99.9% of the problem by just having cmd/cgo reject func (r C.type).

If we really want to solve the type alias issue, then my first thought would be a compiler pragma //go:cgotype that the compiler can recognize.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/490819 mentions this issue: cmd/cgo: reject attempts to declare methods on C types

@findleyr
Copy link
Member

findleyr commented May 9, 2023

CC @eliasnaur as this will break gio: https://git.sr.ht/~eliasnaur/gio-shader/tree/main/item/piet/elements_abi.go#L29

@eliasnaur
Copy link
Contributor

@findleyr thanks for the heads up. (Un)fortunately, Go does not yet break Gio because of #60725.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests