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/compile: suggest clearer error regarding when a non-pointer type cannot be used as type parameter #51515

Open
changkun opened this issue Mar 7, 2022 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@changkun
Copy link
Member

changkun commented Mar 7, 2022

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

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

changkun commented Mar 7, 2022

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

dr2chase commented Mar 8, 2022

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.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 8, 2022
@changkun
Copy link
Member Author

changkun commented Mar 9, 2022

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).

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Status: Triage Backlog
Development

No branches or pull requests

5 participants