feature: new rule identical-ifelseif-conditions (#1454)

README.md

@@ -552,6 +552,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-ifelseif-conditions`](./RULES_DESCRIPTIONS.md#identical-ifelseif-conditions)          |  n/a   | Spots identical conditions in  `if ... else if` chains.       |    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   |

RULES_DESCRIPTIONS.md

@@ -46,6 +46,7 @@ List of all available rules.
   - [function-result-limit](#function-result-limit)
   - [get-return](#get-return)
   - [identical-branches](#identical-branches)
+  - [identical-ifelseif-conditions](#identical-ifelseif-conditions)
   - [identical-switch-conditions](#identical-switch-conditions)
   - [if-return](#if-return)
   - [import-alias-naming](#import-alias-naming)
@@ -743,6 +744,13 @@ _Description_: An `if-then-else` conditional with identical implementations in b
 
 _Configuration_: N/A
 
+## identical-ifelseif-conditions
+
+_Description_: an `if ... else if` chain  with identical conditions can lead to
+unreachable code and is a potential source of bugs while making the code harder to read and maintain.
+
+_Configuration_: N/A
+
 ## identical-switch-conditions
 
 _Description_: a `switch` statement with cases with the same condition can lead to

config/config.go

@@ -107,6 +107,7 @@ var allRules = append([]lint.Rule{
 	&rule.EnforceSwitchStyleRule{},
 	&rule.IdenticalSwitchConditionsRule{},
 	&rule.EnforceElseRule{},
+	&rule.IdenticalIfElseIfConditionsRule{},
 }, defaultRules...)
 
 // allFormatters is a list of all available formatters to output the linting results.

rule/identical_ifelseif_condition.go

@@ -0,0 +1,148 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+
+	"github.com/mgechev/revive/internal/astutils"
+	"github.com/mgechev/revive/lint"
+)
+
+// IdenticalIfElseIfConditionsRule warns on if...else if chains with identical conditions.
+type IdenticalIfElseIfConditionsRule struct{}
+
+// Apply applies the rule to given file.
+func (*IdenticalIfElseIfConditionsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	getStmtLine := func(s ast.Stmt) int {
+		return file.ToPosition(s.Pos()).Line
+	}
+
+	w := &rootWalkerIfElseIfIdenticalConditions{getStmtLine: getStmtLine, 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 (*IdenticalIfElseIfConditionsRule) Name() string {
+	return "identical-ifelseif-conditions"
+}
+
+type rootWalkerIfElseIfIdenticalConditions struct {
+	getStmtLine func(ast.Stmt) int
+	onFailure   func(lint.Failure)
+}
+
+func (w *rootWalkerIfElseIfIdenticalConditions) Visit(node ast.Node) ast.Visitor {
+	n, ok := node.(*ast.IfStmt)
+	if !ok {
+		return w
+	}
+
+	_, isIfElseIf := n.Else.(*ast.IfStmt)
+	if isIfElseIf {
+		walker := &lintIfChainIdenticalConditions{
+			onFailure:   w.onFailure,
+			getStmtLine: w.getStmtLine,
+			rootWalker:  w,
+		}
+
+		ast.Walk(walker, n)
+		return nil // the walker already analyzed inner branches
+	}
+
+	return w
+}
+
+// walkBranch analyzes the given branch.
+func (w *rootWalkerIfElseIfIdenticalConditions) walkBranch(branch ast.Stmt) {
+	if branch == nil {
+		return
+	}
+
+	walker := &rootWalkerIfElseIfIdenticalConditions{
+		onFailure:   w.onFailure,
+		getStmtLine: w.getStmtLine,
+	}
+
+	ast.Walk(walker, branch)
+}
+
+type lintIfChainIdenticalConditions struct {
+	getStmtLine func(ast.Stmt) int
+	onFailure   func(lint.Failure)
+	conditions  map[string]int                         // condition hash -> line of the condition
+	rootWalker  *rootWalkerIfElseIfIdenticalConditions // the walker to use to recursively analyze inner branches
+}
+
+// addCondition adds a condition to the set of if...else if conditions.
+// If the set already contains the same condition it returns the line number of the identical condition.
+func (w *lintIfChainIdenticalConditions) addCondition(condition ast.Expr, conditionLine int) (line int, match bool) {
+	if condition == nil {
+		return 0, false
+	}
+
+	if w.conditions == nil {
+		w.resetConditions()
+	}
+
+	hash := astutils.NodeHash(condition)
+	identical, ok := w.conditions[hash]
+	if ok {
+		return identical, true
+	}
+
+	w.conditions[hash] = conditionLine
+	return 0, false
+}
+
+func (w *lintIfChainIdenticalConditions) resetConditions() {
+	w.conditions = map[string]int{}
+}
+
+func (w *lintIfChainIdenticalConditions) Visit(node ast.Node) ast.Visitor {
+	n, ok := node.(*ast.IfStmt)
+	if !ok {
+		return w
+	}
+
+	// recursively analyze the then-branch
+	w.rootWalker.walkBranch(n.Body)
+
+	if n.Init == nil { // only check if without initialization to avoid false positives
+		currentCondLine := w.rootWalker.getStmtLine(n)
+		identicalCondLine, match := w.addCondition(n.Cond, currentCondLine)
+		if match {
+			w.onFailure(lint.Failure{
+				Confidence: 1.0,
+				Node:       n,
+				Category:   lint.FailureCategoryLogic,
+				Failure:    fmt.Sprintf(`"if...else if" chain with identical conditions (lines %d and %d)`, identicalCondLine, currentCondLine),
+			})
+		}
+	}
+
+	if n.Else != nil {
+		if chainedIf, ok := n.Else.(*ast.IfStmt); ok {
+			w.Visit(chainedIf)
+		} else {
+			w.rootWalker.walkBranch(n.Else)
+		}
+	}
+
+	w.resetConditions()
+	return nil
+}

test/identical_ifelseif_conditions_test.go

@@ -0,0 +1,11 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/rule"
+)
+
+func TestIdenticalIfElseIfConditions(t *testing.T) {
+	testRule(t, "identical_ifelseif_conditions", &rule.IdenticalIfElseIfConditionsRule{})
+}

testdata/identical_ifelseif_conditions.go

@@ -0,0 +1,53 @@
+package fixtures
+
+func identicalBranches() {
+	// no failure for nested ifs
+	if true {
+		if true {
+		}
+	} else {
+		if true {
+		}
+	}
+
+	// single failure
+	if a > 0 {
+		print("something")
+	} else if a < 0 {
+		print("something else")
+	} else if a == 0 {
+		print("other thing")
+	} else if a > 0 { // MATCH /"if...else if" chain with identical conditions (lines 14 and 20)/
+		println()
+	} else {
+		print("something")
+	}
+
+	// multiple failures in the same if...else if chain
+	if a > 0 {
+		print("something")
+	} else if a < 0 {
+		print("something else")
+	} else if a == 0 {
+		print("other thing")
+	} else if a > 0 { // MATCH /"if...else if" chain with identical conditions (lines 27 and 33)/
+		println()
+	} else if a == 0 { // MATCH /"if...else if" chain with identical conditions (lines 31 and 35)/
+		print("other thing")
+	} else {
+		print("something")
+	}
+
+	// failures in nested if...else if
+	if true {
+		if false {
+		} else if false { // MATCH /"if...else if" chain with identical conditions (lines 43 and 44)/
+		}
+	} else if foo() {
+
+	} else {
+		if false {
+		} else if false { // MATCH /"if...else if" chain with identical conditions (lines 49 and 50)/
+		}
+	}
+}