S1008: added support for multi-arguments return statement.

simple/s1008/s1008.go

@@ -5,6 +5,7 @@ import (
 	"go/ast"
 	"go/constant"
 	"go/token"
+	"strings"
 
 	"honnef.co/go/tools/analysis/code"
 	"honnef.co/go/tools/analysis/facts/generated"
@@ -38,8 +39,22 @@ return false`,
 var Analyzer = SCAnalyzer.Analyzer
 
 var (
-	checkIfReturnQIf  = pattern.MustParse(`(IfStmt nil cond [(ReturnStmt [ret@(Builtin (Or "true" "false"))])] nil)`)
-	checkIfReturnQRet = pattern.MustParse(`(ReturnStmt [ret@(Builtin (Or "true" "false"))])`)
+	checkIfReturnQIf = pattern.MustParse(`
+		(IfStmt 
+			nil 
+			cond 
+			[fullret@(ReturnStmt _)] 
+			nil)
+	`)
+	checkIfReturnQRet = pattern.MustParse(`
+		(Binding "fullret" (ReturnStmt _))
+	`)
+	checkReturnValue = pattern.MustParse(`
+		(ReturnStmt 
+			(List 
+				ret@(Builtin (Or "true" "false"))
+				tail@(Any)))
+	`)
 )
 
 func run(pass *analysis.Pass) (interface{}, error) {
@@ -57,47 +72,95 @@ func run(pass *analysis.Pass) (interface{}, error) {
 				return
 			}
 		}
-		m1, ok := code.Match(pass, checkIfReturnQIf, n1)
+		fullm1, ok := code.Match(pass, checkIfReturnQIf, n1)
 		if !ok {
 			return
 		}
-		m2, ok := code.Match(pass, checkIfReturnQRet, n2)
+		fullm2, ok := code.Match(pass, checkIfReturnQRet, n2)
 		if !ok {
 			return
 		}
 
-		if op, ok := m1.State["cond"].(*ast.BinaryExpr); ok {
+		if op, ok := fullm1.State["cond"].(*ast.BinaryExpr); ok {
 			switch op.Op {
 			case token.EQL, token.LSS, token.GTR, token.NEQ, token.LEQ, token.GEQ:
 			default:
 				return
 			}
 		}
 
-		ret1 := m1.State["ret"].(*ast.Ident)
-		ret2 := m2.State["ret"].(*ast.Ident)
+		fullret1 := fullm1.State["fullret"].(*ast.ReturnStmt)
+		m1, ok := code.Match(pass, checkReturnValue, fullret1)
+		if !ok {
+			return
+		}
+
+		fullret2 := fullm2.State["fullret"].(*ast.ReturnStmt)
+		m2, ok := code.Match(pass, checkReturnValue, fullret2)
+		if !ok {
+			return
+		}
+
+		ret1, tail1 := getRetAndTail(m1)
+		tail1String := renderTailString(pass, tail1)
+
+		ret2, tail2 := getRetAndTail(m2)
+		tail2String := renderTailString(pass, tail2)
+
+		if tail1String != tail2String {
+			// we want to process only return with the same values
+			return
+		}
 
 		if ret1.Name == ret2.Name {
 			// we want the function to return true and false, not the
 			// same value both times.
 			return
 		}
 
-		cond := m1.State["cond"].(ast.Expr)
+		cond := fullm1.State["cond"].(ast.Expr)
 		origCond := cond
 		if ret1.Name == "false" {
 			cond = negate(pass, cond)
 		}
 		report.Report(pass, n1,
-			fmt.Sprintf("should use 'return %s' instead of 'if %s { return %s }; return %s'",
+			fmt.Sprintf(
+				"should use 'return %s%s' instead of 'if %s { return %s%s }; return %s%s'",
 				report.Render(pass, cond),
-				report.Render(pass, origCond), report.Render(pass, ret1), report.Render(pass, ret2)),
+				tail1String,
+				report.Render(
+					pass,
+					origCond,
+				),
+				report.Render(pass, ret1),
+				tail1String,
+				report.Render(pass, ret2),
+				tail2String,
+			),
 			report.FilterGenerated())
 	}
 	code.Preorder(pass, fn, (*ast.BlockStmt)(nil))
 	return nil, nil
 }
 
+func getRetAndTail(m *pattern.Matcher) (*ast.Ident, []ast.Expr) {
+	ret1 := m.State["ret"].(*ast.Ident)
+	var tail []ast.Expr
+	if t, ok := m.State["tail"]; ok {
+		tail, _ = t.([]ast.Expr)
+	}
+	return ret1, tail
+}
+
+func renderTailString(pass *analysis.Pass, tail []ast.Expr) string {
+	var tailStringBuilder strings.Builder
+	if len(tail) != 0 {
+		tailStringBuilder.WriteString(", ")
+		tailStringBuilder.WriteString(report.RenderArgs(pass, tail))
+	}
+	return tailStringBuilder.String()
+}
+
 func negate(pass *analysis.Pass, expr ast.Expr) ast.Expr {
 	switch expr := expr.(type) {
 	case *ast.BinaryExpr:

simple/s1008/testdata/go1.0/CheckIfReturn/if-multiple-return.go

@@ -0,0 +1,35 @@
+package pkg
+
+func multireturn_fn1(x string) (bool, error) {
+	if len(x) > 0 { //@ diag(`should use 'return len(x) > 0, nil' instead of 'if len(x) > 0 { return true, nil }; return false, nil'`)
+		return true, nil
+	}
+	return false, nil
+}
+
+func multireturn_fn2(x string) (bool, error) {
+	if len(x) > 0 {
+	}
+	return false, nil
+}
+
+func multireturn_fn3(x string) (bool, error, error) {
+	if len(x) > 0 { //@ diag(`should use 'return len(x) > 0, nil, nil' instead of 'if len(x) > 0 { return true, nil, nil }; return false, nil, nil'`)
+		return true, nil, nil
+	}
+	return false, nil, nil
+}
+
+func multireturn_fn4(x string) (bool, int, error) {
+	if len(x) > 0 { //@ diag(`should use 'return len(x) == 0, 0, nil' instead of 'if len(x) > 0 { return false, 0, nil }; return true, 0, nil'`)
+		return false, 0, nil
+	}
+	return true, 0, nil
+}
+
+func multireturn_fn5(x string) (bool, int, error) {
+	if len(x) > 0 { 
+		return false, 20, nil
+	}
+	return true, 30, nil
+}