Skip to content

Commit

Permalink
test and fix check blanks in var declarations (#219)
Browse files Browse the repository at this point in the history
  • Loading branch information
gburt authored Jan 11, 2023
1 parent c2c614f commit 662ffb0
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 61 deletions.
23 changes: 20 additions & 3 deletions errcheck/analyzer_test.go
Original file line number Diff line number Diff line change
@@ -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
})
}
141 changes: 83 additions & 58 deletions errcheck/errcheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
28 changes: 28 additions & 0 deletions errcheck/testdata/src/blank/main.go
Original file line number Diff line number Diff line change
@@ -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)
}
}

0 comments on commit 662ffb0

Please sign in to comment.