diff --git a/checknoglobals/check_no_globals.go b/checknoglobals/check_no_globals.go index 9ae889d..edf9193 100644 --- a/checknoglobals/check_no_globals.go +++ b/checknoglobals/check_no_globals.go @@ -5,6 +5,7 @@ import ( "fmt" "go/ast" "go/token" + "go/types" "strings" "golang.org/x/tools/go/analysis" @@ -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) @@ -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() { @@ -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 @@ -146,7 +159,7 @@ 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 { @@ -154,7 +167,7 @@ func checkNoGlobals(pass *analysis.Pass) (interface{}, error) { onlyAllowedValues := false for _, vn := range valueSpec.Values { - if isAllowed(fileCommentMap, vn) { + if isAllowed(fileCommentMap, vn, pass.TypesInfo) { onlyAllowedValues = true continue } @@ -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 } diff --git a/checknoglobals/testdata/src/10/code.go b/checknoglobals/testdata/src/10/code.go index ec08b68..f769728 100644 --- a/checknoglobals/testdata/src/10/code.go +++ b/checknoglobals/testdata/src/10/code.go @@ -1,7 +1,6 @@ package code import ( - "errors" "net/http" "regexp" ) @@ -9,12 +8,6 @@ import ( // 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+$`) diff --git a/checknoglobals/testdata/src/7/code.go b/checknoglobals/testdata/src/7/code.go index be3d623..ea7565f 100644 --- a/checknoglobals/testdata/src/7/code.go +++ b/checknoglobals/testdata/src/7/code.go @@ -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) } diff --git a/checknoglobals/testdata/src/8/code.go b/checknoglobals/testdata/src/8/code.go index a0472c5..0f71887 100644 --- a/checknoglobals/testdata/src/8/code.go +++ b/checknoglobals/testdata/src/8/code.go @@ -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 }