From 662ffb028a1942c42a38bc62d8db5ee3c3cba9e8 Mon Sep 17 00:00:00 2001 From: Gabriel Burt Date: Wed, 11 Jan 2023 14:07:02 -0500 Subject: [PATCH] test and fix check blanks in var declarations (#219) --- errcheck/analyzer_test.go | 23 ++++- errcheck/errcheck.go | 141 ++++++++++++++++------------ errcheck/testdata/src/blank/main.go | 28 ++++++ 3 files changed, 131 insertions(+), 61 deletions(-) diff --git a/errcheck/analyzer_test.go b/errcheck/analyzer_test.go index 7a55be0..42e4ee7 100644 --- a/errcheck/analyzer_test.go +++ b/errcheck/analyzer_test.go @@ -1,12 +1,29 @@ package errcheck import ( - "golang.org/x/tools/go/analysis/analysistest" "path/filepath" "testing" + + "golang.org/x/tools/go/analysis/analysistest" ) func TestAnalyzer(t *testing.T) { - packageDir := filepath.Join(analysistest.TestData(), "src/a/") - _ = analysistest.Run(t, packageDir, Analyzer) + t.Run("default flags", func(t *testing.T) { + packageDir := filepath.Join(analysistest.TestData(), "src/a/") + _ = analysistest.Run(t, packageDir, Analyzer) + }) + + t.Run("check blanks", func(t *testing.T) { + packageDir := filepath.Join(analysistest.TestData(), "src/blank/") + Analyzer.Flags.Set("blank", "true") + _ = analysistest.Run(t, packageDir, Analyzer) + Analyzer.Flags.Set("blank", "false") // reset it + }) + + t.Run("check asserts", func(t *testing.T) { + packageDir := filepath.Join(analysistest.TestData(), "src/assert/") + Analyzer.Flags.Set("assert", "true") + _ = analysistest.Run(t, packageDir, Analyzer) + Analyzer.Flags.Set("assert", "false") // reset it + }) } diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 0a4067f..a5ee371 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -569,73 +569,98 @@ func (v *visitor) Visit(node ast.Node) ast.Visitor { if !v.ignoreCall(stmt.Call) && v.callReturnsError(stmt.Call) { v.addErrorAtPosition(stmt.Call.Lparen, stmt.Call) } + case *ast.GenDecl: + if stmt.Tok != token.VAR { + break + } + + for _, spec := range stmt.Specs { + vspec := spec.(*ast.ValueSpec) + + if len(vspec.Values) == 0 { + // ignore declarations w/o assignments + continue + } + + var lhs []ast.Expr + for _, name := range vspec.Names { + lhs = append(lhs, ast.Expr(name)) + } + v.checkAssignment(lhs, vspec.Values) + } + case *ast.AssignStmt: - if len(stmt.Rhs) == 1 { - // single value on rhs; check against lhs identifiers - if call, ok := stmt.Rhs[0].(*ast.CallExpr); ok { - if !v.blank { - break - } - if v.ignoreCall(call) { - break - } - isError := v.errorsByArg(call) - for i := 0; i < len(stmt.Lhs); i++ { - if id, ok := stmt.Lhs[i].(*ast.Ident); ok { - // We shortcut calls to recover() because errorsByArg can't - // check its return types for errors since it returns interface{}. - if id.Name == "_" && (v.isRecover(call) || isError[i]) { - v.addErrorAtPosition(id.NamePos, call) - } + v.checkAssignment(stmt.Lhs, stmt.Rhs) + + default: + } + return v +} + +func (v *visitor) checkAssignment(lhs, rhs []ast.Expr) { + if len(rhs) == 1 { + // single value on rhs; check against lhs identifiers + if call, ok := rhs[0].(*ast.CallExpr); ok { + if !v.blank { + return + } + if v.ignoreCall(call) { + return + } + isError := v.errorsByArg(call) + for i := 0; i < len(lhs); i++ { + if id, ok := lhs[i].(*ast.Ident); ok { + // We shortcut calls to recover() because errorsByArg can't + // check its return types for errors since it returns interface{}. + if id.Name == "_" && (v.isRecover(call) || isError[i]) { + v.addErrorAtPosition(id.NamePos, call) } } - } else if assert, ok := stmt.Rhs[0].(*ast.TypeAssertExpr); ok { - if !v.asserts { - break - } - if assert.Type == nil { - // type switch - break - } - if len(stmt.Lhs) < 2 { - // assertion result not read - v.addErrorAtPosition(stmt.Rhs[0].Pos(), nil) - } else if id, ok := stmt.Lhs[1].(*ast.Ident); ok && v.blank && id.Name == "_" { - // assertion result ignored - v.addErrorAtPosition(id.NamePos, nil) - } } - } else { - // multiple value on rhs; in this case a call can't return - // multiple values. Assume len(stmt.Lhs) == len(stmt.Rhs) - for i := 0; i < len(stmt.Lhs); i++ { - if id, ok := stmt.Lhs[i].(*ast.Ident); ok { - if call, ok := stmt.Rhs[i].(*ast.CallExpr); ok { - if !v.blank { - continue - } - if v.ignoreCall(call) { - continue - } - if id.Name == "_" && v.callReturnsError(call) { - v.addErrorAtPosition(id.NamePos, call) - } - } else if assert, ok := stmt.Rhs[i].(*ast.TypeAssertExpr); ok { - if !v.asserts { - continue - } - if assert.Type == nil { - // Shouldn't happen anyway, no multi assignment in type switches - continue - } - v.addErrorAtPosition(id.NamePos, nil) + } else if assert, ok := rhs[0].(*ast.TypeAssertExpr); ok { + if !v.asserts { + return + } + if assert.Type == nil { + // type switch + return + } + if len(lhs) < 2 { + // assertion result not read + v.addErrorAtPosition(rhs[0].Pos(), nil) + } else if id, ok := lhs[1].(*ast.Ident); ok && v.blank && id.Name == "_" { + // assertion result ignored + v.addErrorAtPosition(id.NamePos, nil) + } + } + } else { + // multiple value on rhs; in this case a call can't return + // multiple values. Assume len(lhs) == len(rhs) + for i := 0; i < len(lhs); i++ { + if id, ok := lhs[i].(*ast.Ident); ok { + if call, ok := rhs[i].(*ast.CallExpr); ok { + if !v.blank { + continue + } + if v.ignoreCall(call) { + continue + } + if id.Name == "_" && v.callReturnsError(call) { + v.addErrorAtPosition(id.NamePos, call) + } + } else if assert, ok := rhs[i].(*ast.TypeAssertExpr); ok { + if !v.asserts { + continue } + if assert.Type == nil { + // Shouldn't happen anyway, no multi assignment in type switches + continue + } + v.addErrorAtPosition(id.NamePos, nil) } } } - default: } - return v } func isErrorType(t types.Type) bool { diff --git a/errcheck/testdata/src/blank/main.go b/errcheck/testdata/src/blank/main.go index eaf3281..d69cbf6 100644 --- a/errcheck/testdata/src/blank/main.go +++ b/errcheck/testdata/src/blank/main.go @@ -1,9 +1,37 @@ package blank +import "fmt" + func a() error { return nil } +func b() (string, error) { + return "", nil +} + +func c() string { + return "" +} + func main() { _ = a() // want "unchecked error" + a() // want "unchecked error" + b() // want "unchecked error" + c() // ignored, doesn't return an error + + { + r, err := b() // fine, we're checking the error + fmt.Printf("r = %v, err = %v\n", r, err) + } + + { + r, _ := b() // want "unchecked error" + fmt.Printf("r = %v\n", r) + } + + { + var r, _ = b() // want "unchecked error" + fmt.Printf("r = %v\n", r) + } }