Resolves #151: mark incorrect usages of Succeed()

README.md

@@ -227,6 +227,55 @@ ginkgolinter checks the following:
 * If the first parameter is a function with the format of `func(error)bool`, ginkgolinter makes sure that the second 
   parameter exists and its type is string.
 
+### Wrong Usage of the `Succeed` gomega Matcher [BUG]
+The `Succeed` gomega matcher asserts that a *single* error value is nil. If this matcher is used against more than one
+value, it will always fail.
+
+The linter will not suggest a fix for this rule. This rule cannot be suppressed.
+
+These are valid forms of this matcher for sync assertions:
+  ```go
+  // This is valid, but not readable; should be replaced with `Expect(os.Remove("someFile")).To(Succeed())`
+  // or `Expect(err).NotTo(HaveOccurred())`
+  err := os.Remove("someFile")
+  Expect(err).To(Succeed())
+
+  ExpectWithOffset(1, os.Remove("someOtherFile")).ToNot(Succeed())
+  ```
+
+The following usages are not valid:
+  ```go
+  contents, err := os.ReadFile("someFile")
+  ExpectWithOffset(1, contents, err).To(Succeed())
+
+  Expect(os.ReadFile("someOtherFile")).ToNot(Succeed())
+  ```
+
+For async assertions, the matcher works similarly, but there is a special case for functions that take in a single
+`gomega.Gomega` argument and return no values; in this special case, `Succeed` can still be used.
+
+The following usages are valid forms of this matcher for async assertions:
+  ```go
+  EventuallyWithOffset(1, func() error {
+    _, err := os.ReadFile("someFile")
+    return err
+  }).Should(Succeed())
+
+  Eventually(os.Remove).WithArguments("someFile").ShouldNot(Succeed())
+
+  // special case where the function is allowed to return no error value
+  Consistently(func(g.Gomega) {
+    contents, err := os.ReadFile("someFile")
+    g.Expect(contents, err).ToNot(BeEmpty())
+  }).ShouldNot(Succeed())
+  ```
+
+The following usages are not valid:
+  ```go
+  EventuallyWithOffset(1, os.ReadFile).WithArguments("someFile").Should(Succeed())
+  ```
+
+
 ### Async timing interval: timeout is shorter than polling interval [BUG]
 ***Note***: Only applied when the `suppress-async-assertion` flag is **not set** *and* the `validate-async-intervals` 
 flag **is** set.
@@ -373,7 +422,7 @@ Expect(err == nil).To(BeFalse()) // should be: Expect(err).To(HaveOccurred())
 Expect(err != nil).To(BeTrue()) // should be: Expect(err).To(HaveOccurred())
 Expect(funcReturnsError()).To(BeNil()) // should be: Expect(funcReturnsError()).To(Succeed())
 
-and so on
+// and so on
 ```
 It also supports the embedded `Not()` matcher; e.g.
 
@@ -418,6 +467,22 @@ Expect(x1 == c1).Should(BeTrue()) // ==> Expect(x1).Should(Equal(c1))
 Expect(c1 == x1).Should(BeTrue()) // ==> Expect(x1).Should(Equal(c1))
 ```
 
+### `Succeed` Matcher Not Used on Function Call [STYLE]
+The linter finds assertions of the `Succeed` matcher used on an actual argument that has an error
+type but is not a function call, e.g. `Expect(err).To(Succeed())`, and suggests replacing them
+with `Expect(err).ToNot(HaveOccurred())`. The latter is much more readable.
+
+```go
+resp := getResponse()
+Expect(resp.Err).To(Succeed()) // ==> Expect(resp.Err).ToNot(HaveOccurred())
+Expect(getResponse().Err).To(Succeed()) // ==> Expect(getResponse().Err).ToNot(HaveOccurred())
+
+err := someOperation()
+ExpectWithOffset(1, err).ToNot(Succeed()) // ==> ExpectWithOffset(1, err).To(HaveOccurred())
+
+ExpectWithOffset(1, someOperation()).ToNot(Succeed())  // this is fine
+```
+
 ### Don't Allow Using `Expect` with `Should` or `ShouldNot` [STYLE]
 This optional rule forces the usage of the `Expect` method only with the `To`, `ToNot` or `NotTo` 
 assertion methods; e.g.
@@ -485,6 +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 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,
@@ -43,6 +44,7 @@ func NewAnalyzer() *analysis.Analyzer {
 	a.Flags.Var(&config.SuppressErr, "suppress-err-assertion", "Suppress warning for wrong error assertions")
 	a.Flags.Var(&config.SuppressCompare, "suppress-compare-assertion", "Suppress warning for wrong comparison assertions")
 	a.Flags.Var(&config.SuppressAsync, "suppress-async-assertion", "Suppress warning for function call in async assertion, like Eventually")
+	a.Flags.Var(&config.SuppressSucceed, "suppress-succeed-assertion", "Suppress warning for Succeed used against expressions that are not function calls")
 	a.Flags.Var(&config.ValidateAsyncIntervals, "validate-async-intervals", "best effort validation of async intervals (timeout and polling); ignored the suppress-async-assertion flag is true")
 	a.Flags.Var(&config.SuppressTypeCompare, "suppress-type-compare-assertion", "Suppress warning for comparing values from different types, like int32 and uint32")
 	a.Flags.Var(&config.AllowHaveLen0, "allow-havelen-0", "Do not warn for HaveLen(0); default = false")

analyzer_test.go

@@ -85,6 +85,10 @@ func TestAllUseCases(t *testing.T) {
 			testName: "MatchError",
 			testData: "a/matcherror",
 		},
+		{
+			testName: "Succeed",
+			testData: "a/succeed",
+		},
 		{
 			testName: "issue 124: custom matcher form other packages",
 			testData: "a/issue-124",
@@ -141,6 +145,11 @@ func TestFlags(t *testing.T) {
 			testData: []string{"a/focusconfig"},
 			flags:    map[string]string{"forbid-focus-container": "true"},
 		},
+		{
+			testName: "test the suppress-succeed-assertion flag",
+			testData: []string{"a/succeedconfig"},
+			flags:    map[string]string{"suppress-succeed-assertion": "true"},
+		},
 		{
 			testName: "test the suppress-type-compare-assertion flag",
 			testData: []string{"a/comparetypesconfig"},

doc.go

@@ -22,7 +22,10 @@ currently, the linter searches for following:
 * validate the MatchError gomega matcher [Bug]
 
 * trigger a warning when using the Equal or the BeIdentical matcher with two different types, as these matchers will
-  fail in runtime.
+  fail at runtime [Bug]
+
+* trigger a warning when using the Succeed matcher on anything but a single error argument, as this matcher will fail
+  at runtime [Bug]
 
 * async timing interval: timeout is shorter than polling interval [Bug]
 For example:
@@ -78,6 +81,8 @@ This should be replaced with:
 
 * replaces Expect(...).Should(...) with Expect(...).To() [Style]
 
+* replaces Expect(err).Should(Succeed()) with Expect(err).ToNot(HaveOccurred()) [Style]
+
 * async timing interval: multiple timeout or polling interval [Style]
 For example:
 	Eventually(context.Background(), func() bool { return true }, time.Second*10).WithTimeout(time.Second * 10).WithPolling(time.Millisecond * 500).Should(BeTrue())

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