mirror of
				https://github.com/mgechev/revive.git
				synced 2025-10-30 23:37:49 +02:00 
			
		
		
		
	feature: new rules identical-switch-branches and identical-ifelseif-branches
				
					
				
			These new rules result from the refactoring of `identical-branches` that will cover only simple/single if..else statements.
This commit is contained in:
		| @@ -552,7 +552,9 @@ 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-branches`](./RULES_DESCRIPTIONS.md#identical-ifelseif-branches)          |  n/a   | Spots `if ... else if` chains with identical 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-branches`](./RULES_DESCRIPTIONS.md#identical-switch-branches)          |  n/a   | Spots `switch` with identical 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   | | ||||
|   | ||||
| @@ -46,7 +46,9 @@ List of all available rules. | ||||
|   - [function-result-limit](#function-result-limit) | ||||
|   - [get-return](#get-return) | ||||
|   - [identical-branches](#identical-branches) | ||||
|   - [identical-ifelseif-branches](#identical-ifelseif-branches) | ||||
|   - [identical-ifelseif-conditions](#identical-ifelseif-conditions) | ||||
|   - [identical-switch-branches](#identical-switch-branches) | ||||
|   - [identical-switch-conditions](#identical-switch-conditions) | ||||
|   - [if-return](#if-return) | ||||
|   - [import-alias-naming](#import-alias-naming) | ||||
| @@ -744,6 +746,13 @@ _Description_: An `if-then-else` conditional with identical implementations in b | ||||
|  | ||||
| _Configuration_: N/A | ||||
|  | ||||
| ## identical-ifelseif-branches | ||||
|  | ||||
| _Description_: an `if ... else if` chain with identical branches makes maintenance harder | ||||
| and might be a source of bugs. Duplicated branches should be consolidated in one. | ||||
|  | ||||
| _Configuration_: N/A | ||||
|  | ||||
| ## identical-ifelseif-conditions | ||||
|  | ||||
| _Description_: an `if ... else if` chain  with identical conditions can lead to | ||||
| @@ -751,6 +760,12 @@ unreachable code and is a potential source of bugs while making the code harder | ||||
|  | ||||
| _Configuration_: N/A | ||||
|  | ||||
| ## identical-switch-branches | ||||
|  | ||||
| _Description_: a `switch` with identical branches makes maintenance harder | ||||
| and might be a source of bugs. Duplicated branches should be consolidated | ||||
| in one case clause. | ||||
|  | ||||
| ## identical-switch-conditions | ||||
|  | ||||
| _Description_: a `switch` statement with cases with the same condition can lead to | ||||
|   | ||||
| @@ -108,6 +108,8 @@ var allRules = append([]lint.Rule{ | ||||
| 	&rule.IdenticalSwitchConditionsRule{}, | ||||
| 	&rule.EnforceElseRule{}, | ||||
| 	&rule.IdenticalIfElseIfConditionsRule{}, | ||||
| 	&rule.IdenticalIfElseIfBranchesRule{}, | ||||
| 	&rule.IdenticalSwitchBranchesRule{}, | ||||
| }, defaultRules...) | ||||
|  | ||||
| // allFormatters is a list of all available formatters to output the linting results. | ||||
|   | ||||
| @@ -1,15 +1,13 @@ | ||||
| package rule | ||||
|  | ||||
| import ( | ||||
| 	"fmt" | ||||
| 	"go/ast" | ||||
| 	"go/token" | ||||
|  | ||||
| 	"github.com/mgechev/revive/internal/astutils" | ||||
| 	"github.com/mgechev/revive/lint" | ||||
| ) | ||||
|  | ||||
| // IdenticalBranchesRule warns on constant logical expressions. | ||||
| // IdenticalBranchesRule warns on if...else statements with both branches being the same. | ||||
| type IdenticalBranchesRule struct{} | ||||
|  | ||||
| // Apply applies the rule to given file. | ||||
| @@ -20,7 +18,7 @@ func (*IdenticalBranchesRule) Apply(file *lint.File, _ lint.Arguments) []lint.Fa | ||||
| 		failures = append(failures, failure) | ||||
| 	} | ||||
|  | ||||
| 	w := &lintIdenticalBranches{file: file, onFailure: onFailure} | ||||
| 	w := &lintIdenticalBranches{onFailure: onFailure} | ||||
| 	for _, decl := range file.AST.Decls { | ||||
| 		fn, ok := decl.(*ast.FuncDecl) | ||||
| 		if !ok || fn.Body == nil { | ||||
| @@ -38,111 +36,37 @@ func (*IdenticalBranchesRule) Name() string { | ||||
| 	return "identical-branches" | ||||
| } | ||||
|  | ||||
| // lintIdenticalBranches implements the root or main AST walker of the rule. | ||||
| // This walker will activate other walkers depending on the satement under analysis: | ||||
| // - simple if ... else ... | ||||
| // - if ... else if ... chain, | ||||
| // - switch (not yet implemented). | ||||
| type lintIdenticalBranches struct { | ||||
| 	file      *lint.File // only necessary to retrieve the line number of branches | ||||
| 	onFailure func(lint.Failure) | ||||
| } | ||||
|  | ||||
| func (w *lintIdenticalBranches) Visit(node ast.Node) ast.Visitor { | ||||
| 	switch n := node.(type) { | ||||
| 	case *ast.IfStmt: | ||||
| 		if w.isIfElseIf(n) { | ||||
| 			walker := &lintIfChainIdenticalBranches{ | ||||
| 				onFailure:  w.onFailure, | ||||
| 				file:       w.file, | ||||
| 				rootWalker: w, | ||||
| 			} | ||||
|  | ||||
| 			ast.Walk(walker, n) | ||||
| 			return nil // the walker already analyzed inner branches | ||||
| 		} | ||||
|  | ||||
| 		w.lintSimpleIf(n) | ||||
| 		return w | ||||
|  | ||||
| 	case *ast.SwitchStmt: | ||||
| 		if n.Tag == nil { | ||||
| 			return w // do not lint untagged switches (order of case evaluation might be important) | ||||
| 		} | ||||
|  | ||||
| 		w.lintSwitch(n) | ||||
| 		return nil // switch branches already analyzed | ||||
| 	default: | ||||
| 	ifStmt, ok := node.(*ast.IfStmt) | ||||
| 	if !ok { | ||||
| 		return w | ||||
| 	} | ||||
| } | ||||
|  | ||||
| func (w *lintIdenticalBranches) lintSwitch(switchStmt *ast.SwitchStmt) { | ||||
| 	doesFallthrough := func(stmts []ast.Stmt) bool { | ||||
| 		if len(stmts) == 0 { | ||||
| 			return false | ||||
| 		} | ||||
|  | ||||
| 		ft, ok := stmts[len(stmts)-1].(*ast.BranchStmt) | ||||
| 		return ok && ft.Tok == token.FALLTHROUGH | ||||
| 	if ifStmt.Else == nil { | ||||
| 		return w // if without else | ||||
| 	} | ||||
|  | ||||
| 	hashes := map[string]int{} // map hash(branch code) -> branch line | ||||
| 	for _, cc := range switchStmt.Body.List { | ||||
| 		caseClause := cc.(*ast.CaseClause) | ||||
| 		if doesFallthrough(caseClause.Body) { | ||||
| 			continue // skip fallthrough branches | ||||
| 		} | ||||
| 		branch := &ast.BlockStmt{ | ||||
| 			List: caseClause.Body, | ||||
| 		} | ||||
| 		hash := astutils.NodeHash(branch) | ||||
| 		branchLine := w.file.ToPosition(caseClause.Pos()).Line | ||||
| 		if matchLine, ok := hashes[hash]; ok { | ||||
| 			w.newFailure( | ||||
| 				switchStmt, | ||||
| 				fmt.Sprintf(`"switch" with identical branches (lines %d and %d)`, matchLine, branchLine), | ||||
| 				1.0, | ||||
| 			) | ||||
| 		} | ||||
|  | ||||
| 		hashes[hash] = branchLine | ||||
| 		w.walkBranch(branch) | ||||
| 	} | ||||
| } | ||||
|  | ||||
| // walkBranch analyzes the given branch. | ||||
| func (w *lintIdenticalBranches) walkBranch(branch ast.Stmt) { | ||||
| 	if branch == nil { | ||||
| 		return | ||||
| 	elseBranch, ok := ifStmt.Else.(*ast.BlockStmt) | ||||
| 	if !ok { // if-else-if construction, the rule only copes with single if...else statements | ||||
| 		return w | ||||
| 	} | ||||
|  | ||||
| 	walker := &lintIdenticalBranches{ | ||||
| 		onFailure: w.onFailure, | ||||
| 		file:      w.file, | ||||
| 	if w.identicalBranches(ifStmt.Body, elseBranch) { | ||||
| 		w.onFailure(lint.Failure{ | ||||
| 			Confidence: 1.0, | ||||
| 			Node:       ifStmt, | ||||
| 			Category:   lint.FailureCategoryLogic, | ||||
| 			Failure:    "both branches of the if are identical", | ||||
| 		}) | ||||
| 	} | ||||
|  | ||||
| 	ast.Walk(walker, branch) | ||||
| } | ||||
|  | ||||
| func (*lintIdenticalBranches) isIfElseIf(node *ast.IfStmt) bool { | ||||
| 	_, ok := node.Else.(*ast.IfStmt) | ||||
| 	return ok | ||||
| } | ||||
|  | ||||
| func (w *lintIdenticalBranches) lintSimpleIf(n *ast.IfStmt) { | ||||
| 	if n.Else == nil { | ||||
| 		return | ||||
| 	} | ||||
|  | ||||
| 	elseBranch, ok := n.Else.(*ast.BlockStmt) | ||||
| 	if !ok { // if-else-if construction (should never be the case but keep the check for safer refactoring) | ||||
| 		return | ||||
| 	} | ||||
|  | ||||
| 	if w.identicalBranches(n.Body, elseBranch) { | ||||
| 		w.newFailure(n, "both branches of the if are identical", 1.0) | ||||
| 	} | ||||
| 	ast.Walk(w, ifStmt.Body) | ||||
| 	ast.Walk(w, ifStmt.Else) | ||||
| 	return nil | ||||
| } | ||||
|  | ||||
| func (*lintIdenticalBranches) identicalBranches(body, elseBranch *ast.BlockStmt) bool { | ||||
| @@ -155,111 +79,3 @@ func (*lintIdenticalBranches) identicalBranches(body, elseBranch *ast.BlockStmt) | ||||
|  | ||||
| 	return bodyStr == elseStr | ||||
| } | ||||
|  | ||||
| func (w *lintIdenticalBranches) newFailure(node ast.Node, msg string, confidence float64) { | ||||
| 	w.onFailure(lint.Failure{ | ||||
| 		Confidence: confidence, | ||||
| 		Node:       node, | ||||
| 		Category:   lint.FailureCategoryLogic, | ||||
| 		Failure:    msg, | ||||
| 	}) | ||||
| } | ||||
|  | ||||
| type lintIfChainIdenticalBranches struct { | ||||
| 	file                *lint.File // only necessary to retrieve the line number of branches | ||||
| 	onFailure           func(lint.Failure) | ||||
| 	branches            []ast.Stmt             // hold branches to compare | ||||
| 	rootWalker          *lintIdenticalBranches // the walker to use to recursively analize inner branches | ||||
| 	hasComplexCondition bool                   // indicates if one of the if conditions is "complex" | ||||
| } | ||||
|  | ||||
| // addBranch adds a branch to the list of branches to be compared. | ||||
| func (w *lintIfChainIdenticalBranches) addBranch(branch ast.Stmt) { | ||||
| 	if branch == nil { | ||||
| 		return | ||||
| 	} | ||||
|  | ||||
| 	if w.branches == nil { | ||||
| 		w.resetBranches() | ||||
| 	} | ||||
|  | ||||
| 	w.branches = append(w.branches, branch) | ||||
| } | ||||
|  | ||||
| // resetBranches resets (clears) the list of branches to compare. | ||||
| func (w *lintIfChainIdenticalBranches) resetBranches() { | ||||
| 	w.branches = []ast.Stmt{} | ||||
| 	w.hasComplexCondition = false | ||||
| } | ||||
|  | ||||
| func (w *lintIfChainIdenticalBranches) 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 | ||||
| 		w.addBranch(n.Body) | ||||
| 	} | ||||
|  | ||||
| 	if w.isComplexCondition(n.Cond) { | ||||
| 		w.hasComplexCondition = true | ||||
| 	} | ||||
|  | ||||
| 	if n.Else != nil { | ||||
| 		if chainedIf, ok := n.Else.(*ast.IfStmt); ok { | ||||
| 			w.Visit(chainedIf) | ||||
| 		} else { | ||||
| 			w.addBranch(n.Else) | ||||
| 			w.rootWalker.walkBranch(n.Else) | ||||
| 		} | ||||
| 	} | ||||
|  | ||||
| 	identicalBranches := w.identicalBranches(w.branches) | ||||
| 	for _, branchPair := range identicalBranches { | ||||
| 		msg := fmt.Sprintf(`"if...else if" chain with identical branches (lines %d and %d)`, branchPair[0], branchPair[1]) | ||||
| 		confidence := 1.0 | ||||
| 		if w.hasComplexCondition { | ||||
| 			confidence = 0.8 | ||||
| 		} | ||||
| 		w.rootWalker.newFailure(w.branches[0], msg, confidence) | ||||
| 	} | ||||
|  | ||||
| 	w.resetBranches() | ||||
| 	return nil | ||||
| } | ||||
|  | ||||
| // isComplexCondition returns true if the given expression is "complex", false otherwise. | ||||
| // An expression is considered complex if it has a function call. | ||||
| func (*lintIfChainIdenticalBranches) isComplexCondition(expr ast.Expr) bool { | ||||
| 	calls := astutils.PickNodes(expr, func(n ast.Node) bool { | ||||
| 		_, ok := n.(*ast.CallExpr) | ||||
| 		return ok | ||||
| 	}) | ||||
|  | ||||
| 	return len(calls) > 0 | ||||
| } | ||||
|  | ||||
| // identicalBranches yields pairs of (line numbers) of identical branches from the given branches. | ||||
| func (w *lintIfChainIdenticalBranches) identicalBranches(branches []ast.Stmt) [][]int { | ||||
| 	result := [][]int{} | ||||
| 	if len(branches) < 2 { | ||||
| 		return result // only one branch to compare thus we return | ||||
| 	} | ||||
|  | ||||
| 	hashes := map[string]int{} // branch code hash -> branch line | ||||
| 	for _, branch := range branches { | ||||
| 		hash := astutils.NodeHash(branch) | ||||
| 		branchLine := w.file.ToPosition(branch.Pos()).Line | ||||
| 		if match, ok := hashes[hash]; ok { | ||||
| 			result = append(result, []int{match, branchLine}) | ||||
| 		} | ||||
|  | ||||
| 		hashes[hash] = branchLine | ||||
| 	} | ||||
|  | ||||
| 	return result | ||||
| } | ||||
|   | ||||
							
								
								
									
										186
									
								
								rule/identical_ifelseif_branches.go
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										186
									
								
								rule/identical_ifelseif_branches.go
									
									
									
									
									
										Normal file
									
								
							| @@ -0,0 +1,186 @@ | ||||
| package rule | ||||
|  | ||||
| import ( | ||||
| 	"fmt" | ||||
| 	"go/ast" | ||||
|  | ||||
| 	"github.com/mgechev/revive/internal/astutils" | ||||
| 	"github.com/mgechev/revive/lint" | ||||
| ) | ||||
|  | ||||
| // IdenticalIfElseIfBranchesRule warns on if...else if chains with identical branches. | ||||
| type IdenticalIfElseIfBranchesRule struct{} | ||||
|  | ||||
| // Apply applies the rule to given file. | ||||
| func (*IdenticalIfElseIfBranchesRule) 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 := &rootWalkerIfElseIfIdenticalBranches{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 (*IdenticalIfElseIfBranchesRule) Name() string { | ||||
| 	return "identical-ifelseif-branches" | ||||
| } | ||||
|  | ||||
| type rootWalkerIfElseIfIdenticalBranches struct { | ||||
| 	getStmtLine func(ast.Stmt) int | ||||
| 	onFailure   func(lint.Failure) | ||||
| } | ||||
|  | ||||
| func (w *rootWalkerIfElseIfIdenticalBranches) Visit(node ast.Node) ast.Visitor { | ||||
| 	n, ok := node.(*ast.IfStmt) | ||||
| 	if !ok { | ||||
| 		return w | ||||
| 	} | ||||
|  | ||||
| 	_, isIfElseIf := n.Else.(*ast.IfStmt) | ||||
| 	if isIfElseIf { | ||||
| 		walker := &lintIfChainIdenticalBranches{ | ||||
| 			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 *rootWalkerIfElseIfIdenticalBranches) walkBranch(branch ast.Stmt) { | ||||
| 	if branch == nil { | ||||
| 		return | ||||
| 	} | ||||
|  | ||||
| 	walker := &rootWalkerIfElseIfIdenticalBranches{ | ||||
| 		onFailure:   w.onFailure, | ||||
| 		getStmtLine: w.getStmtLine, | ||||
| 	} | ||||
|  | ||||
| 	ast.Walk(walker, branch) | ||||
| } | ||||
|  | ||||
| type lintIfChainIdenticalBranches struct { | ||||
| 	getStmtLine         func(ast.Stmt) int | ||||
| 	onFailure           func(lint.Failure) | ||||
| 	branches            []ast.Stmt                           // hold branches to compare | ||||
| 	rootWalker          *rootWalkerIfElseIfIdenticalBranches // the walker to use to recursively analyze inner branches | ||||
| 	hasComplexCondition bool                                 // indicates if one of the if conditions is "complex" | ||||
| } | ||||
|  | ||||
| // addBranch adds a branch to the list of branches to be compared. | ||||
| func (w *lintIfChainIdenticalBranches) addBranch(branch ast.Stmt) { | ||||
| 	if branch == nil { | ||||
| 		return | ||||
| 	} | ||||
|  | ||||
| 	if w.branches == nil { | ||||
| 		w.resetBranches() | ||||
| 	} | ||||
|  | ||||
| 	w.branches = append(w.branches, branch) | ||||
| } | ||||
|  | ||||
| // resetBranches resets (clears) the list of branches to compare. | ||||
| func (w *lintIfChainIdenticalBranches) resetBranches() { | ||||
| 	w.branches = []ast.Stmt{} | ||||
| 	w.hasComplexCondition = false | ||||
| } | ||||
|  | ||||
| func (w *lintIfChainIdenticalBranches) 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 | ||||
| 		w.addBranch(n.Body) | ||||
| 	} | ||||
|  | ||||
| 	if w.isComplexCondition(n.Cond) { | ||||
| 		w.hasComplexCondition = true | ||||
| 	} | ||||
|  | ||||
| 	if n.Else != nil { | ||||
| 		if chainedIf, ok := n.Else.(*ast.IfStmt); ok { | ||||
| 			w.Visit(chainedIf) | ||||
| 		} else { | ||||
| 			w.addBranch(n.Else) | ||||
| 			w.rootWalker.walkBranch(n.Else) | ||||
| 		} | ||||
| 	} | ||||
|  | ||||
| 	identicalBranches := w.identicalBranches(w.branches) | ||||
| 	for _, branchPair := range identicalBranches { | ||||
| 		msg := fmt.Sprintf(`"if...else if" chain with identical branches (lines %d and %d)`, branchPair[0], branchPair[1]) | ||||
| 		confidence := 1.0 | ||||
| 		if w.hasComplexCondition { | ||||
| 			confidence = 0.8 | ||||
| 		} | ||||
| 		w.onFailure(lint.Failure{ | ||||
| 			Confidence: confidence, | ||||
| 			Node:       w.branches[0], | ||||
| 			Category:   lint.FailureCategoryLogic, | ||||
| 			Failure:    msg, | ||||
| 		}) | ||||
| 	} | ||||
|  | ||||
| 	w.resetBranches() | ||||
| 	return nil | ||||
| } | ||||
|  | ||||
| // isComplexCondition returns true if the given expression is "complex", false otherwise. | ||||
| // An expression is considered complex if it has a function call. | ||||
| func (*lintIfChainIdenticalBranches) isComplexCondition(expr ast.Expr) bool { | ||||
| 	calls := astutils.PickNodes(expr, func(n ast.Node) bool { | ||||
| 		_, ok := n.(*ast.CallExpr) | ||||
| 		return ok | ||||
| 	}) | ||||
|  | ||||
| 	return len(calls) > 0 | ||||
| } | ||||
|  | ||||
| // identicalBranches yields pairs of (line numbers) of identical branches from the given branches. | ||||
| func (w *lintIfChainIdenticalBranches) identicalBranches(branches []ast.Stmt) [][]int { | ||||
| 	result := [][]int{} | ||||
| 	if len(branches) < 2 { | ||||
| 		return result // no other branch to compare thus we return | ||||
| 	} | ||||
|  | ||||
| 	hashes := map[string]int{} // branch code hash -> branch line | ||||
| 	for _, branch := range branches { | ||||
| 		hash := astutils.NodeHash(branch) | ||||
| 		branchLine := w.getStmtLine(branch) | ||||
| 		if match, ok := hashes[hash]; ok { | ||||
| 			result = append(result, []int{match, branchLine}) | ||||
| 		} | ||||
|  | ||||
| 		hashes[hash] = branchLine | ||||
| 	} | ||||
|  | ||||
| 	return result | ||||
| } | ||||
							
								
								
									
										94
									
								
								rule/identical_switch_branches.go
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										94
									
								
								rule/identical_switch_branches.go
									
									
									
									
									
										Normal file
									
								
							| @@ -0,0 +1,94 @@ | ||||
| package rule | ||||
|  | ||||
| import ( | ||||
| 	"fmt" | ||||
| 	"go/ast" | ||||
| 	"go/token" | ||||
|  | ||||
| 	"github.com/mgechev/revive/internal/astutils" | ||||
| 	"github.com/mgechev/revive/lint" | ||||
| ) | ||||
|  | ||||
| // IdenticalSwitchBranchesRule warns on identical switch branches. | ||||
| type IdenticalSwitchBranchesRule struct{} | ||||
|  | ||||
| // Apply applies the rule to given file. | ||||
| func (*IdenticalSwitchBranchesRule) 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 := &lintIdenticalSwitchBranches{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 (*IdenticalSwitchBranchesRule) Name() string { | ||||
| 	return "identical-switch-branches" | ||||
| } | ||||
|  | ||||
| type lintIdenticalSwitchBranches struct { | ||||
| 	getStmtLine func(ast.Stmt) int | ||||
| 	onFailure   func(lint.Failure) | ||||
| } | ||||
|  | ||||
| func (w *lintIdenticalSwitchBranches) Visit(node ast.Node) ast.Visitor { | ||||
| 	switchStmt, ok := node.(*ast.SwitchStmt) | ||||
| 	if !ok { | ||||
| 		return w | ||||
| 	} | ||||
|  | ||||
| 	if switchStmt.Tag == nil { | ||||
| 		return w // do not lint untagged switches (order of case evaluation might be important) | ||||
| 	} | ||||
|  | ||||
| 	doesFallthrough := func(stmts []ast.Stmt) bool { | ||||
| 		if len(stmts) == 0 { | ||||
| 			return false | ||||
| 		} | ||||
|  | ||||
| 		ft, ok := stmts[len(stmts)-1].(*ast.BranchStmt) | ||||
| 		return ok && ft.Tok == token.FALLTHROUGH | ||||
| 	} | ||||
|  | ||||
| 	hashes := map[string]int{} // map hash(branch code) -> branch line | ||||
| 	for _, cc := range switchStmt.Body.List { | ||||
| 		caseClause := cc.(*ast.CaseClause) | ||||
| 		if doesFallthrough(caseClause.Body) { | ||||
| 			continue // skip fallthrough branches | ||||
| 		} | ||||
| 		branch := &ast.BlockStmt{ | ||||
| 			List: caseClause.Body, | ||||
| 		} | ||||
| 		hash := astutils.NodeHash(branch) | ||||
| 		branchLine := w.getStmtLine(caseClause) | ||||
| 		if matchLine, ok := hashes[hash]; ok { | ||||
| 			w.onFailure(lint.Failure{ | ||||
| 				Confidence: 1.0, | ||||
| 				Node:       node, | ||||
| 				Category:   lint.FailureCategoryLogic, | ||||
| 				Failure:    fmt.Sprintf(`"switch" with identical branches (lines %d and %d)`, matchLine, branchLine), | ||||
| 			}) | ||||
| 		} | ||||
|  | ||||
| 		hashes[hash] = branchLine | ||||
| 		ast.Walk(w, branch) | ||||
| 	} | ||||
|  | ||||
| 	return nil // switch branches already analyzed | ||||
| } | ||||
							
								
								
									
										11
									
								
								test/identical_ifelseif_branches_test.go
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										11
									
								
								test/identical_ifelseif_branches_test.go
									
									
									
									
									
										Normal file
									
								
							| @@ -0,0 +1,11 @@ | ||||
| package test | ||||
|  | ||||
| import ( | ||||
| 	"testing" | ||||
|  | ||||
| 	"github.com/mgechev/revive/rule" | ||||
| ) | ||||
|  | ||||
| func TestIdenticalIfElseIfBranches(t *testing.T) { | ||||
| 	testRule(t, "identical_ifelseif_branches", &rule.IdenticalIfElseIfBranchesRule{}) | ||||
| } | ||||
							
								
								
									
										11
									
								
								test/identical_switch_branches_test.go
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										11
									
								
								test/identical_switch_branches_test.go
									
									
									
									
									
										Normal file
									
								
							| @@ -0,0 +1,11 @@ | ||||
| package test | ||||
|  | ||||
| import ( | ||||
| 	"testing" | ||||
|  | ||||
| 	"github.com/mgechev/revive/rule" | ||||
| ) | ||||
|  | ||||
| func TestIdenticalSwitchBranches(t *testing.T) { | ||||
| 	testRule(t, "identical_switch_branches", &rule.IdenticalSwitchBranchesRule{}) | ||||
| } | ||||
							
								
								
									
										145
									
								
								testdata/identical_branches.go
									
									
									
									
										vendored
									
									
								
							
							
						
						
									
										145
									
								
								testdata/identical_branches.go
									
									
									
									
										vendored
									
									
								
							| @@ -37,149 +37,4 @@ func identicalBranches() { | ||||
| 	} else { | ||||
| 		println("else") | ||||
| 	} | ||||
|  | ||||
| 	if true { // MATCH /"if...else if" chain with identical branches (lines 41 and 49)/ | ||||
| 		print("something") | ||||
| 	} else if true { | ||||
| 		print("something else") | ||||
| 	} else if true { | ||||
| 		print("other thing") | ||||
| 	} else if false { | ||||
| 		println() | ||||
| 	} else { | ||||
| 		print("something") | ||||
| 	} | ||||
|  | ||||
| 	if true { // MATCH /"if...else if" chain with identical branches (lines 53 and 59)/ | ||||
| 		print("something") | ||||
| 	} else if true { | ||||
| 		print("something else") | ||||
| 	} else if true { | ||||
| 		print("other thing") | ||||
| 	} else if false { | ||||
| 		print("something") | ||||
| 	} else { | ||||
| 		println() | ||||
| 	} | ||||
|  | ||||
| 	if true { | ||||
| 		print("something") | ||||
| 	} else if true { | ||||
| 		print("something else") | ||||
| 		if true { // MATCH /"if...else if" chain with identical branches (lines 69 and 71)/ | ||||
| 			print("something") | ||||
| 		} else if false { | ||||
| 			print("something") | ||||
| 		} else { | ||||
| 			println() | ||||
| 		} | ||||
| 	} | ||||
|  | ||||
| 	// Should not warn because even if branches are identical, the err variable is not | ||||
| 	if err := something(); err != nil { | ||||
| 		println(err) | ||||
| 	} else if err := somethingElse(); err != nil { | ||||
| 		println(err) | ||||
| 	} | ||||
|  | ||||
| 	// Multiple identical pair of branches | ||||
| 	if a { | ||||
| 		foo() | ||||
| 	} else if b { | ||||
| 		bar() | ||||
| 	} else if c { | ||||
| 		foo() | ||||
| 	} else if d { | ||||
| 		bar() | ||||
| 	} | ||||
| 	// MATCH:86 /"if...else if" chain with identical branches (lines 86 and 90)/ | ||||
| 	// MATCH:86 /"if...else if" chain with identical branches (lines 88 and 92)/ | ||||
|  | ||||
| 	if createFile() { // json:{"MATCH": "\"if...else if\" chain with identical branches (lines 98 and 102)","Confidence": 0.8} | ||||
| 		doSomething() | ||||
| 	} else if !delete() { | ||||
| 		return new("cannot delete file") | ||||
| 	} else if createFile() { | ||||
| 		doSomething() | ||||
| 	} else { | ||||
| 		return new("file error") | ||||
| 	} | ||||
|  | ||||
| 	// Test confidence is reset | ||||
| 	if a { // json:{"MATCH": "\"if...else if\" chain with identical branches (lines 109 and 111)","Confidence": 1} | ||||
| 		foo() | ||||
| 	} else if b { | ||||
| 		foo() | ||||
| 	} else { | ||||
| 		bar() | ||||
| 	} | ||||
|  | ||||
| 	switch a { // MATCH /"switch" with identical branches (lines 119 and 123)/ | ||||
| 	// expected values | ||||
| 	case 1: | ||||
| 		foo() | ||||
| 	case 2: | ||||
| 		bar() | ||||
| 	case 3: | ||||
| 		foo() | ||||
| 	default: | ||||
| 		return newError("blah") | ||||
| 	} | ||||
|  | ||||
| 	// MATCH:131 /"switch" with identical branches (lines 133 and 137)/ | ||||
| 	// MATCH:131 /"switch" with identical branches (lines 135 and 139)/ | ||||
| 	switch a { | ||||
| 	// expected values | ||||
| 	case 1: | ||||
| 		foo() | ||||
| 	case 2: | ||||
| 		bar() | ||||
| 	case 3: | ||||
| 		foo() | ||||
| 	default: | ||||
| 		bar() | ||||
| 	} | ||||
|  | ||||
| 	switch a { // MATCH /"switch" with identical branches (lines 145 and 147)/ | ||||
| 	// expected values | ||||
| 	case 1: | ||||
| 		foo() | ||||
| 	case 3: | ||||
| 		foo() | ||||
| 	default: | ||||
| 		if true { // MATCH /"if...else if" chain with identical branches (lines 150 and 152)/ | ||||
| 			something() | ||||
| 		} else if true { | ||||
| 			something() | ||||
| 		} else { | ||||
| 			if true { // MATCH /both branches of the if are identical/ | ||||
| 				print("identical") | ||||
| 			} else { | ||||
| 				print("identical") | ||||
| 			} | ||||
| 		} | ||||
| 	} | ||||
|  | ||||
| 	// Skip untagged switch | ||||
| 	switch { | ||||
| 	case a > b: | ||||
| 		foo() | ||||
| 	default: | ||||
| 		foo() | ||||
| 	} | ||||
|  | ||||
| 	// Do not warn on fallthrough | ||||
| 	switch a { | ||||
| 	case 1: | ||||
| 		foo() | ||||
| 		fallthrough | ||||
| 	case 2: | ||||
| 		fallthrough | ||||
| 	case 3: | ||||
| 		foo() | ||||
| 	case 4: | ||||
| 		fallthrough | ||||
| 	default: | ||||
| 		bar() | ||||
| 	} | ||||
| } | ||||
|   | ||||
							
								
								
									
										80
									
								
								testdata/identical_ifelseif_branches.go
									
									
									
									
										vendored
									
									
										Normal file
									
								
							
							
						
						
									
										80
									
								
								testdata/identical_ifelseif_branches.go
									
									
									
									
										vendored
									
									
										Normal file
									
								
							| @@ -0,0 +1,80 @@ | ||||
| package fixtures | ||||
|  | ||||
| func identicalIfElseIfBranches() { | ||||
|  | ||||
| 	if true { // MATCH /"if...else if" chain with identical branches (lines 5 and 13)/ | ||||
| 		print("something") | ||||
| 	} else if true { | ||||
| 		print("something else") | ||||
| 	} else if true { | ||||
| 		print("other thing") | ||||
| 	} else if false { | ||||
| 		println() | ||||
| 	} else { | ||||
| 		print("something") | ||||
| 	} | ||||
|  | ||||
| 	if true { // MATCH /"if...else if" chain with identical branches (lines 17 and 23)/ | ||||
| 		print("something") | ||||
| 	} else if true { | ||||
| 		print("something else") | ||||
| 	} else if true { | ||||
| 		print("other thing") | ||||
| 	} else if false { | ||||
| 		print("something") | ||||
| 	} else { | ||||
| 		println() | ||||
| 	} | ||||
|  | ||||
| 	if true { | ||||
| 		print("something") | ||||
| 	} else if true { | ||||
| 		print("something else") | ||||
| 		if true { // MATCH /"if...else if" chain with identical branches (lines 33 and 35)/ | ||||
| 			print("something") | ||||
| 		} else if false { | ||||
| 			print("something") | ||||
| 		} else { | ||||
| 			println() | ||||
| 		} | ||||
| 	} | ||||
|  | ||||
| 	// Should not warn because even if branches are identical, the err variable is not | ||||
| 	if err := something(); err != nil { | ||||
| 		println(err) | ||||
| 	} else if err := somethingElse(); err != nil { | ||||
| 		println(err) | ||||
| 	} | ||||
|  | ||||
| 	// Multiple identical pair of branches | ||||
| 	if a { | ||||
| 		foo() | ||||
| 	} else if b { | ||||
| 		bar() | ||||
| 	} else if c { | ||||
| 		foo() | ||||
| 	} else if d { | ||||
| 		bar() | ||||
| 	} | ||||
| 	// MATCH:50 /"if...else if" chain with identical branches (lines 50 and 54)/ | ||||
| 	// MATCH:50 /"if...else if" chain with identical branches (lines 52 and 56)/ | ||||
|  | ||||
| 	if createFile() { // json:{"MATCH": "\"if...else if\" chain with identical branches (lines 62 and 66)","Confidence": 0.8} | ||||
| 		doSomething() | ||||
| 	} else if !delete() { | ||||
| 		return new("cannot delete file") | ||||
| 	} else if createFile() { | ||||
| 		doSomething() | ||||
| 	} else { | ||||
| 		return new("file error") | ||||
| 	} | ||||
|  | ||||
| 	// Test confidence is reset | ||||
| 	if a { // json:{"MATCH": "\"if...else if\" chain with identical branches (lines 73 and 75)","Confidence": 1} | ||||
| 		foo() | ||||
| 	} else if b { | ||||
| 		foo() | ||||
| 	} else { | ||||
| 		bar() | ||||
| 	} | ||||
| } | ||||
							
								
								
									
										77
									
								
								testdata/identical_switch_branches.go
									
									
									
									
										vendored
									
									
										Normal file
									
								
							
							
						
						
									
										77
									
								
								testdata/identical_switch_branches.go
									
									
									
									
										vendored
									
									
										Normal file
									
								
							| @@ -0,0 +1,77 @@ | ||||
| package fixtures | ||||
|  | ||||
| func identicalSwitchBranches() { | ||||
| 	switch a { // MATCH /"switch" with identical branches (lines 6 and 10)/ | ||||
| 	// expected values | ||||
| 	case 1: | ||||
| 		foo() | ||||
| 	case 2: | ||||
| 		bar() | ||||
| 	case 3: | ||||
| 		foo() | ||||
| 	default: | ||||
| 		return newError("blah") | ||||
| 	} | ||||
|  | ||||
| 	// MATCH:18 /"switch" with identical branches (lines 20 and 24)/ | ||||
| 	// MATCH:18 /"switch" with identical branches (lines 22 and 26)/ | ||||
| 	switch a { | ||||
| 	// expected values | ||||
| 	case 1: | ||||
| 		foo() | ||||
| 	case 2: | ||||
| 		bar() | ||||
| 	case 3: | ||||
| 		foo() | ||||
| 	default: | ||||
| 		bar() | ||||
| 	} | ||||
|  | ||||
| 	switch a { // MATCH /"switch" with identical branches (lines 32 and 34)/ | ||||
| 	// expected values | ||||
| 	case 1: | ||||
| 		foo() | ||||
| 	case 3: | ||||
| 		foo() | ||||
| 	default: | ||||
|  | ||||
| 	} | ||||
|  | ||||
| 	// Skip untagged switch | ||||
| 	switch { | ||||
| 	case a > b: | ||||
| 		foo() | ||||
| 	default: | ||||
| 		foo() | ||||
| 	} | ||||
|  | ||||
| 	// Do not warn on fallthrough | ||||
|  | ||||
| 	switch a { | ||||
| 	case 1: | ||||
| 		foo() | ||||
| 		fallthrough | ||||
| 	case 2: | ||||
| 		fallthrough | ||||
| 	case 3: | ||||
| 		foo() | ||||
| 	case 4: | ||||
| 		fallthrough | ||||
| 	default: | ||||
| 		bar() | ||||
| 	} | ||||
|  | ||||
| 	// skip type switch | ||||
| 	switch v := value.(type) { | ||||
| 	case int: | ||||
| 		println("dup", v) | ||||
| 	case string: | ||||
| 		println("dup", v) | ||||
| 	case bool: | ||||
| 		println("dup", v) | ||||
| 	case float64: | ||||
| 		println("dup", v) | ||||
| 	default: | ||||
| 		println("dup", v) | ||||
| 	} | ||||
| } | ||||
		Reference in New Issue
	
	Block a user