diff --git a/README.md b/README.md index 267867d..31acc21 100644 --- a/README.md +++ b/README.md @@ -551,6 +551,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`function-result-limit`](./RULES_DESCRIPTIONS.md#function-result-limit) | int (defaults to 3)| Specifies the maximum number of results a function can return | no | no | | [`get-return`](./RULES_DESCRIPTIONS.md#get-return) | n/a | Warns on getters that do not yield any result | no | no | | [`identical-branches`](./RULES_DESCRIPTIONS.md#identical-branches) | n/a | Spots if-then-else statements with identical `then` and `else` branches | no | no | +| [`identical-switch-conditions`](./RULES_DESCRIPTIONS.md#identical-switch-conditions) | n/a | Spots identical conditions in case clauses of `switch` statements. | no | no | | [`if-return`](./RULES_DESCRIPTIONS.md#if-return) | n/a | Redundant if when returning an error. | no | no | | [`import-alias-naming`](./RULES_DESCRIPTIONS.md#import-alias-naming) | string or map[string]string (defaults to allow regex pattern `^[a-z][a-z0-9]{0,}$`) | Conventions around the naming of import aliases. | no | no | | [`import-shadowing`](./RULES_DESCRIPTIONS.md#import-shadowing) | n/a | Spots identifiers that shadow an import | no | no | diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 23d0ed5..a76be68 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -45,6 +45,7 @@ List of all available rules. - [function-result-limit](#function-result-limit) - [get-return](#get-return) - [identical-branches](#identical-branches) + - [identical-switch-conditions](#identical-switch-conditions) - [if-return](#if-return) - [import-alias-naming](#import-alias-naming) - [import-shadowing](#import-shadowing) @@ -733,6 +734,13 @@ _Description_: An `if-then-else` conditional with identical implementations in b _Configuration_: N/A +## identical-switch-conditions + +_Description_: a `switch` statement with cases with the same condition can lead to +unreachable code and is a potential source of bugs while making the code harder to read and maintain. + +_Configuration_: N/A + ## if-return _Description_: Checking if an error is _nil_ to just after return the error or nil is redundant. diff --git a/config/config.go b/config/config.go index cc86766..843739c 100644 --- a/config/config.go +++ b/config/config.go @@ -105,6 +105,7 @@ var allRules = append([]lint.Rule{ &rule.UnnecessaryFormatRule{}, &rule.UseFmtPrintRule{}, &rule.EnforceSwitchStyleRule{}, + &rule.IdenticalSwitchConditionsRule{}, }, defaultRules...) // allFormatters is a list of all available formatters to output the linting results. diff --git a/rule/identical_switch_conditions.go b/rule/identical_switch_conditions.go new file mode 100644 index 0000000..15826d9 --- /dev/null +++ b/rule/identical_switch_conditions.go @@ -0,0 +1,78 @@ +package rule + +import ( + "fmt" + "go/ast" + "go/token" + + "github.com/mgechev/revive/internal/astutils" + "github.com/mgechev/revive/lint" +) + +// IdenticalSwitchConditionsRule warns on switch case clauses with identical conditions. +type IdenticalSwitchConditionsRule struct{} + +// Apply applies the rule to given file. +func (*IdenticalSwitchConditionsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := &lintIdenticalSwitchConditions{toPosition: file.ToPosition, onFailure: onFailure} + for _, decl := range file.AST.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + + ast.Walk(w, fn.Body) + } + + return failures +} + +// Name returns the rule name. +func (*IdenticalSwitchConditionsRule) Name() string { + return "identical-switch-conditions" +} + +type lintIdenticalSwitchConditions struct { + toPosition func(token.Pos) token.Position + onFailure func(lint.Failure) +} + +func (w *lintIdenticalSwitchConditions) Visit(node ast.Node) ast.Visitor { + switchStmt, ok := node.(*ast.SwitchStmt) + if !ok { // not a switch statement, keep walking the AST + return w + } + + if switchStmt.Tag != nil { + return w // Not interested in tagged switches + } + + hashes := map[string]int{} // map hash(condition code) -> condition line + for _, cc := range switchStmt.Body.List { + caseClause := cc.(*ast.CaseClause) + caseClauseLine := w.toPosition(caseClause.Pos()).Line + for _, expr := range caseClause.List { + hash := astutils.NodeHash(expr) + if matchLine, ok := hashes[hash]; ok { + w.onFailure(lint.Failure{ + Confidence: 1.0, + Node: caseClause, + Category: lint.FailureCategoryLogic, + Failure: fmt.Sprintf(`case clause at line %d has the same condition`, matchLine), + }) + } + + hashes[hash] = caseClauseLine + } + + ast.Walk(w, caseClause) + } + + return nil // switch branches already analyzed +} diff --git a/test/identical_switch_conditions_test.go b/test/identical_switch_conditions_test.go new file mode 100644 index 0000000..6e36627 --- /dev/null +++ b/test/identical_switch_conditions_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestIdenticalSwitchConditions(t *testing.T) { + testRule(t, "identical_switch_conditions", &rule.IdenticalSwitchConditionsRule{}) +} diff --git a/testdata/identical_switch_conditions.go b/testdata/identical_switch_conditions.go new file mode 100644 index 0000000..c8f80e6 --- /dev/null +++ b/testdata/identical_switch_conditions.go @@ -0,0 +1,55 @@ +package fixtures + +func enforceSwitchStyle3() { + + switch expression { // skipt tagged switch + case value: + default: + } + + switch { + case a > 0, a < 0: + case a == 0: + case a < 0: // MATCH /case clause at line 11 has the same condition/ + default: + } + + switch { + case a > 0, a < 0, a > 0: // MATCH /case clause at line 18 has the same condition/ + case a == 0: + case a < 0: // MATCH /case clause at line 18 has the same condition/ + default: + } + + switch something { + case 1: + switch { + case a > 0, a < 0, a > 0: // MATCH /case clause at line 27 has the same condition/ + case a == 0: + } + default: + } + + switch { + case a == 0: + switch { + case a > 0, a < 0, a > 0: // MATCH /case clause at line 36 has the same condition/ + case a == 0: + } + default: + } + + switch { + case lnOpts.IsSocketOpts(): + // ... + // check for timeout + fallthrough + case lnOpts.IsTimeout(), lnOpts.IsSocketOpts(): // MATCH /case clause at line 43 has the same condition/ + // timeout listener with socket options. + // ... + case lnOpts.IsTimeout(): // MATCH /case clause at line 47 has the same condition/ + // ... + default: + // ... + } +}