initial commit: ifinit and boolnames

ifinit/analyzer.go

@@ -0,0 +1,200 @@
+// Package ifinit reports if-statement init clauses that span more than one line.
+package ifinit
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+const doc = `checks that if-statement init clauses occupy a single line
+
+A multi-line init clause pushes the condition being tested far away from the
+"if" keyword, so the reader has to scan past the setup to find out what is
+actually being decided. The fix hoists the init clause into its own statement
+above the if.`
+
+var Analyzer = &analysis.Analyzer{
+	Name:     "ifinit",
+	Doc:      doc,
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	result, ok := pass.ResultOf[inspect.Analyzer]
+	if !ok {
+		return nil, fmt.Errorf("inspect analyzer result is unavailable")
+	}
+
+	insp, ok := result.(*inspector.Inspector)
+	if !ok {
+		return nil, fmt.Errorf("inspect analyzer returned %T, want *inspector.Inspector", result)
+	}
+
+	insp.WithStack([]ast.Node{(*ast.IfStmt)(nil)}, func(n ast.Node, isPush bool, stack []ast.Node) bool {
+		if !isPush {
+			return false
+		}
+
+		stmt, ok := n.(*ast.IfStmt)
+		if !ok {
+			return true
+		}
+		if stmt.Init == nil {
+			return true
+		}
+
+		start := pass.Fset.Position(stmt.Init.Pos())
+		end := pass.Fset.Position(stmt.Init.End())
+		if start.Line == end.Line {
+			return true
+		}
+
+		diag := analysis.Diagnostic{
+			Pos:     stmt.Init.Pos(),
+			End:     stmt.Init.End(),
+			Message: fmt.Sprintf("if statement init clause spans %d lines; it must be on a single line", end.Line-start.Line+1),
+		}
+
+		if fix, ok := hoist(pass, stmt, stack); ok {
+			diag.SuggestedFixes = []analysis.SuggestedFix{fix}
+		}
+
+		pass.Report(diag)
+
+		return true
+	})
+
+	return nil, nil
+}
+
+// hoist rewrites "if <init>; <cond> {" into "<init>" on its own line followed by
+// "if <cond> {". The init clause's own text is left untouched, so comments and
+// internal line breaks survive.
+func hoist(pass *analysis.Pass, stmt *ast.IfStmt, stack []ast.Node) (analysis.SuggestedFix, bool) {
+	if !isHoistable(stmt, stack) {
+		return analysis.SuggestedFix{}, false
+	}
+
+	if !isShadowSafe(pass, stmt) {
+		return analysis.SuggestedFix{}, false
+	}
+
+	indent, ok := lineIndent(pass, stmt.Pos())
+	if !ok {
+		return analysis.SuggestedFix{}, false
+	}
+
+	return analysis.SuggestedFix{
+		Message: "Hoist the init clause above the if statement",
+		TextEdits: []analysis.TextEdit{
+			// Drop the leading "if ".
+			{Pos: stmt.Pos(), End: stmt.Init.Pos()},
+			// Turn the "; " separator into a line break and a fresh "if ".
+			{Pos: stmt.Init.End(), End: stmt.Cond.Pos(), NewText: []byte("\n" + indent + "if ")},
+		},
+	}, true
+}
+
+func isHoistable(stmt *ast.IfStmt, stack []ast.Node) bool {
+	if len(stack) < 2 {
+		return false
+	}
+
+	switch parent := stack[len(stack)-2].(type) {
+	case *ast.IfStmt:
+		// An "else if": the init clause cannot move above the chain without
+		// restructuring the whole thing into a nested else block.
+		return parent.Else != ast.Stmt(stmt)
+	case *ast.LabeledStmt:
+		// Hoisting would leave the label pointing at the init clause instead.
+		return false
+	}
+
+	return true
+}
+
+// shadowSafe reports whether hoisting stmt's init clause out of the if's scope
+// leaves every name it binds meaning the same thing.
+//
+// Only ":=" declares anything, and only in the if statement's own scope. Moving
+// that declaration up one level is safe exactly when none of the names it binds
+// already resolve to something in the enclosing scope: otherwise the hoisted
+// ":=" either assigns to that existing object instead of declaring a fresh one,
+// or collides with a later declaration in the same block.
+//
+// The check is deliberately conservative. It refuses whenever a name resolves at
+// all -- including to a package-level or universe name -- rather than trying to
+// prove that nothing reads the shadowed object afterwards.
+func isShadowSafe(pass *analysis.Pass, stmt *ast.IfStmt) bool {
+	assign, ok := stmt.Init.(*ast.AssignStmt)
+	if !ok || assign.Tok != token.DEFINE {
+		return true
+	}
+
+	scope := pass.TypesInfo.Scopes[stmt]
+	if scope == nil {
+		return false
+	}
+
+	enclosing := scope.Parent()
+	if enclosing == nil {
+		return false
+	}
+
+	for _, lhs := range assign.Lhs {
+		ident, ok := lhs.(*ast.Ident)
+		if !ok {
+			return false
+		}
+
+		if ident.Name == "_" {
+			continue
+		}
+
+		// token.NoPos makes the lookup ignore declaration order, so a name
+		// declared later in the same block counts as a conflict too.
+		if _, obj := enclosing.LookupParent(ident.Name, token.NoPos); obj != nil {
+			return false
+		}
+	}
+
+	return true
+}
+
+// lineIndent returns the whitespace preceding pos on its line, and reports
+// whether pos is the first non-whitespace token on that line.
+func lineIndent(pass *analysis.Pass, pos token.Pos) (string, bool) {
+	if pass.ReadFile == nil {
+		return "", false
+	}
+
+	file := pass.Fset.File(pos)
+	if file == nil {
+		return "", false
+	}
+
+	src, err := pass.ReadFile(file.Name())
+	if err != nil {
+		return "", false
+	}
+
+	lineStart := file.Offset(file.LineStart(file.Line(pos)))
+	offset := file.Offset(pos)
+	if offset > len(src) {
+		return "", false
+	}
+
+	prefix := string(src[lineStart:offset])
+	if strings.TrimLeft(prefix, " \t") != "" {
+		return "", false
+	}
+
+	return prefix, true
+}