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: report unused type parameters #50914

Closed
vegarsti opened this issue Jan 30, 2022 · 7 comments
Closed

proposal: cmd/vet: report unused type parameters #50914

vegarsti opened this issue Jan 30, 2022 · 7 comments

Comments

@vegarsti
Copy link

vegarsti commented Jan 30, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18beta1 darwin/arm64

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/vegarsti/Library/Caches/go-build"
GOENV="/Users/vegarsti/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/vegarsti/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/vegarsti/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/vegarsti/sdk/go1.18beta1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/vegarsti/sdk/go1.18beta1/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.18beta1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/vegarsti/dev/go-generic-test/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/rp/9sx14sg570j1_s4lmkmgwm9r0000gn/T/go-build1502445601=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

File main.go contains

package main

func f[T int]() {}

and run go1.18beta1 vet main.go.

What did you expect to see?

Something like "f: type parameter T is not used"

What did you see instead?

No output; exit code 0 indicating no issues found in file.

Rationale

go vet doesn't report on unused arguments.
This is nice, because you may need to conform to a certain interface even though you don't use all arguments.
In this example program, we specify a type parameter, but we don't apply the argument to anything.
This is different, in my opinion, because the type parameter list is simply redundant in this case.
Implementing this may help for example where a user has started out implementing a function as generic, decided not to, but forgot to remove the type parameter list.

I apologize if this has been discussed already! I didn't find anything on here.

@vegarsti vegarsti changed the title cmd/vet: Report on unused type parameter cmd/vet: Should report on unused type parameter Jan 30, 2022
@vegarsti vegarsti changed the title cmd/vet: Should report on unused type parameter cmd/vet: Should report on unused type parameters Jan 30, 2022
@seankhliao seankhliao changed the title cmd/vet: Should report on unused type parameters proposal: cmd/vet: Should report on unused type parameters Jan 30, 2022
@seankhliao seankhliao added this to the Proposal milestone Jan 30, 2022
@vegarsti
Copy link
Author

Thanks @seankhliao!

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 30, 2022
@ianlancetaylor ianlancetaylor changed the title proposal: cmd/vet: Should report on unused type parameters proposal: cmd/vet: report unused type parameters Jan 30, 2022
@ianlancetaylor
Copy link
Contributor

That would make it difficult to write code like

type Zero[T any] struct{}
func (Zero[T]) Get() T {
    var zero T
    return zero
}

That's a fairly simple version but one can imagine more complicated ones.

For functions, it's possible to imagine having a family of functions that all share the same type parameters, even if they don't all use them.

Giving an error for unused local variables is helpful because they normally indicate a bug in the program. That is less clear for unused type parameters.

@vegarsti
Copy link
Author

vegarsti commented Jan 31, 2022

Thanks for the quick reply @ianlancetaylor!

For functions, it's possible to imagine having a family of functions that all share the same type parameters, even if they don't all use them.

That makes sense. I can see that it would be annoying to get an error from vet in such cases. And as you say, there's probably other scenarios where reporting an error would have a negative effect.

I don't quite see how the example you posted with a type is the same kind as with the function example above. You don't have to, of course, but could you explain why this is similar? I would think that there are no unused type parameters in the method Get(), since T is used as a return type.
I admit I haven't spent time trying out generic types yet, so that might be why I don't see it.

As for this issue, I was probably too quick! I'm fine with this being closed.

@timothy-king
Copy link
Contributor

I think in the spirit of the suggestion for named types with methods, the vet check would likely require checking if the type parameter type is used in the underlying type or within any method for the named type.

type N[T any} struct {
  x T
}

AFAICT a pattern this does rule out is to use generic functions for type assertions for tilde and bar types.

type S int

func (S) Bar() {}
func f[T interface {
	Bar()
	~int | ~string
}]() {
}
func _() {
	f[S]()
}

Seems somewhat reasonable?

@bcmills
Copy link
Contributor

bcmills commented Feb 1, 2022

This proposal seems analogous to reporting unused function arguments, which we also don't do today.

I don't think we should report on unused type parameters going forward, either. It may be that the implementation of a struct or function initially depends on all of its type parameters but is then refactored such that one or more of the parameters is no longer needed. However, the extra parameter cannot be removed without breaking compatibility: existing users may already instantiate it explicitly.

This seems like a fine check for more aggressive linters, but I think the false-positive rate will be too high in the long run for cmd/vet.

@rsc
Copy link
Contributor

rsc commented Feb 2, 2022

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

@rsc rsc moved this from Incoming to Likely Decline in Proposals (old) Feb 2, 2022
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Feb 9, 2022
@rsc
Copy link
Contributor

rsc commented Feb 9, 2022

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

@rsc rsc closed this as completed Feb 9, 2022
@golang golang locked and limited conversation to collaborators Feb 9, 2023
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

7 participants