Merge branch 'master' of github.com:mgechev/revive

* 'master' of github.com:mgechev/revive:
  add-constant (new rule) (#39)
  New rule: unreachable-code (#38)
  New formatter: checkstyle (#37)
  fix modifies-parameter name in README (#36)

README.md

@@ -96,7 +96,8 @@ go get -u github.com/mgechev/revive
   - `ndjson` - outputs the failures as stream in newline delimited JSON (NDJSON) format.
   - `friendly` - outputs the failures when found. Shows summary of all the failures.
   - `stylish` - formats the failures in a table. Keep in mind that it doesn't stream the output so it might be perceived as slower compared to others.
-
+  - `checkstyle` - outputs the failures in XML format compatible with that of Java's [Checkstyle](https://checkstyle.org/).
+  
 ### Sample Invocations
 
 ```shell
@@ -215,10 +216,12 @@ 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   |
+| `modifies-parameter`  |  n/a   | Warns on assignments to function parameters                      |    no    |  no   |
 | `confusing-results`   |  n/a   | Suggests to name potentially confusing function results          |    no    |  no   |
 | `deep-exit`           |  n/a   | Looks for program exits in funcs other than `main()` or `init()` |    no    |  no   |
 | `unused-parameter`    |  n/a   | Suggests to rename or remove unused function parameters          |    no    |  no   |
+| `unreachable-code`    |  n/a   | Warns on unreachable code                                        |    no    |  no   |
+| `add-constant`        |  map   | Suggests using constant for magic numbers and string literals |    no    |  no   |
 
 ## Available Formatters
 

config.go

@@ -55,6 +55,8 @@ var allRules = append([]lint.Rule{
 	&rule.ConfusingResultsRule{},
 	&rule.DeepExitRule{},
 	&rule.UnusedParamRule{},
+	&rule.UnreachableCodeRule{},
+	&rule.AddConstantRule{},
 }, defaultRules...)
 
 var allFormatters = []lint.Formatter{
@@ -63,6 +65,7 @@ var allFormatters = []lint.Formatter{
 	&formatter.JSON{},
 	&formatter.NDJSON{},
 	&formatter.Default{},
+	&formatter.Checkstyle{},
 }
 
 func getFormatters() map[string]lint.Formatter {

fixtures/add-constant.go

@@ -0,0 +1,16 @@
+package fixtures
+
+func foo(a, b, c, d int) {
+	a = 1.0 // ignore
+	b = "ignore"
+	c = 2              // ignore
+	println("lit", 12) // MATCH /avoid magic numbers like '12', create a named constant for it/
+	if a == 12.50 {    // MATCH /avoid magic numbers like '12.50', create a named constant for it/
+		if b == "lit" {
+			c = "lit" // MATCH /string literal "lit" appears, at least, 3 times, create a named constant for it/
+		}
+		for i := 0; i < 1; i++ {
+			println("lit")
+		}
+	}
+}

fixtures/unreachable-code.go

@@ -0,0 +1,37 @@
+package fixtures
+
+import (
+	"fmt"
+	"log"
+	"os"
+)
+
+func foo() int {
+	log.Fatalf("%s", "About to fail") // ignore
+	return 0                          // MATCH /unreachable code after this statement/
+	return 1
+	Println("unreachable")
+}
+
+func f() {
+	fmt.Println("Hello, playground")
+	if true {
+		return // MATCH /unreachable code after this statement/
+		Println("unreachable")
+		os.Exit(2) // ignore
+		Println("also unreachable")
+	}
+	return // MATCH /unreachable code after this statement/
+	fmt.Println("Bye, playground")
+}
+
+func g() {
+	fmt.Println("Hello, playground")
+	if true {
+		return // ignore if next stmt is labeled
+	label:
+		os.Exit(2) // ignore
+	}
+
+	fmt.Println("Bye, playground")
+}

formatter/checkstyle.go

@@ -0,0 +1,76 @@
+package formatter
+
+import (
+	"bytes"
+	"encoding/xml"
+	"github.com/mgechev/revive/lint"
+	plainTemplate "text/template"
+)
+
+// Checkstyle is an implementation of the Formatter interface
+// which formats the errors to Checkstyle-like format.
+type Checkstyle struct {
+	Metadata lint.FormatterMetadata
+}
+
+// Name returns the name of the formatter
+func (f *Checkstyle) Name() string {
+	return "checkstyle"
+}
+
+type issue struct {
+	Line       int
+	Col        int
+	What       string
+	Confidence float64
+	Severity   lint.Severity
+	RuleName   string
+}
+
+// Format formats the failures gotten from the lint.
+func (f *Checkstyle) Format(failures <-chan lint.Failure, config lint.RulesConfig) (string, error) {
+	var issues = map[string][]issue{}
+	for failure := range failures {
+		buf := new(bytes.Buffer)
+		xml.Escape(buf, []byte(failure.Failure))
+		what := buf.String()
+		iss := issue{
+			Line:       failure.Position.Start.Line,
+			Col:        failure.Position.Start.Column,
+			What:       what,
+			Confidence: failure.Confidence,
+			Severity:   severity(config, failure),
+			RuleName:   failure.RuleName,
+		}
+		fn := failure.GetFilename()
+		if issues[fn] == nil {
+			issues[fn] = make([]issue, 0)
+		}
+		issues[fn] = append(issues[fn], iss)
+	}
+
+	t, err := plainTemplate.New("revive").Parse(checkstyleTemplate)
+	if err != nil {
+		return "", err
+	}
+
+	buf := new(bytes.Buffer)
+
+	err = t.Execute(buf, issues)
+	if err != nil {
+		return "", err
+	}
+
+	return buf.String(), nil
+}
+
+const checkstyleTemplate = `<?xml version='1.0' encoding='UTF-8'?>
+<checkstyle version="5.0">
+{{- range $k, $v := . }}
+    <file name="{{ $k }}">
+      {{- range $i, $issue := $v }}
+      <error line="{{ $issue.Line }}" column="{{ $issue.Col }}" message="{{ $issue.What }} (confidence {{ $issue.Confidence}})" severity="{{ $issue.Severity }}" source="revive/{{ $issue.RuleName }}"/>
+      {{- end }}
+    </file>
+{{- end }}
+</checkstyle>`

rule/add-constant.go

@@ -0,0 +1,151 @@
+package rule
+
+import (
+	"fmt"
+	"github.com/mgechev/revive/lint"
+	"go/ast"
+	"strconv"
+	"strings"
+)
+
+const (
+	defaultStrLitLimit = 2
+	kindFLOAT          = "FLOAT"
+	kindINT            = "INT"
+	kindSTRING         = "STRING"
+)
+
+type whiteList map[string]map[string]bool
+
+func newWhiteList() whiteList {
+	return map[string]map[string]bool{kindINT: map[string]bool{}, kindFLOAT: map[string]bool{}, kindSTRING: map[string]bool{}}
+}
+
+func (wl whiteList) add(kind string, list string) {
+	elems := strings.Split(list, ",")
+	for _, e := range elems {
+		wl[kind][e] = true
+	}
+}
+
+// AddConstantRule lints unused params in functions.
+type AddConstantRule struct{}
+
+// Apply applies the rule to given file.
+func (r *AddConstantRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
+	strLitLimit := defaultStrLitLimit
+	var whiteList = newWhiteList()
+	if len(arguments) > 0 {
+		args, ok := arguments[0].(map[string]interface{})
+		if !ok {
+			panic(fmt.Sprintf("Invalid argument to the add-constant rule. Expecting a k,v map, got %T", arguments[0]))
+		}
+		for k, v := range args {
+			kind := ""
+			switch k {
+			case "allowFloats":
+				kind = kindFLOAT
+				fallthrough
+			case "allowInts":
+				if kind == "" {
+					kind = kindINT
+				}
+				fallthrough
+			case "allowStrs":
+				if kind == "" {
+					kind = kindSTRING
+				}
+				list, ok := v.(string)
+				if !ok {
+					panic(fmt.Sprintf("Invalid argument to the add-constant rule, string expected. Got '%v' (%T)", v, v))
+				}
+				whiteList.add(kind, list)
+			case "maxLitCount":
+				sl, ok := v.(string)
+				if !ok {
+					panic(fmt.Sprintf("Invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v' (%T)", v, v))
+				}
+
+				limit, err := strconv.Atoi(sl)
+				if err != nil {
+					panic(fmt.Sprintf("Invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v'", v))
+				}
+				strLitLimit = limit
+			}
+		}
+	}
+
+	var failures []lint.Failure
+
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	w := lintAddConstantRule{onFailure: onFailure, strLits: make(map[string]int, 0), strLitLimit: strLitLimit, whiteLst: whiteList}
+
+	ast.Walk(w, file.AST)
+
+	return failures
+}
+
+// Name returns the rule name.
+func (r *AddConstantRule) Name() string {
+	return "add-constant"
+}
+
+type lintAddConstantRule struct {
+	onFailure   func(lint.Failure)
+	strLits     map[string]int
+	strLitLimit int
+	whiteLst    whiteList
+}
+
+func (w lintAddConstantRule) Visit(node ast.Node) ast.Visitor {
+	switch n := node.(type) {
+	case *ast.GenDecl:
+		return nil // skip declarations
+	case *ast.BasicLit:
+		switch kind := n.Kind.String(); kind {
+		case kindFLOAT, kindINT:
+			w.checkNumLit(kind, n)
+		case kindSTRING:
+			w.checkStrLit(n)
+		}
+	}
+
+	return w
+
+}
+
+func (w lintAddConstantRule) checkStrLit(n *ast.BasicLit) {
+	if w.whiteLst[kindSTRING][n.Value] {
+		return
+	}
+
+	count := w.strLits[n.Value]
+	if count >= 0 {
+		w.strLits[n.Value] = count + 1
+		if w.strLits[n.Value] > w.strLitLimit {
+			w.onFailure(lint.Failure{
+				Confidence: 1,
+				Node:       n,
+				Category:   "style",
+				Failure:    fmt.Sprintf("string literal %s appears, at least, %d times, create a named constant for it", n.Value, w.strLits[n.Value]),
+			})
+			w.strLits[n.Value] = -1 // mark it to avoid failing again on the same literal
+		}
+	}
+}
+
+func (w lintAddConstantRule) checkNumLit(kind string, n *ast.BasicLit) {
+	if w.whiteLst[kind][n.Value] {
+		return
+	}
+
+	w.onFailure(lint.Failure{
+		Confidence: 1,
+		Node:       n,
+		Category:   "style",
+		Failure:    fmt.Sprintf("avoid magic numbers like '%s', create a named constant for it", n.Value),
+	})
+}

rule/unreachable-code.go

@@ -0,0 +1,114 @@
+package rule
+
+import (
+	"go/ast"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// UnreachableCodeRule lints unreachable code.
+type UnreachableCodeRule struct{}
+
+// Apply applies the rule to given file.
+func (r *UnreachableCodeRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	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 := lintUnreachableCode{onFailure, branchingFunctions}
+	ast.Walk(w, file.AST)
+	return failures
+}
+
+// Name returns the rule name.
+func (r *UnreachableCodeRule) Name() string {
+	return "unreachable-code"
+}
+
+type lintUnreachableCode struct {
+	onFailure          func(lint.Failure)
+	branchingFunctions map[string]map[string]bool
+}
+
+func (w lintUnreachableCode) Visit(node ast.Node) ast.Visitor {
+	blk, ok := node.(*ast.BlockStmt)
+	if !ok {
+		return w
+	}
+
+	if len(blk.List) < 2 {
+		return w
+	}
+loop:
+	for i, stmt := range blk.List[:len(blk.List)-1] {
+		// println("iterating ", len(blk.List))
+		next := blk.List[i+1]
+		if _, ok := next.(*ast.LabeledStmt); ok {
+			continue // skip if next statement is labeled
+		}
+
+		switch s := stmt.(type) {
+		case *ast.ReturnStmt:
+			w.onFailure(newUnreachableCodeFailure(s))
+			break loop
+		case *ast.BranchStmt:
+			token := s.Tok.String()
+			if token != "fallthrough" {
+				w.onFailure(newUnreachableCodeFailure(s))
+				break loop
+			}
+		case *ast.ExprStmt:
+			ce, ok := s.X.(*ast.CallExpr)
+			if !ok {
+				continue
+			}
+			// it's a function call
+			fc, ok := ce.Fun.(*ast.SelectorExpr)
+			if !ok {
+				continue
+			}
+
+			id, ok := fc.X.(*ast.Ident)
+
+			if !ok {
+				continue
+			}
+			fn := fc.Sel.Name
+			pkg := id.Name
+			if !w.branchingFunctions[pkg][fn] { // it isn't a call to a branching function
+				continue
+			}
+
+			if _, ok := next.(*ast.ReturnStmt); ok { // return statement needed to satisfy function signature
+				continue
+			}
+
+			w.onFailure(newUnreachableCodeFailure(s))
+			break loop
+		}
+	}
+
+	return w
+}
+
+func newUnreachableCodeFailure(node ast.Node) lint.Failure {
+	return lint.Failure{
+		Confidence: 1,
+		Node:       node,
+		Category:   "logic",
+		Failure:    "unreachable code after this statement",
+	}
+}

test/add-constant_test.go

@@ -0,0 +1,21 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/lint"
+	"github.com/mgechev/revive/rule"
+)
+
+func TestAddConstant(t *testing.T) {
+	args := []interface{}{map[string]interface{}{
+		"maxLitCount": "2",
+		"allowStrs":   "\"\"",
+		"allowInts":   "0,1,2",
+		"allowFloats": "0.0,1.0",
+	}}
+
+	testRule(t, "add-constant", &rule.AddConstantRule{}, &lint.RuleConfig{
+		Arguments: args,
+	})
+}

test/unreachable-code_test.go

@@ -0,0 +1,11 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/rule"
+)
+
+func TestUnreachableCode(t *testing.T) {
+	testRule(t, "unreachable-code", &rule.UnreachableCodeRule{})
+}