golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.8k stars 17.64k forks source link

cmd/compile: suggest clearer error regarding when a non-pointer type cannot be used as type parameter #51515

Open changkun opened 2 years ago

changkun commented 2 years ago

Consider this example:

package main

type C interface {
    Foo()
    Bar()
}

type A struct{}

func (a A) Foo() {}
func (a A) Bar() {}

type B struct{}

func (b B) Foo()  {}
func (b *B) Bar() {}

func Want[T C]() (x T) { return }

func main() {
    Want[A]() // OK
    Want[B]() // ERROR
}

We may consider A and B both implements C. But the code produces error:

B does not implement C (Bar method has pointer receiver)
After parse the spec (precise but complex):
According to the spec (https://tip.golang.org/ref/spec#Implementing_an_interface) regarding the definition of a type `T` implements interface `I`: ``` A type T implements an interface I if T is not an interface and is an element of the type set of I; or... ``` Type set describes: ``` ... an interface specifies a (possibly empty) list of methods. The type set defined by such an interface is the set of types which implement all of those methods, and the corresponding [method set](https://tip.golang.org/ref/spec#Method_sets) consists exactly of the methods specified by the interface. ``` Method set describes: ``` The method set of a pointer to a defined type T (where T is neither a pointer nor an interface) is the set of all methods declared with receiver *T or T. ``` The preceived understanding is: Let `T` be a struct type. A pointer (`*T`) to a defined type `T`. Since `T` contains a set of methods (`I`) declared with receiver `*T` or `T`, hence `I` is a type set, and `*T` is an element of the type set `I`.

It is clear that only a pointer type is an element of a type set C (which is a method set with receiver *T or T), but this observation is much less intuitive to be aware in the beginning.

Maybe a more clear error message might be:

B does not implement C because B has a pointer receiver method (*B).Bar(),
and only *B can be used as a type parameter of C.

Previously related #44201

ianlancetaylor commented 2 years ago

I think your suggested error messages is too long. And it doesn't contain any information that is not in the existing error message.

I don't mind changing the error message if we can find an improvement, but perhaps this is something to be addressed in blog posts rather than by changing the compiler.

changkun commented 2 years ago

I think your suggested error messages is too long. And it doesn't contain any information that is not in the existing error message.

I think it comparably contains more straightforward suggestion on what is acceptable in this case only *B can be used. The suggested message might be too long, a shorter one is also possible and preferred. As same as the purpose of #44201, it goal here is to have a clearer suggestion on what is possible, maybe:

B does not implement C (Bar method has pointer receiver; use *B as type parameter instead)
dr2chase commented 2 years ago

I hope that we can improve the error message. One problem for phrasing the message is that there's two ways to fix the underlying problem; either change (*B).Bar to be B.Bar, or change Want[B] to Want[*B]. (see https://go.dev/play/p/tFf5JftajqK?v=gotip )

I might phrase it as B does not implement C, (*B).Bar is not compatible with C.Bar. The original message doesn't plainly state which Bar method it is talking about, and expects the person who made the mistake to be able to figure it out. One minor source of confusion is that *B does implement C, and that B.Foo is compatible in that case. Once you know, you know, but most people don't have that committed to memory right away.

changkun commented 2 years ago

I hope that we can improve the error message. One problem for phrasing the message is that there's two ways to fix the underlying problem; either change (*B).Bar to be B.Bar, or change Want[B] to Want[*B]. (see https://go.dev/play/p/tFf5JftajqK?v=gotip )

I agree there are two possible fixes. But speaking of likelihood, I would argue that "change (*B).Bar to be B.Bar" have lower probability for the following reason: 1) the change might result in unexpected copy of B and a method uses *B is more likely because of the method wants to avoid copy. 2) There might be more methods that uses *B, which will require all methods to use B.

One minor source of confusion is that *B does implement C, and that B.Foo is compatible in that case. Once you know, you know, but most people don't have that committed to memory right away.

Comparing the original

B does not implement C (Bar method has pointer receiver)

to the discussed possibilities:

B does not implement C and only *B implements C (Bar method has pointer receiver)           // (1)
B does not implement C (Bar method has pointer receiver; use *B as type parameter instead)  // (2)
B does not implement C (Bar method has pointer receiver; change (*B).Bar() to (B).Bar())    // (3)

I think (1) is clear and avoids explicit suggestion, but only telling the factual information. (2) suggests a solution that is more likly as a fix, and (3) seems less accpetable at first sight (to me).