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/chain.go

@@ -0,0 +1,9 @@
+package ifelse
+
+// Chain contains information about an if-else chain.
+type Chain struct {
+	IfTerminator      Terminator // what happens at the end of the "if" block
+	ElseTerminator    Terminator // what happens at the end of the "else" block
+	HasIfInitializer  bool       // is there an "if"-initializer somewhere in the chain?
+	HasPriorNonReturn 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,50 @@
+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
+}
+
+// DeviatingCalls lists known control flow deviating function calls.
+var DeviatingCalls = map[Call]Kind{
+	{"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
+}
+
+func (f Call) String() string {
+	switch {
+	case f.Pkg != "":
+		return fmt.Sprintf("%s.%s", f.Pkg, f.Name)
+	default:
+		return f.Name
+	}
+}

internal/ifelse/kind.go

@@ -0,0 +1,79 @@
+package ifelse
+
+// Kind is a classifier for if-else branches. It says whether the branch is empty,
+// and whether the branch ends with a statement that can deviate control flow.
+type Kind int
+
+const (
+	Empty Kind = iota
+	Regular
+	Return
+	Continue
+	Break
+	Goto
+	Panic
+	Exit
+)
+
+func (k Kind) IsEmpty() bool  { return k == Empty }
+func (k Kind) IsReturn() bool { return k == Return }
+
+// DeviatesControlFlow returns true if the program follows regular control flow at
+// branch termination (that is to say, control flows to the first statement following
+// the if-else chain).
+func (k Kind) DeviatesControlFlow() bool {
+	switch k {
+	case Empty, Regular:
+		return false
+	case Return, Continue, Break, Goto, Panic, Exit:
+		return true
+	default:
+		panic("invalid kind")
+	}
+}
+
+func (k Kind) 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")
+	}
+}
+
+func (k Kind) 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/rule.go

@@ -0,0 +1,110 @@
+package ifelse
+
+import (
+	"go/ast"
+	"go/token"
+
+	"github.com/mgechev/revive/lint"
+)
+
+type (
+	Rule interface {
+		CheckIfElse(chain Chain) (failMsg string)
+	}
+	Target int
+)
+
+// 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
+}
+
+const (
+	TargetIf   Target = iota // linter line-number will target the "if" statemenet
+	TargetElse               // linter lint-number will target the "else" statmenet
+)
+
+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.HasIfInitializer = true
+	}
+	chain.IfTerminator = BlockTerminator(ifStmt.Body)
+
+	switch elseBlock := ifStmt.Else.(type) {
+	case *ast.IfStmt:
+		if !chain.IfTerminator.DeviatesControlFlow() {
+			chain.HasPriorNonReturn = true
+		}
+		v.visitChain(elseBlock, chain)
+	case *ast.BlockStmt:
+		// look for other if-else chains nested inside this else { } block
+		ast.Walk(v, elseBlock)
+		chain.ElseTerminator = BlockTerminator(elseBlock)
+		if failMsg := v.rule.CheckIfElse(chain); failMsg != "" {
+			if chain.HasIfInitializer {
+				// 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.targetNode(ifStmt),
+				Failure:    failMsg,
+			})
+		}
+	default:
+		panic("invalid node type for else")
+	}
+}
+
+func (v *visitor) targetNode(ifStmt *ast.IfStmt) ast.Node {
+	switch v.target {
+	case TargetIf:
+		return ifStmt
+	case TargetElse:
+		return ifStmt.Else
+	default:
+		panic("bad target")
+	}
+}

internal/ifelse/terminator.go

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