Add cyclomatic complexity and improve tests

fixtures/cyclomatic.go

@@ -0,0 +1,23 @@
+// Test of return+else warning.
+
+// Package pkg ...
+package pkg
+
+import "log"
+
+func f(x int) bool { // MATCH /function f has cyclomatic complexity 4/
+	if x > 0 && true || false {
+		return true
+	} else {
+		log.Printf("non-positive x: %d", x)
+	}
+	return false
+}
+
+func g(f func() bool) string { // MATCH /function g has cyclomatic complexity 2/
+	if ok := f(); ok {
+		return "it's okay"
+	} else {
+		return "it's NOT okay!"
+	}
+}

rule/cyclomatic.go

@@ -0,0 +1,116 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+	"strconv"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// Based on https://github.com/fzipp/gocyclo
+
+// CyclomaticRule lints given else constructs.
+type CyclomaticRule struct{}
+
+// Apply applies the rule to given file.
+func (r *CyclomaticRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	complexity, err := strconv.Atoi(arguments[0])
+	if err != nil {
+		panic("invalid argument for cyclomatic complexity")
+	}
+
+	fileAst := file.AST
+	walker := lintCyclomatic{
+		file:       file,
+		complexity: complexity,
+		onFailure: func(failure lint.Failure) {
+			failures = append(failures, failure)
+		},
+	}
+
+	ast.Walk(walker, fileAst)
+
+	return failures
+}
+
+// Name returns the rule name.
+func (r *CyclomaticRule) Name() string {
+	return "errorf"
+}
+
+type lintCyclomatic struct {
+	file       *lint.File
+	complexity int
+	onFailure  func(lint.Failure)
+}
+
+func (w lintCyclomatic) Visit(n ast.Node) ast.Visitor {
+	f := w.file
+	for _, decl := range f.AST.Decls {
+		if fn, ok := decl.(*ast.FuncDecl); ok {
+			c := complexity(fn)
+			if c > w.complexity {
+				w.onFailure(lint.Failure{
+					Confidence: 1,
+					Category:   "maintenance",
+					Failure:    fmt.Sprintf("function %s has cyclomatic complexity %d", funcName(fn), c),
+					Node:       fn,
+				})
+			}
+		}
+	}
+	return nil
+}
+
+// funcName returns the name representation of a function or method:
+// "(Type).Name" for methods or simply "Name" for functions.
+func funcName(fn *ast.FuncDecl) string {
+	if fn.Recv != nil {
+		if fn.Recv.NumFields() > 0 {
+			typ := fn.Recv.List[0].Type
+			return fmt.Sprintf("(%s).%s", recvString(typ), fn.Name)
+		}
+	}
+	return fn.Name.Name
+}
+
+// recvString returns a string representation of recv of the
+// form "T", "*T", or "BADRECV" (if not a proper receiver type).
+func recvString(recv ast.Expr) string {
+	switch t := recv.(type) {
+	case *ast.Ident:
+		return t.Name
+	case *ast.StarExpr:
+		return "*" + recvString(t.X)
+	}
+	return "BADRECV"
+}
+
+// complexity calculates the cyclomatic complexity of a function.
+func complexity(fn *ast.FuncDecl) int {
+	v := complexityVisitor{}
+	ast.Walk(&v, fn)
+	return v.Complexity
+}
+
+type complexityVisitor struct {
+	// Complexity is the cyclomatic complexity
+	Complexity int
+}
+
+// Visit implements the ast.Visitor interface.
+func (v *complexityVisitor) Visit(n ast.Node) ast.Visitor {
+	switch n := n.(type) {
+	case *ast.FuncDecl, *ast.IfStmt, *ast.ForStmt, *ast.RangeStmt, *ast.CaseClause, *ast.CommClause:
+		v.Complexity++
+	case *ast.BinaryExpr:
+		if n.Op == token.LAND || n.Op == token.LOR {
+			v.Complexity++
+		}
+	}
+	return v
+}

testutil/lint_test.go

@@ -19,6 +19,7 @@ import (
 	"go/token"
 	"go/types"
 	"io/ioutil"
+	"os"
 	"path"
 	"regexp"
 	"strconv"
@@ -26,6 +27,7 @@ import (
 	"testing"
 
 	"github.com/mgechev/revive/rule"
+	"github.com/pkg/errors"
 
 	"github.com/mgechev/revive/lint"
 )
@@ -54,11 +56,44 @@ var rules = []lint.Rule{
 	&rule.ContextArgumentsRule{},
 }
 
+func TestVarDeclaration(t *testing.T) {
+	testRule(t, "cyclomatic", &rule.CyclomaticRule{}, &lint.RuleConfig{
+		Arguments: []string{"1"},
+	})
+}
+
+func testRule(t *testing.T, filename string, rule lint.Rule, config ...*lint.RuleConfig) {
+	baseDir := "../fixtures/"
+	filename = filename + ".go"
+	src, err := ioutil.ReadFile(baseDir + filename)
+	if err != nil {
+		t.Fatalf("Bad filename path in test for %s: %v", rule.Name(), err)
+	}
+	stat, err := os.Stat(baseDir + filename)
+	if err != nil {
+		t.Fatalf("Cannot get file info for %s: %v", rule.Name(), err)
+	}
+	ins := parseInstructions(t, filename, src)
+	if ins == nil {
+		t.Errorf("Test file %v does not have instructions", filename)
+		return
+	}
+	if config == nil {
+		assertFailures(t, baseDir, stat, src, []lint.Rule{rule}, map[string]lint.RuleConfig{})
+		return
+	}
+	c := map[string]lint.RuleConfig{}
+	c[rule.Name()] = *config[0]
+	assertFailures(t, baseDir, stat, src, []lint.Rule{rule}, c)
+}
+
 func TestAll(t *testing.T) {
 	baseDir := "../fixtures/"
-	l := lint.New(func(file string) ([]byte, error) {
-		return ioutil.ReadFile(baseDir + file)
-	})
+
+	ignoreFiles := map[string]bool{
+		"cyclomatic.go": true,
+	}
+
 	rx, err := regexp.Compile(*lintMatch)
 	if err != nil {
 		t.Fatalf("Bad -lint.match value %q: %v", *lintMatch, err)
@@ -75,65 +110,80 @@ func TestAll(t *testing.T) {
 		if !rx.MatchString(fi.Name()) {
 			continue
 		}
+		if _, ok := ignoreFiles[fi.Name()]; ok {
+			continue
+		}
 		//t.Logf("Testing %s", fi.Name())
 		src, err := ioutil.ReadFile(path.Join(baseDir, fi.Name()))
 		if err != nil {
 			t.Fatalf("Failed reading %s: %v", fi.Name(), err)
 		}
 
-		ins := parseInstructions(t, fi.Name(), src)
-		if ins == nil {
-			t.Errorf("Test file %v does not have instructions", fi.Name())
-			continue
-		}
-
-		ps, err := l.Lint([]string{fi.Name()}, rules, map[string]lint.RuleConfig{})
+		err = assertFailures(t, baseDir, fi, src, rules, map[string]lint.RuleConfig{})
 		if err != nil {
 			t.Errorf("Linting %s: %v", fi.Name(), err)
 			continue
 		}
+	}
+}
 
-		failures := []lint.Failure{}
-		for f := range ps {
-			failures = append(failures, f)
-		}
+func assertFailures(t *testing.T, baseDir string, fi os.FileInfo, src []byte, rules []lint.Rule, config map[string]lint.RuleConfig) error {
+	l := lint.New(func(file string) ([]byte, error) {
+		return ioutil.ReadFile(baseDir + file)
+	})
 
-		for _, in := range ins {
-			ok := false
-			for i, p := range failures {
-				if p.Position.Start.Line != in.Line {
-					continue
-				}
-				if in.Match == p.Failure {
-					// check replacement if we are expecting one
-					if in.Replacement != "" {
-						// ignore any inline comments, since that would be recursive
-						r := p.ReplacementLine
-						if i := strings.Index(r, " //"); i >= 0 {
-							r = r[:i]
-						}
-						if r != in.Replacement {
-							t.Errorf("Lint failed at %s:%d; got replacement %q, want %q", fi.Name(), in.Line, r, in.Replacement)
-						}
+	ins := parseInstructions(t, fi.Name(), src)
+	if ins == nil {
+		return errors.Errorf("Test file %v does not have instructions", fi.Name())
+	}
+
+	ps, err := l.Lint([]string{fi.Name()}, rules, config)
+	if err != nil {
+
+		return err
+	}
+
+	failures := []lint.Failure{}
+	for f := range ps {
+		failures = append(failures, f)
+	}
+
+	for _, in := range ins {
+		ok := false
+		for i, p := range failures {
+			if p.Position.Start.Line != in.Line {
+				continue
+			}
+			if in.Match == p.Failure {
+				// check replacement if we are expecting one
+				if in.Replacement != "" {
+					// ignore any inline comments, since that would be recursive
+					r := p.ReplacementLine
+					if i := strings.Index(r, " //"); i >= 0 {
+						r = r[:i]
+					}
+					if r != in.Replacement {
+						t.Errorf("Lint failed at %s:%d; got replacement %q, want %q", fi.Name(), in.Line, r, in.Replacement)
 					}
-
-					// remove this problem from ps
-					copy(failures[i:], failures[i+1:])
-					failures = failures[:len(failures)-1]
-
-					// t.Logf("/%v/ matched at %s:%d", in.Match, fi.Name(), in.Line)
-					ok = true
-					break
 				}
-			}
-			if !ok {
-				t.Errorf("Lint failed at %s:%d; /%v/ did not match", fi.Name(), in.Line, in.Match)
+
+				// remove this problem from ps
+				copy(failures[i:], failures[i+1:])
+				failures = failures[:len(failures)-1]
+
+				// t.Logf("/%v/ matched at %s:%d", in.Match, fi.Name(), in.Line)
+				ok = true
+				break
 			}
 		}
-		for _, p := range failures {
-			t.Errorf("Unexpected problem at %s:%d: %v", fi.Name(), p.Position.Start.Line, p.Failure)
+		if !ok {
+			t.Errorf("Lint failed at %s:%d; /%v/ did not match", fi.Name(), in.Line, in.Match)
 		}
 	}
+	for _, p := range failures {
+		t.Errorf("Unexpected problem at %s:%d: %v", fi.Name(), p.Position.Start.Line, p.Failure)
+	}
+	return nil
 }
 
 type instruction struct {