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

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

Closed
bcmills opened this issue Sep 1, 2021 · 17 comments

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 1, 2021

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)

@gopherbot gopherbot added this to the Proposal milestone Sep 1, 2021
@bcmills
Copy link
Contributor Author

bcmills commented Sep 1, 2021

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

robpike commented Sep 1, 2021

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

bcmills commented Sep 2, 2021

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

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

@bcmills
Copy link
Contributor Author

bcmills commented Sep 2, 2021

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

robpike commented Sep 2, 2021

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

bjorndm commented Sep 3, 2021

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

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

mattn commented Sep 3, 2021

It should be allowed since type int string is allowed.

@bcmills
Copy link
Contributor Author

bcmills commented Sep 3, 2021

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

bcmills commented Sep 3, 2021

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

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

bjorndm commented Sep 3, 2021

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

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Sep 5, 2021
@rsc
Copy link
Contributor

rsc commented Sep 15, 2021

I agree that this seems like a premature check.

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 13, 2021
@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

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 rsc moved this from Active to Likely Decline in Proposals (old) Oct 20, 2021
@rsc
Copy link
Contributor

rsc commented Oct 20, 2021

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

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Oct 27, 2021
@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

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

@rsc rsc closed this as completed Oct 27, 2021
@golang golang locked and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

9 participants