code review 13102043: go.tools/cmd/vet: detect useless function comparisons

go.tools/cmd/vet: detect useless function comparisons

Also fix bug in types.go discovered by this check.

Fixes issue 5347.

[adg, 2013-08-20 04:23:31]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.tools

[dsymonds, 2013-08-20 04:30:56]

https://codereview.appspot.com/13102043/diff/6001/cmd/vet/nilfunc.go
File cmd/vet/nilfunc.go (right):

https://codereview.appspot.com/13102043/diff/6001/cmd/vet/nilfunc.go#newcode47
cmd/vet/nilfunc.go:47: f.Badf(expr.Pos(), "function uselessly compared to nil")
I think this needs better phrasing. It's certainly useful to sometimes compare
functions to nil, though it *can* be an error too.

https://codereview.appspot.com/13102043/diff/6001/cmd/vet/nilfunc.go#newcode51
cmd/vet/nilfunc.go:51: // idenitifer.
"identifier"

[adg, 2013-08-20 04:44:23]

https://codereview.appspot.com/13102043/diff/6001/cmd/vet/nilfunc.go
File cmd/vet/nilfunc.go (right):

https://codereview.appspot.com/13102043/diff/6001/cmd/vet/nilfunc.go#newcode47
cmd/vet/nilfunc.go:47: f.Badf(expr.Pos(), "function uselessly compared to nil")
Please suggest better phrasing.

https://codereview.appspot.com/13102043/diff/6001/cmd/vet/nilfunc.go#newcode51
cmd/vet/nilfunc.go:51: // idenitifer.
On 2013/08/20 04:30:56, dsymonds wrote:
> "identifier"

Done.

[dsymonds, 2013-08-20 04:45:53]

https://codereview.appspot.com/13102043/diff/6001/cmd/vet/nilfunc.go
File cmd/vet/nilfunc.go (right):

https://codereview.appspot.com/13102043/diff/6001/cmd/vet/nilfunc.go#newcode47
cmd/vet/nilfunc.go:47: f.Badf(expr.Pos(), "function uselessly compared to nil")
On 2013/08/20 04:44:23, adg wrote:
> Please suggest better phrasing.

"function comparison always <X>", depending on whether it is token.EQL or
token.NEQ.

Then this could also be expanded to do other useless comparisons later perhaps.

[r, 2013-08-20 04:46:21]

https://codereview.appspot.com/13102043/diff/12001/cmd/vet/main.go
File cmd/vet/main.go (right):

https://codereview.appspot.com/13102043/diff/12001/cmd/vet/main.go#newcode47
cmd/vet/main.go:47: "unreachable": flag.Bool("unreachable", false, "check for
unreachable code"),
need a flag

[r, 2013-08-20 04:49:00]

https://codereview.appspot.com/13102043/diff/12001/cmd/vet/nilfunc.go
File cmd/vet/nilfunc.go (right):

https://codereview.appspot.com/13102043/diff/12001/cmd/vet/nilfunc.go#newcode5
cmd/vet/nilfunc.go:5: package main
need a comment.

https://codereview.appspot.com/13102043/diff/12001/cmd/vet/nilfunc.go#newcode29
cmd/vet/nilfunc.go:29: }
i might use a helper:
switch {
case f.isNil(expr.X):
   f.checkNotFunction(expr.Y))
case f.isNil(expr.Y):
   f.checkNotFunction(expr.X))
default:
  return
}

[adg, 2013-08-20 04:56:55]

https://codereview.appspot.com/13102043/diff/6001/cmd/vet/nilfunc.go
File cmd/vet/nilfunc.go (right):

https://codereview.appspot.com/13102043/diff/6001/cmd/vet/nilfunc.go#newcode47
cmd/vet/nilfunc.go:47: f.Badf(expr.Pos(), "function uselessly compared to nil")
On 2013/08/20 04:45:53, dsymonds wrote:
> On 2013/08/20 04:44:23, adg wrote:
> > Please suggest better phrasing.
> 
> "function comparison always <X>", depending on whether it is token.EQL or
> token.NEQ.
> 
> Then this could also be expanded to do other useless comparisons later
perhaps.

Done.

https://codereview.appspot.com/13102043/diff/12001/cmd/vet/nilfunc.go
File cmd/vet/nilfunc.go (right):

https://codereview.appspot.com/13102043/diff/12001/cmd/vet/nilfunc.go#newcode5
cmd/vet/nilfunc.go:5: package main
On 2013/08/20 04:49:00, r wrote:
> need a comment.

Done.

https://codereview.appspot.com/13102043/diff/12001/cmd/vet/nilfunc.go#newcode29
cmd/vet/nilfunc.go:29: }
I simplified the switch statement but I think this is clearer as one function.
(I need the original expression to provide the error message)

[dsymonds, 2013-08-20 05:03:30]

LGTM

https://codereview.appspot.com/13102043/diff/21001/cmd/vet/nilfunc.go
File cmd/vet/nilfunc.go (right):

https://codereview.appspot.com/13102043/diff/21001/cmd/vet/nilfunc.go#newcode55
cmd/vet/nilfunc.go:55: val := "true"
you could actually inline all this below if you want.

f.Badf(e.Pos(), "function comparison always %t", e.Op == token.NEQ)

[adg, 2013-08-20 05:09:32]

Hello golang-dev@googlegroups.com, dsymonds@golang.org, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[dsymonds, 2013-08-20 05:20:07]

LGTM

https://codereview.appspot.com/13102043/diff/8004/cmd/vet/types.go
File cmd/vet/types.go (right):

https://codereview.appspot.com/13102043/diff/8004/cmd/vet/types.go#newcode269
cmd/vet/types.go:269: if sig.Recv() == nil {
good spot

[r, 2013-08-20 05:25:25]

https://codereview.appspot.com/13102043/diff/31001/cmd/vet/nilfunc.go
File cmd/vet/nilfunc.go (right):

https://codereview.appspot.com/13102043/diff/31001/cmd/vet/nilfunc.go#newcode6
cmd/vet/nilfunc.go:6: This file contains the code to check for useless function
comparisons.
A useless comparison is one like f == nil as opposed to f() == nil.

https://codereview.appspot.com/13102043/diff/31001/cmd/vet/nilfunc.go#newcode28
cmd/vet/nilfunc.go:28: // Only want comparisons with nil identifier on one side.
s/nil/a nil/

https://codereview.appspot.com/13102043/diff/31001/cmd/vet/nilfunc.go#newcode55
cmd/vet/nilfunc.go:55: f.Badf(e.Pos(), "function comparison always %v", e.Op ==
token.NEQ)
comparison of function %v with nil is always %v

[adg, 2013-08-20 05:33:53]

https://codereview.appspot.com/13102043/diff/31001/cmd/vet/nilfunc.go
File cmd/vet/nilfunc.go (right):

https://codereview.appspot.com/13102043/diff/31001/cmd/vet/nilfunc.go#newcode6
cmd/vet/nilfunc.go:6: This file contains the code to check for useless function
comparisons.
On 2013/08/20 05:25:26, r wrote:
> A useless comparison is one like f == nil as opposed to f() == nil.

Done.

https://codereview.appspot.com/13102043/diff/31001/cmd/vet/nilfunc.go#newcode28
cmd/vet/nilfunc.go:28: // Only want comparisons with nil identifier on one side.
On 2013/08/20 05:25:26, r wrote:
> s/nil/a nil/

Done.

https://codereview.appspot.com/13102043/diff/31001/cmd/vet/nilfunc.go#newcode55
cmd/vet/nilfunc.go:55: f.Badf(e.Pos(), "function comparison always %v", e.Op ==
token.NEQ)
On 2013/08/20 05:25:26, r wrote:
> comparison of function %v with nil is always %v

Done.

[r, 2013-08-20 05:49:51]

LGTM

[adg, 2013-08-20 06:11:08]

*** Submitted as
https://code.google.com/p/go/source/detail?r=40ea8f657df4&repo=tools ***

go.tools/cmd/vet: detect useless function comparisons

Also fix bug in types.go discovered by this check.

Fixes issue 5347.

R=golang-dev, dsymonds, r
CC=golang-dev
https://codereview.appspot.com/13102043