Check for redundant if err != nil constructs.

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 {

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++
+	}
+}