cmd/cgo: compiler accepts invalid declaration of methods on aliases to C types

[eliasnaur]

This program successfully runs with Go 1.21:

$ cat cgo_methods.go
package main

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

type foo = C.foo

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

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

I believe this program is invalid for the same reason as the program in #57926. Note that in contrast to that issue, this program uses an alias to define a method on a C type.

[ianlancetaylor]

@alandonovan pointed out this hole in the current cgo detection of methods on C types, and I said it didn't seem worth worrying about, because who would put a method on a type alias? Interesting to see that people actually do this.

If anybody sees a simple way to fix this, by all means go for it.

[adonovan]

Clearly the fix has to be done in the compiler (since it requires symbol resolution), which I think means it will generally be possible to hand-craft some valid Go source file that that spuriously triggers the error (since cgo-generated source has no special privilege). I think this means that any approach is going to be at best a heuristic and that false positives are possible. That said, false positives (artificially weird code that doesn't compile when it should) seem less important than false negatives (such as this issue).

Some clues we could use for a heuristic:

• the receiver type name is of the form _C_foo.
• the file name is of the form go-build/XX/X{64}-d.
• the file contains a Code generated by cmd/cgo comment.
• the file uses //line directives.
• the file blank-imports the unsafe package.

Some combination of these, plus a test that the error is positively produced, is probably good enough.

[ianlancetaylor]

Just a note that I don't think we really need a heuristic to detect cgo-generated code. The cmd/go tool knows when it is compiling a Go file generated by cmd/cgo, and it could pass a special option to cmd/compile indicating that. Of course other people could use -gcflags to pass the same option....

[gopherbot]

Change https://go.dev/cl/503395 mentions this issue: cmd/cgo: compiler accepts invalid declaration of methods on aliases to C types

[gopherbot]

Change https://go.dev/cl/503397 mentions this issue: cmd/go: compiler accepts invalid declaration of methods on aliases to C types

[gopherbot]

Change https://go.dev/cl/503396 mentions this issue: cmd/compile: compiler accepts invalid declaration of methods on aliases to C types

[gopherbot]

Change https://go.dev/cl/503596 mentions this issue: go/types,types: compiler should not accept invalid declaration of methods on aliases to C types

[cuiweixie]

Change https://go.dev/cl/503596 mentions this issue: go/types,types: compiler should not accept invalid declaration of methods on aliases to C types

this fix is simple enough. Is it ok? @ianlancetaylor

[cuiweixie]

@alandonovan pointed out this hole in the current cgo detection of methods on C types, and I said it didn't seem worth worrying about, because who would put a method on a type alias? Interesting to see that people actually do this.

If anybody sees a simple way to fix this, by all means go for it.

I think this condition is simple to fix this.

isCgoGeneratedFile(T.obj.Pos()) && strings.HasPrefix(T.obj.Name(), "_Ctype_") 

fix cl

[adonovan]

Well spotted! I didn't realize the compiler already had an official heuristic for isCgoGeneratedFile. Let's use that.

[cuiweixie]

@alandonovan pointed out this hole in the current cgo detection of methods on C types, and I said it didn't seem worth worrying about, because who would put a method on a type alias? Interesting to see that people actually do this.

If anybody sees a simple way to fix this, by all means go for it.

I think this condition is simple to fix this.

isCgoGeneratedFile(T.obj.Pos()) && strings.HasPrefix(T.obj.Name(), "_Ctype_") 

fix cl

@ianlancetaylor is this condition ok,isCgoGeneratedFile(T.obj.Pos()) && strings.HasPrefix(T.obj.Name(), "Ctype") ?

[ianlancetaylor]

@cuiweixie Yes. Thanks.

[deadprogram]

This is another very commonly used package that will no longer compile using Go 1.21: bugst/go-serial#162

[adonovan]

This is another very commonly used package that will no longer compile using Go 1.21: bugst/go-serial#162

At the risk of sounding flippant, I might rephrase that as "another package that got away with invalid code". Fortunately the problematic cases all seem to be internal so it should be an easy fix.

[deadprogram]

@adonovan indeed, I submitted a PR to that repo with fix.

[griesemer]

Reassigned to me for 1.24 to make it visible. CL exists. Should review.

[griesemer]

Pending CL CL 503596 still needs some work.

[gopherbot]

Change https://go.dev/cl/629715 mentions this issue: go/types, types2: disallow new methods on (aliases to) cgo-generated types

[griesemer]

@eliasnaur The pattern you are referring to in your example program appears (or appeared) in gioui.org/app/os_x11.go (see #59944). We fixed a crash with respect to that case. As the author of Gio, presumably you do not rely on this mechanism anymore since you filed this (current) issue? We are wondering if we can remove support from the compiler and make it an error going forward (cgo behavior is not covered by the Go 1 compatibility guarantee).

[eliasnaur]

Absolutely, go ahead and make it an error. Thank you.

The current Gio code does rely on this behaviour, but I'll update it well before Go 1.24 is released if this issue is fixed.

[gopherbot]

Change https://go.dev/cl/630335 mentions this issue: doc: document new restriction on cgo-generated method receicer types

[gopherbot]

Change https://go.dev/cl/630395 mentions this issue: go/types, types2: simplify Checker.resolveBaseTypeName (cleanup)