Add two new rules, to validate the Success and HaveOccurred matchers

* Makes sure the `Succeed()` matcher only accepts a single error value,
  or a function that recieves a Gomega object as its first parameter, in
  async assertions.

* force usage of `Succeed` for functions, and `HaveOccurred` for values.

Added a new command line flag: `--force-succeed=true`, to enable the
second rule (the rule is disabled by default).

Author: nunnatsa

README.md

@@ -249,6 +249,28 @@ This will probably happen when using the old format:
    Eventually(aFunc, 500 * time.Millisecond /*timeout*/, 10 * time.Second /*polling*/).Should(Succeed())
    ```
 
+### Correct usage of the `Succeed()` matcher [Bug]
+The `Succeed()` matcher only accepts a single error value. this rule validates that. 
+
+For example:
+   ```go
+   Expect(42).To(Succeed())
+   ```
+
+But mostly, we want to avoid using this matcher with functions that return multiple values, even if their last 
+returned value is an error, because this is not supported:
+   ```go
+   Expect(os.Open("myFile.txt")).To(Succeed())
+   ```
+
+In async assertions (like `Eventually()`), the `Succeed()` matcher may also been used with functions that accept 
+a Gomega object as their first parameter, and returns nothing, e.g. this is a valid usage of `Eventually`
+  ```go
+  Eventually(func(g Gomega){
+	  g.Expect(true).To(BeTrue())
+  }).WithTimeout(10 * time.Millisecond).WithPolling(time.Millisecond).Should(Succeed())
+  ```
+
 ### Avoid Spec Pollution: Don't Initialize Variables in Container Nodes [BUG/STYLE]:
 ***Note***: Only applied when the `--forbid-spec-pollution=true` flag is set (disabled by default).
 
@@ -476,6 +498,30 @@ will be changed to:
 ```go
 Eventually(aFunc, time.Second*5, time.Second*polling)
 ```
+
+### Correct usage of the `Succeed()` and the `HaveOccurred()` matchers
+This rule enforces using the `Success()` matcher only for functions, and the `HaveOccurred()` matcher only for error
+values.
+
+For example:
+  ```go
+  Expect(err).To(Succeed())
+  ```
+will trigger a warning with a suggestion to replace the mather to
+  ```go
+  Expect(err).ToNot(HaveOccurred())
+  ```
+
+and vice versa:
+  ```go
+  Expect(myErrorFunc()).ToNot(HaveOccurred())
+  ```
+will trigger a warning with a suggestion to replace the mather to
+  ```go
+  Expect(myErrorFunc()).To(Succeed())
+  ```
+***This rule is disabled by default***. Use the `--force-succeed=true` command line flag to enable it.
+
 ## Suppress the linter
 ### Suppress warning from command line
 * Use the `--suppress-len-assertion=true` flag to suppress the wrong length and cap assertions warning

analyzer.go

@@ -25,13 +25,14 @@ func NewAnalyzerWithConfig(config *types.Config) *analysis.Analyzer {
 // NewAnalyzer returns an Analyzer - the package interface with nogo
 func NewAnalyzer() *analysis.Analyzer {
 	config := &types.Config{
-		SuppressLen:     false,
-		SuppressNil:     false,
-		SuppressErr:     false,
-		SuppressCompare: false,
-		ForbidFocus:     false,
-		AllowHaveLen0:   false,
-		ForceExpectTo:   false,
+		SuppressLen:          false,
+		SuppressNil:          false,
+		SuppressErr:          false,
+		SuppressCompare:      false,
+		ForbidFocus:          false,
+		AllowHaveLen0:        false,
+		ForceExpectTo:        false,
+		ForceSucceedForFuncs: false,
 	}
 
 	a := NewAnalyzerWithConfig(config)
@@ -50,6 +51,7 @@ func NewAnalyzer() *analysis.Analyzer {
 	a.Flags.BoolVar(&ignored, "suppress-focus-container", true, "Suppress warning for ginkgo focus containers like FDescribe, FContext, FWhen or FIt. Deprecated and ignored: use --forbid-focus-container instead")
 	a.Flags.Var(&config.ForbidFocus, "forbid-focus-container", "trigger a warning for ginkgo focus containers like FDescribe, FContext, FWhen or FIt; default = false.")
 	a.Flags.Var(&config.ForbidSpecPollution, "forbid-spec-pollution", "trigger a warning for variable assignments in ginkgo containers like Describe, Context and When, instead of in BeforeEach(); default = false.")
+	a.Flags.Var(&config.ForceSucceedForFuncs, "force-succeed", "force using the Succeed matcher for error functions, and the HaveOccurred matcher for non-function error values")
 
 	return a
 }

analyzer_test.go

@@ -93,6 +93,10 @@ func TestAllUseCases(t *testing.T) {
 			testName: "cap",
 			testData: "a/cap",
 		},
+		{
+			testName: "HaveOccurred matcher tests",
+			testData: "a/haveoccurred",
+		},
 	} {
 		t.Run(tc.testName, func(tt *testing.T) {
 			analysistest.Run(tt, analysistest.TestData(), ginkgolinter.NewAnalyzer(), tc.testData)
@@ -169,6 +173,13 @@ func TestFlags(t *testing.T) {
 				"forbid-focus-container": "true",
 			},
 		},
+		{
+			testName: "force Succeed/HaveOccurred",
+			testData: []string{"a/confighaveoccurred"},
+			flags: map[string]string{
+				"force-succeed": "true",
+			},
+		},
 	} {
 		t.Run(tc.testName, func(tt *testing.T) {
 			analyzer := ginkgolinter.NewAnalyzer()

doc.go

@@ -30,6 +30,14 @@ For example:
 This will probably happen when using the old format:
 	Eventually(aFunc, 500 * time.Millisecond, 10 * time.Second).Should(Succeed())
 
+* Success matcher validation: [BUG]
+  The Success matcher expect that the actual argument will be a single error. In async actual assertions, It also allow 
+  functions with Gomega object as the function first parameter.
+For example:
+  Expect(myInt).To(Succeed())
+or
+  Eventually(func() int { return 42 }).Should(Succeed())
+
 * reject variable assignments in ginkgo containers [Bug/Style]:
 For example:
 	var _ = Describe("description", func(){
@@ -96,4 +104,13 @@ methods.
 For example:
 	Eventually(context.Background(), func() bool { return true }, "1s").Should(BeTrue())
 	Eventually(context.Background(), func() bool { return true }, time.Second*60, 15).Should(BeTrue())
+
+* Success <=> Eventually usage [Style]
+  enforce that the Succeed() matcher will be used for error functions, and the HaveOccurred() matcher will
+  be used for error values.
+
+For example:
+  Expect(err).ToNot(Succeed())
+or
+  Expect(funcRetError().ToNot(HaveOccurred())
 `

internal/expression/actual/actual.go

@@ -15,6 +15,7 @@ type Actual struct {
 	Clone        *ast.CallExpr
 	Arg          ArgPayload
 	argType      gotypes.Type
+	isTuple      bool
 	isAsync      bool
 	asyncArg     *AsyncArg
 	actualOffset int
@@ -32,13 +33,16 @@ func New(origExpr, cloneExpr *ast.CallExpr, orig *ast.CallExpr, clone *ast.CallE
 	}
 
 	argType := pass.TypesInfo.TypeOf(orig.Args[actualOffset])
+	isTuple := false
 
 	if tpl, ok := argType.(*gotypes.Tuple); ok {
 		if tpl.Len() > 0 {
 			argType = tpl.At(0).Type()
 		} else {
 			argType = nil
 		}
+
+		isTuple = tpl.Len() > 1
 	}
 
 	isAsyncExpr := gomegainfo.IsAsyncActualMethod(funcName)
@@ -53,6 +57,7 @@ func New(origExpr, cloneExpr *ast.CallExpr, orig *ast.CallExpr, clone *ast.CallE
 		Clone:        clone,
 		Arg:          arg,
 		argType:      argType,
+		isTuple:      isTuple,
 		isAsync:      isAsyncExpr,
 		asyncArg:     asyncArg,
 		actualOffset: actualOffset,
@@ -72,6 +77,10 @@ func (a *Actual) IsAsync() bool {
 	return a.isAsync
 }
 
+func (a *Actual) IsTuple() bool {
+	return a.isTuple
+}
+
 func (a *Actual) ArgGOType() gotypes.Type {
 	return a.argType
 }

internal/expression/actual/actualarg.go

@@ -4,13 +4,10 @@ import (
 	"go/ast"
 	"go/token"
 	gotypes "go/types"
-	"strings"
-
 	"golang.org/x/tools/go/analysis"
 
 	"github.com/nunnatsa/ginkgolinter/internal/expression/value"
 	"github.com/nunnatsa/ginkgolinter/internal/gomegainfo"
-	"github.com/nunnatsa/ginkgolinter/internal/interfaces"
 	"github.com/nunnatsa/ginkgolinter/internal/reverseassertion"
 )
 
@@ -26,44 +23,17 @@ const (
 	CapComparisonActualArgType
 	NilComparisonActualArgType
 	BinaryComparisonActualArgType
+	FuncSigArgType
 	ErrFuncActualArgType
+	GomegaParamArgType
+	MultiRetsArgType
 
-	AsyncInvalidFuncCall
 	ErrorTypeArgType
 
 	EqualZero
 	GreaterThanZero
-
-	LastUnusedDontChange
 )
 
-var argTypeString = map[ArgType]string{
-	UnknownActualArgType:          "UnknownActualArgType",
-	ErrActualArgType:              "ErrActualArgType",
-	LenFuncActualArgType:          "LenFuncActualArgType",
-	CapFuncActualArgType:          "CapFuncActualArgType",
-	ComparisonActualArgType:       "ComparisonActualArgType",
-	LenComparisonActualArgType:    "LenComparisonActualArgType",
-	CapComparisonActualArgType:    "CapComparisonActualArgType",
-	NilComparisonActualArgType:    "NilComparisonActualArgType",
-	BinaryComparisonActualArgType: "BinaryComparisonActualArgType",
-	ErrFuncActualArgType:          "ErrFuncActualArgType",
-	AsyncInvalidFuncCall:          "AsyncInvalidFuncCall",
-	ErrorTypeArgType:              "ErrorTypeArgType",
-	EqualZero:                     "EqualZero",
-	GreaterThanZero:               "GreaterThanZero",
-}
-
-func (a ArgType) String() string {
-	var vals []string
-	for mask := UnknownActualArgType; mask < LastUnusedDontChange; mask <<= 1 {
-		if a&mask != 0 {
-			vals = append(vals, argTypeString[mask])
-		}
-	}
-
-	return strings.Join(vals, "|")
-}
 func (a ArgType) Is(val ArgType) bool {
 	return a&val != 0
 }
@@ -85,19 +55,15 @@ func getActualArgPayload(origActualExpr, actualExprClone *ast.CallExpr, pass *an
 
 		case *ast.BinaryExpr:
 			arg = parseBinaryExpr(expr, argExprClone.(*ast.BinaryExpr), pass)
-		}
-
-	}
 
-	t := pass.TypesInfo.TypeOf(origArgExpr)
-	if sig, ok := t.(*gotypes.Signature); ok {
-		if sig.Results().Len() == 1 {
-			if interfaces.ImplementsError(sig.Results().At(0).Type().Underlying()) {
-				arg = &ErrFuncArgPayload{}
+		default:
+			t := pass.TypesInfo.TypeOf(origArgExpr)
+			if sig, ok := t.(*gotypes.Signature); ok {
+				arg = getAsyncFuncArg(sig)
 			}
 		}
+
 	}
-	//	}
 
 	if arg != nil {
 		return arg, actualOffset
@@ -200,12 +166,6 @@ func (f *FuncCallArgPayload) ArgType() ArgType {
 	return f.argType
 }
 
-type ErrFuncArgPayload struct{}
-
-func (*ErrFuncArgPayload) ArgType() ArgType {
-	return ErrFuncActualArgType | ErrorTypeArgType
-}
-
 type ErrPayload struct {
 	value.Valuer
 }

internal/expression/actual/asyncfuncarg.go

@@ -0,0 +1,37 @@
+package actual
+
+import (
+	"github.com/nunnatsa/ginkgolinter/internal/gomegahandler"
+	"github.com/nunnatsa/ginkgolinter/internal/interfaces"
+	gotypes "go/types"
+)
+
+func getAsyncFuncArg(sig *gotypes.Signature) ArgPayload {
+	argType := FuncSigArgType
+	if sig.Results().Len() == 1 {
+		if interfaces.ImplementsError(sig.Results().At(0).Type().Underlying()) {
+			argType |= ErrFuncActualArgType | ErrorTypeArgType
+		}
+	}
+
+	if sig.Params().Len() > 0 {
+		arg := sig.Params().At(0).Type()
+		if gomegahandler.IsGomegaType(arg) && sig.Results().Len() == 0 {
+			argType |= FuncSigArgType | GomegaParamArgType
+		}
+	}
+
+	if sig.Results().Len() > 1 {
+		argType |= FuncSigArgType | MultiRetsArgType
+	}
+
+	return &FuncSigArgPayload{argType: argType}
+}
+
+type FuncSigArgPayload struct {
+	argType ArgType
+}
+
+func (f FuncSigArgPayload) ArgType() ArgType {
+	return f.argType
+}

internal/expression/expression.go

@@ -291,6 +291,10 @@ func (e *GomegaExpression) ActualArgTypeIs(other actual.ArgType) bool {
 	return e.actual.Arg.ArgType().Is(other)
 }
 
+func (e *GomegaExpression) IsActualTuple() bool {
+	return e.actual.IsTuple()
+}
+
 // Matcher proxies
 
 func (e *GomegaExpression) GetMatcher() *matcher.Matcher {

internal/expression/matcher/matcher.go

@@ -24,6 +24,8 @@ const ( // gomega matchers
 	or             = "Or"
 	withTransform  = "WithTransform"
 	matchError     = "MatchError"
+	haveOccurred   = "HaveOccurred"
+	succeed        = "Succeed"
 )
 
 type Matcher struct {