New rule(s): Wrong Usage of the MatchError gomega Matcher

The `MatchError` gomega matcher asserts an error value (and if it's not nil).

There are four valid formats for using this Matcher:
* error value; e.g. `Expect(err).To(MatchError(anotherErr))`
* string, to be equal to the output of the `Error()` method; e.g. `Expect(err).To(MatchError("Not Found"))`
* A gomega matcher that asserts strings; e.g. `Expect(err).To(MatchError(ContainSubstring("Found")))`
* [from v0.29.0] a function that receive a single error parameter and returns a single boolean value.
  In this format, an additional single string parameter, with the function description, is also required; e.g.
  `Expect(err).To(MatchError(isNotFound, "is the error is a not found error"))`

These four format are checked on runtime, but sometimes it's too late. ginkgolinter performs a static analysis and so it
will find these issues on build time.

ginkgolinter checks the following:
* Is the first parameter is one of the four options above.
* That there are no additional parameters passed to the matcher; e.g.
  `MatchError(isNotFoundFunc, "a valid description" , "not used string")`. In this case, the matcher won't fail on run
  time, but the additional parameters are not in use and ignored.
* 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.

Makefile

@@ -5,9 +5,12 @@ HASH_FLAG := -X github.com/nunnatsa/ginkgolinter/version.gitHash=$(COMMIT_HASH)
 
 BUILD_ARGS := -ldflags "$(VERSION_FLAG) $(HASH_FLAG)"
 
-build:
+build: unit-test
 	go build $(BUILD_ARGS) -o ginkgolinter ./cmd/ginkgolinter
 
+unit-test:
+	go test ./...
+
 build-for-windows:
 	GOOS=windows GOARCH=amd64 go build $(BUILD_ARGS) -o bin/ginkgolinter-amd64.exe ./cmd/ginkgolinter
 

README.md

@@ -44,7 +44,7 @@ It is not enabled by default, though. There are two ways to run ginkgolinter wit
      enable:
        - ginkgolinter
    ```
-## Linter Checks
+## Linter Rules
 The linter checks the ginkgo and gomega assertions in golang test code. Gomega may be used together with ginkgo tests, 
 For example:
 ```go
@@ -177,7 +177,8 @@ var _ = Describe("checking something", Focus, func() {
 })
 ```
 
-These container, or the `Focus` spec, must not be part of the final source code, and should only be used locally by the developer.
+These container, or the `Focus` spec, must not be part of the final source code, and should only be used locally by the 
+developer.
 
 ***This rule is disabled by default***. Use the `--forbid-focus-container=true` command line flag to enable it.  
 
@@ -205,8 +206,30 @@ To suppress this warning entirely, use the `--suppress-type-compare-assertion=tr
 
 To suppress a specific file or line, use the `// ginkgo-linter:ignore-type-compare-warning` comment (see [below](#suppress-warning-from-the-code))
 
+### Wrong Usage of the `MatchError` gomega Matcher [BUG]
+The `MatchError` gomega matcher asserts an error value (and if it's not nil).
+There are four valid formats for using this Matcher:
+* error value; e.g. `Expect(err).To(MatchError(anotherErr))`
+* string, to be equal to the output of the `Error()` method; e.g. `Expect(err).To(MatchError("Not Found"))`
+* A gomega matcher that asserts strings; e.g. `Expect(err).To(MatchError(ContainSubstring("Found")))`
+* [from v0.29.0] a function that receive a single error parameter and returns a single boolean value. 
+  In this format, an additional single string parameter, with the function description, is also required; e.g.
+  `Expect(err).To(MatchError(isNotFound, "is the error is a not found error"))`
+
+These four format are checked on runtime, but sometimes it's too late. ginkgolinter performs a static analysis and so it
+will find these issues on build time.
+
+ginkgolinter checks the following:
+* Is the first parameter is one of the four options above.
+* That there are no additional parameters passed to the matcher; e.g.
+  `MatchError(isNotFoundFunc, "a valid description" , "not used string")`. In this case, the matcher won't fail on run 
+  time, but the additional parameters are not in use and ignored.   
+* 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 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.
+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.
 
 There are several wrong patterns:
 ```go
@@ -240,7 +263,8 @@ The output of the linter,when finding issues, looks like this:
 The linter will also warn about the `HaveLen(0)` matcher, and will suggest to replace it with `BeEmpty()`
 
 ### Wrong `nil` Assertion [STYLE]
-The linter finds assertion of the comparison to nil, with all kind of matchers, instead of using the existing `BeNil()` matcher; We want to assert the item, rather than a comparison result.
+The linter finds assertion of the comparison to nil, with all kind of matchers, instead of using the existing `BeNil()` 
+matcher; We want to assert the item, rather than a comparison result.
 
 There are several wrong patterns:
 

ginkgo_linter.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/nunnatsa/ginkgolinter/ginkgohandler"
 	"github.com/nunnatsa/ginkgolinter/gomegahandler"
+	"github.com/nunnatsa/ginkgolinter/interfaces"
 	"github.com/nunnatsa/ginkgolinter/reverseassertion"
 	"github.com/nunnatsa/ginkgolinter/types"
 	"github.com/nunnatsa/ginkgolinter/version"
@@ -40,8 +41,13 @@ const (
 	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"
+	matchErrorArgWrongType        = linterName + ": the MatchError matcher used to assert a non error type (%s)"
+	matchErrorWrongTypeAssertion  = linterName + ": MatchError first parameter (%s) must be error, string, GomegaMatcher or func(error)bool are allowed"
+	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)"
 )
+
 const ( // gomega matchers
 	beEmpty        = "BeEmpty"
 	beEquivalentTo = "BeEquivalentTo"
@@ -61,6 +67,7 @@ const ( // gomega matchers
 	and            = "And"
 	or             = "Or"
 	withTransform  = "WithTransform"
+	matchError     = "MatchError"
 )
 
 const ( // gomega actuals
@@ -133,6 +140,8 @@ currently, the linter searches for following:
 
 * 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.
 
@@ -179,6 +188,8 @@ func (l *ginkgoLinter) run(pass *analysis.Pass) (interface{}, error) {
 			continue
 		}
 
+		//gomegaMatcher = getMatcherInterface(pass, file)
+
 		ast.Inspect(file, func(n ast.Node) bool {
 			if ginkgoHndlr != nil && fileConfig.ForbidFocus {
 				spec, ok := n.(*ast.ValueSpec)
@@ -304,6 +315,8 @@ func checkExpression(pass *analysis.Pass, config types.Config, assertionExp *ast
 		}
 		return bool(config.SuppressCompare) || checkComparison(expr, pass, matcher, handler, first, second, op, oldExpr)
 
+	} else if checkMatchError(pass, assertionExp, actualArg, handler, oldExpr) {
+		return false
 	} else if isExprError(pass, actualArg) {
 		return bool(config.SuppressErr) || checkNilError(pass, expr, handler, actualArg, oldExpr)
 
@@ -318,6 +331,128 @@ func checkExpression(pass *analysis.Pass, config types.Config, assertionExp *ast
 	return true
 }
 
+func checkMatchError(pass *analysis.Pass, origExp *ast.CallExpr, actualArg ast.Expr, handler gomegahandler.Handler, oldExpr string) bool {
+	matcher, ok := origExp.Args[0].(*ast.CallExpr)
+	if !ok {
+		return false
+	}
+
+	return doCheckMatchError(pass, origExp, matcher, actualArg, handler, oldExpr)
+}
+
+func doCheckMatchError(pass *analysis.Pass, origExp *ast.CallExpr, matcher *ast.CallExpr, actualArg ast.Expr, handler gomegahandler.Handler, oldExpr string) bool {
+	if name, ok := handler.GetActualFuncName(matcher); ok {
+		switch name {
+		case matchError:
+		case not:
+			nested, ok := matcher.Args[0].(*ast.CallExpr)
+			if !ok {
+				return false
+			}
+
+			return doCheckMatchError(pass, origExp, nested, actualArg, handler, oldExpr)
+		case and, or:
+			res := true
+			for _, arg := range matcher.Args {
+				if nested, ok := arg.(*ast.CallExpr); ok {
+					if !doCheckMatchError(pass, origExp, nested, actualArg, handler, oldExpr) {
+						res = false
+					}
+				}
+			}
+			return res
+		default:
+			return false
+		}
+	}
+
+	if !isExprError(pass, actualArg) {
+		reportNoFix(pass, origExp.Pos(), matchErrorArgWrongType, goFmt(pass.Fset, actualArg))
+	}
+
+	expr := astcopy.CallExpr(matcher)
+
+	validAssertion, requiredParams := checkMatchErrorAssertion(pass, matcher)
+	if !validAssertion {
+		reportNoFix(pass, expr.Pos(), matchErrorWrongTypeAssertion, goFmt(pass.Fset, matcher.Args[0]))
+		return false
+	}
+
+	numParams := len(matcher.Args)
+	if numParams == requiredParams {
+		if numParams == 2 {
+			t := pass.TypesInfo.TypeOf(matcher.Args[1])
+			if !gotypes.Identical(t, gotypes.Typ[gotypes.String]) {
+				pass.Reportf(expr.Pos(), matchErrorNoFuncDescription)
+				return false
+			}
+		}
+		return true
+	}
+
+	if requiredParams == 2 && numParams == 1 {
+		reportNoFix(pass, expr.Pos(), matchErrorMissingDescription)
+		return false
+	}
+
+	var newArgsSuggestion = []ast.Expr{expr.Args[0]}
+	if requiredParams == 2 {
+		newArgsSuggestion = append(newArgsSuggestion, expr.Args[1])
+	}
+	expr.Args = newArgsSuggestion
+	report(pass, expr, matchErrorRedundantArg, oldExpr)
+	return false
+}
+
+func checkMatchErrorAssertion(pass *analysis.Pass, matcher *ast.CallExpr) (bool, int) {
+	if isErrorMatcherValidArg(pass, matcher.Args[0]) {
+		return true, 1
+	}
+
+	t1 := pass.TypesInfo.TypeOf(matcher.Args[0])
+	if isFuncErrBool(t1) {
+		return true, 2
+	}
+
+	return false, 0
+}
+
+// isFuncErrBool checks if a function is with the signature `func(error) bool`
+func isFuncErrBool(t gotypes.Type) bool {
+	sig, ok := t.(*gotypes.Signature)
+	if !ok {
+		return false
+	}
+	if sig.Params().Len() != 1 || sig.Results().Len() != 1 {
+		return false
+	}
+
+	if !interfaces.ImplementsError(sig.Params().At(0).Type()) {
+		return false
+	}
+
+	b, ok := sig.Results().At(0).Type().(*gotypes.Basic)
+	if ok && b.Name() == "bool" && b.Info() == gotypes.IsBoolean && b.Kind() == gotypes.Bool {
+		return true
+	}
+
+	return false
+}
+
+func isErrorMatcherValidArg(pass *analysis.Pass, arg ast.Expr) bool {
+	if isExprError(pass, arg) {
+		return true
+	}
+
+	if t, ok := pass.TypesInfo.TypeOf(arg).(*gotypes.Basic); ok && t.Kind() == gotypes.String {
+		return true
+	}
+
+	t := pass.TypesInfo.TypeOf(arg)
+
+	return interfaces.ImplementsGomegaMatcher(t)
+}
+
 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 {
@@ -1297,28 +1432,18 @@ func goFmt(fset *token.FileSet, x ast.Expr) string {
 	return b.String()
 }
 
-var errorType *gotypes.Interface
-
-func init() {
-	errorType = gotypes.Universe.Lookup("error").Type().Underlying().(*gotypes.Interface)
-}
-
-func isError(t gotypes.Type) bool {
-	return gotypes.Implements(t, errorType)
-}
-
 func isExprError(pass *analysis.Pass, expr ast.Expr) bool {
 	actualArgType := pass.TypesInfo.TypeOf(expr)
 	switch t := actualArgType.(type) {
 	case *gotypes.Named:
-		if isError(actualArgType) {
+		if interfaces.ImplementsError(actualArgType) {
 			return true
 		}
 	case *gotypes.Tuple:
 		if t.Len() > 0 {
 			switch t0 := t.At(0).Type().(type) {
 			case *gotypes.Named, *gotypes.Pointer:
-				if isError(t0) {
+				if interfaces.ImplementsError(t0) {
 					return true
 				}
 			}

ginkgo_linter_test.go

@@ -81,6 +81,10 @@ func TestAllUseCases(t *testing.T) {
 			testName: "equal with different type",
 			testData: "a/comparetypes",
 		},
+		{
+			testName: "MatchError",
+			testData: "a/matcherror",
+		},
 	} {
 		t.Run(tc.testName, func(tt *testing.T) {
 			analysistest.Run(tt, analysistest.TestData(), ginkgolinter.NewAnalyzer(), tc.testData)

go.mod

@@ -1,6 +1,6 @@
 module github.com/nunnatsa/ginkgolinter
 
-go 1.20
+go 1.21
 
 require (
 	github.com/go-toolsmith/astcopy v1.1.0

go.sum

@@ -12,6 +12,7 @@ golang.org/x/exp/typeparams v0.0.0-20230203172020-98cc5a0785f9/go.mod h1:AbB0pIl
 golang.org/x/mod v0.13.0 h1:I/DsJXRlw/8l/0c24sM9yb0T4z9liZTduXvdAWYiysY=
 golang.org/x/mod v0.13.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
 golang.org/x/sync v0.4.0 h1:zxkM55ReGkDlKSM+Fu41A+zmbZuaPVbGMzvvdUPznYQ=
+golang.org/x/sync v0.4.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y=
 golang.org/x/sys v0.13.0 h1:Af8nKPmuFypiUBjVoU9V20FiaFXOcuZI21p0ycVYYGE=
 golang.org/x/sys v0.13.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/tools v0.14.0 h1:jvNa2pY0M4r62jkRQ6RwEZZyPcymeL9XZMLBbV7U2nc=

gomegahandler/handler.go

@@ -5,6 +5,10 @@ import (
 	"go/token"
 )
 
+const (
+	importPath = `"github.com/onsi/gomega"`
+)
+
 // Handler provide different handling, depend on the way gomega was imported, whether
 // in imported with "." name, custom name or without any name.
 type Handler interface {
@@ -23,7 +27,7 @@ type Handler interface {
 // GetGomegaHandler returns a gomegar handler according to the way gomega was imported in the specific file
 func GetGomegaHandler(file *ast.File) Handler {
 	for _, imp := range file.Imports {
-		if imp.Path.Value != `"github.com/onsi/gomega"` {
+		if imp.Path.Value != importPath {
 			continue
 		}