New Rule: should not compare two different types

The `Equal` and the `BeIdentical` matchers also check the type, not only
the value.

The following code will fail in runtime:
```go
x := 5 // x is int
Expect(x).Should(Eqaul(uint(5)) // x and uint(5) are with different
types
```

When using negative checks, it's even worse, because we get a fale
positive:
```
x := 5
Expect(x).ToNot(Equal(uint(5))
```

To solve this problem, we want to find these errors before running the
tests and failing on runtime. This is even more important for e2e or
functional tests, where we sometimes can't run them on the local
development environment.

This PR adds a new rule to the linter, to find comaprison of values
from different types.

README.md

@@ -181,7 +181,29 @@ These container, or the `Focus` spec, must not be part of the final source code,
 
 ***This rule is disabled by default***. Use the `--forbid-focus-container=true` command line flag to enable it.  
 
+### Comparing values from different types [BUG]
 
+The `Equal` and the `BeIdentical` matchers also check the type, not only the value.
+    
+The following code will fail in runtime:    
+```go
+x := 5 // x is int
+Expect(x).Should(Eqaul(uint(5)) // x and uint(5) are with different
+```
+When using negative checks, it's even worse, because we get a false positive:
+```
+x := 5
+Expect(x).ShouldNot(Equal(uint(5))
+```
+
+The linter suggests two options to solve this warning: either compare with the same type, e.g. 
+using casting, or use the `BeEquivalentTo` matcher.
+
+The linter can't guess what is the best solution in each case, and so it won't auto-fix this warning.
+
+To suppress this warning entirely, use the `--suppress-type-compare-assertion=true` command line parameter. 
+
+To suppress a specific file or line, use the `// ginkgo-linter:ignore-type-compare-warning` comment (see [below](#suppress-warning-from-the-code))
 
 ### Wrong Length Assertion [STYLE]
 The linter finds assertion of the golang built-in `len` function, with all kind of matchers, while there are already gomega matchers for these usecases; We want to assert the item, rather than its length.
@@ -317,6 +339,8 @@ Expect(c1 == x1).Should(BeTrue()) // ==> Expect(x1).Should(Equal(c1))
 * Use the `--suppress-err-assertion=true` flag to suppress the wrong error assertion warning
 * Use the `--suppress-compare-assertion=true` flag to suppress the wrong comparison assertion warning
 * 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 `--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.
 
@@ -345,6 +369,10 @@ To supress the focus container warning, add a comment with (only)
 
 `ginkgo-linter:ignore-focus-container-warning`
 
+To suppress the different type comparison, add a comment with (only)
+
+`ginkgo-linter:ignore-type-compare-warning`
+
 Notice that this comment will not work for an anonymous variable container like
 ```go
 // ginkgo-linter:ignore-focus-container-warning (not working!!)

ginkgo_linter.go

@@ -9,6 +9,7 @@ import (
 	"go/printer"
 	"go/token"
 	gotypes "go/types"
+	"reflect"
 
 	"github.com/go-toolsmith/astcopy"
 	"golang.org/x/tools/go/analysis"
@@ -38,6 +39,8 @@ const (
 	missingAsyncAssertionMessage  = linterName + `: %q: missing assertion method. Expected "Should()" or "ShouldNot()"`
 	focusContainerFound           = linterName + ": Focus container found. This is used only for local debug and should not be part of the actual source code, consider to replace with %q"
 	focusSpecFound                = linterName + ": Focus spec found. This is used only for local debug and should not be part of the actual source code, consider to remove it"
+	compareDifferentTypes         = linterName + ": use %[1]s with different types: Comparing %[2]s with %[3]s; either change the expected value type if possible, or use the BeEquivalentTo() matcher, instead of %[1]s()"
+	compareInterfaces             = linterName + ": be careful comparing interfaces. This can fail in runtime, if the actual implementing types are different"
 )
 const ( // gomega matchers
 	beEmpty        = "BeEmpty"
@@ -55,6 +58,9 @@ const ( // gomega matchers
 	not            = "Not"
 	omega          = "Ω"
 	succeed        = "Succeed"
+	and            = "And"
+	or             = "Or"
+	withTransform  = "WithTransform"
 )
 
 const ( // gomega actuals
@@ -99,6 +105,7 @@ func NewAnalyzer() *analysis.Analyzer {
 	a.Flags.Var(&linter.config.SuppressErr, "suppress-err-assertion", "Suppress warning for wrong error assertions")
 	a.Flags.Var(&linter.config.SuppressCompare, "suppress-compare-assertion", "Suppress warning for wrong comparison assertions")
 	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.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")
@@ -126,6 +133,9 @@ currently, the linter searches for following:
 
 * trigger a warning when a ginkgo focus container (FDescribe, FContext, FWhen or FIt) is found. [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))
@@ -299,9 +309,119 @@ func checkExpression(pass *analysis.Pass, config types.Config, assertionExp *ast
 
 	} else if checkPointerComparison(pass, config, assertionExp, expr, actualArg, handler, oldExpr) {
 		return false
-	} else {
-		return handleAssertionOnly(pass, config, expr, handler, actualArg, oldExpr, true)
+	} else if !handleAssertionOnly(pass, config, expr, handler, actualArg, oldExpr, true) {
+		return false
+	} else if !config.SuppressTypeCompare {
+		return !checkEqualWrongType(pass, assertionExp, actualArg, handler, oldExpr)
+	}
+
+	return true
+}
+
+func checkEqualWrongType(pass *analysis.Pass, origExp *ast.CallExpr, actualArg ast.Expr, handler gomegahandler.Handler, old string) bool {
+	matcher, ok := origExp.Args[0].(*ast.CallExpr)
+	if !ok {
+		return false
+	}
+
+	return checkEqualDifferentTypes(pass, matcher, actualArg, handler, old, false)
+}
+
+func checkEqualDifferentTypes(pass *analysis.Pass, matcher *ast.CallExpr, actualArg ast.Expr, handler gomegahandler.Handler, old string, parentPointer bool) bool {
+	matcherFuncName, ok := handler.GetActualFuncName(matcher)
+	if !ok {
+		return false
+	}
+
+	switch matcherFuncName {
+	case equal, beIdenticalTo: // continue
+	case and, or:
+		foundIssue := false
+		for _, nestedExp := range matcher.Args {
+			nested, ok := nestedExp.(*ast.CallExpr)
+			if !ok {
+				continue
+			}
+			if checkEqualDifferentTypes(pass, nested, actualArg, handler, old, parentPointer) {
+				foundIssue = true
+			}
+		}
+
+		return foundIssue
+	case withTransform:
+		nested, ok := matcher.Args[1].(*ast.CallExpr)
+		if !ok {
+			return false
+		}
+
+		return checkEqualDifferentTypes(pass, nested, actualArg, handler, old, parentPointer)
+
+	case not:
+		nested, ok := matcher.Args[0].(*ast.CallExpr)
+		if !ok {
+			return false
+		}
+
+		return checkEqualDifferentTypes(pass, nested, actualArg, handler, old, parentPointer)
+
+	case haveValue:
+		nested, ok := matcher.Args[0].(*ast.CallExpr)
+		if !ok {
+			return false
+		}
+
+		return checkEqualDifferentTypes(pass, nested, actualArg, handler, old, true)
+	default:
+		return false
+	}
+
+	matcherValue := matcher.Args[0]
+
+	actualType := pass.TypesInfo.TypeOf(actualArg)
+
+	switch act := actualType.(type) {
+	case *gotypes.Tuple:
+		actualType = act.At(0).Type()
+	case *gotypes.Pointer:
+		if parentPointer {
+			actualType = act.Elem()
+		}
+	}
+
+	matcherType := pass.TypesInfo.TypeOf(matcherValue)
+
+	if !reflect.DeepEqual(matcherType, actualType) {
+		// Equal can handle comparison of interface and a value that implements it
+		if isImplementing(matcherType, actualType) || isImplementing(actualType, matcherType) {
+			return false
+		}
+
+		reportNoFix(pass, matcher.Pos(), compareDifferentTypes, matcherFuncName, actualType, matcherType)
+		return true
 	}
+
+	return false
+}
+
+func isImplementing(ifs, impl gotypes.Type) bool {
+	if gotypes.IsInterface(ifs) {
+
+		var (
+			theIfs *gotypes.Interface
+			ok     bool
+		)
+
+		for {
+			theIfs, ok = ifs.(*gotypes.Interface)
+			if ok {
+				break
+			}
+			ifs = ifs.Underlying()
+		}
+
+		return gotypes.Implements(impl, theIfs)
+	}
+	return false
 }
 
 // be careful - never change origExp!!! only modify its clone, expr!!!
@@ -1181,8 +1301,7 @@ func isPointer(pass *analysis.Pass, expr ast.Expr) bool {
 
 func isInterface(pass *analysis.Pass, expr ast.Expr) bool {
 	t := pass.TypesInfo.TypeOf(expr)
-	_, ok := t.(*gotypes.Named)
-	return ok
+	return gotypes.IsInterface(t)
 }
 
 func isNumeric(pass *analysis.Pass, node ast.Expr) bool {

ginkgo_linter_test.go

@@ -77,6 +77,10 @@ func TestAllUseCases(t *testing.T) {
 			testName: "focus",
 			testData: "a/focus",
 		},
+		{
+			testName: "equal with different type",
+			testData: "a/comparetypes",
+		},
 	} {
 		t.Run(tc.testName, func(tt *testing.T) {
 			analysistest.Run(tt, analysistest.TestData(), ginkgolinter.NewAnalyzer(), tc.testData)
@@ -125,6 +129,11 @@ func TestFlags(t *testing.T) {
 			testData: []string{"a/focusconfig"},
 			flags:    map[string]string{"forbid-focus-container": "true"},
 		},
+		{
+			testName: "test the suppress-type-compare-assertion flag",
+			testData: []string{"a/comparetypesconfig"},
+			flags:    map[string]string{"suppress-type-compare-assertion": "true"},
+		},
 	} {
 		t.Run(tc.testName, func(tt *testing.T) {
 			analyzer := ginkgolinter.NewAnalyzer()

testdata/src/a/comparetypes/comparetypes.gomega_test.go

@@ -0,0 +1,79 @@
+package comparetypes_test
+
+import (
+	"fmt"
+	. "github.com/onsi/ginkgo/v2"
+	"github.com/onsi/gomega"
+)
+
+func fint64() int64 {
+	return 5
+}
+
+type mytype int
+
+func fmytype() mytype {
+	return 5
+}
+
+func multi() (int64, error) {
+	return 4, nil
+}
+
+type myinf interface {
+	doSomething()
+}
+
+type imp1 int
+
+func (imp1) doSomething() {
+	fmt.Println("doing something")
+}
+
+type imp2 int
+
+func (imp2) doSomething() {
+	fmt.Println("doing something else")
+}
+
+var _ = Describe("compare different types", func() {
+	It("find false positive check", func() {
+		a := 5
+		gomega.Expect(multi()).ShouldNot(gomega.Equal(4)) // want `ginkgo-linter: use Equal with different types: Comparing int64 with int; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(a).ShouldNot(gomega.Equal(5))
+		gomega.Expect(a).ShouldNot(gomega.Equal(uint(5)))           // want `ginkgo-linter: use Equal with different types: Comparing int with uint; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(fint64()).ShouldNot(gomega.Equal(uint64(5)))  // want `ginkgo-linter: use Equal with different types: Comparing int64 with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(fmytype()).ShouldNot(gomega.Equal(uint64(5))) // want `ginkgo-linter: use Equal with different types: Comparing a/comparetypes_test.mytype with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(a).ShouldNot(gomega.Equal(int32(5)))          // want `ginkgo-linter: use Equal with different types: Comparing int with int32; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(a).ShouldNot(gomega.Equal(uint8(5)))          // want `ginkgo-linter: use Equal with different types: Comparing int with uint8; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(a).ShouldNot(gomega.Equal(mytype(5)))         // want `ginkgo-linter: use Equal with different types: Comparing int with a/comparetypes_test.mytype; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(5).ShouldNot(gomega.Equal(mytype(5)))         // want `ginkgo-linter: use Equal with different types: Comparing int with a/comparetypes_test.mytype; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+
+		b := int16(5)
+		gomega.Expect(a).ShouldNot(gomega.Equal(b)) // want `ginkgo-linter: use Equal with different types: Comparing int with int16; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+
+		c := mytype(5)
+		gomega.Expect(a).ShouldNot(gomega.Equal(c)) // want `ginkgo-linter: use Equal with different types: Comparing int with a/comparetypes_test.mytype; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+	})
+
+	It("compare interfaces", func() {
+		var (
+			a myinf = imp1(3)
+			b myinf = imp2(3)
+		)
+		gomega.Expect(a).ShouldNot(gomega.Equal(b)) // this is not good, but not an error. Should have kind of warning here.
+		gomega.Expect(a).ShouldNot(gomega.BeEquivalentTo(b))
+		gomega.Expect(a).ShouldNot(gomega.BeIdenticalTo(b)) // this is not good, but not an error. Should have kind of warning here.
+	})
+
+	It("check HaveValue(Equal)", func() {
+		a := 5
+		pa := &a
+
+		gomega.Expect(pa).Should(gomega.HaveValue(gomega.Equal(uint64(5))))                                                                // want `ginkgo-linter: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(a).Should(gomega.Not(gomega.Equal(uint64(5))))                                                                       // want `ginkgo-linter: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(a).Should(gomega.And(gomega.Equal(uint64(5)), gomega.Not(gomega.Equal(int32(6)))))                                   // want `ginkgo-linter: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)` `ginkgo-linter: use Equal with different types: Comparing int with int32; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(a).Should(gomega.Or(gomega.Equal(uint64(5)), gomega.Not(gomega.Equal(int32(6))), gomega.Not(gomega.Equal(int8(4))))) // want `ginkgo-linter: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)` `ginkgo-linter: use Equal with different types: Comparing int with int32; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)` `ginkgo-linter: use Equal with different types: Comparing int with int8; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+		gomega.Expect(a).Should(gomega.WithTransform(func(i int) int { return i + 1 }, gomega.Equal(uint(6))))                             // want `ginkgo-linter: use Equal with different types: Comparing int with uint; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
+	})
+})