Check for redundant if err != nil constructs. (#319)
* Check for redundant if err != nil constructs.
Detect and complain about constructs like:
if err := foo(); err != nil {
return err
}
return nil
(Issue https://github.com/golang/lint/issues/312)
* Rewrite lintIfError without using matchers.
Also don't emit lint errors if there are any comments explaining the
construct (between if and return statements).
diff --git a/lint.go b/lint.go
index fb47da0..935ac83 100644
--- a/lint.go
+++ b/lint.go
@@ -199,6 +199,7 @@
f.lintNames()
f.lintVarDecls()
f.lintElses()
+ f.lintIfError()
f.lintRanges()
f.lintErrorf()
f.lintErrors()
@@ -1466,6 +1467,85 @@
})
}
+// 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 0000000..920fa9e
--- /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++
+ }
+}
