Add rules

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!"
+	}
+}

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++
+	}
+}

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 {
+	}
+}

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)

rule/else-error.go

@@ -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
-// }

rule/else.go

@@ -7,11 +7,11 @@ import (
 	"github.com/mgechev/revive/lint"
 )
 
-// LintElseRule lints given else constructs.
+// ElseRule lints given else constructs.
-type LintElseRule struct{}
+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 {
+func (r *ElseRule) Name() string {
-	return "no-else-return"
+	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,
+			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

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
+}

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])
+}

rule/ranges.go

@@ -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
-}

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

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