From 7d066071ce53d2d737c37fc582844440d2fffbf4 Mon Sep 17 00:00:00 2001 From: mgechev Date: Thu, 25 Jan 2018 10:35:27 -0800 Subject: [PATCH] Add rules --- fixtures/else.go | 23 ++++++ fixtures/if-return.go | 101 ++++++++++++++++++++++++ fixtures/range.go | 37 +++++++++ lint/file.go | 5 ++ rule/else-error.go | 107 -------------------------- rule/{no-else-return.go => else.go} | 17 ++-- rule/if-return.go | 115 ++++++++++++++++++++++++++++ rule/range.go | 95 +++++++++++++++++++++++ rule/ranges.go | 58 -------------- rule/var-declarations.go | 1 + testutil/lint_test.go | 5 +- 11 files changed, 391 insertions(+), 173 deletions(-) create mode 100644 fixtures/else.go create mode 100644 fixtures/if-return.go create mode 100644 fixtures/range.go delete mode 100644 rule/else-error.go rename rule/{no-else-return.go => else.go} (76%) create mode 100644 rule/if-return.go create mode 100644 rule/range.go delete mode 100644 rule/ranges.go diff --git a/fixtures/else.go b/fixtures/else.go new file mode 100644 index 0000000..e0a7d8c --- /dev/null +++ b/fixtures/else.go @@ -0,0 +1,23 @@ +// Test of return+else warning. + +// Package pkg ... +package pkg + +import "log" + +func f(x int) bool { + if x > 0 { + return true + } else { // MATCH /if block ends with a return statement, so drop this else and outdent its block/ + log.Printf("non-positive x: %d", x) + } + return false +} + +func g(f func() bool) string { + if ok := f(); ok { + return "it's okay" + } else { // MATCH /if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)/ + return "it's NOT okay!" + } +} diff --git a/fixtures/if-return.go b/fixtures/if-return.go new file mode 100644 index 0000000..7dca822 --- /dev/null +++ b/fixtures/if-return.go @@ -0,0 +1,101 @@ +// Test of redundant if err != nil + +// Package pkg ... +package pkg + +func f() error { + if err := f(); err != nil { + g() + return err + } + return nil +} + +func g() error { + if err := f(); err != nil { // MATCH /redundant if ...; err != nil check, just return error instead./ + return err + } + return nil +} + +func h() error { + if err, x := f(), 1; err != nil { + return err + } + return nil +} + +func i() error { + a := 1 + if err := f(); err != nil { + a++ + return err + } + return nil +} + +func j() error { + var a error + if err := f(); err != nil { + return err + } + return a +} + +func k() error { + if err := f(); err != nil { + // TODO: handle error better + return err + } + return nil +} + +func l() (interface{}, error) { + if err := f(); err != nil { + return nil, err + } + if err := f(); err != nil { + return nil, err + } + if err := f(); err != nil { + return nil, err + } + // Phew, it worked + return nil +} + +func m() error { + if err := f(); err != nil { + return err + } + if err := f(); err != nil { + return err + } + if err := f(); err != nil { + return err + } + // Phew, it worked again. + return nil +} + +func multi() error { + a := 0 + var err error + // unreachable code after return statements is intentional to check that it + // doesn't confuse the linter. + if true { + a++ + if err := f(); err != nil { // MATCH /redundant if ...; err != nil check, just return error instead./ + return err + } + return nil + a++ + } else { + a++ + if err = f(); err != nil { // MATCH /redundant if ...; err != nil check, just return error instead./ + return err + } + return nil + a++ + } +} diff --git a/fixtures/range.go b/fixtures/range.go new file mode 100644 index 0000000..b48f820 --- /dev/null +++ b/fixtures/range.go @@ -0,0 +1,37 @@ +// Test for range construction. + +// Package foo ... +package foo + +func f() { + var m map[string]int + + // with := + for x, _ := range m { // MATCH /should omit 2nd value from range; this loop is equivalent to `for x := range ...`/ + _ = x + } + // with = + var y string + _ = y + for y, _ = range m { // MATCH /should omit 2nd value from range; this loop is equivalent to `for y = range ...`/ + } + + // all OK: + for x := range m { + _ = x + } + for x, y := range m { + _, _ = x, y + } + for _, y := range m { + _ = y + } + var x int + _ = x + for y = range m { + } + for y, x = range m { + } + for _, x = range m { + } +} diff --git a/lint/file.go b/lint/file.go index 72777a6..caa02ad 100644 --- a/lint/file.go +++ b/lint/file.go @@ -23,6 +23,11 @@ type File struct { // IsTest returns if the file contains tests. func (f *File) IsTest() bool { return strings.HasSuffix(f.Name, "_test.go") } +// Content returns the file's content. +func (f *File) Content() []byte { + return f.content +} + // NewFile creates a new file func NewFile(name string, content []byte, pkg *Package) (*File, error) { f, err := parser.ParseFile(pkg.fset, name, content, parser.ParseComments) diff --git a/rule/else-error.go b/rule/else-error.go deleted file mode 100644 index 9f1d8a7..0000000 --- a/rule/else-error.go +++ /dev/null @@ -1,107 +0,0 @@ -package rule - -// // LintElseRule lints given else constructs. -// type LintElseRule struct{} - -// // Apply applies the rule to given file. -// func (r *LintElseRule) Apply(file *project.File, arguments rule.Arguments) []rule.Failure { -// var failures []rule.Failure - -// onFailure := func(failure rule.Failure) { -// failures = append(failures, failure) -// } - -// astFile := file.AST -// w := &lintElseError{astFile, onFailure} -// ast.Walk(w, astFile) -// return failures -// } - -// // Name returns the rule name. -// func (r *LintElseRule) Name() string { -// return "no-else-return" -// } - -// type lintElseError struct { -// file *ast.File -// onFailure func(rule.Failure) -// } - -// func (w *lintElseError) Visit(node ast.Node) ast.Visitor { -// switch v := node.(type) { -// case *ast.BlockStmt: -// for i := 0; i < len(v.List)-1; i++ { -// // if var := whatever; var != nil { return var } -// s, ok := v.List[i].(*ast.IfStmt) -// if !ok || s.Body == nil || len(s.Body.List) != 1 || s.Else != nil { -// continue -// } -// assign, ok := s.Init.(*ast.AssignStmt) -// if !ok || len(assign.Lhs) != 1 || !(assign.Tok == token.DEFINE || assign.Tok == token.ASSIGN) { -// continue -// } -// id, ok := assign.Lhs[0].(*ast.Ident) -// if !ok { -// continue -// } -// expr, ok := s.Cond.(*ast.BinaryExpr) -// if !ok || expr.Op != token.NEQ { -// continue -// } -// if lhs, ok := expr.X.(*ast.Ident); !ok || lhs.Name != id.Name { -// continue -// } -// if rhs, ok := expr.Y.(*ast.Ident); !ok || rhs.Name != "nil" { -// continue -// } -// r, ok := s.Body.List[0].(*ast.ReturnStmt) -// if !ok || len(r.Results) != 1 { -// continue -// } -// if r, ok := r.Results[0].(*ast.Ident); !ok || r.Name != id.Name { -// continue -// } - -// // return nil -// r, ok = v.List[i+1].(*ast.ReturnStmt) -// if !ok || len(r.Results) != 1 { -// continue -// } -// if r, ok := r.Results[0].(*ast.Ident); !ok || r.Name != "nil" { -// continue -// } - -// // check if there are any comments explaining the construct, don't emit an error if there are some. -// if containsComments(w.file, s.Pos(), r.Pos()) { -// continue -// } - -// w.onFailure(rule.Failure{ -// Confidence: 0.9, -// Failure: "redundant if ...; err != nil check, just return error instead.", -// Node: v.List[i], -// }) -// } -// } -// return w -// } - -// func containsComments(f *ast.File, start, end token.Pos) bool { -// for _, cgroup := range f.Comments { -// comments := cgroup.List -// if comments[0].Slash >= end { -// // All comments starting with this group are after end pos. -// return false -// } -// if comments[len(comments)-1].Slash < start { -// // Comments group ends before start pos. -// continue -// } -// for _, c := range comments { -// if start <= c.Slash && c.Slash < end && !strings.HasPrefix(c.Text, "// MATCH ") { -// return true -// } -// } -// } -// return false -// } diff --git a/rule/no-else-return.go b/rule/else.go similarity index 76% rename from rule/no-else-return.go rename to rule/else.go index 33cd52b..5d226cc 100644 --- a/rule/no-else-return.go +++ b/rule/else.go @@ -7,11 +7,11 @@ import ( "github.com/mgechev/revive/lint" ) -// LintElseRule lints given else constructs. -type LintElseRule struct{} +// ElseRule lints given else constructs. +type ElseRule struct{} // Apply applies the rule to given file. -func (r *LintElseRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ElseRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { @@ -24,8 +24,8 @@ func (r *LintElseRule) Apply(file *lint.File, arguments lint.Arguments) []lint.F } // Name returns the rule name. -func (r *LintElseRule) Name() string { - return "no-else-return" +func (r *ElseRule) Name() string { + return "else" } type lintElse struct { @@ -65,8 +65,11 @@ func (w lintElse) Visit(node ast.Node) ast.Visitor { extra = " (move short variable declaration to its own line if necessary)" } w.onFailure(lint.Failure{ - Failure: "if block ends with a return statement, so drop this else and outdent its block" + extra, - Node: ifStmt.Else, + Confidence: 1, + Node: ifStmt.Else, + Category: "indent", + URL: "#indent-error-flow", + Failure: "if block ends with a return statement, so drop this else and outdent its block" + extra, }) } return w diff --git a/rule/if-return.go b/rule/if-return.go new file mode 100644 index 0000000..e53dd0a --- /dev/null +++ b/rule/if-return.go @@ -0,0 +1,115 @@ +package rule + +import ( + "go/ast" + "go/token" + "strings" + + "github.com/mgechev/revive/lint" +) + +// IfReturnRule lints given else constructs. +type IfReturnRule struct{} + +// Apply applies the rule to given file. +func (r *IfReturnRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + astFile := file.AST + w := &lintElseError{astFile, onFailure} + ast.Walk(w, astFile) + return failures +} + +// Name returns the rule name. +func (r *IfReturnRule) Name() string { + return "if-return" +} + +type lintElseError struct { + file *ast.File + onFailure func(lint.Failure) +} + +func (w *lintElseError) Visit(node ast.Node) ast.Visitor { + switch v := node.(type) { + case *ast.BlockStmt: + for i := 0; i < len(v.List)-1; i++ { + // if var := whatever; var != nil { return var } + s, ok := v.List[i].(*ast.IfStmt) + if !ok || s.Body == nil || len(s.Body.List) != 1 || s.Else != nil { + continue + } + assign, ok := s.Init.(*ast.AssignStmt) + if !ok || len(assign.Lhs) != 1 || !(assign.Tok == token.DEFINE || assign.Tok == token.ASSIGN) { + continue + } + id, ok := assign.Lhs[0].(*ast.Ident) + if !ok { + continue + } + expr, ok := s.Cond.(*ast.BinaryExpr) + if !ok || expr.Op != token.NEQ { + continue + } + if lhs, ok := expr.X.(*ast.Ident); !ok || lhs.Name != id.Name { + continue + } + if rhs, ok := expr.Y.(*ast.Ident); !ok || rhs.Name != "nil" { + continue + } + r, ok := s.Body.List[0].(*ast.ReturnStmt) + if !ok || len(r.Results) != 1 { + continue + } + if r, ok := r.Results[0].(*ast.Ident); !ok || r.Name != id.Name { + continue + } + + // return nil + r, ok = v.List[i+1].(*ast.ReturnStmt) + if !ok || len(r.Results) != 1 { + continue + } + if r, ok := r.Results[0].(*ast.Ident); !ok || r.Name != "nil" { + continue + } + + // check if there are any comments explaining the construct, don't emit an error if there are some. + if containsComments(s.Pos(), r.Pos(), w.file) { + continue + } + + w.onFailure(lint.Failure{ + Confidence: .9, + Node: v.List[i], + Failure: "redundant if ...; err != nil check, just return error instead.", + }) + } + } + return w +} + +func containsComments(start, end token.Pos, f *ast.File) bool { + for _, cgroup := range f.Comments { + comments := cgroup.List + if comments[0].Slash >= end { + // All comments starting with this group are after end pos. + return false + } + if comments[len(comments)-1].Slash < start { + // Comments group ends before start pos. + continue + } + for _, c := range comments { + if start <= c.Slash && c.Slash < end && !strings.HasPrefix(c.Text, "// MATCH ") { + return true + } + } + } + return false +} diff --git a/rule/range.go b/rule/range.go new file mode 100644 index 0000000..fa6093c --- /dev/null +++ b/rule/range.go @@ -0,0 +1,95 @@ +package rule + +import ( + "fmt" + "go/ast" + "go/token" + "strings" + + "github.com/mgechev/revive/lint" +) + +// RangeRule lints given else constructs. +type RangeRule struct{} + +// Apply applies the rule to given file. +func (r *RangeRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := &lintRanges{file, onFailure} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *RangeRule) Name() string { + return "range" +} + +type lintRanges struct { + file *lint.File + onFailure func(lint.Failure) +} + +func (w *lintRanges) Visit(node ast.Node) ast.Visitor { + rs, ok := node.(*ast.RangeStmt) + if !ok { + return w + } + if rs.Value == nil { + // for x = range m { ... } + return w // single var form + } + if !isIdent(rs.Value, "_") { + // for ?, y = range m { ... } + return w + } + + newRS := *rs // shallow copy + newRS.Value = nil + + w.onFailure(lint.Failure{ + Failure: fmt.Sprintf("should omit 2nd value from range; this loop is equivalent to `for %s %s range ...`", w.file.Render(rs.Key), rs.Tok), + Confidence: 1, + Node: rs.Value, + ReplacementLine: firstLineOf(w.file, &newRS, rs), + }) + + return w +} + +func firstLineOf(f *lint.File, node, match ast.Node) string { + line := f.Render(node) + if i := strings.Index(line, "\n"); i >= 0 { + line = line[:i] + } + return indentOf(f, match) + line +} + +func indentOf(f *lint.File, node ast.Node) string { + line := srcLine(f.Content(), f.ToPosition(node.Pos())) + for i, r := range line { + switch r { + case ' ', '\t': + default: + return line[:i] + } + } + return line // unusual or empty line +} + +func srcLine(src []byte, p token.Position) string { + // Run to end of line in both directions if not at line start/end. + lo, hi := p.Offset, p.Offset+1 + for lo > 0 && src[lo-1] != '\n' { + lo-- + } + for hi < len(src) && src[hi-1] != '\n' { + hi++ + } + return string(src[lo:hi]) +} diff --git a/rule/ranges.go b/rule/ranges.go deleted file mode 100644 index f8cebe8..0000000 --- a/rule/ranges.go +++ /dev/null @@ -1,58 +0,0 @@ -package rule - -import ( - "fmt" - "go/ast" - - "github.com/mgechev/revive/lint" -) - -// LintRangesRule lints given else constructs. -type LintRangesRule struct{} - -// Apply applies the rule to given file. -func (r *LintRangesRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { - var failures []lint.Failure - - onFailure := func(failure lint.Failure) { - failures = append(failures, failure) - } - - w := &lintRanges{file, onFailure} - ast.Walk(w, file.AST) - return failures -} - -// Name returns the rule name. -func (r *LintRangesRule) Name() string { - return "no-else-return" -} - -type lintRanges struct { - file *lint.File - onFailure func(lint.Failure) -} - -func (w *lintRanges) Visit(node ast.Node) ast.Visitor { - rs, ok := node.(*ast.RangeStmt) - if !ok { - return w - } - if rs.Value == nil { - // for x = range m { ... } - return w // single var form - } - if !isIdent(rs.Value, "_") { - // for ?, y = range m { ... } - return w - } - - w.onFailure(lint.Failure{ - Failure: fmt.Sprintf("should omit 2nd value from range; this loop is equivalent to `for %s %s range ...`", w.file.Render(rs.Key), rs.Tok), - Confidence: 1, - Node: rs.Value, - }) - - // TODO: replacement? - return w -} diff --git a/rule/var-declarations.go b/rule/var-declarations.go index 1266517..29a336b 100644 --- a/rule/var-declarations.go +++ b/rule/var-declarations.go @@ -74,6 +74,7 @@ func (w *lintVarDeclarations) Visit(node ast.Node) ast.Visitor { w.onFailure(lint.Failure{ Confidence: 0.9, Node: rhs, + Category: "zero-value", Failure: fmt.Sprintf("should drop = %s from declaration of var %s; it is the zero value", w.file.Render(rhs), v.Names[0]), }) return nil diff --git a/testutil/lint_test.go b/testutil/lint_test.go index 618fa1b..4b3f9d9 100644 --- a/testutil/lint_test.go +++ b/testutil/lint_test.go @@ -39,6 +39,9 @@ var rules = []lint.Rule{ &rule.BlankImportsRule{}, &rule.ExportedRule{}, &rule.NamesRule{}, + &rule.ElseRule{}, + &rule.IfReturnRule{}, + &rule.RangeRule{}, } func TestAll(t *testing.T) { @@ -74,7 +77,7 @@ func TestAll(t *testing.T) { continue } - ps, err := l.Lint([]string{fi.Name()}, rules, map[string]lint.Arguments{}) + ps, err := l.Lint([]string{fi.Name()}, rules, map[string]lint.RuleConfig{}) if err != nil { t.Errorf("Linting %s: %v", fi.Name(), err) continue