Merge pull request #269 from chavacava/rule-cognitive-complexity

Rule cognitive complexity

README.md

@@ -346,6 +346,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`bare-return`](./RULES_DESCRIPTIONS.md#bare-return) | n/a  | Warns on bare returns   |    no    |  no   |
 | [`unused-receiver`](./RULES_DESCRIPTIONS.md#unused-receiver)   | n/a    | Suggests to rename or remove unused method receivers    |    no    |  no   |
 | [`unhandled-error`](./RULES_DESCRIPTIONS.md#unhandled-error)   | []string   | Warns on unhandled errors returned by funcion calls    |    no    |  yes   |
+| [`cognitive-complexity`](./RULES_DESCRIPTIONS.md#cognitive-complexity)          |  int   | Sets restriction for maximum Cognitive complexity.              |    no    |  no   |
 
 ## Configurable rules
 

RULES_DESCRIPTIONS.md

@@ -13,6 +13,7 @@ List of all available rules.
   - [call-to-gc](#call-to-gc)
   - [confusing-naming](#confusing-naming)
   - [confusing-results](#confusing-results)
+  - [cognitive-complexity](#cognitive-complexity)
   - [constant-logical-expr](#constant-logical-expr)
   - [context-as-argument](#context-as-argument)
   - [context-keys-type](#context-keys-type)
@@ -122,6 +123,21 @@ The garbage collector can be configured through environment variables as describ
 
 _Configuration_: N/A
 
+## cognitive-complexity
+
+_Description_: [Cognitive complexity](https://www.sonarsource.com/resources/white-papers/cognitive-complexity.html) is a measure of how hard code is to understand. 
+While cyclomatic complexity is good to measure "testeability" of the code, cognitive complexity aims to provide a more precise measure of the difficulty of understanding the code.
+Enforcing a maximum complexity per function helps to keep code readable and maintainable.
+
+_Configuration_: (int) the maximum function complexity
+
+Example:
+
+```toml
+[rule.cognitive-complexity]
+  arguments =[7]
+```
+
 ## confusing-naming
 
 _Description_: Methods or fields of `struct` that have names different only by capitalization could be confusing.

config.go

@@ -80,6 +80,7 @@ var allRules = append([]lint.Rule{
 	&rule.BareReturnRule{},
 	&rule.UnusedReceiverRule{},
 	&rule.UnhandledErrorRule{},
+	&rule.CognitiveComplexityRule{},
 }, defaultRules...)
 
 var allFormatters = []lint.Formatter{

fixtures/cognitive-complexity.go

@@ -0,0 +1,281 @@
+// Test of cognitive complexity.
+
+// Package pkg ...
+package pkg
+
+import (
+	"fmt"
+	ast "go/ast"
+	"log"
+	"testing"
+
+	"github.com/blang/semver"
+	"k8s.io/klog"
+)
+
+// Test IF and Boolean expr
+func f(x int) bool { // MATCH /function f has cognitive complexity 3 (> max enabled 0)/
+	if x > 0 && true || false { // +3
+		return true
+	} else {
+		log.Printf("non-positive x: %d", x)
+	}
+	return false
+}
+
+// Test IF
+func g(f func() bool) string { // MATCH /function g has cognitive complexity 1 (> max enabled 0)/
+	if ok := f(); ok { // +1
+		return "it's okay"
+	} else {
+		return "it's NOT okay!"
+	}
+}
+
+// Test Boolean expr
+func h(a, b, c, d, e, f bool) bool { // MATCH /function h has cognitive complexity 3 (> max enabled 0)/
+	return a && b && c || d || e && f // +3
+}
+
+func i(a, b, c, d, e, f bool) bool { // MATCH /function i has cognitive complexity 2 (> max enabled 0)/
+	result := a && b && c || d || e // +2
+	return result
+}
+
+func j(a, b, c, d, e, f bool) bool { // MATCH /function j has cognitive complexity 2 (> max enabled 0)/
+	result := z(a && !(b && c)) // +2
+	return result
+}
+
+func j1(a, b, c, d, e, f bool) bool { // MATCH /function j1 has cognitive complexity 2 (> max enabled 0)/
+	return (a && !(b < 2) || c)
+}
+
+// Test Switch expr
+func k(a, b, c, d, e, f bool) bool { // MATCH /function k has cognitive complexity 1 (> max enabled 0)/
+	switch expr { // +1
+	case cond1:
+	case cond2:
+	default:
+	}
+
+	return result
+}
+
+// Test nesting FOR expr + nested IF
+func l() { // MATCH /function l has cognitive complexity 6 (> max enabled 0)/
+	for i := 1; i <= max; i++ { // +1
+		for j := 2; j < i; j++ { // +1 +1(nesting)
+			if i%j == 0 { // +1 +2(nesting)
+				continue
+			}
+		}
+
+		total += i
+	}
+	return total
+}
+
+// Test nesting IF
+func m() { // MATCH /function m has cognitive complexity 6 (> max enabled 0)/
+	if i <= max { // +1
+		if j < i { // +1 +1(nesting)
+			if i%j == 0 { // +1 +2(nesting)
+				return 0
+			}
+		}
+
+		total += i
+	}
+	return total
+}
+
+// Test nesting IF + nested FOR
+func n() { // MATCH /function n has cognitive complexity 6 (> max enabled 0)/
+	if i > max { // +1
+		for j := 2; j < i; j++ { // +1 +1(nesting)
+			if i%j == 0 { // +1 +2(nesting)
+				continue
+			}
+		}
+
+		total += i
+	}
+	return total
+}
+
+// Test nesting
+func o() { // MATCH /function o has cognitive complexity 12 (> max enabled 0)/
+	if i > max { // +1
+		if j < i { // +1 +1(nesting)
+			if i%j == 0 { // +1 +2(nesting)
+				return
+			}
+		}
+
+		total += i
+	}
+
+	if i > max { // +1
+		if j < i { // +1 +1(nesting)
+			if i%j == 0 { // +1 +2(nesting)
+				return
+			}
+		}
+
+		total += i
+	}
+}
+
+// Tests TYPE SWITCH
+func p() { // MATCH /function p has cognitive complexity 1 (> max enabled 0)/
+	switch n := n.(type) { // +1
+	case *ast.IfStmt:
+		targets := []ast.Node{n.Cond, n.Body, n.Else}
+		v.walk(targets...)
+		return nil
+	case *ast.ForStmt:
+		v.walk(n.Body)
+		return nil
+	case *ast.TypeSwitchStmt:
+		v.walk(n.Body)
+		return nil
+	case *ast.BinaryExpr:
+		v.complexity += v.binExpComplexity(n)
+		return nil
+	}
+}
+
+// Test RANGE
+func q() { // MATCH /function q has cognitive complexity 1 (> max enabled 0)/
+	for _, t := range targets { // +1
+		ast.Walk(v, t)
+	}
+}
+
+// Tests SELECT
+func r() { // MATCH /function r has cognitive complexity 1 (> max enabled 0)/
+	select { // +1
+	case c <- x:
+		x, y = y, x+y
+	case <-quit:
+		fmt.Println("quit")
+		return
+	}
+}
+
+// Test jump to label
+func s() { // MATCH /function s has cognitive complexity 3 (> max enabled 0)/
+FirstLoop:
+	for i := 0; i < 10; i++ { // +1
+		break
+	}
+	for i := 0; i < 10; i++ { // +1
+		break FirstLoop // +1
+	}
+}
+
+func t() { // MATCH /function t has cognitive complexity 2 (> max enabled 0)/
+FirstLoop:
+	for i := 0; i < 10; i++ { // +1
+		goto FirstLoop // +1
+	}
+}
+
+func u() { // MATCH /function u has cognitive complexity 3 (> max enabled 0)/
+FirstLoop:
+	for i := 0; i < 10; i++ { // +1
+		continue
+	}
+	for i := 0; i < 10; i++ { // +1
+		continue FirstLoop // +1
+	}
+}
+
+// Tests FUNC LITERAL
+func v() { // MATCH /function v has cognitive complexity 2 (> max enabled 0)/
+	myFunc := func(b bool) {
+		if b { // +1 +1(nesting)
+
+		}
+	}
+}
+
+func v() {
+	t.Run(tc.desc, func(t *testing.T) {})
+}
+
+func w() { // MATCH /function w has cognitive complexity 3 (> max enabled 0)/
+	defer func(b bool) {
+		if b { // +1 +1(nesting)
+
+		}
+	}(false || true) // +1
+}
+
+// Test from Cognitive Complexity white paper
+func sumOfPrimes(max int) int { // MATCH /function sumOfPrimes has cognitive complexity 7 (> max enabled 0)/
+	total := 0
+OUT:
+	for i := 1; i <= max; i++ { // +1
+		for j := 2; j < i; j++ { // +1 +1(nesting)
+			if i%j == 0 { // +1 +2(nesting)
+				continue OUT // +1
+			}
+		}
+
+		total += i
+	}
+	return total
+}
+
+// Test from K8S
+func (m *Migrator) MigrateIfNeeded(target *EtcdVersionPair) error { // MATCH /function (*Migrator).MigrateIfNeeded has cognitive complexity 18 (> max enabled 0)/
+	klog.Infof("Starting migration to %s", target)
+	err := m.dataDirectory.Initialize(target)
+	if err != nil { // +1
+		return fmt.Errorf("failed to initialize data directory %s: %v", m.dataDirectory.path, err)
+	}
+
+	var current *EtcdVersionPair
+	vfExists, err := m.dataDirectory.versionFile.Exists()
+	if err != nil { // +1
+		return err
+	}
+	if vfExists { // +1
+		current, err = m.dataDirectory.versionFile.Read()
+		if err != nil { // +1 +1
+			return err
+		}
+	} else {
+		return fmt.Errorf("existing data directory '%s' is missing version.txt file, unable to migrate", m.dataDirectory.path)
+	}
+
+	for { // +1
+		klog.Infof("Converging current version '%s' to target version '%s'", current, target)
+		currentNextMinorVersion := &EtcdVersion{Version: semver.Version{Major: current.version.Major, Minor: current.version.Minor + 1}}
+		switch { // +1 +1
+		case current.version.MajorMinorEquals(target.version) || currentNextMinorVersion.MajorMinorEquals(target.version): // +1
+			klog.Infof("current version '%s' equals or is one minor version previous of target version '%s' - migration complete", current, target)
+			err = m.dataDirectory.versionFile.Write(target)
+			if err != nil { // +1 +2
+				return fmt.Errorf("failed to write version.txt to '%s': %v", m.dataDirectory.path, err)
+			}
+			return nil
+		case current.storageVersion == storageEtcd2 && target.storageVersion == storageEtcd3: // +1
+			return fmt.Errorf("upgrading from etcd2 storage to etcd3 storage is not supported")
+		case current.version.Major == 3 && target.version.Major == 2: // +1
+			return fmt.Errorf("downgrading from etcd 3.x to 2.x is not supported")
+		case current.version.Major == target.version.Major && current.version.Minor < target.version.Minor: // +1
+			stepVersion := m.cfg.supportedVersions.NextVersionPair(current)
+			klog.Infof("upgrading etcd from %s to %s", current, stepVersion)
+			current, err = m.minorVersionUpgrade(current, stepVersion)
+		case current.version.Major == 3 && target.version.Major == 3 && current.version.Minor > target.version.Minor: // +1
+			klog.Infof("rolling etcd back from %s to %s", current, target)
+			current, err = m.rollbackEtcd3MinorVersion(current, target)
+		}
+		if err != nil { // +1 +1
+			return err
+		}
+	}
+}

rule/cognitive-complexity.go

@@ -0,0 +1,195 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+
+	"github.com/mgechev/revive/lint"
+	"golang.org/x/tools/go/ast/astutil"
+)
+
+// CognitiveComplexityRule lints given else constructs.
+type CognitiveComplexityRule struct{}
+
+// Apply applies the rule to given file.
+func (r *CognitiveComplexityRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	const expectedArgumentsCount = 1
+	if len(arguments) < expectedArgumentsCount {
+		panic(fmt.Sprintf("not enough arguments for cognitive-complexity, expected %d, got %d", expectedArgumentsCount, len(arguments)))
+	}
+	complexity, ok := arguments[0].(int64)
+	if !ok {
+		panic(fmt.Sprintf("invalid argument type for cognitive-complexity, expected int64, got %T", arguments[0]))
+	}
+
+	linter := cognitiveComplexityLinter{
+		file:          file,
+		maxComplexity: int(complexity),
+		onFailure: func(failure lint.Failure) {
+			failures = append(failures, failure)
+		},
+	}
+
+	linter.lint()
+
+	return failures
+}
+
+// Name returns the rule name.
+func (r *CognitiveComplexityRule) Name() string {
+	return "cognitive-complexity"
+}
+
+type cognitiveComplexityLinter struct {
+	file          *lint.File
+	maxComplexity int
+	onFailure     func(lint.Failure)
+}
+
+func (w cognitiveComplexityLinter) lint() {
+	f := w.file
+	for _, decl := range f.AST.Decls {
+		if fn, ok := decl.(*ast.FuncDecl); ok {
+			v := cognitiveComplexityVisitor{}
+			c := v.subTreeComplexity(fn.Body)
+			if c > w.maxComplexity {
+				w.onFailure(lint.Failure{
+					Confidence: 1,
+					Category:   "maintenance",
+					Failure:    fmt.Sprintf("function %s has cognitive complexity %d (> max enabled %d)", funcName(fn), c, w.maxComplexity),
+					Node:       fn,
+				})
+			}
+		}
+	}
+}
+
+type cognitiveComplexityVisitor struct {
+	complexity   int
+	nestingLevel int
+}
+
+// subTreeComplexity calculates the cognitive complexity of an AST-subtree.
+func (v cognitiveComplexityVisitor) subTreeComplexity(n ast.Node) int {
+	ast.Walk(&v, n)
+	return v.complexity
+}
+
+// Visit implements the ast.Visitor interface.
+func (v *cognitiveComplexityVisitor) Visit(n ast.Node) ast.Visitor {
+	switch n := n.(type) {
+	case *ast.IfStmt:
+		targets := []ast.Node{n.Cond, n.Body, n.Else}
+		v.walk(1, targets...)
+		return nil
+	case *ast.ForStmt:
+		targets := []ast.Node{n.Cond, n.Body}
+		v.walk(1, targets...)
+		return nil
+	case *ast.RangeStmt:
+		v.walk(1, n.Body)
+		return nil
+	case *ast.SelectStmt:
+		v.walk(1, n.Body)
+		return nil
+	case *ast.SwitchStmt:
+		v.walk(1, n.Body)
+		return nil
+	case *ast.TypeSwitchStmt:
+		v.walk(1, n.Body)
+		return nil
+	case *ast.FuncLit:
+		v.walk(0, n.Body) // do not increment the complexity, just do the nesting
+		return nil
+	case *ast.BinaryExpr:
+		v.complexity += v.binExpComplexity(n)
+		return nil // skip visiting binexp sub-tree (already visited by binExpComplexity)
+	case *ast.BranchStmt:
+		if n.Label != nil {
+			v.complexity += 1
+		}
+	}
+	// TODO handle (at least) direct recursion
+
+	return v
+}
+
+func (v *cognitiveComplexityVisitor) walk(complexityIncrement int, targets ...ast.Node) {
+	v.complexity += complexityIncrement + v.nestingLevel
+	nesting := v.nestingLevel
+	v.nestingLevel++
+
+	for _, t := range targets {
+		if t == nil {
+			continue
+		}
+
+		ast.Walk(v, t)
+	}
+
+	v.nestingLevel = nesting
+}
+
+func (cognitiveComplexityVisitor) binExpComplexity(n *ast.BinaryExpr) int {
+	calculator := binExprComplexityCalculator{opsStack: []token.Token{}}
+
+	astutil.Apply(n, calculator.pre, calculator.post)
+
+	return calculator.complexity
+}
+
+type binExprComplexityCalculator struct {
+	complexity    int
+	opsStack      []token.Token // stack of bool operators
+	subexpStarted bool
+}
+
+func (becc *binExprComplexityCalculator) pre(c *astutil.Cursor) bool {
+	switch n := c.Node().(type) {
+	case *ast.BinaryExpr:
+		isBoolOp := n.Op == token.LAND || n.Op == token.LOR
+		if !isBoolOp {
+			break
+		}
+
+		ops := len(becc.opsStack)
+		// if
+		// 		is the first boolop in the expression OR
+		// 		is the first boolop inside a subexpression (...) OR
+		//		is not the same to the previous one
+		// then
+		//      increment complexity
+		if ops == 0 || becc.subexpStarted || n.Op != becc.opsStack[ops-1] {
+			becc.complexity++
+			becc.subexpStarted = false
+		}
+
+		becc.opsStack = append(becc.opsStack, n.Op)
+	case *ast.ParenExpr:
+		becc.subexpStarted = true
+	}
+
+	return true
+}
+
+func (becc *binExprComplexityCalculator) post(c *astutil.Cursor) bool {
+	switch n := c.Node().(type) {
+	case *ast.BinaryExpr:
+		isBoolOp := n.Op == token.LAND || n.Op == token.LOR
+		if !isBoolOp {
+			break
+		}
+
+		ops := len(becc.opsStack)
+		if ops > 0 {
+			becc.opsStack = becc.opsStack[:ops-1]
+		}
+	case *ast.ParenExpr:
+		becc.subexpStarted = false
+	}
+
+	return true
+}

test/cognitive-complexity_test.go

@@ -0,0 +1,14 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/lint"
+	"github.com/mgechev/revive/rule"
+)
+
+func TestCognitiveComplexity(t *testing.T) {
+	testRule(t, "cognitive-complexity", &rule.CognitiveComplexityRule{}, &lint.RuleConfig{
+		Arguments: []interface{}{int64(0)},
+	})
+}