From 7e89359269beb8479cbcd7479930cb8140432c9e Mon Sep 17 00:00:00 2001 From: chavacava Date: Sun, 24 Jun 2018 09:24:50 +0200 Subject: [PATCH] superfuous-else handles more cases (#24) * Adds rule superfluous-else (an extension of indent-error-flow) * Fix superfluous-else rule struct namming. * Adds superfuous-else rule to the rules table * Adds confusing-naming rule * adds multifile test * clean-up * fix config.go * superflous-else now detects "jumps" caused by functions like os.Exit and log.Fatal --- fixtures/superfluous-else.go | 84 ++++++++++++++++++++++++++++++++++++ rule/superfluous-else.go | 58 +++++++++++++++++++------ 2 files changed, 130 insertions(+), 12 deletions(-) diff --git a/fixtures/superfluous-else.go b/fixtures/superfluous-else.go index 436fa76..bca11e5 100644 --- a/fixtures/superfluous-else.go +++ b/fixtures/superfluous-else.go @@ -4,6 +4,7 @@ package pkg import ( + "os" "fmt" "log" ) @@ -71,3 +72,86 @@ func j(f func() bool) string { return "ok" } + +func fatal1() string { + if f() { + a := 1 + log.Fatal("x") + } else { // MATCH /if block ends with call to log.Fatal function, so drop this else and outdent its block/ + log.Printf("non-positive") + } + return "ok" +} + +func fatal2() string { + if f() { + a := 1 + log.Fatalf("x") + } else { // MATCH /if block ends with call to log.Fatalf function, so drop this else and outdent its block/ + log.Printf("non-positive") + } + return "ok" +} + +func fatal3() string { + if f() { + a := 1 + log.Fatalln("x") + } else { // MATCH /if block ends with call to log.Fatalln function, so drop this else and outdent its block/ + log.Printf("non-positive") + } + return "ok" +} + +func exit1() string { + if f() { + a := 1 + os.Exit(2) + } else { // MATCH /if block ends with call to os.Exit function, so drop this else and outdent its block/ + log.Printf("non-positive") + } + return "ok" +} + +func Panic1() string { + if f() { + a := 1 + log.Panic(2) + } else { // MATCH /if block ends with call to log.Panic function, so drop this else and outdent its block/ + log.Printf("non-positive") + } + return "ok" +} + +func Panic2() string { + if f() { + a := 1 + log.Panicf(2) + } else { // MATCH /if block ends with call to log.Panicf function, so drop this else and outdent its block/ + log.Printf("non-positive") + } + return "ok" +} + +func Panic3() string { + if f() { + a := 1 + log.Panicln(2) + } else { // MATCH /if block ends with call to log.Panicln function, so drop this else and outdent its block/ + log.Printf("non-positive") + } + return "ok" +} + +// noreg_19 no-regression test for issue #19 (https://github.com/mgechev/revive/issues/19) +func noreg_19(f func() bool, x int) string { + if err == author.ErrCourseNotFound { + break + } else if err == author.ErrCourseAccess { + // side effect + } else if err == author.AnotherError { + os.Exit(1) // "okay" + } else { + // side effect + } +} \ No newline at end of file diff --git a/rule/superfluous-else.go b/rule/superfluous-else.go index a46ddd4..21f0749 100644 --- a/rule/superfluous-else.go +++ b/rule/superfluous-else.go @@ -1,6 +1,7 @@ package rule import ( + "fmt" "go/ast" "go/token" @@ -13,12 +14,23 @@ type SuperfluousElseRule struct{} // Apply applies the rule to given file. func (r *SuperfluousElseRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { var failures []lint.Failure - onFailure := func(failure lint.Failure) { failures = append(failures, failure) } - w := lintSuperfluousElse{make(map[*ast.IfStmt]bool), onFailure} + var branchingFunctions = map[string]map[string]bool{ + "os": map[string]bool{"Exit": true}, + "log": map[string]bool{ + "Fatal": true, + "Fatalf": true, + "Fatalln": true, + "Panic": true, + "Panicf": true, + "Panicln": true, + }, + } + + w := lintSuperfluousElse{make(map[*ast.IfStmt]bool), onFailure, branchingFunctions} ast.Walk(w, file.AST) return failures } @@ -29,8 +41,9 @@ func (r *SuperfluousElseRule) Name() string { } type lintSuperfluousElse struct { - ignore map[*ast.IfStmt]bool - onFailure func(lint.Failure) + ignore map[*ast.IfStmt]bool + onFailure func(lint.Failure) + branchingFunctions map[string]map[string]bool } func (w lintSuperfluousElse) Visit(node ast.Node) ast.Visitor { @@ -39,6 +52,9 @@ func (w lintSuperfluousElse) Visit(node ast.Node) ast.Visitor { return w } if w.ignore[ifStmt] { + if elseif, ok := ifStmt.Else.(*ast.IfStmt); ok { + w.ignore[elseif] = true + } return w } if elseif, ok := ifStmt.Else.(*ast.IfStmt); ok { @@ -64,18 +80,36 @@ func (w lintSuperfluousElse) Visit(node ast.Node) ast.Visitor { } lastStmt := ifStmt.Body.List[len(ifStmt.Body.List)-1] - if stmt, ok := lastStmt.(*ast.BranchStmt); ok { + switch stmt := lastStmt.(type) { + case *ast.BranchStmt: token := stmt.Tok.String() if token != "fallthrough" { - w.onFailure(lint.Failure{ - Confidence: 1, - Node: ifStmt.Else, - Category: "indent", - URL: "#indent-error-flow", - Failure: "if block ends with a " + token + " statement, so drop this else and outdent its block" + extra, - }) + w.onFailure(newFailure(ifStmt.Else, "if block ends with a "+token+" statement, so drop this else and outdent its block"+extra)) + } + case *ast.ExprStmt: + if ce, ok := stmt.X.(*ast.CallExpr); ok { // it's a function call + if fc, ok := ce.Fun.(*ast.SelectorExpr); ok { + if id, ok := fc.X.(*ast.Ident); ok { + fn := fc.Sel.Name + pkg := id.Name + if w.branchingFunctions[pkg][fn] { // it's a call to a branching function + w.onFailure( + newFailure(ifStmt.Else, fmt.Sprintf("if block ends with call to %s.%s function, so drop this else and outdent its block%s", pkg, fn, extra))) + } + } + } } } return w } + +func newFailure(node ast.Node, msg string) lint.Failure { + return lint.Failure{ + Confidence: 1, + Node: node, + Category: "indent", + URL: "#indent-error-flow", + Failure: msg, + } +}