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

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

Fixes golang/go#5347.

R=golang-dev, dsymonds, r
CC=golang-dev
https://golang.org/cl/13102043

cmd/vet/main.go

@@ -40,6 +40,7 @@ var report = map[string]*bool{
 	"buildtags":   flag.Bool("buildtags", false, "check that +build tags are valid"),
 	"composites":  flag.Bool("composites", false, "check that composite literals used field-keyed elements"),
 	"methods":     flag.Bool("methods", false, "check that canonically named methods are canonically defined"),
+	"nilfunc":     flag.Bool("nilfunc", false, "check for comparisons between functions and nil"),
 	"printf":      flag.Bool("printf", false, "check printf-like invocations"),
 	"rangeloops":  flag.Bool("rangeloops", false, "check that range loop variables are used correctly"),
 	"shadow":      flag.Bool("shadow", false, "check for shadowed variables (experimental; must be set explicitly)"),
@@ -356,6 +357,8 @@ func (f *File) Visit(node ast.Node) ast.Visitor {
 	switch n := node.(type) {
 	case *ast.AssignStmt:
 		f.walkAssignStmt(n)
+	case *ast.BinaryExpr:
+		f.walkBinaryExpr(n)
 	case *ast.CallExpr:
 		f.walkCallExpr(n)
 	case *ast.CompositeLit:
@@ -383,6 +386,10 @@ func (f *File) walkAssignStmt(stmt *ast.AssignStmt) {
 	f.checkShadowAssignment(stmt)
 }
 
+func (f *File) walkBinaryExpr(expr *ast.BinaryExpr) {
+	f.checkNilFuncComparison(expr)
+}
+
 // walkCall walks a call expression.
 func (f *File) walkCall(call *ast.CallExpr, name string) {
 	f.checkFmtPrintfCall(call, name)

cmd/vet/nilfunc.go

@@ -0,0 +1,63 @@
+// Copyright 2013 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+/*
+This file contains the code to check for useless function comparisons.
+A useless comparison is one like f == nil as opposed to f() == nil.
+*/
+
+package main
+
+import (
+	"go/ast"
+	"go/token"
+
+	"code.google.com/p/go.tools/go/types"
+)
+
+func (f *File) checkNilFuncComparison(e *ast.BinaryExpr) {
+	if !vet("nilfunc") {
+		return
+	}
+
+	// Only want == or != comparisons.
+	if e.Op != token.EQL && e.Op != token.NEQ {
+		return
+	}
+
+	// Only want comparisons with a nil identifier on one side.
+	var e2 ast.Expr
+	switch {
+	case f.isNil(e.X):
+		e2 = e.Y
+	case f.isNil(e.Y):
+		e2 = e.X
+	default:
+		return
+	}
+
+	// Only want identifiers or selector expressions.
+	var obj types.Object
+	switch v := e2.(type) {
+	case *ast.Ident:
+		obj = f.pkg.idents[v]
+	case *ast.SelectorExpr:
+		obj = f.pkg.idents[v.Sel]
+	default:
+		return
+	}
+
+	// Only want functions.
+	if _, ok := obj.(*types.Func); !ok {
+		return
+	}
+
+	f.Badf(e.Pos(), "comparison of function %v %v nil is always %v", obj.Name(), e.Op, e.Op == token.NEQ)
+}
+
+// isNil reports whether the provided expression is the built-in nil
+// identifier.
+func (f *File) isNil(e ast.Expr) bool {
+	return f.pkg.types[e] == types.Typ[types.UntypedNil]
+}

cmd/vet/testdata/nilfunc.go

@@ -0,0 +1,37 @@
+// Copyright 2013 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package testdata
+
+import "os"
+
+func F() {}
+
+type T struct {
+	F func()
+}
+
+func (T) M() {}
+
+var Fv = F
+
+func Comparison() {
+	var t T
+	var fn func()
+	if fn == nil || Fv == nil || t.F == nil {
+		// no error; these func vars or fields may be nil
+	}
+	if F == nil { // ERROR "comparison of function F == nil is always false"
+		panic("can't happen")
+	}
+	if t.M == nil { // ERROR "comparison of function M == nil is always false"
+		panic("can't happen")
+	}
+	if F != nil { // ERROR "comparison of function F != nil is always true"
+		if t.M != nil { // ERROR "comparison of function M != nil is always true"
+			return
+		}
+	}
+	panic("can't happen")
+}

cmd/vet/types.go

@@ -266,7 +266,7 @@ func (f *File) isErrorMethodCall(call *ast.CallExpr) bool {
 	}
 	// There must be a receiver for it to be a method call. Otherwise it is
 	// a function, not something that satisfies the error interface.
-	if sig.Recv == nil {
+	if sig.Recv() == nil {
 		return false
 	}
 	// There must be no arguments. Already verified by type checking, but be thorough.