refactor(pkg/linters): extract helpers to eliminate all 33 large-function findings

docs/adr/47022-extract-analyzer-helpers-to-resolve-large-function-violations.md

@@ -0,0 +1,50 @@
+# ADR-47022: Extract Named Helper Functions from Large Analyzer `run` Bodies
+
+**Date**: 2026-07-21
+**Status**: Draft
+**Deciders**: pelikhan, copilot-swe-agent
+
+---
+
+### Context
+
+`make golint-custom` reported 33 large-function violations (>60 body lines) across `pkg/linters` analyzer files. The violations were concentrated in `run` functions and a small number of related helpers that accumulated traversal, type-checking, and fix-generation logic inline — both as `insp.Preorder` anonymous callback bodies and as `for cur := range insp.Root().Preorder(...)` loop bodies. Because the large-function linter is a required quality gate that runs in CI, all 33 violations had to be resolved before the branch could merge. No change to analyzer semantics (diagnostics, positions, or suggested-fix `TextEdit`s) was permissible.
+
+### Decision
+
+We will extract the body of each oversized inline function literal or cursor loop body into a named package-level function (e.g., `analyzeWriteCall`, `checkHTTPCall`, `checkHardcodedFilePath`). Each `run` function will be reduced to setup, index construction, and a single-line delegation call. Constants that were previously scoped to `run` and needed by the extracted helper will be promoted to package-level. No behavioral changes are introduced: diagnostic messages, positions, and `SuggestedFix` values are identical before and after extraction.
+
+### Alternatives Considered
+
+#### Alternative 1: Suppress violations with `//nolint:largefunc` directives
+
+Add per-function `//nolint` directives to silence the 33 findings without restructuring code. This would pass the gate but would accumulate permanent suppression debt, mask future growth of these functions, and set a precedent that `pkg/linters` functions are exempt from the quality rule.
+
+#### Alternative 2: Raise or remove the large-function threshold for `pkg/linters`
+
+Adjust `.golangci.yml` or the custom linter configuration to either increase the line-length limit (e.g., to 120 lines) or exclude the `pkg/linters` directory entirely. This resolves the gate failures without code changes but defeats the purpose of the linter for exactly the code that defines the linter rules — an especially bad precedent — and does not improve readability.
+
+#### Alternative 3: Restructure analyzers as method sets on analyzer structs
+
+Replace the current `func run(pass *analysis.Pass)` pattern with a per-analyzer struct that implements sub-passes as methods, eliminating the need to thread `pass`, `generatedFiles`, and `noLintIndex` as parameters to every extracted function. This is a deeper architectural change that would affect all 25 files uniformly and could be a worthwhile future refactor, but it is out of scope for a targeted lint-compliance fix: it introduces higher risk of behavior regression and a much larger diff.
+
+### Consequences
+
+#### Positive
+- All 33 `largefunc` violations in `pkg/linters` are cleared; `make golint-custom` passes.
+- Each extracted function has a single, named responsibility, making stack traces and profiling output more informative.
+- Reviewers can read the `run` function as a high-level outline and drill into named helpers independently.
+- The extraction pattern is uniform and mechanical, minimizing review surface.
+
+#### Negative
+- Extracted functions require all context to be passed explicitly (`pass`, `generatedFiles`, `noLintIndex`, and in some cases `seenImportFiles` or `inspector.Cursor`), increasing parameter counts relative to closures that captured variables implicitly.
+- Two files (`execcommandwithoutcontext`, `nilctxpassed`) required a new import of `"golang.org/x/tools/go/ast/inspector"` to expose the `inspector.Cursor` type in the extracted function signatures, adding a minor import-set change.
+
+#### Neutral
+- The `formatArgOffset` constant in `errorfwrapv` was promoted from a function-local `const` to package-level to eliminate a redundant parameter; this has no behavioral effect but changes the constant's visibility scope.
+- All diagnostic messages, positions, and suggested-fix `TextEdit`s are unchanged — this is a structural refactor only.
+- The 25 changed files share a consistent before/after pattern, which means the diff is large in line count but low in cognitive complexity per file.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/linters/appendbytestring/appendbytestring.go

@@ -38,68 +38,70 @@ func run(pass *analysis.Pass) (any, error) {
 		return nil, err
 	}
 
-	nodeFilter := []ast.Node{
-		(*ast.CallExpr)(nil),
-	}
-
+	nodeFilter := []ast.Node{(*ast.CallExpr)(nil)}
 	insp.Preorder(nodeFilter, func(n ast.Node) {
-		call, ok := n.(*ast.CallExpr)
-		if !ok {
-			return
-		}
+		analyzeAppendByteString(pass, n, generatedFiles, noLintIndex)
+	})
+	return nil, nil
+}
 
-		// Match append(b, x...) with exactly 2 arguments and an ellipsis.
-		ident, ok := call.Fun.(*ast.Ident)
-		if !ok || ident.Name != "append" {
-			return
-		}
-		if len(call.Args) != 2 || !call.Ellipsis.IsValid() {
-			return
-		}
+// analyzeAppendByteString checks whether a call is an append(b, []byte(s)...)
+// that can be simplified to append(b, s...) and reports a diagnostic if so.
+func analyzeAppendByteString(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) {
+	call, ok := n.(*ast.CallExpr)
+	if !ok {
+		return
+	}
 
-		pos := pass.Fset.PositionFor(call.Pos(), false)
-		if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) {
-			return
-		}
-		if nolint.HasDirectiveForLinter(pos, noLintIndex, "appendbytestring") {
-			return
-		}
+	// Match append(b, x...) with exactly 2 arguments and an ellipsis.
+	ident, ok := call.Fun.(*ast.Ident)
+	if !ok || ident.Name != "append" {
+		return
+	}
+	if len(call.Args) != 2 || !call.Ellipsis.IsValid() {
+		return
+	}
 
-		// The first argument must be []byte.
-		if !astutil.IsByteSlice(pass, call.Args[0]) {
-			return
-		}
+	pos := pass.Fset.PositionFor(call.Pos(), false)
+	if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) {
+		return
+	}
+	if nolint.HasDirectiveForLinter(pos, noLintIndex, "appendbytestring") {
+		return
+	}
 
-		// The second argument must be a []byte(s) conversion where s is a string.
-		conv, ok := call.Args[1].(*ast.CallExpr)
-		if !ok {
-			return
-		}
-		if !astutil.IsByteSliceConversion(pass, conv) {
-			return
-		}
-		if len(conv.Args) != 1 {
-			return
-		}
-		strArg := conv.Args[0]
-		if !astutil.IsStringType(pass, strArg) {
-			return
-		}
+	// The first argument must be []byte.
+	if !astutil.IsByteSlice(pass, call.Args[0]) {
+		return
+	}
 
-		sText := astutil.NodeText(pass.Fset, strArg)
-		if sText == "" {
-			return
-		}
+	// The second argument must be a []byte(s) conversion where s is a string.
+	conv, ok := call.Args[1].(*ast.CallExpr)
+	if !ok {
+		return
+	}
+	if !astutil.IsByteSliceConversion(pass, conv) {
+		return
+	}
+	if len(conv.Args) != 1 {
+		return
+	}
+	strArg := conv.Args[0]
+	if !astutil.IsStringType(pass, strArg) {
+		return
+	}
 
-		pass.Report(analysis.Diagnostic{
-			Pos:            call.Pos(),
-			End:            call.End(),
-			Message:        fmt.Sprintf("append(b, []byte(%s)...) can be simplified to append(b, %s...); the []byte conversion is unnecessary", sText, sText),
-			SuggestedFixes: buildFix(pass, conv, strArg),
-		})
-	})
+	sText := astutil.NodeText(pass.Fset, strArg)
+	if sText == "" {
+		return
+	}
 
-	return nil, nil
+	pass.Report(analysis.Diagnostic{
+		Pos:            call.Pos(),
+		End:            call.End(),
+		Message:        fmt.Sprintf("append(b, []byte(%s)...) can be simplified to append(b, %s...); the []byte conversion is unnecessary", sText, sText),
+		SuggestedFixes: buildFix(pass, conv, strArg),
+	})
 }
 
 // buildFix returns a SuggestedFix rewriting append(b, []byte(s)...) to append(b, s...).

pkg/linters/appendoneelement/appendoneelement.go

@@ -39,88 +39,94 @@ func run(pass *analysis.Pass) (any, error) {
 		return nil, err
 	}
 
-	nodeFilter := []ast.Node{
-		(*ast.CallExpr)(nil),
-	}
-
+	nodeFilter := []ast.Node{(*ast.CallExpr)(nil)}
 	insp.Preorder(nodeFilter, func(n ast.Node) {
-		call, ok := n.(*ast.CallExpr)
-		if !ok {
-			return
-		}
-
-		// Must be append(x, y...) with exactly 2 arguments and ellipsis.
-		ident, ok := call.Fun.(*ast.Ident)
-		if !ok || ident.Name != "append" {
-			return
-		}
-		if pass.TypesInfo.ObjectOf(ident) != types.Universe.Lookup("append") {
-			return
-		}
-		if len(call.Args) != 2 || !call.Ellipsis.IsValid() {
-			return
-		}
+		analyzeAppendOneElement(pass, n, generatedFiles, noLintIndex)
+	})
+	return nil, nil
+}
 
-		pos := pass.Fset.PositionFor(call.Pos(), false)
-		if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) {
-			return
-		}
-		if nolint.HasDirectiveForLinter(pos, noLintIndex, "appendoneelement") {
-			return
-		}
+// analyzeAppendOneElement checks whether a call is an append(s, []T{x}...) that
+// can be simplified to append(s, x) and reports a diagnostic if so.
+func analyzeAppendOneElement(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) {
+	call, ok := n.(*ast.CallExpr)
+	if !ok {
+		return
+	}
 
-		// The second argument must be a composite literal with exactly one element.
-		lit, ok := call.Args[1].(*ast.CompositeLit)
-		if !ok {
-			return
-		}
-		// Must be a slice type ([]T). *ast.ArrayType covers both slices and arrays;
-		// only slices have Len == nil.
-		arrayType, ok := lit.Type.(*ast.ArrayType)
-		if !ok || arrayType.Len != nil {
-			return
-		}
-		if len(lit.Elts) != 1 {
-			return
-		}
+	ident, ok := call.Fun.(*ast.Ident)
+	if !ok || ident.Name != "append" {
+		return
+	}
+	if pass.TypesInfo.ObjectOf(ident) != types.Universe.Lookup("append") {
+		return
+	}
+	if len(call.Args) != 2 || !call.Ellipsis.IsValid() {
+		return
+	}
 
-		elem := lit.Elts[0]
-		if _, ok := elem.(*ast.KeyValueExpr); ok {
-			return
-		}
-		if nestedLit, ok := elem.(*ast.CompositeLit); ok && nestedLit.Type == nil {
-			return
-		}
-		elemText := astutil.NodeText(pass.Fset, elem)
-		if elemText == "" {
-			return
-		}
-		litText := astutil.NodeText(pass.Fset, lit)
-		if litText == "" {
-			return
-		}
+	pos := pass.Fset.PositionFor(call.Pos(), false)
+	if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) {
+		return
+	}
+	if nolint.HasDirectiveForLinter(pos, noLintIndex, "appendoneelement") {
+		return
+	}
 
-		sliceText := astutil.NodeText(pass.Fset, call.Args[0])
-		if sliceText == "" {
-			return
-		}
+	sliceText, elemText, litText, ok := matchSingleElementSpread(pass, call)
+	if !ok {
+		return
+	}
 
-		pass.Report(analysis.Diagnostic{
-			Pos:     call.Pos(),
-			End:     call.End(),
-			Message: fmt.Sprintf("append(s, %s...) can be simplified to append(s, %s)", litText, elemText),
-			SuggestedFixes: []analysis.SuggestedFix{{
-				Message: fmt.Sprintf("Replace %s... with %s", litText, elemText),
-				TextEdits: []analysis.TextEdit{
-					{
-						Pos:     call.Pos(),
-						End:     call.End(),
-						NewText: fmt.Appendf(nil, "append(%s, %s)", sliceText, elemText),
-					},
-				},
+	pass.Report(analysis.Diagnostic{
+		Pos:     call.Pos(),
+		End:     call.End(),
+		Message: fmt.Sprintf("append(s, %s...) can be simplified to append(s, %s)", litText, elemText),
+		SuggestedFixes: []analysis.SuggestedFix{{
+			Message: fmt.Sprintf("Replace %s... with %s", litText, elemText),
+			TextEdits: []analysis.TextEdit{{
+				Pos:     call.Pos(),
+				End:     call.End(),
+				NewText: fmt.Appendf(nil, "append(%s, %s)", sliceText, elemText),
 			}},
-		})
+		}},
 	})
+}
 
-	return nil, nil
+// matchSingleElementSpread validates that call.Args[1] is a single-element slice
+// literal spread and returns the text representations of slice, element, and literal.
+func matchSingleElementSpread(pass *analysis.Pass, call *ast.CallExpr) (sliceText, elemText, litText string, ok bool) {
+	lit, litOK := call.Args[1].(*ast.CompositeLit)
+	if !litOK {
+		return "", "", "", false
+	}
+	arrayType, atOK := lit.Type.(*ast.ArrayType)
+	if !atOK || arrayType.Len != nil {
+		return "", "", "", false
+	}
+	if len(lit.Elts) != 1 {
+		return "", "", "", false
+	}
+
+	elem := lit.Elts[0]
+	if _, ok := elem.(*ast.KeyValueExpr); ok {
+		return "", "", "", false
+	}
+	if nestedLit, ok := elem.(*ast.CompositeLit); ok && nestedLit.Type == nil {
+		return "", "", "", false
+	}
+
+	elemText = astutil.NodeText(pass.Fset, elem)
+	if elemText == "" {
+		return "", "", "", false
+	}
+	litText = astutil.NodeText(pass.Fset, lit)
+	if litText == "" {
+		return "", "", "", false
+	}
+	sliceText = astutil.NodeText(pass.Fset, call.Args[0])
+	if sliceText == "" {
+		return "", "", "", false
+	}
+	return sliceText, elemText, litText, true
 }