Add new optional rule: force Expect with To

Added new rule: should not use `Expect` with the `Should` or `ShouldNot`
assertion methods.

This rule is not enabled by default. To enable it, use the
`--force-expect-to=true` flag.

The new rule does support auto fix.

README.md

@@ -356,6 +356,17 @@ Expect(x1 == c1).Should(BeTrue()) // ==> Expect(x1).Should(Equal(c1))
 Expect(c1 == x1).Should(BeTrue()) // ==> Expect(x1).Should(Equal(c1))
 ```
 
+### 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.
+```go
+Expect("abc").Should(HaveLen(3)) // => Expect("abc").To(HaveLen(3))
+Expect("abc").ShouldNot(BeEmpty()) // => Expect("abc").ToNot(BeEmpty())
+```
+This rule support auto fixing.
+
+***This rule is disabled by default***. Use the `--force-expect-to=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 assertion warning

doc.go

@@ -0,0 +1,54 @@
+package ginkgolinter
+
+const doc = `enforces standards of using ginkgo and gomega
+
+or
+       ginkgolinter version
+
+version: %s
+
+currently, the linter searches for following:
+* trigger a warning when using Eventually or Constantly with a function call. This is in order to prevent the case when 
+  using a function call instead of a function. Function call returns a value only once, and so the original value
+  is tested again and again and is never changed. [Bug]
+
+* trigger a warning when comparing a pointer to a value. [Bug]
+
+* trigger a warning for missing assertion method: [Bug]
+	Eventually(checkSomething)
+
+* trigger a warning when a ginkgo focus container (FDescribe, FContext, FWhen or FIt) is found. [Bug]
+
+* 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.
+
+* wrong length assertions. We want to assert the item rather than its length. [Style]
+For example:
+	Expect(len(x)).Should(Equal(1))
+This should be replaced with:
+	Expect(x)).Should(HavelLen(1))
+	
+* wrong nil assertions. We want to assert the item rather than a comparison result. [Style]
+For example:
+	Expect(x == nil).Should(BeTrue())
+This should be replaced with:
+	Expect(x).Should(BeNil())
+
+* wrong error assertions. For example: [Style]
+	Expect(err == nil).Should(BeTrue())
+This should be replaced with:
+	Expect(err).ShouldNot(HaveOccurred())
+
+* wrong boolean comparison, for example: [Style]
+	Expect(x == 8).Should(BeTrue())
+This should be replaced with:
+	Expect(x).Should(BeEqual(8))
+
+* replaces Equal(true/false) with BeTrue()/BeFalse() [Style]
+
+* replaces HaveLen(0) with BeEmpty() [Style]
+
+* replaces Expect(...).Should(...) with Expect(...).To() [stype]
+`

ginkgo_linter.go

@@ -46,6 +46,7 @@ const (
 	matchErrorMissingDescription  = linterName + ": missing function description as second parameter of MatchError"
 	matchErrorRedundantArg        = linterName + ": redundant MatchError arguments; consider removing them; consider using `%s` instead"
 	matchErrorNoFuncDescription   = linterName + ": The second parameter of MatchError must be the function description (string)"
+	forceExpectToTemplate         = linterName + ": must not use Expect with %s"
 )
 
 const ( // gomega matchers
@@ -96,6 +97,7 @@ func NewAnalyzer() *analysis.Analyzer {
 			SuppressCompare: false,
 			ForbidFocus:     false,
 			AllowHaveLen0:   false,
+			ForceExpectTo:   false,
 		},
 	}
 
@@ -114,64 +116,13 @@ func NewAnalyzer() *analysis.Analyzer {
 	a.Flags.Var(&linter.config.SuppressAsync, "suppress-async-assertion", "Suppress warning for function call in async assertion, like Eventually")
 	a.Flags.Var(&linter.config.SuppressTypeCompare, "suppress-type-compare-assertion", "Suppress warning for comparing values from different types, like int32 and uint32")
 	a.Flags.Var(&linter.config.AllowHaveLen0, "allow-havelen-0", "Do not warn for HaveLen(0); default = false")
-
+	a.Flags.Var(&linter.config.ForceExpectTo, "force-expect-to", "force using `Expect` with `To`, `ToNot` or `NotTo`. reject using `Expect` with `Should` or `ShouldNot`; default = false (not forced)")
 	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(&linter.config.ForbidFocus, "forbid-focus-container", "trigger a warning for ginkgo focus containers like FDescribe, FContext, FWhen or FIt; default = false.")
 
 	return a
 }
 
-const doc = `enforces standards of using ginkgo and gomega
-
-or
-       ginkgolinter version
-
-version: %s
-
-currently, the linter searches for following:
-* trigger a warning when using Eventually or Constantly with a function call. This is in order to prevent the case when 
-  using a function call instead of a function. Function call returns a value only once, and so the original value
-  is tested again and again and is never changed. [Bug]
-
-* trigger a warning when comparing a pointer to a value. [Bug]
-
-* trigger a warning for missing assertion method: [Bug]
-	Eventually(checkSomething)
-
-* trigger a warning when a ginkgo focus container (FDescribe, FContext, FWhen or FIt) is found. [Bug]
-
-* 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.
-
-* wrong length assertions. We want to assert the item rather than its length. [Style]
-For example:
-	Expect(len(x)).Should(Equal(1))
-This should be replaced with:
-	Expect(x)).Should(HavelLen(1))
-	
-* wrong nil assertions. We want to assert the item rather than a comparison result. [Style]
-For example:
-	Expect(x == nil).Should(BeTrue())
-This should be replaced with:
-	Expect(x).Should(BeNil())
-
-* wrong error assertions. For example: [Style]
-	Expect(err == nil).Should(BeTrue())
-This should be replaced with:
-	Expect(err).ShouldNot(HaveOccurred())
-
-* wrong boolean comparison, for example: [Style]
-	Expect(x == 8).Should(BeTrue())
-This should be replaced with:
-	Expect(x).Should(BeEqual(8))
-
-* replaces Equal(true/false) with BeTrue()/BeFalse() [Style]
-
-* replaces HaveLen(0) with BeEmpty() [Style]
-`
-
 // main assertion function
 func (l *ginkgoLinter) run(pass *analysis.Pass) (interface{}, error) {
 	for _, file := range pass.Files {
@@ -295,14 +246,46 @@ func checkExpression(pass *analysis.Pass, config types.Config, assertionExp *ast
 		return true
 	}
 
-	goNested, diags := doCheckExpression(pass, config, assertionExp, actualArg, expr, handler, oldExpr)
-	if len(diags) > 0 {
-		report(pass, diags)
+	goNested, diagnostics := doCheckExpression(pass, config, assertionExp, actualArg, expr, handler, oldExpr)
+
+	if config.ForceExpectTo {
+		if valid, diags := forceExpectTo(pass, expr, handler, oldExpr); !valid {
+			diagnostics = append(diagnostics, diags...)
+		}
+	}
+
+	if len(diagnostics) > 0 {
+		report(pass, diagnostics)
 	}
 
 	return goNested
 }
 
+func forceExpectTo(pass *analysis.Pass, expr *ast.CallExpr, handler gomegahandler.Handler, oldExpr string) (bool, []analysis.Diagnostic) {
+	if asrtFun, ok := expr.Fun.(*ast.SelectorExpr); ok {
+		if actualFuncName, ok := handler.GetActualFuncName(expr); ok && actualFuncName == expect {
+			var (
+				name     string
+				newIdent *ast.Ident
+			)
+
+			switch name = asrtFun.Sel.Name; name {
+			case "Should":
+				newIdent = ast.NewIdent("To")
+			case "ShouldNot":
+				newIdent = ast.NewIdent("ToNot")
+			default:
+				return true, nil
+			}
+
+			handler.ReplaceFunction(expr, newIdent)
+			return false, prepareReport(pass, expr, fmt.Sprintf(forceExpectToTemplate, name)+"; consider using `%s` instead", oldExpr)
+		}
+	}
+
+	return false, nil
+}
+
 func doCheckExpression(pass *analysis.Pass, config types.Config, assertionExp *ast.CallExpr, actualArg ast.Expr, expr *ast.CallExpr, handler gomegahandler.Handler, oldExpr string) (bool, []analysis.Diagnostic) {
 	if !bool(config.SuppressLen) && isActualIsLenFunc(actualArg) {
 		return checkLengthMatcher(expr, pass, handler, oldExpr)
@@ -1359,17 +1342,18 @@ func report(pass *analysis.Pass, diagnostics []analysis.Diagnostic) {
 		pass.Report(diagnostics[0])
 
 	default:
+		// use only the latest SuggestedFix, to avoid conflicts
 		var fixes []analysis.SuggestedFix
 		for i := range diagnostics[:len(diagnostics)-1] {
 			if len(diagnostics[i].SuggestedFixes) > 0 {
-				fixes = append(fixes, diagnostics[i].SuggestedFixes...)
+				fixes = diagnostics[i].SuggestedFixes
 				diagnostics[i].SuggestedFixes = nil
 			}
 			pass.Report(diagnostics[i])
 		}
 		lastDiag := diagnostics[len(diagnostics)-1]
-		if len(fixes) > 0 {
-			lastDiag.SuggestedFixes = append(lastDiag.SuggestedFixes, fixes...)
+		if len(lastDiag.SuggestedFixes) == 0 && len(fixes) > 0 {
+			lastDiag.SuggestedFixes = fixes
 		}
 		pass.Report(lastDiag)
 	}

ginkgo_linter_test.go

@@ -142,6 +142,11 @@ func TestFlags(t *testing.T) {
 			testData: []string{"a/comparetypesconfig"},
 			flags:    map[string]string{"suppress-type-compare-assertion": "true"},
 		},
+		{
+			testName: "test the force-expect-to flag",
+			testData: []string{"a/forceExpectTo"},
+			flags:    map[string]string{"force-expect-to": "true"},
+		},
 	} {
 		t.Run(tc.testName, func(tt *testing.T) {
 			analyzer := ginkgolinter.NewAnalyzer()

gomegahandler/handler.go

@@ -70,7 +70,12 @@ func (h dotHandler) GetActualFuncName(expr *ast.CallExpr) (string, bool) {
 
 // ReplaceFunction replaces the function with another one, for fix suggestions
 func (dotHandler) ReplaceFunction(caller *ast.CallExpr, newExpr *ast.Ident) {
-	caller.Fun = newExpr
+	switch f := caller.Fun.(type) {
+	case *ast.Ident:
+		caller.Fun = newExpr
+	case *ast.SelectorExpr:
+		f.Sel = newExpr
+	}
 }
 
 func (dotHandler) getDefFuncName(expr *ast.CallExpr) string {

testdata/src/a/forceExpectTo/gomegavar.go

@@ -0,0 +1,22 @@
+package forceExpectTo
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("gomega var", func() {
+	It("in a valid Eventually", func() {
+		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: wrong length assertion; consider using .g\.Expect\("a"\)\.Should\(HaveLen\(1\)\). instead` `ginkgo-linter: must not use Expect with Should; consider using .g\.Expect\("a"\)\.To\(HaveLen\(1\)\). instead`
+		}).Should(Succeed())
+	})
+
+	It("in an invalid Eventually", func() {
+		Eventually(func(g Gomega) { // want `ginkgo-linter: "Eventually": missing assertion method\. Expected "Should\(\)" or "ShouldNot\(\)"`
+			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: wrong length assertion; consider using .g\.Expect\("a"\)\.Should\(HaveLen\(1\)\). instead` `ginkgo-linter: must not use Expect with Should; consider using .g\.Expect\("a"\)\.To\(HaveLen\(1\)\). instead`
+		})
+	})
+})

testdata/src/a/forceExpectTo/nodotimport.go

@@ -0,0 +1,19 @@
+package forceExpectTo
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	"github.com/onsi/gomega"
+)
+
+var _ = Describe("simple case", func() {
+	It("just replace assertion method", func() {
+		gomega.Expect("a").Should(gomega.HaveLen(1))            // want `ginkgo-linter: must not use Expect with Should; consider using .gomega\.Expect\("a"\)\.To\(gomega\.HaveLen\(1\)\). instead`
+		gomega.Expect("a").ShouldNot(gomega.BeEmpty())          // want `ginkgo-linter: must not use Expect with ShouldNot; consider using .gomega\.Expect\("a"\)\.ToNot\(gomega\.BeEmpty\(\)\). instead`
+		gomega.Expect("a").Should(gomega.Not(gomega.BeEmpty())) // want `ginkgo-linter: must not use Expect with ShouldNot; consider using .gomega\.Expect\("a"\)\.ToNot\(gomega\.BeEmpty\(\)\). instead`
+	})
+
+	It("mix with len assertion", func() {
+		gomega.Expect(len("a")).Should(gomega.Equal(1))    // want `ginkgo-linter: wrong length assertion; consider using .gomega\.Expect\("a"\)\.Should\(gomega\.HaveLen\(1\)\). instead` `ginkgo-linter: must not use Expect with Should; consider using .gomega\.Expect\("a"\)\.To\(gomega\.HaveLen\(1\)\). instead`
+		gomega.Expect(len("a")).ShouldNot(gomega.Equal(0)) // want `ginkgo-linter: wrong length assertion; consider using .gomega\.Expect\("a"\)\.ShouldNot\(gomega\.BeEmpty\(\)\). instead` `ginkgo-linter: must not use Expect with ShouldNot; consider using .gomega\.Expect\("a"\)\.ToNot\(gomega\.BeEmpty\(\)\). instead`
+	})
+})

testdata/src/a/forceExpectTo/simple.go

@@ -0,0 +1,21 @@
+package forceExpectTo
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("simple case", func() {
+	It("just replace assertion method", func() {
+		Expect("a").Should(HaveLen(1))     // want `ginkgo-linter: must not use Expect with Should; consider using .Expect\("a"\)\.To\(HaveLen\(1\)\). instead`
+		Expect("a").ShouldNot(BeEmpty())   // want `ginkgo-linter: must not use Expect with ShouldNot; consider using .Expect\("a"\)\.ToNot\(BeEmpty\(\)\). instead`
+		Expect("a").Should(Not(BeEmpty())) // want `ginkgo-linter: must not use Expect with ShouldNot; consider using .Expect\("a"\)\.ToNot\(BeEmpty\(\)\). instead`
+		Ω("a").Should(HaveLen(1))
+		Ω("a").ShouldNot(BeEmpty())
+	})
+
+	It("mix with len assertion", func() {
+		Expect(len("a")).Should(Equal(1))    // want `ginkgo-linter: wrong length assertion; consider using .Expect\("a"\)\.Should\(HaveLen\(1\)\). instead` `ginkgo-linter: must not use Expect with Should; consider using .Expect\("a"\)\.To\(HaveLen\(1\)\). instead`
+		Expect(len("a")).ShouldNot(Equal(0)) // want `ginkgo-linter: wrong length assertion; consider using .Expect\("a"\)\.ShouldNot\(BeEmpty\(\)\). instead` `ginkgo-linter: must not use Expect with ShouldNot; consider using .Expect\("a"\)\.ToNot\(BeEmpty\(\)\). instead`
+	})
+})

tests/e2e.sh

@@ -6,22 +6,26 @@ cp ginkgolinter testdata/src/a
 cd testdata/src/a
 
 # no suppress
-[[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2540 ]]
+[[ $(./ginkgolinter a/... 2>&1 | wc -l) == 2547 ]]
 # 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) == 1518 ]]
+[[ $(./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) == 1519 ]]
 # 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) == 816 ]]
+[[ $(./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) == 823 ]]
 # 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) == 278 ]]
+[[ $(./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) == 279 ]]
 # 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) == 288 ]]
+[[ $(./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) == 289 ]]
 # 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) == 191 ]]
+[[ $(./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) == 192 ]]
 # 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) == 209 ]]
+[[ $(./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) == 210 ]]
 # 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) == 287 ]]
+[[ $(./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) == 288 ]]
 # allow HaveLen(0)
-[[ $(./ginkgolinter --allow-havelen-0=true a/... 2>&1 | wc -l) == 2526 ]]
+[[ $(./ginkgolinter --allow-havelen-0=true a/... 2>&1 | wc -l) == 2533 ]]
+# force Expect with To
+[[ $(./ginkgolinter --force-expect-to=true a/... 2>&1 | wc -l) == 3209 ]]
 # 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) == 154 ]]
+[[ $(./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) == 817 ]]

types/config.go

@@ -26,6 +26,7 @@ type Config struct {
 	ForbidFocus         Boolean
 	SuppressTypeCompare Boolean
 	AllowHaveLen0       Boolean
+	ForceExpectTo       Boolean
 }
 
 func (s *Config) AllTrue() bool {
@@ -42,6 +43,7 @@ func (s *Config) Clone() Config {
 		ForbidFocus:         s.ForbidFocus,
 		SuppressTypeCompare: s.SuppressTypeCompare,
 		AllowHaveLen0:       s.AllowHaveLen0,
+		ForceExpectTo:       s.ForceExpectTo,
 	}
 }