Address review style issues and bugs, add more extensive tests

README.md

@@ -550,7 +550,7 @@ Eventually(aFunc, time.Second*5, time.Second*polling)
 * Use the `--suppress-async-assertion=true` flag to suppress the function call in async assertion warning
 * Use the `--forbid-focus-container=true` flag to activate the focused container assertion (deactivated by default)
 * Use the `--suppress-type-compare-assertion=true` to suppress the type compare assertion warning
-* Use the `--suppress-succeed-assertion=true` to suppress the wrong succeed assertion warning
+* Use the `--suppress-succeed-assertion=true` to suppress the wrong succeed assertion style warning
 * Use the `--allow-havelen-0=true` flag to avoid warnings about `HaveLen(0)`; Note: this parameter is only supported from
   command line, and not from a comment.
 

analyzer.go

@@ -29,6 +29,7 @@ func NewAnalyzer() *analysis.Analyzer {
 		SuppressNil:     false,
 		SuppressErr:     false,
 		SuppressCompare: false,
+		SuppressSucceed: false,
 		ForbidFocus:     false,
 		AllowHaveLen0:   false,
 		ForceExpectTo:   false,

internal/interfaces/interfaces.go

@@ -2,7 +2,9 @@ package interfaces
 
 import (
 	"go/token"
+	"go/types"
 	gotypes "go/types"
+	"strings"
 )
 
 var (
@@ -74,3 +76,37 @@ func ImplementsError(t gotypes.Type) bool {
 func ImplementsGomegaMatcher(t gotypes.Type) bool {
 	return t != nil && gotypes.Implements(t, gomegaMatcherType)
 }
+
+// Note: we cannot check for an argument which _implements_ gomega.Gomega without adding a Go
+// module dependency on gomega.  This is because the gomega.Gomega interface definition references
+// gomega.AsyncAssertion, whose methods return gomega.AsyncAssertion. Because Go does not have
+// interface covariance or contravariance, any "local copy" of gomega.AsyncAssertion cannot
+// be satisified by any actual `gomega.AsyncAssertion` implementation, as the methods do not return
+// local.AsyncAssertion but rather gomega.AsyncAssertion.
+//
+// Also, Gomega probably doesn't even accept an argument whose type implements the interface, but
+// rather whose type _is_ the interface. So this check should suffice.
+func IsGomega(t gotypes.Type) bool {
+	named, isNamed := t.(*gotypes.Named)
+	if !isNamed {
+		return false
+	}
+
+	obj := named.Obj()
+
+	if obj.Name() != "Gomega" {
+		return false
+	}
+
+	return isPackageSymbol(named.Obj(), "github.com/onsi/gomega/types", "Gomega")
+}
+
+func isPackageSymbol(obj types.Object, pkgPath, name string) bool {
+	if obj.Name() != name {
+		return false
+	}
+
+	vendorPieces := strings.Split(pkgPath, "/vendor/")
+
+	return pkgPath == vendorPieces[len(vendorPieces)-1]
+}

linter/ginkgo_linter.go

@@ -50,7 +50,7 @@ const (
 	useBeforeEachTemplate          = "use BeforeEach() to assign variable %s"
 	succeedSyncOnlyWithErr         = "Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail"
 	succeedSyncOnlyWithErrFuncCall = "Succeed matcher should be asserted against a function call; consider replacing with %s"
-	succeedAsyncOnlyWithErrFunc    = "Succeed matcher must be asserted against a function that returns exactly one error"
+	succeedAsyncOnlyWithErrFunc    = "Succeed matcher must be asserted against a function that returns exactly one error or a function which takes in a single gomega.Gomega argument and returns nothing"
 )
 
 const ( // gomega matchers
@@ -533,9 +533,8 @@ func checkSucceedSync(
 	assertionExpCopy := astcopy.CallExpr(assertionExp)
 	matcherCopy := assertionExpCopy.Args[0].(*ast.CallExpr)
 
-	isAsync := false
 	return doCheckSucceed(
-		pass, assertionExpCopy, matcherCopy, actualArgs, handler, reportBuilder, isAsync,
+		pass, assertionExpCopy, matcherCopy, actualArgs, handler, reportBuilder, false,
 		suppressStyleIssue,
 	)
 }
@@ -565,8 +564,7 @@ func checkSucceedAsync(
 	assertionExpCopy := astcopy.CallExpr(assertionExp)
 	matcherCopy := assertionExpCopy.Args[0].(*ast.CallExpr)
 
-	isAsync := true
-	return doCheckSucceed(pass, assertionExpCopy, matcherCopy, actualArgs, handler, reportBuilder, isAsync, suppressWarnings)
+	return doCheckSucceed(pass, assertionExpCopy, matcherCopy, actualArgs, handler, reportBuilder, true, suppressWarnings)
 }
 
 // Function that looks for Succeed matcher by recursively unwrapping the `matcher` argument and,
@@ -597,12 +595,13 @@ func doCheckSucceed(
 	}
 
 	if isAsync {
-		if len(actualArgs) != 1 || !isExprErrFunc(pass, actualArgs[0]) {
+		if len(actualArgs) != 1 ||
+			!(isExprErrFunc(pass, actualArgs[0]) || isExprGomegaFunc(pass, actualArgs[0])) {
 			reportBuilder.AddIssue(false, succeedAsyncOnlyWithErrFunc)
 			return true
 		}
 	} else {
-		if len(actualArgs) != 1 || !isExprExactError(pass, actualArgs[0]) {
+		if len(actualArgs) != 1 || !isExprSingleError(pass, actualArgs[0]) {
 			reportBuilder.AddIssue(false, succeedSyncOnlyWithErr)
 			return true
 		}
@@ -1765,7 +1764,7 @@ func isExprError(pass *analysis.Pass, expr ast.Expr) bool {
 
 // Returns whether the expression implements type error or is a tuple of length 1 whose only
 // element implements type error.
-func isExprExactError(pass *analysis.Pass, expr ast.Expr) bool {
+func isExprSingleError(pass *analysis.Pass, expr ast.Expr) bool {
 	if !isExprError(pass, expr) {
 		return false
 	}
@@ -1782,8 +1781,8 @@ func isExprExactError(pass *analysis.Pass, expr ast.Expr) bool {
 // Returns whether the expression is a function that returns one error value
 func isExprErrFunc(pass *analysis.Pass, expr ast.Expr) bool {
 	actualArgType := pass.TypesInfo.TypeOf(expr)
-	t, isErrFunc := actualArgType.(*gotypes.Signature)
-	if !isErrFunc {
+	t, isFunc := actualArgType.(*gotypes.Signature)
+	if !isFunc {
 		return false
 	}
 
@@ -1794,13 +1793,29 @@ func isExprErrFunc(pass *analysis.Pass, expr ast.Expr) bool {
 	return false
 }
 
+// Returns whether the expression is a function that takes in a single gomega.Gomega argument and
+// returns nothing, e.g. `func(g gomega.Gomega) { g.Expect(myStringMaker()).ToNot(BeEmpty()) }`
+func isExprGomegaFunc(pass *analysis.Pass, expr ast.Expr) bool {
+	actualArgType := pass.TypesInfo.TypeOf(expr)
+	t, isFunc := actualArgType.(*gotypes.Signature)
+	if !isFunc {
+		return false
+	}
+
+	if t.Results().Len() != 0 || t.Params().Len() != 1 {
+		return false
+	}
+
+	return interfaces.IsGomega(t.Params().At(0).Type())
+}
+
 // Returns whether the expression is a function call that returns one error value
 func isExprErrFuncCall(pass *analysis.Pass, expr ast.Expr) bool {
 	_, isCallExpr := expr.(*ast.CallExpr)
 	if !isCallExpr {
 		return false
 	}
-	return isExprExactError(pass, expr)
+	return isExprSingleError(pass, expr)
 }
 
 func isPointer(pass *analysis.Pass, expr ast.Expr) bool {

testdata/src/a/forceExpectTo/gomegavar.go

@@ -7,10 +7,9 @@ import (
 
 var _ = Describe("gomega var", func() {
 	It("in a valid Eventually", func() {
-		Eventually(func(g Gomega) error {
+		Eventually(func(g Gomega) {
 			g.Expect("a").Should(HaveLen(1))    // want `ginkgo-linter: must not use Expect with Should\. Consider using .g\.Expect\("a"\)\.To\(HaveLen\(1\)\). instead`
 			g.Expect(len("a")).Should(Equal(1)) // want `ginkgo-linter: multiple issues: must not use Expect with Should; wrong length assertion\. Consider using .g\.Expect\("a"\)\.To\(HaveLen\(1\)\). instead`
-			return nil
 		}).Should(Succeed())
 	})
 

testdata/src/a/succeed/succeed.go

@@ -7,65 +7,76 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-func safeDivide(num int, denom int) (int, error) {
-	if denom == 0 {
-		return 0, errors.New("divide by zero")
+func returnsFloatAndErr(arg int) (float64, error) {
+	if arg == 0 {
+		return 0, errors.New("an error")
 	}
-	return num / denom, nil
+	return 1.0 / float64(arg), nil
 }
 
-func attemptToCallNetwork(shouldTry bool) error {
-	if !shouldTry {
+func returnsErr(arg bool) error {
+	if !arg {
 		return nil
 	}
-	return errors.New("don't know how to connect to network")
+	return errors.New("an error")
 }
 
 var _ = Describe("Ensure succeed assertion is used correctly", Label("succeed"), func() {
-	Expect(safeDivide(0, 0)).To(Not(Succeed())) // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
-	Expect(safeDivide(0, 0)).NotTo(Succeed())   // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
-	Expect(safeDivide(0, 1)).To(Succeed())      // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
+	Expect(returnsFloatAndErr(0)).To(Not(Succeed())) // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
+	Expect(returnsFloatAndErr(0)).NotTo(Succeed())   // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
+	Expect(returnsFloatAndErr(1)).To(Succeed())      // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
 
-	value0, err0 := safeDivide(0, 0)
-	value1, _ := safeDivide(0, 1)
+	value0, err0 := returnsFloatAndErr(0)
+	value1, _ := returnsFloatAndErr(1)
 
 	Expect(value0, err0).ToNot(Succeed()) // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
 	Expect(value1).To(Succeed())          // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
 
-	wrap := struct {
-		err error
+	resp := struct {
+		response any
+		err      error
 	}{
-		err: err0,
+		response: nil,
+		err:      err0,
 	}
 
 	Expect(err0).ToNot(Succeed())              // want `ginkgo-linter: Succeed matcher should be asserted against a function call; consider replacing with Expect\(err0\).To\(HaveOccurred\(\)\)`
 	ExpectWithOffset(1, err0).NotTo(Succeed()) // want `ginkgo-linter: Succeed matcher should be asserted against a function call; consider replacing with ExpectWithOffset\(1, err0\).To\(HaveOccurred\(\)\)`
 	Expect(err0).NotTo(Not(Succeed()))         // want `ginkgo-linter: Succeed matcher should be asserted against a function call; consider replacing with Expect\(err0\).To\(Not\(HaveOccurred\(\)\)\)`
-	Expect(wrap.err).To(Succeed())             // want `ginkgo-linter: Succeed matcher should be asserted against a function call; consider replacing with Expect\(wrap.err\).ToNot\(HaveOccurred\(\)\)`
+	Expect(resp.err).To(Succeed())             // want `ginkgo-linter: Succeed matcher should be asserted against a function call; consider replacing with Expect\(resp.err\).ToNot\(HaveOccurred\(\)\)`
 	// ginkgo-linter:ignore-succeed-warning
 	Expect(err0).ToNot(Succeed())
 
-	Expect(attemptToCallNetwork(false)).To(Succeed())
-	Expect(attemptToCallNetwork(true)).To(Not(Succeed()))
-	Expect(attemptToCallNetwork(false)).NotTo(Succeed())
+	Expect(returnsErr(false)).To(Succeed())
+	Expect(returnsErr(true)).To(Not(Succeed()))
+	Expect(returnsErr(false)).NotTo(Succeed())
 
 	Eventually(func() error {
-		return attemptToCallNetwork(false)
+		return returnsErr(false)
 	}).Should(Succeed())
 
 	ConsistentlyWithOffset(2, func() error {
-		return attemptToCallNetwork(false)
+		return returnsErr(false)
 	}).Should(Succeed())
 
-	Eventually(func() (int, error) { // want `ginkgo-linter: Succeed matcher must be asserted against a function that returns exactly one error`
-		return safeDivide(0, 1)
+	Eventually(func() (float64, error) { // want `ginkgo-linter: Succeed matcher must be asserted against a function that returns exactly one error or a function which takes in a single gomega.Gomega argument and returns nothing`
+		return returnsFloatAndErr(1)
 	}).Should(Succeed())
 
-	Consistently(func() (int, error) { // want `ginkgo-linter: Succeed matcher must be asserted against a function that returns exactly one error`
-		return safeDivide(0, 0)
+	Consistently(func() (float64, error) { // want `ginkgo-linter: Succeed matcher must be asserted against a function that returns exactly one error or a function which takes in a single gomega.Gomega argument and returns nothing`
+		return returnsFloatAndErr(0)
 	}).ShouldNot(Succeed())
 
-	Consistently(func() (int, error) { // want `ginkgo-linter: Succeed matcher must be asserted against a function that returns exactly one error`
-		return safeDivide(0, 0)
+	Consistently(func() (float64, error) { // want `ginkgo-linter: Succeed matcher must be asserted against a function that returns exactly one error or a function which takes in a single gomega.Gomega argument and returns nothing`
+		return returnsFloatAndErr(0)
 	}).ShouldNot(Not(Succeed()))
+
+	Eventually(func(g Gomega) {
+		err := returnsErr(false)
+		g.Expect(err).ToNot(HaveOccurred())
+	}).Should(Not(Succeed()))
+
+	Consistently(func(g Gomega) {
+		g.Expect(returnsFloatAndErr(1)).ToNot(Succeed()) // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
+	}).Should(Succeed())
 })

testdata/src/a/succeed/succeed_aliased_import.go

@@ -0,0 +1,66 @@
+package succeed
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	gom "github.com/onsi/gomega"
+)
+
+var _ = Describe("Ensure succeed assertion is used correctly", Label("succeed"), func() {
+	gom.Expect(returnsFloatAndErr(0)).To(gom.Not(gom.Succeed())) // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
+	gom.Expect(returnsFloatAndErr(0)).NotTo(gom.Succeed())       // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
+	gom.Expect(returnsFloatAndErr(1)).To(gom.Succeed())          // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
+
+	value0, err0 := returnsFloatAndErr(0)
+	value1, _ := returnsFloatAndErr(1)
+
+	gom.Expect(value0, err0).ToNot(gom.Succeed()) // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
+	gom.Expect(value1).To(gom.Succeed())          // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
+
+	resp := struct {
+		response any
+		err      error
+	}{
+		response: nil,
+		err:      err0,
+	}
+
+	gom.Expect(err0).ToNot(gom.Succeed())              // want `ginkgo-linter: Succeed matcher should be asserted against a function call; consider replacing with gom.Expect\(err0\).To\(gom.HaveOccurred\(\)\)`
+	gom.ExpectWithOffset(1, err0).NotTo(gom.Succeed()) // want `ginkgo-linter: Succeed matcher should be asserted against a function call; consider replacing with gom.ExpectWithOffset\(1, err0\).To\(gom.HaveOccurred\(\)\)`
+	gom.Expect(err0).NotTo(gom.Not(gom.Succeed()))     // want `ginkgo-linter: Succeed matcher should be asserted against a function call; consider replacing with gom.Expect\(err0\).To\(gom.Not\(gom.HaveOccurred\(\)\)\)`
+	gom.Expect(resp.err).To(gom.Succeed())             // want `ginkgo-linter: Succeed matcher should be asserted against a function call; consider replacing with gom.Expect\(resp.err\).ToNot\(gom.HaveOccurred\(\)\)`
+	// ginkgo-linter:ignore-succeed-warning
+	gom.Expect(err0).ToNot(gom.Succeed())
+
+	gom.Expect(returnsErr(false)).To(gom.Succeed())
+	gom.Expect(returnsErr(true)).To(gom.Not(gom.Succeed()))
+	gom.Expect(returnsErr(false)).NotTo(gom.Succeed())
+
+	gom.Eventually(func() error {
+		return returnsErr(false)
+	}).Should(gom.Succeed())
+
+	gom.ConsistentlyWithOffset(2, func() error {
+		return returnsErr(false)
+	}).Should(gom.Succeed())
+
+	gom.Eventually(func() (float64, error) { // want `ginkgo-linter: Succeed matcher must be asserted against a function that returns exactly one error or a function which takes in a single gomega.Gomega argument and returns nothing`
+		return returnsFloatAndErr(1)
+	}).Should(gom.Succeed())
+
+	gom.Consistently(func() (float64, error) { // want `ginkgo-linter: Succeed matcher must be asserted against a function that returns exactly one error or a function which takes in a single gomega.Gomega argument and returns nothing`
+		return returnsFloatAndErr(0)
+	}).ShouldNot(gom.Succeed())
+
+	gom.Consistently(func() (float64, error) { // want `ginkgo-linter: Succeed matcher must be asserted against a function that returns exactly one error or a function which takes in a single gomega.Gomega argument and returns nothing`
+		return returnsFloatAndErr(0)
+	}).ShouldNot(gom.Not(gom.Succeed()))
+
+	gom.Eventually(func(g gom.Gomega) {
+		err := returnsErr(false)
+		g.Expect(err).ToNot(gom.HaveOccurred())
+	}).Should(gom.Not(gom.Succeed()))
+
+	gom.Consistently(func(g gom.Gomega) {
+		g.Expect(returnsFloatAndErr(1)).ToNot(gom.Succeed()) // want `ginkgo-linter: Succeed matcher must be asserted against exactly one error value or a function call returning the same, or it will always fail`
+	}).Should(gom.Succeed())
+})