new checker contains (#152)

* new checker `contains` Co-authored-by: ccoVeille <[email protected]> * contains: maintainer review --------- Co-authored-by: ccoVeille <[email protected]> Co-authored-by: Anton Telyshev <[email protected]>
Antonboom · Jun 26, 2024 · 8afa731 · 8afa731
1 parent cea8c9c
commit 8afa731
Show file tree

Hide file tree

Showing 11 changed files with 441 additions and 0 deletions.
diff --git a/README.md b/README.md
@@ -94,6 +94,7 @@ https://golangci-lint.run/usage/linters/#testifylint
 | [blank-import](#blank-import)                       | ✅                  | ❌       |
 | [bool-compare](#bool-compare)                       | ✅                  | ✅       |
 | [compares](#compares)                               | ✅                  | ✅       |
+| [contains](#contains)                               | ✅                  | ✅       |
 | [empty](#empty)                                     | ✅                  | ✅       |
 | [error-is-as](#error-is-as)                         | ✅                  | 🤏      |
 | [error-nil](#error-nil)                             | ✅                  | ✅       |
@@ -216,6 +217,27 @@ due to the inappropriate recursive nature of `assert.Equal` (based on
 
 ---
 
+### contains
+
+```go
+❌
+assert.True(t, strings.Contains(a, "abc123"))
+assert.False(t, !strings.Contains(a, "abc123"))
+
+assert.False(t, strings.Contains(a, "abc123"))
+assert.True(t, !strings.Contains(a, "abc123"))
+
+✅
+assert.Contains(t, a, "abc123")
+assert.NotContains(t, a, "abc123")
+```
+
+**Autofix**: true. <br>
+**Enabled by default**: true. <br>
+**Reason**: Code simplification and more appropriate `testify` API with clearer failure message.
+
+---
+
 ### empty
 
 ```go

diff --git a/analyzer/checkers_factory_test.go b/analyzer/checkers_factory_test.go
@@ -21,6 +21,7 @@ func Test_newCheckers(t *testing.T) {
 		checkers.NewLen(),
 		checkers.NewNegativePositive(),
 		checkers.NewCompares(),
+		checkers.NewContains(),
 		checkers.NewErrorNil(),
 		checkers.NewNilCompare(),
 		checkers.NewErrorIsAs(),
@@ -38,6 +39,7 @@ func Test_newCheckers(t *testing.T) {
 		checkers.NewLen(),
 		checkers.NewNegativePositive(),
 		checkers.NewCompares(),
+		checkers.NewContains(),
 		checkers.NewErrorNil(),
 		checkers.NewNilCompare(),
 		checkers.NewErrorIsAs(),

diff --git a/analyzer/testdata/src/checkers-default/contains/contains_test.go b/analyzer/testdata/src/checkers-default/contains/contains_test.go
diff --git a/analyzer/testdata/src/checkers-default/contains/contains_test.go.golden b/analyzer/testdata/src/checkers-default/contains/contains_test.go.golden
@@ -0,0 +1,66 @@
+// Code generated by testifylint/internal/testgen. DO NOT EDIT.
+
+package contains
+
+import (
+	"bytes"
+	"errors"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestContainsChecker(t *testing.T) {
+	var (
+		s           = "abc123"
+		b           = []byte(s)
+		errSentinel = errors.New("user not found")
+	)
+
+	// Invalid.
+	{
+		assert.Contains(t, s, "abc123")                                              // want "contains: use assert\\.Contains"
+		assert.Containsf(t, s, "abc123", "msg with args %d %s", 42, "42")            // want "contains: use assert\\.Containsf"
+		assert.Contains(t, string(b), "abc123")                                      // want "contains: use assert\\.Contains"
+		assert.Containsf(t, string(b), "abc123", "msg with args %d %s", 42, "42")    // want "contains: use assert\\.Containsf"
+		assert.Contains(t, s, "abc123")                                              // want "contains: use assert\\.Contains"
+		assert.Containsf(t, s, "abc123", "msg with args %d %s", 42, "42")            // want "contains: use assert\\.Containsf"
+		assert.Contains(t, string(b), "abc123")                                      // want "contains: use assert\\.Contains"
+		assert.Containsf(t, string(b), "abc123", "msg with args %d %s", 42, "42")    // want "contains: use assert\\.Containsf"
+		assert.NotContains(t, s, "abc123")                                           // want "contains: use assert\\.NotContains"
+		assert.NotContainsf(t, s, "abc123", "msg with args %d %s", 42, "42")         // want "contains: use assert\\.NotContainsf"
+		assert.NotContains(t, string(b), "abc123")                                   // want "contains: use assert\\.NotContains"
+		assert.NotContainsf(t, string(b), "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf"
+		assert.NotContains(t, s, "abc123")                                           // want "contains: use assert\\.NotContains"
+		assert.NotContainsf(t, s, "abc123", "msg with args %d %s", 42, "42")         // want "contains: use assert\\.NotContainsf"
+		assert.NotContains(t, string(b), "abc123")                                   // want "contains: use assert\\.NotContains"
+		assert.NotContainsf(t, string(b), "abc123", "msg with args %d %s", 42, "42") // want "contains: use assert\\.NotContainsf"
+	}
+
+	// Valid.
+	{
+		assert.Contains(t, s, "abc123")
+		assert.Containsf(t, s, "abc123", "msg with args %d %s", 42, "42")
+		assert.Contains(t, string(b), "abc123")
+		assert.Containsf(t, string(b), "abc123", "msg with args %d %s", 42, "42")
+		assert.NotContains(t, s, "abc123")
+		assert.NotContainsf(t, s, "abc123", "msg with args %d %s", 42, "42")
+		assert.NotContains(t, string(b), "abc123")
+		assert.NotContainsf(t, string(b), "abc123", "msg with args %d %s", 42, "42")
+	}
+
+	// Ignored.
+	{
+		assert.Contains(t, errSentinel.Error(), "user")
+		assert.Containsf(t, errSentinel.Error(), "user", "msg with args %d %s", 42, "42")
+		assert.True(t, bytes.Contains(b, []byte("a")))
+		assert.Truef(t, bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42")
+		assert.False(t, !bytes.Contains(b, []byte("a")))
+		assert.Falsef(t, !bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42")
+		assert.False(t, bytes.Contains(b, []byte("a")))
+		assert.Falsef(t, bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42")
+		assert.True(t, !bytes.Contains(b, []byte("a")))
+		assert.Truef(t, !bytes.Contains(b, []byte("a")), "msg with args %d %s", 42, "42")
+	}
+}
diff --git a/analyzer/testdata/src/debug/contains_replacements_test.go b/analyzer/testdata/src/debug/contains_replacements_test.go
@@ -0,0 +1,70 @@
+package debug
+
+import (
+	"bytes"
+	"fmt"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestContainsNotContainsReplacements_String(t *testing.T) {
+	var tm tMock
+
+	const substr = "abc123"
+
+	for _, s := range []string{"", "random", substr + substr} {
+		t.Run(s, func(t *testing.T) {
+			assert.Equal(t,
+				assert.True(tm, strings.Contains(s, substr)),
+				assert.Contains(tm, s, substr),
+			)
+
+			assert.Equal(t,
+				assert.False(tm, !strings.Contains(s, substr)),
+				assert.Contains(tm, s, substr),
+			)
+
+			assert.Equal(t,
+				assert.False(tm, strings.Contains(s, substr)),
+				assert.NotContains(tm, s, substr),
+			)
+
+			assert.Equal(t,
+				assert.True(tm, !strings.Contains(s, substr)),
+				assert.NotContains(tm, s, substr),
+			)
+		})
+	}
+}
+
+func TestContainsNotContainsReplacements_Bytes(t *testing.T) {
+	var tm tMock
+
+	subbytes := []byte("abc123")
+
+	for _, s := range [][]byte{nil, []byte("random"), append(subbytes, subbytes...)} {
+		t.Run(fmt.Sprintf("[]byte(%s)", s), func(t *testing.T) {
+			assert.Equal(t,
+				assert.True(tm, bytes.Contains(s, subbytes)),
+				assert.Contains(tm, s, subbytes),
+			)
+
+			assert.Equal(t,
+				assert.False(tm, !bytes.Contains(s, subbytes)),
+				assert.Contains(tm, s, subbytes),
+			)
+
+			assert.Equal(t,
+				assert.False(tm, bytes.Contains(s, subbytes)),
+				assert.NotContains(tm, s, subbytes),
+			)
+
+			assert.Equal(t,
+				assert.True(tm, !bytes.Contains(s, subbytes)),
+				assert.NotContains(tm, s, subbytes),
+			)
+		})
+	}
+}
diff --git a/internal/checkers/checkers_registry.go b/internal/checkers/checkers_registry.go
@@ -13,6 +13,7 @@ var registry = checkersRegistry{
 	{factory: asCheckerFactory(NewLen), enabledByDefault: true},
 	{factory: asCheckerFactory(NewNegativePositive), enabledByDefault: true},
 	{factory: asCheckerFactory(NewCompares), enabledByDefault: true},
+	{factory: asCheckerFactory(NewContains), enabledByDefault: true},
 	{factory: asCheckerFactory(NewErrorNil), enabledByDefault: true},
 	{factory: asCheckerFactory(NewNilCompare), enabledByDefault: true},
 	{factory: asCheckerFactory(NewErrorIsAs), enabledByDefault: true},

diff --git a/internal/checkers/checkers_registry_test.go b/internal/checkers/checkers_registry_test.go
@@ -41,6 +41,7 @@ func TestAll(t *testing.T) {
 		"len",
 		"negative-positive",
 		"compares",
+		"contains",
 		"error-nil",
 		"nil-compare",
 		"error-is-as",
@@ -76,6 +77,7 @@ func TestEnabledByDefault(t *testing.T) {
 		"len",
 		"negative-positive",
 		"compares",
+		"contains",
 		"error-nil",
 		"nil-compare",
 		"error-is-as",

diff --git a/internal/checkers/contains.go b/internal/checkers/contains.go
@@ -0,0 +1,72 @@
+package checkers
+
+import (
+	"go/ast"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+// Contains detects situations like
+//
+//	assert.True(t, strings.Contains(a, "abc123"))
+//	assert.False(t, !strings.Contains(a, "abc123"))
+//
+//	assert.False(t, strings.Contains(a, "abc123"))
+//	assert.True(t, !strings.Contains(a, "abc123"))
+//
+// and requires
+//
+//	assert.Contains(t, a, "abc123")
+//	assert.NotContains(t, a, "abc123")
+type Contains struct{}
+
+// NewContains constructs Contains checker.
+func NewContains() Contains   { return Contains{} }
+func (Contains) Name() string { return "contains" }
+
+func (checker Contains) Check(pass *analysis.Pass, call *CallMeta) *analysis.Diagnostic {
+	if len(call.Args) < 1 {
+		return nil
+	}
+
+	expr := call.Args[0]
+	unpacked, isNeg := isNegation(expr)
+	if isNeg {
+		expr = unpacked
+	}
+
+	ce, ok := expr.(*ast.CallExpr)
+	if !ok || len(ce.Args) != 2 {
+		return nil
+	}
+
+	if !isStringsContainsCall(pass, ce) {
+		return nil
+	}
+
+	var proposed string
+	switch call.Fn.NameFTrimmed {
+	default:
+		return nil
+
+	case "True":
+		proposed = "Contains"
+		if isNeg {
+			proposed = "NotContains"
+		}
+
+	case "False":
+		proposed = "NotContains"
+		if isNeg {
+			proposed = "Contains"
+		}
+	}
+
+	return newUseFunctionDiagnostic(checker.Name(), call, proposed,
+		newSuggestedFuncReplacement(call, proposed, analysis.TextEdit{
+			Pos:     call.Args[0].Pos(),
+			End:     call.Args[0].End(),
+			NewText: formatAsCallArgs(pass, ce.Args[0], ce.Args[1]),
+		}),
+	)
+}
diff --git a/internal/checkers/helpers_pkg_func.go b/internal/checkers/helpers_pkg_func.go
@@ -12,6 +12,10 @@ func isRegexpMustCompileCall(pass *analysis.Pass, ce *ast.CallExpr) bool {
 	return isPkgFnCall(pass, ce, "regexp", "MustCompile")
 }
 
+func isStringsContainsCall(pass *analysis.Pass, ce *ast.CallExpr) bool {
+	return isPkgFnCall(pass, ce, "strings", "Contains")
+}
+
 func isPkgFnCall(pass *analysis.Pass, ce *ast.CallExpr, pkg, fn string) bool {
 	se, ok := ce.Fun.(*ast.SelectorExpr)
 	if !ok {