Skip to content

Commit

Permalink
Improve global error detection (#32)
Browse files Browse the repository at this point in the history 
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
  • Loading branch information
@leonklingele @leighmcculloch
leonklingele and leighmcculloch committed Jan 16, 2023
1 parent 993015c commit 03b61a6
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 43 deletions.
27 changes: 20 additions & 7 deletions checknoglobals/check_no_globals.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"go/ast"
"go/token"
"go/types"
"strings"

"golang.org/x/tools/go/analysis"
Expand Down Expand Up @@ -48,12 +49,12 @@ func flags() flag.FlagSet {
return *flags
}

func isAllowed(cm ast.CommentMap, v ast.Node) bool {
func isAllowed(cm ast.CommentMap, v ast.Node, ti *types.Info) bool {
switch i := v.(type) {
case *ast.GenDecl:
return hasEmbedComment(cm, i)
case *ast.Ident:
return i.Name == "_" || i.Name == "version" || looksLikeError(i) || identHasEmbedComment(cm, i)
return i.Name == "_" || i.Name == "version" || isError(i, ti) || identHasEmbedComment(cm, i)
case *ast.CallExpr:
if expr, ok := i.Fun.(*ast.SelectorExpr); ok {
return isAllowedSelectorExpression(expr)
Expand Down Expand Up @@ -86,10 +87,14 @@ func isAllowedSelectorExpression(v *ast.SelectorExpr) bool {
return false
}

// isError reports whether the AST identifier looks like
// an error and implements the error interface.
func isError(i *ast.Ident, ti *types.Info) bool {
return looksLikeError(i) && implementsError(i, ti)
}

// looksLikeError returns true if the AST identifier starts
// with 'err' or 'Err', or false otherwise.
//
// TODO: https://github.com/leighmcculloch/gochecknoglobals/issues/5
func looksLikeError(i *ast.Ident) bool {
prefix := "err"
if i.IsExported() {
Expand All @@ -98,6 +103,14 @@ func looksLikeError(i *ast.Ident) bool {
return strings.HasPrefix(i.Name, prefix)
}

// implementsError reports whether the AST identifier
// implements the error interface.
func implementsError(i *ast.Ident, ti *types.Info) bool {
t := ti.TypeOf(i)
et := types.Universe.Lookup("error").Type().Underlying().(*types.Interface)
return types.Implements(t, et)
}

func identHasEmbedComment(cm ast.CommentMap, i *ast.Ident) bool {
if i.Obj == nil {
return false
Expand Down Expand Up @@ -146,15 +159,15 @@ func checkNoGlobals(pass *analysis.Pass) (interface{}, error) {
if genDecl.Tok != token.VAR {
continue
}
if isAllowed(fileCommentMap, genDecl) {
if isAllowed(fileCommentMap, genDecl, pass.TypesInfo) {
continue
}
for _, spec := range genDecl.Specs {
valueSpec := spec.(*ast.ValueSpec)
onlyAllowedValues := false

for _, vn := range valueSpec.Values {
if isAllowed(fileCommentMap, vn) {
if isAllowed(fileCommentMap, vn, pass.TypesInfo) {
onlyAllowedValues = true
continue
}
Expand All @@ -168,7 +181,7 @@ func checkNoGlobals(pass *analysis.Pass) (interface{}, error) {
}

for _, vn := range valueSpec.Names {
if isAllowed(fileCommentMap, vn) {
if isAllowed(fileCommentMap, vn, pass.TypesInfo) {
continue
}

Expand Down
7 changes: 0 additions & 7 deletions checknoglobals/testdata/src/10/code.go
View file
Original file line number Diff line number Diff line change
@@ -1,20 +1,13 @@
package code

import (
"errors"
"net/http"
"regexp"
)

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

// ErrNotFound is an error and should be OK.
var ErrNotFound = errors.New("this is error")

// ErrIsNotErr is an error and should be OK.
var ErrIsNotErr = 1

// IsOnlyDigitsRe is a global regexp that should be OK.
var IsOnlyDigitsRe = regexp.MustCompile(`^\d+$`)

Expand Down
65 changes: 53 additions & 12 deletions checknoglobals/testdata/src/7/code.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,71 @@ import (
)

// Those are not errors
var myVar = 1 // want "myVar is a global variable"
var myVar = 1 // want "myVar is a global variable"
var myVarFunc = func() int { return 1 }() // want "myVarFunc is a global variable"

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

// Those errors are not named correctly
var myErrVar = errors.New("myErrVar") // want "myErrVar is a global variable"
var myVarErr = errors.New("myVarErr") // want "myVarErr is a global variable"
var myVarError = errors.New("myVarErr") // want "myVarError is a global variable"
var customErr = customError{"customErr"} // want "customErr is a global variable"
var myErrVar = errors.New("myErrVar") // want "myErrVar is a global variable"
var myVarErr = errors.New("myVarErr") // want "myVarErr is a global variable"
var myVarError = errors.New("myVarErr") // want "myVarError is a global variable"
var customErr = customError{"customErr"} // want "customErr is a global variable"
var myErrVarFunc = func() error { return errors.New("myErrVarFunc") }() // want "myErrVarFunc is a global variable"
var myVarErrFunc = func() error { return errors.New("myVarErrFunc") }() // want "myVarErrFunc is a global variable"
var myVarErrorFunc = func() error { return errors.New("myVarErrorFunc") }() // want "myVarErrorFunc is a global variable"
var customErrFunc = func() error { return errors.New("customErrFunc") }() // want "customErrFunc is a global variable"

// Those are actual errors which should be ignored
var errUnexported = errors.New("errUnexported")
var ErrExported = errors.New("ErrExported")
var errCustomUnexported = customError{"errCustomUnexported"}
var ErrCustomExported = customError{"ErrCustomExported"}
var errCustomUnexported = &customError{"errCustomUnexported"}
var ErrCustomExported = &customError{"ErrCustomExported"}
var errCustomUnexported2 = customError2{"errCustomUnexported"}
var ErrCustomExported2 = customError2{"ErrCustomExported"}
var errCustomUnexported3 = customError3("errCustomUnexported")
var ErrCustomExported3 = customError3("ErrCustomExported")
var errUnexportedFunc = func() error { return errors.New("errUnexportedFunc") }()
var ErrExportedFunc = func() error { return errors.New("ErrExportedFunc") }()
var errCustomUnexportedFunc = func() *customError { return &customError{"errCustomUnexportedFunc"} }()
var errCustomUnexportedFuncErr = func() error { return &customError{"errCustomUnexportedFuncErr"} }()
var ErrCustomExportedFunc = func() *customError { return &customError{"ErrCustomExportedFunc"} }()
var ErrCustomExportedFuncErr = func() error { return &customError{"ErrCustomExportedFuncErr"} }()

// Those errors do not really implement the error interface
var errCustomNonPointerUnexported = customError{"errCustomNonPointerUnexported"} // want "errCustomNonPointerUnexported is a global variable"
var ErrCustomNonPointerExported = customError{"ErrCustomNonPointerExported"} // want "ErrCustomNonPointerExported is a global variable"
var errCustomNonPointerUnexportedFunc = func() customError { return customError{"errCustomNonPointerUnexportedFunc"} }() // want "errCustomNonPointerUnexportedFunc is a global variable"
var ErrCustomNonPointerExportedFunc = func() customError { return customError{"ErrCustomNonPointerExportedFunc"} }() // want "ErrCustomNonPointerExportedFunc is a global variable"

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

type customError struct{ e string }

func (e *customError) Error() string { return e.e }

func (e *customError) SomeOtherMethod() string { return e.e }

type customError2 struct{ e string }

func (e customError2) Error() string { return e.e }

func (e customError2) SomeOtherMethod() string { return e.e }

type customError3 string

func (e customError3) Error() string { return string(e) }

func (e customError3) SomeOtherMethod() string { return string(e) }
59 changes: 42 additions & 17 deletions checknoglobals/testdata/src/8/code.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,31 +6,56 @@ import (

var (
// Those are not errors
myVar = 1 // want "myVar is a global variable"

// Those are fake errors which are currently not detected
// because they start with 'err' or 'Err' and we don't
// check if such a variable implements the error interface.
errFakeErrorUnexported = 1
ErrFakeErrorExported = 1
myVar = 1 // want "myVar is a global variable"
myVarFunc = func() int { return 1 }() // want "myVarFunc is a global variable"

// Fake errors
errFakeErrorUnexported = 1 // want "errFakeErrorUnexported is a global variable"
ErrFakeErrorExported = 1 // want "ErrFakeErrorExported is a global variable"
fakeUnexportedErr = 1 // want "fakeUnexportedErr is a global variable"
FakeExportedErr = 1 // want "FakeExportedErr is a global variable"
errFakeErrorUnexportedFunc = func() int { return 1 }() // want "errFakeErrorUnexportedFunc is a global variable"
ErrFakeErrorExportedFunc = func() int { return 1 }() // want "ErrFakeErrorExportedFunc is a global variable"
fakeUnexportedErrFunc = func() int { return 1 }() // want "fakeUnexportedErrFunc is a global variable"
FakeExportedErrFunc = func() int { return 1 }() // want "FakeExportedErrFunc is a global variable"

// Those errors are not named correctly
myErrVar = errors.New("myErrVar") // want "myErrVar is a global variable"
myVarErr = errors.New("myVarErr") // want "myVarErr is a global variable"
myVarError = errors.New("myVarErr") // want "myVarError is a global variable"
customErr = customError{"customErr"} // want "customErr is a global variable"
myErrVar = errors.New("myErrVar") // want "myErrVar is a global variable"
myVarErr = errors.New("myVarErr") // want "myVarErr is a global variable"
myVarError = errors.New("myVarErr") // want "myVarError is a global variable"
customErr = customError{"customErr"} // want "customErr is a global variable"
myErrVarFunc = func() error { return errors.New("myErrVarFunc") }() // want "myErrVarFunc is a global variable"
myVarErrFunc = func() error { return errors.New("myVarErrFunc") }() // want "myVarErrFunc is a global variable"
myVarErrorFunc = func() error { return errors.New("myVarErrorFunc") }() // want "myVarErrorFunc is a global variable"
customErrFunc = func() error { return errors.New("customErrFunc") }() // want "customErrFunc is a global variable"

// Those are actual errors which should be ignored
errUnexported = errors.New("errUnexported")
ErrExported = errors.New("ErrExported")
errCustomUnexported = customError{"errCustomUnexported"}
ErrCustomExported = customError{"ErrCustomExported"}
errUnexported = errors.New("errUnexported")
ErrExported = errors.New("ErrExported")
errCustomUnexported = &customError{"errCustomUnexported"}
ErrCustomExported = &customError{"ErrCustomExported"}
errUnexportedFunc = func() error { return errors.New("errUnexportedFunc") }()
ErrExportedFunc = func() error { return errors.New("ErrExportedFunc") }()
errCustomUnexportedFunc = func() *customError { return &customError{"errCustomUnexportedFunc"} }()
errCustomUnexportedFuncErr = func() error { return &customError{"errCustomUnexportedFuncErr"} }()
ErrCustomExportedFunc = func() *customError { return &customError{"ErrCustomExportedFunc"} }()
ErrCustomExportedFuncErr = func() error { return &customError{"ErrCustomExportedFuncErr"} }()

// Those errors do not really implement the error interface
errCustomNonPointerUnexported = customError{"errCustomNonPointerUnexported"} // want "errCustomNonPointerUnexported is a global variable"
ErrCustomNonPointerExported = customError{"ErrCustomNonPointerExported"} // want "ErrCustomNonPointerExported is a global variable"
errCustomNonPointerUnexportedFunc = func() customError { return customError{"errCustomNonPointerUnexportedFunc"} }() // want "errCustomNonPointerUnexportedFunc is a global variable"
ErrCustomNonPointerExportedFunc = func() customError { return customError{"ErrCustomNonPointerExportedFunc"} }() // want "ErrCustomNonPointerExportedFunc is a global variable"

// Those actual errors have a declared error type
declaredErr error = errors.New("declaredErr") // want "declaredErr is a global variable"
errDeclared error = errors.New("errDeclared")
declaredErr error = errors.New("declaredErr") // want "declaredErr is a global variable"
errDeclared error = errors.New("errDeclared")
declaredErrFunc error = func() error { return errors.New("declaredErrFunc") }() // want "declaredErrFunc is a global variable"
errDeclaredFunc error = func() error { return errors.New("errDeclaredFunc") }()
)

type customError struct{ e string }

func (e *customError) Error() string { return e.e }

func (e *customError) SomeOtherMethod() string { return e.e }

0 comments on commit 03b61a6

Please sign in to comment.