Skip to content

Commit 03b61a6

Browse files
leonklingeleleighmcculloch
leonklingele
and
leighmcculloch
authored
Improve global error detection (#32)
Co-authored-by: Leigh McCulloch <[email protected]>
1 parent 993015c commit 03b61a6

File tree

4 files changed

+115
-43
lines changed

4 files changed

+115
-43
lines changed

‎checknoglobals/check_no_globals.go

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"go/ast"
77
"go/token"
8+
"go/types"
89
"strings"
910

1011
"golang.org/x/tools/go/analysis"
@@ -48,12 +49,12 @@ func flags() flag.FlagSet {
4849
return *flags
4950
}
5051

51-
func isAllowed(cm ast.CommentMap, v ast.Node) bool {
52+
func isAllowed(cm ast.CommentMap, v ast.Node, ti *types.Info) bool {
5253
switch i := v.(type) {
5354
case *ast.GenDecl:
5455
return hasEmbedComment(cm, i)
5556
case *ast.Ident:
56-
return i.Name == "_" || i.Name == "version" || looksLikeError(i) || identHasEmbedComment(cm, i)
57+
return i.Name == "_" || i.Name == "version" || isError(i, ti) || identHasEmbedComment(cm, i)
5758
case *ast.CallExpr:
5859
if expr, ok := i.Fun.(*ast.SelectorExpr); ok {
5960
return isAllowedSelectorExpression(expr)
@@ -86,10 +87,14 @@ func isAllowedSelectorExpression(v *ast.SelectorExpr) bool {
8687
return false
8788
}
8889

90+
// isError reports whether the AST identifier looks like
91+
// an error and implements the error interface.
92+
func isError(i *ast.Ident, ti *types.Info) bool {
93+
return looksLikeError(i) && implementsError(i, ti)
94+
}
95+
8996
// looksLikeError returns true if the AST identifier starts
9097
// with 'err' or 'Err', or false otherwise.
91-
//
92-
// TODO: https://github.com/leighmcculloch/gochecknoglobals/issues/5
9398
func looksLikeError(i *ast.Ident) bool {
9499
prefix := "err"
95100
if i.IsExported() {
@@ -98,6 +103,14 @@ func looksLikeError(i *ast.Ident) bool {
98103
return strings.HasPrefix(i.Name, prefix)
99104
}
100105

106+
// implementsError reports whether the AST identifier
107+
// implements the error interface.
108+
func implementsError(i *ast.Ident, ti *types.Info) bool {
109+
t := ti.TypeOf(i)
110+
et := types.Universe.Lookup("error").Type().Underlying().(*types.Interface)
111+
return types.Implements(t, et)
112+
}
113+
101114
func identHasEmbedComment(cm ast.CommentMap, i *ast.Ident) bool {
102115
if i.Obj == nil {
103116
return false
@@ -146,15 +159,15 @@ func checkNoGlobals(pass *analysis.Pass) (interface{}, error) {
146159
if genDecl.Tok != token.VAR {
147160
continue
148161
}
149-
if isAllowed(fileCommentMap, genDecl) {
162+
if isAllowed(fileCommentMap, genDecl, pass.TypesInfo) {
150163
continue
151164
}
152165
for _, spec := range genDecl.Specs {
153166
valueSpec := spec.(*ast.ValueSpec)
154167
onlyAllowedValues := false
155168

156169
for _, vn := range valueSpec.Values {
157-
if isAllowed(fileCommentMap, vn) {
170+
if isAllowed(fileCommentMap, vn, pass.TypesInfo) {
158171
onlyAllowedValues = true
159172
continue
160173
}
@@ -168,7 +181,7 @@ func checkNoGlobals(pass *analysis.Pass) (interface{}, error) {
168181
}
169182

170183
for _, vn := range valueSpec.Names {
171-
if isAllowed(fileCommentMap, vn) {
184+
if isAllowed(fileCommentMap, vn, pass.TypesInfo) {
172185
continue
173186
}
174187

‎checknoglobals/testdata/src/10/code.go

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,13 @@
11
package code
22

33
import (
4-
"errors"
54
"net/http"
65
"regexp"
76
)
87

98
// myVar is just a bad named global var.
109
var myVar = 1 // want "myVar is a global variable"
1110

12-
// ErrNotFound is an error and should be OK.
13-
var ErrNotFound = errors.New("this is error")
14-
15-
// ErrIsNotErr is an error and should be OK.
16-
var ErrIsNotErr = 1
17-
1811
// IsOnlyDigitsRe is a global regexp that should be OK.
1912
var IsOnlyDigitsRe = regexp.MustCompile(`^\d+$`)
2013

‎checknoglobals/testdata/src/7/code.go

Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,30 +5,71 @@ import (
55
)
66

77
// Those are not errors
8-
var myVar = 1 // want "myVar is a global variable"
8+
var myVar = 1 // want "myVar is a global variable"
9+
var myVarFunc = func() int { return 1 }() // want "myVarFunc is a global variable"
910

10-
// Those are fake errors which are currently not detected
11-
// because they start with 'err' or 'Err' and we don't
12-
// check if such a variable implements the error interface.
13-
var errFakeErrorUnexported = 1
14-
var ErrFakeErrorExported = 1
11+
// Fake errors
12+
var errFakeErrorUnexported = 1 // want "errFakeErrorUnexported is a global variable"
13+
var ErrFakeErrorExported = 1 // want "ErrFakeErrorExported is a global variable"
14+
var fakeUnexportedErr = 1 // want "fakeUnexportedErr is a global variable"
15+
var FakeExportedErr = 1 // want "FakeExportedErr is a global variable"
16+
var errFakeErrorUnexportedFunc = func() int { return 1 }() // want "errFakeErrorUnexportedFunc is a global variable"
17+
var ErrFakeErrorExportedFunc = func() int { return 1 }() // want "ErrFakeErrorExportedFunc is a global variable"
18+
var fakeUnexportedErrFunc = func() int { return 1 }() // want "fakeUnexportedErrFunc is a global variable"
19+
var FakeExportedErrFunc = func() int { return 1 }() // want "FakeExportedErrFunc is a global variable"
1520

1621
// Those errors are not named correctly
17-
var myErrVar = errors.New("myErrVar") // want "myErrVar is a global variable"
18-
var myVarErr = errors.New("myVarErr") // want "myVarErr is a global variable"
19-
var myVarError = errors.New("myVarErr") // want "myVarError is a global variable"
20-
var customErr = customError{"customErr"} // want "customErr is a global variable"
22+
var myErrVar = errors.New("myErrVar") // want "myErrVar is a global variable"
23+
var myVarErr = errors.New("myVarErr") // want "myVarErr is a global variable"
24+
var myVarError = errors.New("myVarErr") // want "myVarError is a global variable"
25+
var customErr = customError{"customErr"} // want "customErr is a global variable"
26+
var myErrVarFunc = func() error { return errors.New("myErrVarFunc") }() // want "myErrVarFunc is a global variable"
27+
var myVarErrFunc = func() error { return errors.New("myVarErrFunc") }() // want "myVarErrFunc is a global variable"
28+
var myVarErrorFunc = func() error { return errors.New("myVarErrorFunc") }() // want "myVarErrorFunc is a global variable"
29+
var customErrFunc = func() error { return errors.New("customErrFunc") }() // want "customErrFunc is a global variable"
2130

2231
// Those are actual errors which should be ignored
2332
var errUnexported = errors.New("errUnexported")
2433
var ErrExported = errors.New("ErrExported")
25-
var errCustomUnexported = customError{"errCustomUnexported"}
26-
var ErrCustomExported = customError{"ErrCustomExported"}
34+
var errCustomUnexported = &customError{"errCustomUnexported"}
35+
var ErrCustomExported = &customError{"ErrCustomExported"}
36+
var errCustomUnexported2 = customError2{"errCustomUnexported"}
37+
var ErrCustomExported2 = customError2{"ErrCustomExported"}
38+
var errCustomUnexported3 = customError3("errCustomUnexported")
39+
var ErrCustomExported3 = customError3("ErrCustomExported")
40+
var errUnexportedFunc = func() error { return errors.New("errUnexportedFunc") }()
41+
var ErrExportedFunc = func() error { return errors.New("ErrExportedFunc") }()
42+
var errCustomUnexportedFunc = func() *customError { return &customError{"errCustomUnexportedFunc"} }()
43+
var errCustomUnexportedFuncErr = func() error { return &customError{"errCustomUnexportedFuncErr"} }()
44+
var ErrCustomExportedFunc = func() *customError { return &customError{"ErrCustomExportedFunc"} }()
45+
var ErrCustomExportedFuncErr = func() error { return &customError{"ErrCustomExportedFuncErr"} }()
46+
47+
// Those errors do not really implement the error interface
48+
var errCustomNonPointerUnexported = customError{"errCustomNonPointerUnexported"} // want "errCustomNonPointerUnexported is a global variable"
49+
var ErrCustomNonPointerExported = customError{"ErrCustomNonPointerExported"} // want "ErrCustomNonPointerExported is a global variable"
50+
var errCustomNonPointerUnexportedFunc = func() customError { return customError{"errCustomNonPointerUnexportedFunc"} }() // want "errCustomNonPointerUnexportedFunc is a global variable"
51+
var ErrCustomNonPointerExportedFunc = func() customError { return customError{"ErrCustomNonPointerExportedFunc"} }() // want "ErrCustomNonPointerExportedFunc is a global variable"
2752

2853
// Those actual errors have a declared error type
2954
var declaredErr error = errors.New("declaredErr") // want "declaredErr is a global variable"
3055
var errDeclared error = errors.New("errDeclared")
56+
var declaredErrFunc error = func() error { return errors.New("declaredErrFunc") }() // want "declaredErrFunc is a global variable"
57+
var errDeclaredFunc error = func() error { return errors.New("errDeclaredFunc") }()
3158

3259
type customError struct{ e string }
3360

3461
func (e *customError) Error() string { return e.e }
62+
63+
func (e *customError) SomeOtherMethod() string { return e.e }
64+
65+
type customError2 struct{ e string }
66+
67+
func (e customError2) Error() string { return e.e }
68+
69+
func (e customError2) SomeOtherMethod() string { return e.e }
70+
71+
type customError3 string
72+
73+
func (e customError3) Error() string { return string(e) }
74+
75+
func (e customError3) SomeOtherMethod() string { return string(e) }

‎checknoglobals/testdata/src/8/code.go

Lines changed: 42 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,31 +6,56 @@ import (
66

77
var (
88
// Those are not errors
9-
myVar = 1 // want "myVar is a global variable"
10-
11-
// Those are fake errors which are currently not detected
12-
// because they start with 'err' or 'Err' and we don't
13-
// check if such a variable implements the error interface.
14-
errFakeErrorUnexported = 1
15-
ErrFakeErrorExported = 1
9+
myVar = 1 // want "myVar is a global variable"
10+
myVarFunc = func() int { return 1 }() // want "myVarFunc is a global variable"
11+
12+
// Fake errors
13+
errFakeErrorUnexported = 1 // want "errFakeErrorUnexported is a global variable"
14+
ErrFakeErrorExported = 1 // want "ErrFakeErrorExported is a global variable"
15+
fakeUnexportedErr = 1 // want "fakeUnexportedErr is a global variable"
16+
FakeExportedErr = 1 // want "FakeExportedErr is a global variable"
17+
errFakeErrorUnexportedFunc = func() int { return 1 }() // want "errFakeErrorUnexportedFunc is a global variable"
18+
ErrFakeErrorExportedFunc = func() int { return 1 }() // want "ErrFakeErrorExportedFunc is a global variable"
19+
fakeUnexportedErrFunc = func() int { return 1 }() // want "fakeUnexportedErrFunc is a global variable"
20+
FakeExportedErrFunc = func() int { return 1 }() // want "FakeExportedErrFunc is a global variable"
1621

1722
// Those errors are not named correctly
18-
myErrVar = errors.New("myErrVar") // want "myErrVar is a global variable"
19-
myVarErr = errors.New("myVarErr") // want "myVarErr is a global variable"
20-
myVarError = errors.New("myVarErr") // want "myVarError is a global variable"
21-
customErr = customError{"customErr"} // want "customErr is a global variable"
23+
myErrVar = errors.New("myErrVar") // want "myErrVar is a global variable"
24+
myVarErr = errors.New("myVarErr") // want "myVarErr is a global variable"
25+
myVarError = errors.New("myVarErr") // want "myVarError is a global variable"
26+
customErr = customError{"customErr"} // want "customErr is a global variable"
27+
myErrVarFunc = func() error { return errors.New("myErrVarFunc") }() // want "myErrVarFunc is a global variable"
28+
myVarErrFunc = func() error { return errors.New("myVarErrFunc") }() // want "myVarErrFunc is a global variable"
29+
myVarErrorFunc = func() error { return errors.New("myVarErrorFunc") }() // want "myVarErrorFunc is a global variable"
30+
customErrFunc = func() error { return errors.New("customErrFunc") }() // want "customErrFunc is a global variable"
2231

2332
// Those are actual errors which should be ignored
24-
errUnexported = errors.New("errUnexported")
25-
ErrExported = errors.New("ErrExported")
26-
errCustomUnexported = customError{"errCustomUnexported"}
27-
ErrCustomExported = customError{"ErrCustomExported"}
33+
errUnexported = errors.New("errUnexported")
34+
ErrExported = errors.New("ErrExported")
35+
errCustomUnexported = &customError{"errCustomUnexported"}
36+
ErrCustomExported = &customError{"ErrCustomExported"}
37+
errUnexportedFunc = func() error { return errors.New("errUnexportedFunc") }()
38+
ErrExportedFunc = func() error { return errors.New("ErrExportedFunc") }()
39+
errCustomUnexportedFunc = func() *customError { return &customError{"errCustomUnexportedFunc"} }()
40+
errCustomUnexportedFuncErr = func() error { return &customError{"errCustomUnexportedFuncErr"} }()
41+
ErrCustomExportedFunc = func() *customError { return &customError{"ErrCustomExportedFunc"} }()
42+
ErrCustomExportedFuncErr = func() error { return &customError{"ErrCustomExportedFuncErr"} }()
43+
44+
// Those errors do not really implement the error interface
45+
errCustomNonPointerUnexported = customError{"errCustomNonPointerUnexported"} // want "errCustomNonPointerUnexported is a global variable"
46+
ErrCustomNonPointerExported = customError{"ErrCustomNonPointerExported"} // want "ErrCustomNonPointerExported is a global variable"
47+
errCustomNonPointerUnexportedFunc = func() customError { return customError{"errCustomNonPointerUnexportedFunc"} }() // want "errCustomNonPointerUnexportedFunc is a global variable"
48+
ErrCustomNonPointerExportedFunc = func() customError { return customError{"ErrCustomNonPointerExportedFunc"} }() // want "ErrCustomNonPointerExportedFunc is a global variable"
2849

2950
// Those actual errors have a declared error type
30-
declaredErr error = errors.New("declaredErr") // want "declaredErr is a global variable"
31-
errDeclared error = errors.New("errDeclared")
51+
declaredErr error = errors.New("declaredErr") // want "declaredErr is a global variable"
52+
errDeclared error = errors.New("errDeclared")
53+
declaredErrFunc error = func() error { return errors.New("declaredErrFunc") }() // want "declaredErrFunc is a global variable"
54+
errDeclaredFunc error = func() error { return errors.New("errDeclaredFunc") }()
3255
)
3356

3457
type customError struct{ e string }
3558

3659
func (e *customError) Error() string { return e.e }
60+
61+
func (e *customError) SomeOtherMethod() string { return e.e }

0 commit comments

Comments
 (0)