From 579c7c761fe9ca9c80141b95ad717525224c40e3 Mon Sep 17 00:00:00 2001 From: chavacava Date: Fri, 8 May 2020 17:52:30 +0200 Subject: [PATCH 1/5] utils.gofmt now accepts a interface{} --- rule/utils.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rule/utils.go b/rule/utils.go index 6ba542b..38677c8 100644 --- a/rule/utils.go +++ b/rule/utils.go @@ -182,8 +182,8 @@ func isExprABooleanLit(n ast.Node) (lexeme string, ok bool) { return oper.Name, (oper.Name == trueName || oper.Name == falseName) } -// gofmt returns a string representation of the expression. -func gofmt(x ast.Expr) string { +// gofmt returns a string representation of an AST subtree. +func gofmt(x interface{}) string { buf := bytes.Buffer{} fs := token.NewFileSet() printer.Fprint(&buf, fs, x) From 0c49c6a991c6135d15f53e00044a2b0aa4985b96 Mon Sep 17 00:00:00 2001 From: chavacava Date: Fri, 8 May 2020 22:08:00 +0200 Subject: [PATCH 2/5] identical-branches initial implementation --- rule/identical-branches.go | 98 +++++++++++++++++++++++++++++++++ test/identical-branches_test.go | 12 ++++ testdata/identical-branches.go | 40 ++++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 rule/identical-branches.go create mode 100644 test/identical-branches_test.go create mode 100644 testdata/identical-branches.go diff --git a/rule/identical-branches.go b/rule/identical-branches.go new file mode 100644 index 0000000..4307efc --- /dev/null +++ b/rule/identical-branches.go @@ -0,0 +1,98 @@ +package rule + +import ( + "go/ast" + "reflect" + + "github.com/mgechev/revive/lint" +) + +// IdenticalBranchesRule warns on constant logical expressions. +type IdenticalBranchesRule struct{} + +// Apply applies the rule to given file. +func (r *IdenticalBranchesRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + astFile := file.AST + w := &lintIdenticalBranches{astFile, onFailure} + ast.Walk(w, astFile) + return failures +} + +// Name returns the rule name. +func (r *IdenticalBranchesRule) Name() string { + return "identical-branches" +} + +type lintIdenticalBranches struct { + file *ast.File + onFailure func(lint.Failure) +} + +func (w *lintIdenticalBranches) Visit(node ast.Node) ast.Visitor { + n, ok := node.(*ast.IfStmt) + if !ok { + return w + } + + if n.Else == nil { + return w + } + branches := []*ast.BlockStmt{n.Body} + + elseBranch, ok := n.Else.(*ast.BlockStmt) + if !ok { // if-else-if construction + return w + } + branches = append(branches, elseBranch) + + if w.identicalBranches(branches) { + w.newFailure(n, "both branches of the if are identical") + } + + return w +} + +func (w *lintIdenticalBranches) identicalBranches(branches []*ast.BlockStmt) bool { + if len(branches) < 2 { + return false + } + + ref := gofmt(branches[0]) + for i := 1; i < len(branches); i++ { + if gofmt(branches[i]) != ref { + return false + } + } + + return true +} + +func (w *lintIdenticalBranches) identicalBranches2(branches []*ast.BlockStmt) bool { + if len(branches) < 2 { + return false + } + + ref := branches[0] + for i := 1; i < len(branches); i++ { + if !reflect.DeepEqual(branches[i], ref) { + return false + } + } + + return true +} + +func (w lintIdenticalBranches) newFailure(node ast.Node, msg string) { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: node, + Category: "logic", + Failure: msg, + }) +} diff --git a/test/identical-branches_test.go b/test/identical-branches_test.go new file mode 100644 index 0000000..0d1a6c5 --- /dev/null +++ b/test/identical-branches_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// IdenticalBranches rule. +func TestIdenticalBranches(t *testing.T) { + testRule(t, "identical-branches", &rule.IdenticalBranchesRule{}) +} diff --git a/testdata/identical-branches.go b/testdata/identical-branches.go new file mode 100644 index 0000000..8b850da --- /dev/null +++ b/testdata/identical-branches.go @@ -0,0 +1,40 @@ +package fixtures + +func identicalBranches() { + if true { // MATCH /both branches of the if are identical/ + + } else { + + } + + if true { + + } + + if true { + print() + } else { + } + + if true { // MATCH /both branches of the if are identical/ + print() + } else { + print() + } + + if true { + if true { // MATCH /both branches of the if are identical/ + print() + } else { + print() + } + } else { + println() + } + + if true { + println("something") + } else { + println("else") + } +} From fb92af247c6d24dfc1e94c4aac4bd76f65226fbb Mon Sep 17 00:00:00 2001 From: chavacava Date: Fri, 8 May 2020 22:20:59 +0200 Subject: [PATCH 3/5] adds identical-branches to configuration --- config.go | 1 + 1 file changed, 1 insertion(+) diff --git a/config.go b/config.go index 95543b8..6e9806c 100644 --- a/config.go +++ b/config.go @@ -84,6 +84,7 @@ var allRules = append([]lint.Rule{ &rule.CognitiveComplexityRule{}, &rule.StringOfIntRule{}, &rule.EarlyReturnRule{}, + &rule.IdenticalBranchesRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ From b43b33a4e6ca96c8baf3178dd2f99335f3c73bc3 Mon Sep 17 00:00:00 2001 From: chavacava Date: Fri, 8 May 2020 22:21:34 +0200 Subject: [PATCH 4/5] adds identical-branches to doc --- README.md | 2 ++ RULES_DESCRIPTIONS.md | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/README.md b/README.md index 3e51462..7c6809c 100644 --- a/README.md +++ b/README.md @@ -350,6 +350,8 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`cognitive-complexity`](./RULES_DESCRIPTIONS.md#cognitive-complexity) | int | Sets restriction for maximum Cognitive complexity. | no | no | | [`string-of-int`](./RULES_DESCRIPTIONS.md#string-of-int) | n/a | Warns on suspicious casts from int to string | no | yes | | [`early-return`](./RULES_DESCRIPTIONS.md#early-return) | n/a | Spots if-then-else statements that can be refactored to simplify code reading | no | no | +| [`identical-branches`](./RULES_DESCRIPTIONS.md#identical-branches) | n/a | Spots if-then-else statements with identical `then` and `else` branches | no | no | + ## Configurable rules Here you can find how you can configure some of the existing rules: diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 901543c..4cedcd9 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -33,6 +33,7 @@ List of all available rules. - [flag-parameter](#flag-parameter) - [function-result-limit](#function-result-limit) - [get-return](#get-return) + - [identical-branches](#identical-branches) - [if-return](#if-return) - [increment-decrement](#increment-decrement) - [indent-error-flow](#indent-error-flow) @@ -313,6 +314,12 @@ _Description_: Typically, functions with names prefixed with _Get_ are supposed _Configuration_: N/A +## identical-branches + +_Description_: an `if-then-else` conditional with identical implementations in both branches is an error. + +_Configuration_: N/A + ## if-return _Description_: Checking if an error is _nil_ to just after return the error or nil is redundant. From 5939a81c8a6701aa8afa17a5169897945dee337f Mon Sep 17 00:00:00 2001 From: chavacava Date: Fri, 8 May 2020 22:43:14 +0200 Subject: [PATCH 5/5] removes unused code --- rule/identical-branches.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/rule/identical-branches.go b/rule/identical-branches.go index 4307efc..094a791 100644 --- a/rule/identical-branches.go +++ b/rule/identical-branches.go @@ -2,7 +2,6 @@ package rule import ( "go/ast" - "reflect" "github.com/mgechev/revive/lint" ) @@ -73,21 +72,6 @@ func (w *lintIdenticalBranches) identicalBranches(branches []*ast.BlockStmt) boo return true } -func (w *lintIdenticalBranches) identicalBranches2(branches []*ast.BlockStmt) bool { - if len(branches) < 2 { - return false - } - - ref := branches[0] - for i := 1; i < len(branches); i++ { - if !reflect.DeepEqual(branches[i], ref) { - return false - } - } - - return true -} - func (w lintIdenticalBranches) newFailure(node ast.Node, msg string) { w.onFailure(lint.Failure{ Confidence: 1,