diff --git a/README.md b/README.md index 6529d35..aa907d2 100644 --- a/README.md +++ b/README.md @@ -316,7 +316,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`cyclomatic`](./RULES_DESCRIPTIONS.md#cyclomatic) | int | Sets restriction for maximum Cyclomatic complexity. | no | no | | [`max-public-structs`](./RULES_DESCRIPTIONS.md#max-public-structs) | int | The maximum number of public structs in a file. | no | no | | [`file-header`](./RULES_DESCRIPTIONS.md#file-header) | string | Header which each file should have. | no | no | -| [`empty-block`](./RULES_DESCRIPTIONS.md#empty-block) | n/a | Warns on empty code blocks | no | no | +| [`empty-block`](./RULES_DESCRIPTIONS.md#empty-block) | n/a | Warns on empty code blocks | no | yes | | [`superfluous-else`](./RULES_DESCRIPTIONS.md#superfluous-else) | n/a | Prevents redundant else statements (extends [`indent-error-flow`](./RULES_DESCRIPTIONS.md#indent-error-flow)) | no | no | | [`confusing-naming`](./RULES_DESCRIPTIONS.md#confusing-naming) | n/a | Warns on methods with names that differ only by capitalization | no | no | | [`get-return`](./RULES_DESCRIPTIONS.md#get-return) | n/a | Warns on getters that do not yield any result | no | no | @@ -351,6 +351,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`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 | | [`unconditional-recursion`](./RULES_DESCRIPTIONS.md#unconditional-recursion) | n/a | Warns on function calls that will lead to (direct) infinite recursion | 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 diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 305c573..afdcd18 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) @@ -314,6 +315,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. diff --git a/config.go b/config.go index 6c9c1d3..61e4355 100644 --- a/config.go +++ b/config.go @@ -85,6 +85,7 @@ var allRules = append([]lint.Rule{ &rule.StringOfIntRule{}, &rule.EarlyReturnRule{}, &rule.UnconditionalRecursionRule{}, + &rule.IdenticalBranchesRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/go.mod b/go.mod index 2ea4dd0..d8d31f2 100644 --- a/go.mod +++ b/go.mod @@ -10,5 +10,5 @@ require ( github.com/mitchellh/go-homedir v1.1.0 github.com/olekukonko/tablewriter v0.0.4 github.com/pkg/errors v0.9.1 - golang.org/x/tools v0.0.0-20200507205054-480da3ebd79c + golang.org/x/tools v0.0.0-20200509030707-2212a7e161a5 ) diff --git a/go.sum b/go.sum index 36a3f27..a683a92 100644 --- a/go.sum +++ b/go.sum @@ -52,6 +52,8 @@ golang.org/x/tools v0.0.0-20200505023115-26f46d2f7ef8 h1:BMFHd4OFnFtWX46Xj4DN6vv golang.org/x/tools v0.0.0-20200505023115-26f46d2f7ef8/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200507205054-480da3ebd79c h1:TDspWmUQsjdWzrHnd5imfaJSfhR4AO/R7kG++T2cONw= golang.org/x/tools v0.0.0-20200507205054-480da3ebd79c/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= +golang.org/x/tools v0.0.0-20200509030707-2212a7e161a5 h1:MeC2gMlMdkd67dn17MEby3rGXRxZtWeiRXOnISfTQ74= +golang.org/x/tools v0.0.0-20200509030707-2212a7e161a5/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4= diff --git a/rule/empty-block.go b/rule/empty-block.go index 7861394..4eb0ce8 100644 --- a/rule/empty-block.go +++ b/rule/empty-block.go @@ -2,6 +2,7 @@ package rule import ( "go/ast" + "go/types" "github.com/mgechev/revive/lint" ) @@ -17,7 +18,12 @@ func (r *EmptyBlockRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure failures = append(failures, failure) } - w := lintEmptyBlock{make([]*ast.BlockStmt, 0), onFailure} + err := file.Pkg.TypeCheck() + if err != nil { + panic("unable to type check " + file.Name + ":" + err.Error()) + } + + w := lintEmptyBlock{make(map[*ast.BlockStmt]bool, 0), file.Pkg, onFailure} ast.Walk(w, file.AST) return failures } @@ -28,49 +34,35 @@ func (r *EmptyBlockRule) Name() string { } type lintEmptyBlock struct { - ignore []*ast.BlockStmt + ignore map[*ast.BlockStmt]bool + pkg *lint.Package onFailure func(lint.Failure) } func (w lintEmptyBlock) Visit(node ast.Node) ast.Visitor { - fd, ok := node.(*ast.FuncDecl) - if ok { - w.ignore = append(w.ignore, fd.Body) + switch n := node.(type) { + case *ast.FuncDecl: + w.ignore[n.Body] = true return w - } - - fl, ok := node.(*ast.FuncLit) - if ok { - w.ignore = append(w.ignore, fl.Body) + case *ast.FuncLit: + w.ignore[n.Body] = true return w - } + case *ast.RangeStmt: + t := w.pkg.TypeOf(n.X) - block, ok := node.(*ast.BlockStmt) - if !ok { - return w - } - - if mustIgnore(block, w.ignore) { - return w - } - - if len(block.List) == 0 { - w.onFailure(lint.Failure{ - Confidence: 1, - Node: block, - Category: "logic", - Failure: "this block is empty, you can remove it", - }) + if _, ok := t.(*types.Chan); ok { + w.ignore[n.Body] = true + } + case *ast.BlockStmt: + if !w.ignore[n] && len(n.List) == 0 { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: n, + Category: "logic", + Failure: "this block is empty, you can remove it", + }) + } } return w } - -func mustIgnore(block *ast.BlockStmt, blackList []*ast.BlockStmt) bool { - for _, b := range blackList { - if b == block { - return true - } - } - return false -} diff --git a/rule/identical-branches.go b/rule/identical-branches.go new file mode 100644 index 0000000..094a791 --- /dev/null +++ b/rule/identical-branches.go @@ -0,0 +1,82 @@ +package rule + +import ( + "go/ast" + + "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) newFailure(node ast.Node, msg string) { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: node, + Category: "logic", + Failure: msg, + }) +} 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) 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/empty-block.go b/testdata/empty-block.go index 9f04d5b..81b7262 100644 --- a/testdata/empty-block.go +++ b/testdata/empty-block.go @@ -2,18 +2,18 @@ package fixtures -func f(x int) bool {} // Must not match +func f(x int) {} // Must not match -type foo struct {} +type foo struct{} -func (f foo) f(x *int) string {} // Must not match -func (f *foo) g(y *int) string {} // Must not match +func (f foo) f(x *int) {} // Must not match +func (f *foo) g(y *int) {} // Must not match -func g(f func() bool) string { +func g(f func() bool) { { // MATCH /this block is empty, you can remove it/ } - a := func(e error){} // Must not match + _ = func(e error) {} // Must not match if ok := f(); ok { // MATCH /this block is empty, you can remove it/ // only a comment @@ -35,4 +35,13 @@ func g(f func() bool) string { } + // issue 386 + var c = make(chan int) + for range c { + } + + var s = "a string" + for range s { // MATCH /this block is empty, you can remove it/ + } + } 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") + } +}