From 7e89359269beb8479cbcd7479930cb8140432c9e Mon Sep 17 00:00:00 2001 From: chavacava Date: Sun, 24 Jun 2018 09:24:50 +0200 Subject: [PATCH 1/2] 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, + } +} From b2532b3c33749e78ff3ce1ab95d4e1fb860a6bc2 Mon Sep 17 00:00:00 2001 From: chavacava Date: Sun, 24 Jun 2018 09:26:21 +0200 Subject: [PATCH 2/2] New rule: modifies-parameter (#25) * 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 * working version --- README.md | 1 + config.go | 1 + fixtures/modifies-param.go | 26 ++++++++++ rule/modifies-param.go | 94 +++++++++++++++++++++++++++++++++++++ test/modifies-param_test.go | 12 +++++ 5 files changed, 134 insertions(+) create mode 100644 fixtures/modifies-param.go create mode 100644 rule/modifies-param.go create mode 100644 test/modifies-param_test.go diff --git a/README.md b/README.md index b0af7a8..a16247f 100644 --- a/README.md +++ b/README.md @@ -212,6 +212,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | `superfluous-else` | n/a | Prevents redundant else statements (extends `indent-error-flow`) | no | no | | `confusing-naming` | n/a | Warns on methods with names that differ only by capitalization | no | no | | `get-return ` | n/a | Warns on getters that do not yield any result | no | no | +| `modifies-param` | n/a | Warns on assignments to function parameters | no | no | ## Available Formatters diff --git a/config.go b/config.go index e9d7c3e..385f303 100644 --- a/config.go +++ b/config.go @@ -50,6 +50,7 @@ var allRules = append([]lint.Rule{ &rule.EmptyBlockRule{}, &rule.SuperfluousElseRule{}, &rule.GetReturnRule{}, + &rule.ModifiesParamRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/fixtures/modifies-param.go b/fixtures/modifies-param.go new file mode 100644 index 0000000..350ad7f --- /dev/null +++ b/fixtures/modifies-param.go @@ -0,0 +1,26 @@ +package fixtures + +import ( + "go/ast" +) + +func one(a int) { + a,b:= 1,2 // MATCH /parameter 'a' seems to be modified/ + a++ // MATCH /parameter 'a' seems to be modified/ +} + +func two(b, c float32) { + if c>0.0 { + b = 1 // MATCH /parameter 'b' seems to be modified/ + } +} + +type foo struct { + a string +} + +func three(s *foo) { + s.a = "foooooo" +} + + diff --git a/rule/modifies-param.go b/rule/modifies-param.go new file mode 100644 index 0000000..f849fad --- /dev/null +++ b/rule/modifies-param.go @@ -0,0 +1,94 @@ +package rule + +import ( + "fmt" + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// ModifiesParamRule lints given else constructs. +type ModifiesParamRule struct{} + +// Apply applies the rule to given file. +func (r *ModifiesParamRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintModifiesParamRule{onFailure: onFailure} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *ModifiesParamRule) Name() string { + return "modifies-parameter" +} + +type lintModifiesParamRule struct { + params map[string]bool + onFailure func(lint.Failure) +} + +func retrieveParamNames(pl []*ast.Field) map[string]bool { + result := make(map[string]bool, len(pl)) + for _, p := range pl { + for _, n := range p.Names { + result[n.Name] = true + } + } + return result +} + +func checkAssign(lhs []ast.Expr, w lintModifiesParamRule) { + for _, e := range lhs { + id, ok := e.(*ast.Ident) + if ok { + if w.params[id.Name] { + w.onFailure(lint.Failure{ + Confidence: 0.5, + Node: id, + Category: "bad practice", + URL: "#modifies-parameter", + Failure: fmt.Sprintf("parameter '%s' seems to be a modified", id.Name), + }) + } + } + } +} + +func (w lintModifiesParamRule) Visit(node ast.Node) ast.Visitor { + switch v := node.(type) { + case *ast.FuncDecl: + w.params = retrieveParamNames(v.Type.Params.List) + case *ast.IncDecStmt: + if id, ok := v.X.(*ast.Ident); ok { + checkParam(id, &w) + } + case *ast.AssignStmt: + lhs := v.Lhs + for _, e := range lhs { + id, ok := e.(*ast.Ident) + if ok { + checkParam(id, &w) + } + } + } + + return w +} + +func checkParam(id *ast.Ident, w *lintModifiesParamRule) { + if w.params[id.Name] { + w.onFailure(lint.Failure{ + Confidence: 0.5, // confidence is low because of shadow variables + Node: id, + Category: "bad practice", + URL: "#modifies-parameter", + Failure: fmt.Sprintf("parameter '%s' seems to be modified", id), + }) + } +} diff --git a/test/modifies-param_test.go b/test/modifies-param_test.go new file mode 100644 index 0000000..c77a300 --- /dev/null +++ b/test/modifies-param_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// TestModifiesParam rule. +func TestModifiesParam(t *testing.T) { + testRule(t, "modifies-param", &rule.ModifiesParamRule{}) +}