diff --git a/README.md b/README.md index 364183d..882dec3 100644 --- a/README.md +++ b/README.md @@ -351,6 +351,7 @@ 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 | +| [`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 4cedcd9..afdcd18 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -55,6 +55,7 @@ List of all available rules. - [time-naming](#time-naming) - [var-naming](#var-naming) - [var-declaration](#var-declaration) + - [unconditional-recursion](#unconditional-recursion) - [unexported-return](#unexported-return) - [unhandled-error](#unhandled-error) - [unnecessary-stmt](#unnecessary-stmt) @@ -489,6 +490,12 @@ _Description_: This rule proposes simplifications of variable declarations. _Configuration_: N/A +## unconditional-recursion + +_Description_: Unconditional recursive calls will produce infinite recursion, thus program stack overflow. This rule detects and warns about unconditional (direct) recursive calls. + +_Configuration_: N/A + ## unexported-return _Description_: This rule warns when an exported function or method returns a value of an un-exported type. diff --git a/config.go b/config.go index e2ee979..cf1a175 100644 --- a/config.go +++ b/config.go @@ -85,6 +85,7 @@ var allRules = append([]lint.Rule{ &rule.CognitiveComplexityRule{}, &rule.StringOfIntRule{}, &rule.EarlyReturnRule{}, + &rule.UnconditionalRecursionRule{}, &rule.IdenticalBranchesRule{}, }, defaultRules...) diff --git a/rule/unconditional-recursion.go b/rule/unconditional-recursion.go new file mode 100644 index 0000000..b645b75 --- /dev/null +++ b/rule/unconditional-recursion.go @@ -0,0 +1,183 @@ +package rule + +import ( + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// UnconditionalRecursionRule lints given else constructs. +type UnconditionalRecursionRule struct{} + +// Apply applies the rule to given file. +func (r *UnconditionalRecursionRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintUnconditionalRecursionRule{onFailure: onFailure} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *UnconditionalRecursionRule) Name() string { + return "unconditional-recursion" +} + +type funcDesc struct { + reciverId *ast.Ident + id *ast.Ident +} + +func (fd *funcDesc) equal(other *funcDesc) bool { + receiversAreEqual := (fd.reciverId == nil && other.reciverId == nil) || fd.reciverId != nil && other.reciverId != nil && fd.reciverId.Name == other.reciverId.Name + idsAreEqual := (fd.id == nil && other.id == nil) || fd.id.Name == other.id.Name + + return receiversAreEqual && idsAreEqual +} + +type funcStatus struct { + funcDesc *funcDesc + seenConditionalExit bool +} + +type lintUnconditionalRecursionRule struct { + onFailure func(lint.Failure) + currentFunc *funcStatus +} + +// Visit will traverse the file AST. +// The rule is based in the following algorithm: inside each function body we search for calls to the function itself. +// We do not search inside conditional control structures (if, for, switch, ...) because any recursive call inside them is conditioned +// We do search inside conditional control structures are statements that will take the control out of the function (return, exit, panic) +// If we find conditional control exits, it means the function is NOT unconditionally-recursive +// If we find a recursive call before finding any conditional exit, a failure is generated +// In resume: if we found a recursive call control-dependant from the entry point of the function then we raise a failure. +func (w lintUnconditionalRecursionRule) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.FuncDecl: + var rec *ast.Ident + switch { + case n.Recv == nil || n.Recv.NumFields() < 1 || len(n.Recv.List[0].Names) < 1: + rec = nil + default: + rec = n.Recv.List[0].Names[0] + } + + w.currentFunc = &funcStatus{&funcDesc{rec, n.Name}, false} + case *ast.CallExpr: + var funcId *ast.Ident + var selector *ast.Ident + switch c := n.Fun.(type) { + case *ast.Ident: + selector = nil + funcId = c + case *ast.SelectorExpr: + var ok bool + selector, ok = c.X.(*ast.Ident) + if !ok { // a.b....Foo() + return nil + } + funcId = c.Sel + default: + return w + } + + if w.currentFunc != nil && // not in a func body + !w.currentFunc.seenConditionalExit && // there is a conditional exit in the function + w.currentFunc.funcDesc.equal(&funcDesc{selector, funcId}) { + w.onFailure(lint.Failure{ + Category: "logic", + Confidence: 1, + Node: n, + Failure: "unconditional recursive call", + }) + } + case *ast.IfStmt: + w.updateFuncStatus(n.Body) + w.updateFuncStatus(n.Else) + return nil + case *ast.SelectStmt: + w.updateFuncStatus(n.Body) + return nil + case *ast.RangeStmt: + w.updateFuncStatus(n.Body) + return nil + case *ast.TypeSwitchStmt: + w.updateFuncStatus(n.Body) + return nil + case *ast.SwitchStmt: + w.updateFuncStatus(n.Body) + return nil + case *ast.GoStmt: + for _, a := range n.Call.Args { + ast.Walk(w, a) // check if arguments have a recursive call + } + return nil // recursive async call is not an issue + case *ast.ForStmt: + if n.Cond != nil { + return nil + } + // unconditional loop + return w + } + + return w +} + +func (w *lintUnconditionalRecursionRule) updateFuncStatus(node ast.Node) { + if node == nil || w.currentFunc == nil || w.currentFunc.seenConditionalExit { + return + } + + w.currentFunc.seenConditionalExit = w.hasControlExit(node) +} + +var exitFunctions = map[string]map[string]bool{ + "os": map[string]bool{"Exit": true}, + "syscall": map[string]bool{"Exit": true}, + "log": map[string]bool{ + "Fatal": true, + "Fatalf": true, + "Fatalln": true, + "Panic": true, + "Panicf": true, + "Panicln": true, + }, +} + +func (w *lintUnconditionalRecursionRule) hasControlExit(node ast.Node) bool { + // isExit returns true if the given node makes control exit the function + isExit := func(node ast.Node) bool { + switch n := node.(type) { + case *ast.ReturnStmt: + return true + case *ast.CallExpr: + if isIdent(n.Fun, "panic") { + return true + } + se, ok := n.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + + id, ok := se.X.(*ast.Ident) + if !ok { + return false + } + + fn := se.Sel.Name + pkg := id.Name + if exitFunctions[pkg] != nil && exitFunctions[pkg][fn] { // it's a call to an exit function + return true + } + } + + return false + } + + return len(pick(node, isExit, nil)) != 0 +} diff --git a/test/unconditional-recursion_test.go b/test/unconditional-recursion_test.go new file mode 100644 index 0000000..d47d29c --- /dev/null +++ b/test/unconditional-recursion_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestUnconditionalRecursion(t *testing.T) { + testRule(t, "unconditional-recursion", &rule.UnconditionalRecursionRule{}) +} diff --git a/testdata/unconditional-recursion.go b/testdata/unconditional-recursion.go new file mode 100644 index 0000000..7a3d30f --- /dev/null +++ b/testdata/unconditional-recursion.go @@ -0,0 +1,180 @@ +package fixtures + +import ( + "log" + "os" + "time" +) + +func ur1() { + ur1() // MATCH /unconditional recursive call/ +} + +func ur1bis() { + if true { + print() + } else { + switch { + case true: + println() + default: + for i := 0; i < 10; i++ { + print() + } + } + + } + + ur1bis() // MATCH /unconditional recursive call/ +} + +func ur2tris() { + for { + println() + ur2tris() // MATCH /unconditional recursive call/ + } +} + +func ur2() { + if true { + return + } + + ur2() +} + +func ur3() { + ur1() +} + +func urn4() { + if true { + print() + } else if false { + return + } + + ur4() +} + +func urn5() { + if true { + return + } + + if true { + println() + } + + ur5() +} + +func ur2tris() { + for true == false { + println() + ur2tris() + } +} + +type myType struct { + foo int + bar int +} + +func (mt *myType) Foo() int { + return mt.Foo() // MATCH /unconditional recursive call/ +} + +func (mt *myType) Bar() int { + return mt.bar +} + +func ur6() { + switch { + case true: + return + default: + println() + } + + ur6() +} + +func ur7(a interface{}) { + switch a.(type) { + case int: + return + default: + println() + } + + ur7() +} + +func ur8(a []int) { + for _, i := range a { + return + } + + ur8() +} + +func ur9(a []int) { + for _, i := range a { + ur9() + } +} + +func ur10() { + select { + case <-aChannel: + case <-time.After(2 * time.Second): + return + } + ur10() +} + +func ur11() { + go ur11() +} + +func ur12() { + go foo(ur12()) // MATCH /unconditional recursive call/ + go bar(1, "string", ur12(), 1.0) // MATCH /unconditional recursive call/ + go foo(bar()) +} + +func urn13() { + if true { + panic("") + } + urn13() +} + +func urn14() { + if true { + os.Exit(1) + } + urn14() +} + +func urn15() { + if true { + log.Panic("") + } + urn15() +} + +func urn16(ch chan int) { + for range ch { + log.Panic("") + } + urn16(ch) +} + +func urn17(ch chan int) { + for range ch { + print("") + } + urn17(ch) // MATCH /unconditional recursive call/ +}