Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(4095)

Delta Between Two Patch Sets: cmd/vet/nilfunc.go

Issue 13102043: code review 13102043: go.tools/cmd/vet: detect useless function comparisons (Closed)
Left Patch Set: diff -r a7b751825c27 https://code.google.com/p/go.tools Created 10 years, 7 months ago
Right Patch Set: diff -r d47ca4bb6e48 https://code.google.com/p/go.tools Created 10 years, 7 months ago
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments. Please Sign in to add in-line comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
« no previous file with change/comment | « cmd/vet/main.go ('k') | cmd/vet/testdata/nilfunc.go » ('j') | no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
LEFTRIGHT
1 // Copyright 2013 The Go Authors. All rights reserved. 1 // Copyright 2013 The Go Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style 2 // Use of this source code is governed by a BSD-style
3 // license that can be found in the LICENSE file. 3 // license that can be found in the LICENSE file.
4
5 /*
6 This file contains the code to check for useless function comparisons.
7 A useless comparison is one like f == nil as opposed to f() == nil.
8 */
4 9
5 package main 10 package main
6 11
7 import ( 12 import (
8 "go/ast" 13 "go/ast"
9 "go/token" 14 "go/token"
10 15
11 "code.google.com/p/go.tools/go/types" 16 "code.google.com/p/go.tools/go/types"
12 ) 17 )
13 18
14 func (f *File) checkNilFuncComparison(expr *ast.BinaryExpr) { 19 func (f *File) checkNilFuncComparison(e *ast.BinaryExpr) {
15 » // Only want == or != comparisons. 20 » if !vet("nilfunc") {
16 » if expr.Op != token.EQL && expr.Op != token.NEQ {
17 return 21 return
18 } 22 }
19 23
20 » // Only want comparisons with nil identifier on one side. 24 » // Only want == or != comparisons.
21 » var e ast.Expr 25 » if e.Op != token.EQL && e.Op != token.NEQ {
26 » » return
27 » }
28
29 » // Only want comparisons with a nil identifier on one side.
30 » var e2 ast.Expr
22 switch { 31 switch {
23 » case isNil(expr.X) && !isNil(expr.Y): 32 » case f.isNil(e.X):
24 » » e = expr.Y 33 » » e2 = e.Y
25 » case !isNil(expr.X) && isNil(expr.Y): 34 » case f.isNil(e.Y):
26 » » e = expr.X 35 » » e2 = e.X
27 default: 36 default:
28 return 37 return
29 } 38 }
30 39
31 // Only want identifiers or selector expressions. 40 // Only want identifiers or selector expressions.
32 var obj types.Object 41 var obj types.Object
33 » switch v := e.(type) { 42 » switch v := e2.(type) {
34 case *ast.Ident: 43 case *ast.Ident:
35 obj = f.pkg.idents[v] 44 obj = f.pkg.idents[v]
36 case *ast.SelectorExpr: 45 case *ast.SelectorExpr:
37 obj = f.pkg.idents[v.Sel] 46 obj = f.pkg.idents[v.Sel]
38 default: 47 default:
39 return 48 return
40 } 49 }
41 50
42 // Only want functions. 51 // Only want functions.
43 if _, ok := obj.(*types.Func); !ok { 52 if _, ok := obj.(*types.Func); !ok {
44 return 53 return
45 } 54 }
46 55
47 » f.Badf(expr.Pos(), "function uselessly compared to nil") 56 » f.Badf(e.Pos(), "comparison of function %v %v nil is always %v", obj.Nam e(), e.Op, e.Op == token.NEQ)
dsymonds 2013/08/20 04:30:56 I think this needs better phrasing. It's certainly
adg 2013/08/20 04:44:23 Please suggest better phrasing.
dsymonds 2013/08/20 04:45:53 "function comparison always <X>", depending on whe
adg 2013/08/20 04:56:55 Done.
48 } 57 }
49 58
50 // isNil reports whether the provided expression is the built-in nil 59 // isNil reports whether the provided expression is the built-in nil
51 // idenitifer. 60 // identifier.
dsymonds 2013/08/20 04:30:56 "identifier"
adg 2013/08/20 04:44:23 Done.
52 func isNil(e ast.Expr) bool { 61 func (f *File) isNil(e ast.Expr) bool {
53 » id, ok := e.(*ast.Ident) 62 » return f.pkg.types[e] == types.Typ[types.UntypedNil]
54 » if !ok {
55 » » return false
56 » }
57 » return id.Name == "nil" && id.Obj == nil
58 } 63 }
LEFTRIGHT

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b