diff --git a/README.md b/README.md index 74009f0b..ab9230b8 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,7 @@ https://golangci-lint.run/docs/linters/configuration/#testifylint | [negative-positive](#negative-positive) | ✅ | ✅ | | [nil-compare](#nil-compare) | ✅ | ✅ | | [regexp](#regexp) | ✅ | ✅ | +| [redundant-error-assertion](#redundant-error-assertion) | ✅ | ✅ | | [require-error](#require-error) | ✅ | ❌ | | [suite-broken-parallel](#suite-broken-parallel) | ✅ | ✅ | | [suite-dont-use-pkg](#suite-dont-use-pkg) | ✅ | ✅ | @@ -997,6 +998,35 @@ assert.NotRegexp(t, `\[.*\] TRACE message`, out) --- +### redundant-error-assertion + +```go +❌ +assert.Error(t, err) +assert.ErrorContains(t, err, "not found") + +assert.Error(t, err) +assert.ErrorIs(t, err, io.EOF) + +assert.Error(t, err) +assert.ErrorAs(t, err, &target) + +assert.Error(t, err) +assert.EqualError(t, err, "end of file") + +✅ +assert.ErrorContains(t, err, "not found") +assert.ErrorIs(t, err, io.EOF) +assert.ErrorAs(t, err, &target) +assert.EqualError(t, err, "end of file") +``` + +**Autofix**: true.
+**Enabled by default**: true.
+**Reason**: Code simplification. + +--- + ### require-error ```go diff --git a/analyzer/checkers_factory_test.go b/analyzer/checkers_factory_test.go index ef45c4c9..fa17760e 100644 --- a/analyzer/checkers_factory_test.go +++ b/analyzer/checkers_factory_test.go @@ -61,6 +61,7 @@ func Test_newCheckers(t *testing.T) { } enabledByDefaultAdvancedCheckers := []checkers.AdvancedChecker{ + checkers.NewRedundantErrorAssertion(), checkers.NewBlankImport(), checkers.NewGoRequire(), checkers.NewMockExpect(), @@ -70,6 +71,7 @@ func Test_newCheckers(t *testing.T) { checkers.NewSuiteSubtestRun(), } allAdvancedCheckers := []checkers.AdvancedChecker{ + checkers.NewRedundantErrorAssertion(), checkers.NewBlankImport(), checkers.NewGoRequire(), checkers.NewMockExpect(), diff --git a/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go b/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go new file mode 100644 index 00000000..b6596bb2 --- /dev/null +++ b/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go @@ -0,0 +1,115 @@ +package redundanterrorassertion + +import ( + "errors" + "io" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRedundantErrorAssertion(t *testing.T) { + t.Parallel() + + var ( + errA = errors.New("err A") + errB = errors.New("err B") + target *os.PathError + ) + + assObj, reqObj := assert.New(t), require.New(t) + + // Invalid. + { + assert.Error(t, errA) // want "redundant-error-assertion: remove assert\\.Error: assert\\.ErrorContains already asserts that the error is not nil" + assert.ErrorContains(t, errA, "err") + } + + { + require.Error(t, errA) // want "redundant-error-assertion: remove require\\.Error: assert\\.ErrorContains already asserts that the error is not nil" + assert.ErrorContains(t, errA, "err") + } + + { + require.Error(t, errA) // want "redundant-error-assertion: remove require\\.Error: require\\.ErrorContains already asserts that the error is not nil" + require.ErrorContains(t, errA, "err") + } + + { + assert.Error(t, errA) // want "redundant-error-assertion: remove assert\\.Error: require\\.ErrorContains already asserts that the error is not nil" + require.ErrorContains(t, errA, "err") + } + + { + assert.EqualError(t, errA, "err A") + assert.Error(t, errA) // want "redundant-error-assertion: remove assert\\.Error: assert\\.EqualError already asserts that the error is not nil" + } + + { + require.EqualError(t, errA, "err A") + require.Error(t, errA) // want "redundant-error-assertion: remove require\\.Error: require\\.EqualError already asserts that the error is not nil" + } + + { + assObj.Error(errA) // want "redundant-error-assertion: remove assObj\\.Error: assObj\\.ErrorAs already asserts that the error is not nil" + assObj.ErrorAs(errA, &target) + } + + { + reqObj.Error(errA) // want "redundant-error-assertion: remove reqObj\\.Error: reqObj\\.ErrorAs already asserts that the error is not nil" + reqObj.ErrorAs(errA, &target) + } + + { + assert.Errorf(t, errA, "msg") // want "redundant-error-assertion: remove assert\\.Errorf: reqObj\\.ErrorAs already asserts that the error is not nil" + reqObj.ErrorAs(errA, &target) + } + + { + require.Errorf(t, errA, "msg") // want "redundant-error-assertion: remove require\\.Errorf: reqObj\\.ErrorAs already asserts that the error is not nil" + reqObj.ErrorAs(errA, &target) + } + + { + require.Errorf(t, errA, "msg") // want "redundant-error-assertion: remove require\\.Errorf: require\\.ErrorIs already asserts that the error is not nil" + require.ErrorIs(t, errA, io.EOF) + } + + // Valid. + { + { + assert.Error(t, errA) + assert.ErrorContains(t, errB, "err") + } + + { + require.Error(t, errA) + assert.ErrorContains(t, errB, "err") + } + + { + assert.Error(t, errA) + } + + if assert.Error(t, errA) { + assert.ErrorContains(t, errA, "err") + } + + { + { + assert.Error(t, errA) + } + assert.ErrorContains(t, errA, "err") + } + + { + errA = errB + assert.Error(t, errA) + + errA = io.EOF + assert.ErrorContains(t, errA, "EOF") + } + } +} diff --git a/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go.golden b/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go.golden new file mode 100644 index 00000000..4fdfa2f1 --- /dev/null +++ b/analyzer/testdata/src/checkers-default/redundant-error-assertion/redundant_error_assertion_test.go.golden @@ -0,0 +1,104 @@ +package redundanterrorassertion + +import ( + "errors" + "io" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRedundantErrorAssertion(t *testing.T) { + t.Parallel() + + var ( + errA = errors.New("err A") + errB = errors.New("err B") + target *os.PathError + ) + + assObj, reqObj := assert.New(t), require.New(t) + + // Invalid. + { + assert.ErrorContains(t, errA, "err") + } + + { + assert.ErrorContains(t, errA, "err") + } + + { + require.ErrorContains(t, errA, "err") + } + + { + require.ErrorContains(t, errA, "err") + } + + { + assert.EqualError(t, errA, "err A") + } + + { + require.EqualError(t, errA, "err A") + } + + { + assObj.ErrorAs(errA, &target) + } + + { + reqObj.ErrorAs(errA, &target) + } + + { + reqObj.ErrorAs(errA, &target) + } + + { + reqObj.ErrorAs(errA, &target) + } + + { + require.ErrorIs(t, errA, io.EOF) + } + + // Valid. + { + { + assert.Error(t, errA) + assert.ErrorContains(t, errB, "err") + } + + { + require.Error(t, errA) + assert.ErrorContains(t, errB, "err") + } + + { + assert.Error(t, errA) + } + + if assert.Error(t, errA) { + assert.ErrorContains(t, errA, "err") + } + + { + { + assert.Error(t, errA) + } + assert.ErrorContains(t, errA, "err") + } + + { + errA = errB + assert.Error(t, errA) + + errA = io.EOF + assert.ErrorContains(t, errA, "EOF") + } + } +} diff --git a/internal/checkers/checkers_registry.go b/internal/checkers/checkers_registry.go index f942754d..8306d0a1 100644 --- a/internal/checkers/checkers_registry.go +++ b/internal/checkers/checkers_registry.go @@ -26,6 +26,7 @@ var registry = checkersRegistry{ {factory: asCheckerFactory(NewSuiteExtraAssertCall), enabledByDefault: true}, {factory: asCheckerFactory(NewSuiteDontUsePkg), enabledByDefault: true}, {factory: asCheckerFactory(NewUselessAssert), enabledByDefault: true}, + {factory: asCheckerFactory(NewRedundantErrorAssertion), enabledByDefault: true}, {factory: asCheckerFactory(NewFormatter), enabledByDefault: true}, // Advanced checkers. {factory: asCheckerFactory(NewBlankImport), enabledByDefault: true}, diff --git a/internal/checkers/checkers_registry_test.go b/internal/checkers/checkers_registry_test.go index 527a7114..621a9ba8 100644 --- a/internal/checkers/checkers_registry_test.go +++ b/internal/checkers/checkers_registry_test.go @@ -54,6 +54,7 @@ func TestAll(t *testing.T) { "suite-extra-assert-call", "suite-dont-use-pkg", "useless-assert", + "redundant-error-assertion", "formatter", "blank-import", "go-require", @@ -96,6 +97,7 @@ func TestEnabledByDefault(t *testing.T) { "suite-extra-assert-call", "suite-dont-use-pkg", "useless-assert", + "redundant-error-assertion", "formatter", "blank-import", "go-require", diff --git a/internal/checkers/redundant_assert.go b/internal/checkers/redundant_assert.go new file mode 100644 index 00000000..a2677078 --- /dev/null +++ b/internal/checkers/redundant_assert.go @@ -0,0 +1,240 @@ +package checkers + +import ( + "fmt" + "go/ast" + "go/token" + "go/types" + "math" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/ast/inspector" + + "github.com/Antonboom/testifylint/internal/analysisutil" +) + +// RedundantErrorAssertion detects redundant error assertions when a stronger error assertion +// on the same error value is present in the same block. +type RedundantErrorAssertion struct{} + +// NewRedundantErrorAssertion constructs RedundantErrorAssertion checker. +func NewRedundantErrorAssertion() RedundantErrorAssertion { return RedundantErrorAssertion{} } +func (RedundantErrorAssertion) Name() string { return "redundant-error-assertion" } + +func (checker RedundantErrorAssertion) Check(pass *analysis.Pass, insp *inspector.Inspector) []analysis.Diagnostic { + callsByBlock := make(map[*ast.BlockStmt][]redundantAssertCallMeta) + + insp.WithStack([]ast.Node{(*ast.CallExpr)(nil)}, func(node ast.Node, push bool, stack []ast.Node) bool { + if !push { + return false + } + + callExpr := node.(*ast.CallExpr) + testifyCall := NewCallMeta(pass, callExpr) + if testifyCall == nil { + return true + } + + parentBlock := findNearestNode[*ast.BlockStmt](stack) + if parentBlock == nil { + return true + } + + parentStmt := findNearestNode[*ast.ExprStmt](stack) + if parentStmt == nil || parentStmt.X != callExpr { + return true + } + + callsByBlock[parentBlock] = append(callsByBlock[parentBlock], redundantAssertCallMeta{ + call: callExpr, + testifyCall: testifyCall, + parentStmt: parentStmt, + parentBlock: parentBlock, + }) + return true + }) + + diagnostics := make([]analysis.Diagnostic, 0) + + for block, calls := range callsByBlock { + for currIdx, curr := range calls { + if curr.testifyCall.Fn.NameFTrimmed != "Error" { + continue + } + if len(curr.testifyCall.Args) < 1 { + continue + } + + currErr := analysisutil.NodeString(pass.Fset, curr.testifyCall.Args[0]) + if currErr == "" { + continue + } + + closestStrongerFn := "" + closestDistance := math.MaxInt + + for otherIdx, other := range calls { + if curr.call == other.call || len(other.testifyCall.Args) < 1 { + continue + } + + otherFn := other.testifyCall.Fn.NameFTrimmed + switch otherFn { + default: + continue + case "ErrorContains", "ErrorIs", "ErrorAs", "EqualError": + } + + otherErr := analysisutil.NodeString(pass.Fset, other.testifyCall.Args[0]) + if currErr != otherErr { + continue + } + + fromStmt, toStmt := curr.parentStmt, other.parentStmt + if fromStmt.Pos() > toStmt.Pos() { + fromStmt, toStmt = toStmt, fromStmt + } + if isReassignedBetween(block, curr.testifyCall.Args[0], fromStmt, toStmt, pass.Fset, pass.TypesInfo) { + continue + } + + distance := absInt(currIdx - otherIdx) + if distance >= closestDistance { + continue + } + closestDistance = distance + closestStrongerFn = other.testifyCall.String() + } + + if closestStrongerFn != "" { + removedCall := curr.testifyCall.String() + diagnostics = append(diagnostics, *newDiagnostic( + checker.Name(), + curr.testifyCall, + fmt.Sprintf("remove %s: %s already asserts that the error is not nil", removedCall, closestStrongerFn), + analysis.SuggestedFix{ + Message: "Remove redundant error assertion", + TextEdits: []analysis.TextEdit{{ + Pos: curr.parentStmt.Pos(), + End: toStmtLineEnd(pass, curr.parentStmt.End()), + NewText: []byte(""), + }}, + }, + )) + } + } + } + + return diagnostics +} + +type redundantAssertCallMeta struct { + call *ast.CallExpr + testifyCall *CallMeta + parentStmt *ast.ExprStmt + parentBlock *ast.BlockStmt +} + +func isReassignedBetween( + block *ast.BlockStmt, + errExpr ast.Expr, + fromStmt *ast.ExprStmt, + toStmt *ast.ExprStmt, + fset *token.FileSet, + info *types.Info, +) bool { + if block == nil || errExpr == nil || fromStmt == nil || toStmt == nil { + return false + } + + var ( + errObj = referencedObject(errExpr, info) + errStr string + ) + if errObj == nil { + errStr = analysisutil.NodeString(fset, errExpr) + if errStr == "" { + return false + } + } + + fromPos, toPos := fromStmt.Pos(), toStmt.Pos() + if fromPos > toPos { + fromPos, toPos = toPos, fromPos + } + + for _, stmt := range block.List { + if stmt.Pos() <= fromPos || stmt.Pos() >= toPos { + continue + } + + reassigned := false + ast.Inspect(stmt, func(node ast.Node) bool { + assignStmt, ok := node.(*ast.AssignStmt) + if !ok { + return true + } + + for _, lhs := range assignStmt.Lhs { + if sameErrTarget(lhs, errObj, errStr, fset, info) { + reassigned = true + return false + } + } + return true + }) + if reassigned { + return true + } + } + + return false +} + +func sameErrTarget(lhs ast.Expr, errObj types.Object, errStr string, fset *token.FileSet, info *types.Info) bool { + if errObj != nil { + if lhsObj := referencedObject(lhs, info); lhsObj != nil && lhsObj == errObj { + return true + } + } + + return errStr != "" && analysisutil.NodeString(fset, lhs) == errStr +} + +func referencedObject(expr ast.Expr, info *types.Info) types.Object { + if info == nil { + return nil + } + + switch expr := expr.(type) { + case *ast.Ident: + return info.ObjectOf(expr) + case *ast.SelectorExpr: + if sel, ok := info.Selections[expr]; ok && sel != nil { + return sel.Obj() + } + return info.ObjectOf(expr.Sel) + default: + return nil + } +} + +func absInt(v int) int { + if v < 0 { + return -v + } + return v +} + +func toStmtLineEnd(pass *analysis.Pass, pos token.Pos) token.Pos { + file := pass.Fset.File(pos) + if file == nil { + return pos + } + + line := file.Line(pos) + if line >= file.LineCount() { + return pos + } + return file.LineStart(line + 1) +}