Bug fix: missing async wrong error checks

the linter didn't check error with nil assertion also in async assertions, such as: ```go Eventually(func() err {return nil}).Should(BeNil()) ``` Also, the linter didn't check for MatchError issues in async assertions, e.g. ```go Eventually(func() string {return "hello"}).Should(MatchError("hello")) This PR adds these chacks. ```
nunnatsa · Apr 1, 2024 · b14486e · b14486e
1 parent 5ce34c8
commit b14486e
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 30 deletions.
diff --git a/internal/interfaces/interfaces.go b/internal/interfaces/interfaces.go
@@ -72,5 +72,5 @@ func ImplementsError(t gotypes.Type) bool {
 }
 
 func ImplementsGomegaMatcher(t gotypes.Type) bool {
-	return gotypes.Implements(t, gomegaMatcherType)
+	return t != nil && gotypes.Implements(t, gomegaMatcherType)
 }
diff --git a/linter/ginkgo_linter.go b/linter/ginkgo_linter.go
@@ -307,7 +307,7 @@ func checkExpression(pass *analysis.Pass, config types.Config, assertionExp *ast
 	reportBuilder := reports.NewBuilder(pass.Fset, expr)
 
 	goNested := false
-	if checkAsyncAssertion(pass, config, expr, actualExpr, handler, reportBuilder, timePkg) {
+	if checkAsyncAssertion(pass, assertionExp, config, expr, actualExpr, handler, reportBuilder, timePkg) {
 		goNested = true
 	} else {
 
@@ -394,7 +394,7 @@ func doCheckExpression(pass *analysis.Pass, config types.Config, assertionExp *a
 		}
 		return bool(config.SuppressCompare) || checkComparison(expr, pass, matcher, handler, first, second, op, reportBuilder)
 
-	} else if checkMatchError(pass, assertionExp, actualArg, handler, reportBuilder) {
+	} else if checkMatchError(pass, assertionExp, actualArg, handler, reportBuilder, isExprError) {
 		return false
 	} else if isExprError(pass, actualArg) {
 		return bool(config.SuppressErr) || checkNilError(pass, expr, handler, actualArg, reportBuilder)
@@ -410,16 +410,16 @@ func doCheckExpression(pass *analysis.Pass, config types.Config, assertionExp *a
 	return true
 }
 
-func checkMatchError(pass *analysis.Pass, origExp *ast.CallExpr, actualArg ast.Expr, handler gomegahandler.Handler, reportBuilder *reports.Builder) bool {
+func checkMatchError(pass *analysis.Pass, origExp *ast.CallExpr, actualArg ast.Expr, handler gomegahandler.Handler, reportBuilder *reports.Builder, isErrFunc func(*analysis.Pass, ast.Expr) bool) bool {
 	matcher, ok := origExp.Args[0].(*ast.CallExpr)
 	if !ok {
 		return false
 	}
 
-	return doCheckMatchError(pass, origExp, matcher, actualArg, handler, reportBuilder)
+	return doCheckMatchError(pass, origExp, matcher, actualArg, handler, reportBuilder, isErrFunc)
 }
 
-func doCheckMatchError(pass *analysis.Pass, origExp *ast.CallExpr, matcher *ast.CallExpr, actualArg ast.Expr, handler gomegahandler.Handler, reportBuilder *reports.Builder) bool {
+func doCheckMatchError(pass *analysis.Pass, origExp *ast.CallExpr, matcher *ast.CallExpr, actualArg ast.Expr, handler gomegahandler.Handler, reportBuilder *reports.Builder, isErrFunc func(*analysis.Pass, ast.Expr) bool) bool {
 	name, ok := handler.GetActualFuncName(matcher)
 	if !ok {
 		return false
@@ -432,12 +432,12 @@ func doCheckMatchError(pass *analysis.Pass, origExp *ast.CallExpr, matcher *ast.
 			return false
 		}
 
-		return doCheckMatchError(pass, origExp, nested, actualArg, handler, reportBuilder)
+		return doCheckMatchError(pass, origExp, nested, actualArg, handler, reportBuilder, isErrFunc)
 	case and, or:
 		res := false
 		for _, arg := range matcher.Args {
 			if nested, ok := arg.(*ast.CallExpr); ok {
-				if valid := doCheckMatchError(pass, origExp, nested, actualArg, handler, reportBuilder); valid {
+				if valid := doCheckMatchError(pass, origExp, nested, actualArg, handler, reportBuilder, isErrFunc); valid {
 					res = true
 				}
 			}
@@ -447,7 +447,7 @@ func doCheckMatchError(pass *analysis.Pass, origExp *ast.CallExpr, matcher *ast.
 		return false
 	}
 
-	if !isExprError(pass, actualArg) {
+	if !isErrFunc(pass, actualArg) {
 		reportBuilder.AddIssue(false, matchErrorArgWrongType, goFmt(pass.Fset, actualArg))
 	}
 
@@ -469,7 +469,6 @@ func doCheckMatchError(pass *analysis.Pass, origExp *ast.CallExpr, matcher *ast.
 		}
 		return true
 	}
-
 	if requiredParams == 2 && numParams == 1 {
 		reportBuilder.AddIssue(false, matchErrorMissingDescription)
 		return true
@@ -482,6 +481,7 @@ func doCheckMatchError(pass *analysis.Pass, origExp *ast.CallExpr, matcher *ast.
 	expr.Args = newArgsSuggestion
 
 	reportBuilder.AddIssue(true, matchErrorRedundantArg)
+
 	return true
 }
 
@@ -495,7 +495,7 @@ func checkMatchErrorAssertion(pass *analysis.Pass, matcher *ast.CallExpr) (bool,
 		return true, 2
 	}
 
-	return false, 0
+	return false, 1
 }
 
 // isFuncErrBool checks if a function is with the signature `func(error) bool`
@@ -734,7 +734,7 @@ func checkPointerComparison(pass *analysis.Pass, config types.Config, origExp *a
 
 // check async assertion does not assert function call. This is a real bug in the test. In this case, the assertion is
 // done on the returned value, instead of polling the result of a function, for instance.
-func checkAsyncAssertion(pass *analysis.Pass, config types.Config, expr *ast.CallExpr, actualExpr *ast.CallExpr, handler gomegahandler.Handler, reportBuilder *reports.Builder, timePkg string) bool {
+func checkAsyncAssertion(pass *analysis.Pass, origExp *ast.CallExpr, config types.Config, expr *ast.CallExpr, actualExpr *ast.CallExpr, handler gomegahandler.Handler, reportBuilder *reports.Builder, timePkg string) bool {
 	funcName, ok := handler.GetActualFuncName(actualExpr)
 	if !ok {
 		return false
@@ -795,6 +795,14 @@ func checkAsyncAssertion(pass *analysis.Pass, config types.Config, expr *ast.Cal
 		}
 	}
 
+	if !config.SuppressErr {
+		if isExprErrFunc(pass, actualExpr.Args[funcIndex]) {
+			checkNilError(pass, expr, handler, actualExpr, reportBuilder)
+		}
+	}
+
+	checkMatchError(pass, origExp, actualExpr.Args[funcIndex], handler, reportBuilder, isExprErrFunc)
+
 	handleAssertionOnly(pass, config, expr, handler, actualExpr, reportBuilder)
 	return true
 }
@@ -1629,6 +1637,20 @@ func isExprError(pass *analysis.Pass, expr ast.Expr) bool {
 	return false
 }
 
+func isExprErrFunc(pass *analysis.Pass, expr ast.Expr) bool {
+	actualArgType := pass.TypesInfo.TypeOf(expr)
+	t, isErrFunc := actualArgType.(*gotypes.Signature)
+	if !isErrFunc {
+		return false
+	}
+
+	if t.Results().Len() == 1 {
+		return interfaces.ImplementsError(t.Results().At(0).Type())
+	}
+
+	return false
+}
+
 func isPointer(pass *analysis.Pass, expr ast.Expr) bool {
 	t := pass.TypesInfo.TypeOf(expr)
 	return is[*gotypes.Pointer](t)

diff --git a/testdata/src/a/errnil/async.go b/testdata/src/a/errnil/async.go
@@ -0,0 +1,42 @@
+package errnil
+
+import (
+	"context"
+	"errors"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+type myError string
+
+func (e myError) Error() string {
+	return string(e)
+}
+
+func f1() error {
+	return errors.New("example")
+}
+
+var _ = Describe("async and len()", func() {
+	It("should report wrong length assertion in async functions", func() {
+		ctx := context.Background()
+		Eventually(func() error { return nil }).Should(BeNil())                                            // want `wrong error assertion. Consider using .Eventually\(func\(\) error { return nil }\)\.Should\(Succeed\(\)\). instead`
+		Eventually(errFunc).Should(BeNil())                                                                // want `wrong error assertion. Consider using .Eventually\(errFunc\)\.Should\(Succeed\(\)\). instead`
+		Eventually(func() error { return nil }).Should(Equal(nil))                                         // want `wrong error assertion. Consider using .Eventually\(func\(\) error { return nil }\)\.Should\(Succeed\(\)\). instead`
+		Eventually(errFunc).Should(Equal(nil))                                                             // want `wrong error assertion. Consider using .Eventually\(errFunc\)\.Should\(Succeed\(\)\). instead`
+		Eventually(func() error { return myError("fake error") }).ShouldNot(BeNil())                       // want `wrong error assertion. Consider using .Eventually\(func\(\) error { return myError\("fake error"\) }\)\.ShouldNot\(Succeed\(\)\). instead`
+		Eventually(ctx, func() error { return myError("fake error") }).ShouldNot(Equal(nil))               // want `wrong error assertion. Consider using .Eventually\(ctx, func\(\) error { return myError\("fake error"\) }\)\.ShouldNot\(Succeed\(\)\). instead`
+		Eventually(ctx, func() error { return myError("fake error") }).ShouldNot(MatchError("fake error")) // valid
+		Eventually(func() error { return myError("fake error") }).ShouldNot(MatchError(f1))                // want `MatchError first parameter \(f1\) must be error, string, GomegaMatcher or func\(error\)bool are allowed`
+		Eventually(func() string { return "fake error" }).ShouldNot(MatchError(Equal("fake error")))       // want `the MatchError matcher used to assert a non error type`
+	})
+	It("should report wrong length assertion in async functions", func() {
+		Consistently(func() error { return nil }).Should(BeNil())   // want `wrong error assertion. Consider using .Consistently\(func\(\) error { return nil }\)\.Should\(Succeed\(\)\). instead`
+		EventuallyWithOffset(1, errFunc).Should(BeNil())            // want `wrong error assertion. Consider using .EventuallyWithOffset\(1, errFunc\)\.Should\(Succeed\(\)\). instead`
+		ConsistentlyWithOffset(1, errFunc).Should(BeNil())          // want `wrong error assertion. Consider using .ConsistentlyWithOffset\(1, errFunc\)\.Should\(Succeed\(\)\). instead`
+		Consistently(context.Background(), errFunc).Should(BeNil()) // want `wrong error assertion. Consider using .Consistently\(context.Background\(\), errFunc\)\.Should\(Succeed\(\)\). instead`
+		ctx := context.Background()
+		ConsistentlyWithOffset(1, ctx, errFunc).Should(BeNil()) // want `wrong error assertion. Consider using .ConsistentlyWithOffset\(1, ctx, errFunc\)\.Should\(Succeed\(\)\). instead`
+	})
+})
diff --git a/testdata/src/a/errnil/a.go → testdata/src/a/errnil/at.go b/testdata/src/a/errnil/a.go → testdata/src/a/errnil/at.go
diff --git a/testdata/src/a/matcherror/matcherror.go b/testdata/src/a/matcherror/matcherror.go
@@ -3,6 +3,7 @@ package matcherror
 import (
 	"errors"
 	"fmt"
+
 	"github.com/onsi/gomega/types"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -79,7 +80,7 @@ var _ = Describe("Check MatchError", func() {
 		one := 1
 		Expect(one).To(MatchError("example")) // want `ginkgo-linter: the MatchError matcher used to assert a non error type \(one\)`
 		Expect(e1).To(MatchError(isExample))  // want `ginkgo-linter: missing function description as second parameter of MatchError`
-		Expect(e1).To(MatchError(f1))         // want `ginkgo-linter: multiple issues: MatchError first parameter \(f1\) must be error, string, GomegaMatcher or func\(error\)bool are allowed; redundant MatchError arguments; consider removing them`
+		Expect(e1).To(MatchError(f1))         // want `ginkgo-linter: MatchError first parameter \(f1\) must be error, string, GomegaMatcher or func\(error\)bool are allowed`
 	})
 
 	It("two arguments - valid", func() {

diff --git a/testdata/src/a/matcherror/matcherror.gomega.go b/testdata/src/a/matcherror/matcherror.gomega.go
@@ -3,6 +3,7 @@ package matcherror
 import (
 	"errors"
 	"fmt"
+
 	"github.com/onsi/gomega/types"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -33,7 +34,7 @@ var _ = Describe("Check MatchError", func() {
 		one := 1
 		gomega.Expect(one).To(gomega.MatchError("example")) // want `ginkgo-linter: the MatchError matcher used to assert a non error type \(one\)`
 		gomega.Expect(e1).To(gomega.MatchError(isExample))  // want `ginkgo-linter: missing function description as second parameter of MatchError`
-		gomega.Expect(e1).To(gomega.MatchError(f1))         // want `ginkgo-linter: multiple issues: MatchError first parameter \(f1\) must be error, string, GomegaMatcher or func\(error\)bool are allowed; redundant MatchError arguments; consider removing them`
+		gomega.Expect(e1).To(gomega.MatchError(f1))         // want `ginkgo-linter: MatchError first parameter \(f1\) must be error, string, GomegaMatcher or func\(error\)bool are allowed`
 	})
 
 	It("two arguments - valid", func() {

diff --git a/tests/e2e.sh b/tests/e2e.sh
@@ -6,34 +6,34 @@ cp ginkgolinter testdata/src/a
 cd testdata/src/a
 
 # no suppress
-[[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2591 ]]
+[[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2604 ]]
 # suppress all but nil
-[[ $(./ginkgolinter --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 1516 ]]
+[[ $(./ginkgolinter --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 1522 ]]
 # suppress all but len
-[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 876 ]]
+[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 879 ]]
 # suppress all but err
-[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 276 ]]
+[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 287 ]]
 # suppress all but compare
-[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 319 ]]
+[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-async-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 322 ]]
 # suppress all but async
-[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 183 ]]
+[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 185 ]]
 # suppress all but focus
-[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 216 ]]
+[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 219 ]]
 # suppress all but compare different types
-[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-compare-assertion=true a/... 2>&1 | wc -l) == 267 ]]
+[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-compare-assertion=true a/... 2>&1 | wc -l) == 269 ]]
 # allow HaveLen(0)
-[[ $(./ginkgolinter --allow-havelen-0=true a/... 2>&1 | wc -l) == 2578 ]]
+[[ $(./ginkgolinter --allow-havelen-0=true a/... 2>&1 | wc -l) == 2591 ]]
 # force Expect with To
-[[ $(./ginkgolinter --force-expect-to=true a/... 2>&1 | wc -l) == 2773 ]]
+[[ $(./ginkgolinter --force-expect-to=true a/... 2>&1 | wc -l) == 2786 ]]
 # suppress all - should only return the few non-suppressble
-[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=false --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 152 ]]
+[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=false --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 155 ]]
 # suppress all, force  Expect with To
-[[ $(./ginkgolinter --force-expect-to=true --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=false --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 775 ]]
+[[ $(./ginkgolinter --force-expect-to=true --suppress-nil-assertion=true --suppress-len-assertion=true --suppress-err-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-focus-container=false --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 778 ]]
 # enable async interval validation
-[[ $(./ginkgolinter --validate-async-intervals=true a/... 2>&1 | wc -l) == 2686 ]]
+[[ $(./ginkgolinter --validate-async-intervals=true a/... 2>&1 | wc -l) == 2699 ]]
 # suppress spec pollution
-[[ $(./ginkgolinter --forbid-spec-pollution=true a/... 2>&1 | wc -l) == 2689 ]]
+[[ $(./ginkgolinter --forbid-spec-pollution=true a/... 2>&1 | wc -l) == 2702 ]]
 # suppress all but spec pollution
-[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-spec-pollution=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 250 ]]
+[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-spec-pollution=true --suppress-type-compare-assertion=true a/... 2>&1 | wc -l) == 253 ]]
 # suppress all but spec pollution && focus containers
-[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-spec-pollution=true --suppress-type-compare-assertion=true --forbid-focus-container=true a/... 2>&1 | wc -l) == 314 ]]
+[[ $(./ginkgolinter --suppress-nil-assertion=true --suppress-err-assertion=true --suppress-len-assertion=true --suppress-compare-assertion=true --suppress-async-assertion=true --forbid-spec-pollution=true --suppress-type-compare-assertion=true --forbid-focus-container=true a/... 2>&1 | wc -l) == 317 ]]