proposal: cmd/vet: warn if a method receiver uses a known type-name as the name of a type parameter

[bcmills]

Consider the program in https://go2goplay.golang.org/p/O9E46VGCBcx, inspired by @griesemer's comment in #46477 (comment):

type T[P any] struct{}

func (T[P]) m() {}

// Is m0 a partial specialization?
// It looks like one, but isn't actually.
func (T[int]) m0() {
	var x int
	fmt.Printf("x is a %T\n", x)
}

func main() {
	var r T[string]
	r.m0()
}

Today (go1.18-717f337d1), this method compiles, binding the type parameter P (which is renamed to int in the m0 method) to string, and then the m0 method prints x is a string (https://go2goplay.golang.org/p/O9E46VGCBcx).

However, as a reader of this code — and especially coming from a background of working with languages like Rust and C++ that allow method specialization — the behavior of this program is somewhat surprising.

Ideally, as a reader I would like to see the same type parameter name (P) in all method definitions, in much the same way that I would like to see the same receiver name in all method definitions. I can understand if we don't want to require that in general for generic definitions, but I think it would at least be useful for vet to reject definitions that look like specializations but aren't.

Specifically, I propose that vet should emit a warning when both:

• the name of a type parameter in a method receiver does not match its name in the corresponding type declaration,
• and the name does match the name of some other type that is in scope.

So this code would receive a warning:

func (T[int]) m0() {  // vet: "int" is a new name for type parameter "P", not a method specialization
	var x int
	fmt.Printf("x is a %T\n", x)
}

but this would not:

type T[P any] struct{}

func (T[Q]) m1() {
	var x Q
	fmt.Printf("x is a %T\n", x)
}

(CC @timothy-king @ianlancetaylor @mdempsky)

[bcmills]

(#47657 (comment) also hints at the possibility of method-specialization at some point down the road, in which case this sort of definition would be even more confusing.)

[robpike]

The 'frequency' criterion for vet would imply we'd need to see this problem in the wild before deciding whether vet should catch it. This preemption might be unnecessary. And also unlikely to be needed, it seems to me.

[bcmills]

The 'frequency' criterion for vet would imply we'd need to see this problem in the wild before deciding whether vet should catch it.

How would you measure occurrences in the wild? I would expect that the vast majority would be caught during testing and code review, but only after a non-trivial amount of time trying to debug the problem.

The non-trivial debugging step is what I would hope to eliminate by adding the vet warning.

[jimmyfrasche]

If func (T[int]) m1() is allowed now how can it be used as the syntax for specialization later without breaking backwards compatibility?

[bcmills]

@jimmyfrasche, it cannot be used as the syntax for specialization later. (Specialization, if added, would need to have a different syntax, but I think that is a matter for a separate proposal.)

[robpike]

How would you measure occurrences in the wild? I would expect that the vast majority would be caught during testing and code review, but only after a non-trivial amount of time trying to debug the problem.

Vet has never aimed to catch things that testing will catch anyway, but to find the things that testing is likely to miss.

[bjorndm]

@bcmills I don't know why func (T[int]) m1() is allowed now, but I'd say it's clearly the Wrong Thing. This shouldn't be a vet check, it should simply not be allowed, not only because it is confusing, but also because it makes adding specialization later harder.

[zephyrtronium]

@bjorndm
func (T[int]) m1() is allowed for the same reason that func m(int T) is allowed. It's just a parameter; the programmer is free to choose any identifier that isn't a keyword for its name.

[mattn]

It should be allowed since type int string is allowed.

[bcmills]

@bjorndm, I don't disagree but I think that is a matter for a separate proposal.

(This proposal is, “given that this is allowed today, we should warn about the confusing cases”.)

[bcmills]

@mattn, if the original declaration were something like

type T[int constraints.Numeric] struct{}

func (T[int]) m0() {
	…
}

then I think it would be much less confusing: if the type parameter is consistently named int, then the fact that that shadows the built-in int is not surprising.

The part that makes it confusing in the original T[P any] example is the fact that the int in T[int] looks like an argument (passed for a parameter _explicitly declared as _ P) rather than an alias for the parameter named P.

[timothy-king]

if the type parameter is consistently named int, then the fact that that shadows the built-in int is not surprising.

@bcmills to nitpick the previous example a bit, I suspect this will still be mildly surprising to a decent number of people reading this code. type int string is already surprising to many people. But I agree that it is plausible that the author type T[int constraints.Numeric] struct{} might not be surprised. So I like keeping the second bullet for a vet check.

[bjorndm]

@zephyrtronium, @mattn I didn't know you could do that at all! Very confusing indeed. But stepping back, I can see that this boat has sailed, and that arguably, being able to redefine a built in type, could be useful in some cases.

However this is not applicable to this issue. Generics are still new.
@bcmills Hmm, yes, the thing that is most confusing is the inconsistency. The type parameters should be named the same for the type definition and the methods connected to it, unless _ is used. However I guess that is more of a stye issue for a lint-style cheker than for go vet.

[rsc]

I agree that this seems like a premature check.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

[rsc]

No change in consensus, so declined.
— rsc for the proposal review group