diff --git a/lint.go b/lint.go index fb47da00..935ac83e 100644 --- a/lint.go +++ b/lint.go @@ -199,6 +199,7 @@ func (f *file) lint() { f.lintNames() f.lintVarDecls() f.lintElses() + f.lintIfError() f.lintRanges() f.lintErrorf() f.lintErrors() @@ -1466,6 +1467,85 @@ func (f *file) lintContextArgs() { }) } +// containsComments returns whether the interval [start, end) contains any +// comments without "// MATCH " prefix. +func (f *file) containsComments(start, end token.Pos) bool { + for _, cgroup := range f.f.Comments { + comments := cgroup.List + if comments[0].Slash >= end { + // All comments starting with this group are after end pos. + return false + } + if comments[len(comments)-1].Slash < start { + // Comments group ends before start pos. + continue + } + for _, c := range comments { + if start <= c.Slash && c.Slash < end && !strings.HasPrefix(c.Text, "// MATCH ") { + return true + } + } + } + return false +} + +func (f *file) lintIfError() { + f.walk(func(node ast.Node) bool { + switch v := node.(type) { + case *ast.BlockStmt: + for i := 0; i < len(v.List)-1; i++ { + // if var := whatever; var != nil { return var } + s, ok := v.List[i].(*ast.IfStmt) + if !ok || s.Body == nil || len(s.Body.List) != 1 || s.Else != nil { + continue + } + assign, ok := s.Init.(*ast.AssignStmt) + if !ok || len(assign.Lhs) != 1 || !(assign.Tok == token.DEFINE || assign.Tok == token.ASSIGN) { + continue + } + id, ok := assign.Lhs[0].(*ast.Ident) + if !ok { + continue + } + expr, ok := s.Cond.(*ast.BinaryExpr) + if !ok || expr.Op != token.NEQ { + continue + } + if lhs, ok := expr.X.(*ast.Ident); !ok || lhs.Name != id.Name { + continue + } + if rhs, ok := expr.Y.(*ast.Ident); !ok || rhs.Name != "nil" { + continue + } + r, ok := s.Body.List[0].(*ast.ReturnStmt) + if !ok || len(r.Results) != 1 { + continue + } + if r, ok := r.Results[0].(*ast.Ident); !ok || r.Name != id.Name { + continue + } + + // return nil + r, ok = v.List[i+1].(*ast.ReturnStmt) + if !ok || len(r.Results) != 1 { + continue + } + if r, ok := r.Results[0].(*ast.Ident); !ok || r.Name != "nil" { + continue + } + + // check if there are any comments explaining the construct, don't emit an error if there are some. + if f.containsComments(s.Pos(), r.Pos()) { + continue + } + + f.errorf(v.List[i], 0.9, "redundant if ...; err != nil check, just return error instead.") + } + } + return true + }) +} + // receiverType returns the named type of the method receiver, sans "*", // or "invalid-type" if fn.Recv is ill formed. func receiverType(fn *ast.FuncDecl) string { diff --git a/testdata/iferr.go b/testdata/iferr.go new file mode 100644 index 00000000..920fa9ea --- /dev/null +++ b/testdata/iferr.go @@ -0,0 +1,101 @@ +// Test of redundant if err != nil + +// Package pkg ... +package pkg + +func f() error { + if err := f(); err != nil { + g() + return err + } + return nil +} + +func g() error { + if err := f(); err != nil { // MATCH /redundant/ + return err + } + return nil +} + +func h() error { + if err, x := f(), 1; err != nil { + return err + } + return nil +} + +func i() error { + a := 1 + if err := f(); err != nil { + a++ + return err + } + return nil +} + +func j() error { + var a error + if err := f(); err != nil { + return err + } + return a +} + +func k() error { + if err := f(); err != nil { + // TODO: handle error better + return err + } + return nil +} + +func l() (interface{}, error) { + if err := f(); err != nil { + return nil, err + } + if err := f(); err != nil { + return nil, err + } + if err := f(); err != nil { + return nil, err + } + // Phew, it worked + return nil +} + +func m() error { + if err := f(); err != nil { + return err + } + if err := f(); err != nil { + return err + } + if err := f(); err != nil { + return err + } + // Phew, it worked again. + return nil +} + +func multi() error { + a := 0 + var err error + // unreachable code after return statements is intentional to check that it + // doesn't confuse the linter. + if true { + a++ + if err := f(); err != nil { // MATCH /redundant/ + return err + } + return nil + a++ + } else { + a++ + if err = f(); err != nil { // MATCH /redundant/ + return err + } + return nil + a++ + } +}