From b14486e5771a0b340b67a3b73a9a2a6ae6bbc4c4 Mon Sep 17 00:00:00 2001 From: Nahshon Unna-Tsameret Date: Mon, 1 Apr 2024 22:06:06 +0300 Subject: [PATCH] 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. ``` --- internal/interfaces/interfaces.go | 2 +- linter/ginkgo_linter.go | 44 ++++++++++++++----- testdata/src/a/errnil/async.go | 42 ++++++++++++++++++ testdata/src/a/errnil/{a.go => at.go} | 0 testdata/src/a/matcherror/matcherror.go | 3 +- .../src/a/matcherror/matcherror.gomega.go | 3 +- tests/e2e.sh | 32 +++++++------- 7 files changed, 96 insertions(+), 30 deletions(-) create mode 100644 testdata/src/a/errnil/async.go rename testdata/src/a/errnil/{a.go => at.go} (100%) diff --git a/internal/interfaces/interfaces.go b/internal/interfaces/interfaces.go index dafeacd..91849ca 100644 --- 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 index 574fdfa..161d58e 100644 --- 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 new file mode 100644 index 0000000..a159671 --- /dev/null +++ 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 b/testdata/src/a/errnil/at.go similarity index 100% rename from testdata/src/a/errnil/a.go rename to testdata/src/a/errnil/at.go diff --git a/testdata/src/a/matcherror/matcherror.go b/testdata/src/a/matcherror/matcherror.go index 2c68ec2..7e48737 100644 --- 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 index 43f5041..ad1f11f 100644 --- 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 index 0ce7e8b..f087334 100755 --- 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 ]]