refactor: extract shared code for linting if-else chains (#821)

* refactor: extract shared code for linting if-else chains

The rules "early-return", "indent-error-flow" and
"superfluous-else" have a similar structure. This
moves the common logic for classifying if-else chains
to a common package.

A few side benefits:
- "early-return" now handles os.Exit/log.Panicf/etc
- "superfluous-else" now handles (builtin) panic
- "superfluous-else" and "indent-error-flow" now handle if/else
  chains with 2+ "if" branches

* internal/ifelse: style fixes, renames, spelling

internal/ifelse/branch.go

@@ -0,0 +1,68 @@
+package ifelse
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+)
+
+// Branch contains information about a branch within an if-else chain.
+type Branch struct {
+	BranchKind
+	Call // The function called at the end for kind Panic or Exit.
+}
+
+// BlockBranch gets the Branch of an ast.BlockStmt.
+func BlockBranch(block *ast.BlockStmt) Branch {
+	blockLen := len(block.List)
+	if blockLen == 0 {
+		return Branch{BranchKind: Empty}
+	}
+
+	switch stmt := block.List[blockLen-1].(type) {
+	case *ast.ReturnStmt:
+		return Branch{BranchKind: Return}
+	case *ast.BlockStmt:
+		return BlockBranch(stmt)
+	case *ast.BranchStmt:
+		switch stmt.Tok {
+		case token.BREAK:
+			return Branch{BranchKind: Break}
+		case token.CONTINUE:
+			return Branch{BranchKind: Continue}
+		case token.GOTO:
+			return Branch{BranchKind: Goto}
+		}
+	case *ast.ExprStmt:
+		fn, ok := ExprCall(stmt)
+		if !ok {
+			break
+		}
+		kind, ok := DeviatingFuncs[fn]
+		if ok {
+			return Branch{BranchKind: kind, Call: fn}
+		}
+	}
+
+	return Branch{BranchKind: Regular}
+}
+
+// String returns a brief string representation
+func (b Branch) String() string {
+	switch b.BranchKind {
+	case Panic, Exit:
+		return fmt.Sprintf("... %v()", b.Call)
+	default:
+		return b.BranchKind.String()
+	}
+}
+
+// LongString returns a longer form string representation
+func (b Branch) LongString() string {
+	switch b.BranchKind {
+	case Panic, Exit:
+		return fmt.Sprintf("call to %v function", b.Call)
+	default:
+		return b.BranchKind.LongString()
+	}
+}

internal/ifelse/branch_kind.go

@@ -0,0 +1,98 @@
+package ifelse
+
+// BranchKind is a classifier for if-else branches. It says whether the branch is empty,
+// and whether the branch ends with a statement that deviates control flow.
+type BranchKind int
+
+const (
+	// Empty branches do nothing
+	Empty BranchKind = iota
+
+	// Return branches return from the current function
+	Return
+
+	// Continue branches continue a surrounding "for" loop
+	Continue
+
+	// Break branches break a surrounding "for" loop
+	Break
+
+	// Goto branches conclude with a "goto" statement
+	Goto
+
+	// Panic branches panic the current function
+	Panic
+
+	// Exit branches end the program
+	Exit
+
+	// Regular branches do not fit any category above
+	Regular
+)
+
+// IsEmpty tests if the branch is empty
+func (k BranchKind) IsEmpty() bool { return k == Empty }
+
+// Returns tests if the branch returns from the current function
+func (k BranchKind) Returns() bool { return k == Return }
+
+// Deviates tests if the control does not flow to the first
+// statement following the if-else chain.
+func (k BranchKind) Deviates() bool {
+	switch k {
+	case Empty, Regular:
+		return false
+	case Return, Continue, Break, Goto, Panic, Exit:
+		return true
+	default:
+		panic("invalid kind")
+	}
+}
+
+// String returns a brief string representation
+func (k BranchKind) String() string {
+	switch k {
+	case Empty:
+		return ""
+	case Regular:
+		return "..."
+	case Return:
+		return "... return"
+	case Continue:
+		return "... continue"
+	case Break:
+		return "... break"
+	case Goto:
+		return "... goto"
+	case Panic:
+		return "... panic()"
+	case Exit:
+		return "... os.Exit()"
+	default:
+		panic("invalid kind")
+	}
+}
+
+// LongString returns a longer form string representation
+func (k BranchKind) LongString() string {
+	switch k {
+	case Empty:
+		return "an empty block"
+	case Regular:
+		return "a regular statement"
+	case Return:
+		return "a return statement"
+	case Continue:
+		return "a continue statement"
+	case Break:
+		return "a break statement"
+	case Goto:
+		return "a goto statement"
+	case Panic:
+		return "a function call that panics"
+	case Exit:
+		return "a function call that exits the program"
+	default:
+		panic("invalid kind")
+	}
+}

internal/ifelse/chain.go

@@ -0,0 +1,9 @@
+package ifelse
+
+// Chain contains information about an if-else chain.
+type Chain struct {
+	If                   Branch // what happens at the end of the "if" block
+	Else                 Branch // what happens at the end of the "else" block
+	HasInitializer       bool   // is there an "if"-initializer somewhere in the chain?
+	HasPriorNonDeviating bool   // is there a prior "if" block that does NOT deviate control flow?
+}

internal/ifelse/doc.go

@@ -0,0 +1,6 @@
+// Package ifelse provides helpers for analysing the control flow in if-else chains,
+// presently used by the following rules:
+// - early-return
+// - indent-error-flow
+// - superfluous-else
+package ifelse

internal/ifelse/func.go

@@ -0,0 +1,51 @@
+package ifelse
+
+import (
+	"fmt"
+	"go/ast"
+)
+
+// Call contains the name of a function that deviates control flow.
+type Call struct {
+	Pkg  string // The package qualifier of the function, if not built-in.
+	Name string // The function name.
+}
+
+// DeviatingFuncs lists known control flow deviating function calls.
+var DeviatingFuncs = map[Call]BranchKind{
+	{"os", "Exit"}:     Exit,
+	{"log", "Fatal"}:   Exit,
+	{"log", "Fatalf"}:  Exit,
+	{"log", "Fatalln"}: Exit,
+	{"", "panic"}:      Panic,
+	{"log", "Panic"}:   Panic,
+	{"log", "Panicf"}:  Panic,
+	{"log", "Panicln"}: Panic,
+}
+
+// ExprCall gets the Call of an ExprStmt, if any.
+func ExprCall(expr *ast.ExprStmt) (Call, bool) {
+	call, ok := expr.X.(*ast.CallExpr)
+	if !ok {
+		return Call{}, false
+	}
+	switch v := call.Fun.(type) {
+	case *ast.Ident:
+		return Call{Name: v.Name}, true
+	case *ast.SelectorExpr:
+		if ident, ok := v.X.(*ast.Ident); ok {
+			return Call{Name: v.Sel.Name, Pkg: ident.Name}, true
+		}
+	}
+	return Call{}, false
+}
+
+// String returns the function name with package qualifier (if any)
+func (f Call) String() string {
+	switch {
+	case f.Pkg != "":
+		return fmt.Sprintf("%s.%s", f.Pkg, f.Name)
+	default:
+		return f.Name
+	}
+}

internal/ifelse/rule.go

@@ -0,0 +1,92 @@
+package ifelse
+
+import (
+	"go/ast"
+	"go/token"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// Rule is an interface for linters operating on if-else chains
+type Rule interface {
+	CheckIfElse(chain Chain) (failMsg string)
+}
+
+// Apply evaluates the given Rule on if-else chains found within the given AST,
+// and returns the failures.
+//
+// Note that in if-else chain with multiple "if" blocks, only the *last* one is checked,
+// that is to say, given:
+//
+//	if foo {
+//	    ...
+//	} else if bar {
+//		...
+//	} else {
+//		...
+//	}
+//
+// Only the block following "bar" is linted. This is because the rules that use this function
+// do not presently have anything to say about earlier blocks in the chain.
+func Apply(rule Rule, node ast.Node, target Target) []lint.Failure {
+	v := &visitor{rule: rule, target: target}
+	ast.Walk(v, node)
+	return v.failures
+}
+
+type visitor struct {
+	failures []lint.Failure
+	target   Target
+	rule     Rule
+}
+
+func (v *visitor) Visit(node ast.Node) ast.Visitor {
+	ifStmt, ok := node.(*ast.IfStmt)
+	if !ok {
+		return v
+	}
+
+	v.visitChain(ifStmt, Chain{})
+	return nil
+}
+
+func (v *visitor) visitChain(ifStmt *ast.IfStmt, chain Chain) {
+	// look for other if-else chains nested inside this if { } block
+	ast.Walk(v, ifStmt.Body)
+
+	if ifStmt.Else == nil {
+		// no else branch
+		return
+	}
+
+	if as, ok := ifStmt.Init.(*ast.AssignStmt); ok && as.Tok == token.DEFINE {
+		chain.HasInitializer = true
+	}
+	chain.If = BlockBranch(ifStmt.Body)
+
+	switch elseBlock := ifStmt.Else.(type) {
+	case *ast.IfStmt:
+		if !chain.If.Deviates() {
+			chain.HasPriorNonDeviating = true
+		}
+		v.visitChain(elseBlock, chain)
+	case *ast.BlockStmt:
+		// look for other if-else chains nested inside this else { } block
+		ast.Walk(v, elseBlock)
+		chain.Else = BlockBranch(elseBlock)
+		if failMsg := v.rule.CheckIfElse(chain); failMsg != "" {
+			if chain.HasInitializer {
+				// if statement has a := initializer, so we might need to move the assignment
+				// onto its own line in case the body references it
+				failMsg += " (move short variable declaration to its own line if necessary)"
+			}
+			v.failures = append(v.failures, lint.Failure{
+				Confidence: 1,
+				Node:       v.target.node(ifStmt),
+				Failure:    failMsg,
+			})
+		}
+	default:
+		panic("invalid node type for else")
+	}
+}

internal/ifelse/target.go

@@ -0,0 +1,25 @@
+package ifelse
+
+import "go/ast"
+
+// Target decides what line/column should be indicated by the rule in question.
+type Target int
+
+const (
+	// TargetIf means the text refers to the "if"
+	TargetIf Target = iota
+
+	// TargetElse means the text refers to the "else"
+	TargetElse
+)
+
+func (t Target) node(ifStmt *ast.IfStmt) ast.Node {
+	switch t {
+	case TargetIf:
+		return ifStmt
+	case TargetElse:
+		return ifStmt.Else
+	default:
+		panic("bad target")
+	}
+}