build: cap function size, complexity and method surface

.golangci.yml

@@ -5,6 +5,8 @@ linters:
   enable:
     - copyloopvar
     - errorlint # %w discipline: errors.Is/As over == and type assertions
+    - funlen # the styleguide's 70-line cap, enforced rather than declared
+    - gocognit # nesting-weighted complexity: the reason the cap is 70 and not 200
     - gocritic
     - misspell
     - nilerr
@@ -13,13 +15,34 @@ linters:
     - unconvert
     - unparam
   settings:
+    funlen:
+      # The styleguide's hard cap. Statements are not capped as well: a function
+      # is long because of what it does, and two limits on the same axis only
+      # means the tighter one is the rule and the other is noise.
+      lines: 70
+      statements: -1
+      ignore-comments: true
+    gocognit:
+      # Calibrated against the finished tree, whose worst function scores 21.
+      # Raising this is how a function that should have been split stays whole,
+      # so a failure is a prompt to name the inner step, not to move the number.
+      min-complexity: 20
     revive:
       rules:
         - name: exported # GoDoc on every exported symbol
         - name: package-comments
         # A local that shadows an imported package compiles until someone adds a
         # call to that package below it. compilers/openapi had one live instance.
         - name: import-shadowing
+  exclusions:
+    rules:
+      # A table-driven test is long because its table is long, and the table is
+      # data. Splitting one to satisfy a size cap moves cases away from the
+      # assertion that reads them, which is the opposite of what the cap is for.
+      - path: _test\.go
+        linters:
+          - funlen
+          - gocognit
 
 formatters:
   enable:

internal/archtest/methodcap_test.go

@@ -0,0 +1,176 @@
+package archtest_test
+
+import (
+	"fmt"
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"sort"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// maxMethodsPerType caps how many methods one type may carry inside compilers/.
+//
+// It is calibrated against the finished shape, which is why it lands after the
+// restructuring rather than before it: the widest type in the tree is
+// compile.Types at 14, so this leaves the largest legitimate type room to grow
+// by half again. The type it exists for reached 159.
+//
+// A failure is a prompt to ask what the type has started doing, not to raise the
+// number. Nothing arrives at 21 methods by adding one concern.
+//
+// It counts methods *declared* on a type, not the type's method set: a type that
+// embeds another does not inherit its count. That is the dimension the failure
+// actually grew in — 159 methods written across 11 files, one at a time — and it
+// is what an AST rule can measure without a type checker. It also means the cap
+// can be satisfied by embedding, which is the refactoring it exists to prompt
+// rather than a way around it: two types each under the cap, each with a
+// coherent set, is the outcome. A type reaching for that to stay under the
+// number rather than to say something is visible in review in a way that one
+// more method on one more struct never was.
+const maxMethodsPerType = 20
+
+// TestMethodsPerType_StayUnderTheCap measures the dimension the god object grew
+// in, which nothing in force measured while it was growing.
+//
+// Function-size and complexity caps (#83) are worth having and were satisfied
+// throughout — no function in that package ever came near the 70-line limit. The
+// failure was type surface: one struct accumulated every lowering concern, one
+// short method at a time, and every rule the repo enforced stayed green.
+func TestMethodsPerType_StayUnderTheCap(t *testing.T) {
+	t.Parallel()
+	counts := methodCounts(t, filepath.Join(repoRoot(t), "compilers"))
+	require.NotEmpty(t, counts, "the sweep found no methods at all, so an empty result proves nothing")
+
+	var over []string
+	for _, name := range sortedKeys(counts) {
+		if counts[name] > maxMethodsPerType {
+			over = append(over, fmt.Sprintf("%s has %d methods", name, counts[name]))
+		}
+	}
+	assert.Empty(t, over, "no type under compilers/ may carry more than %d methods", maxMethodsPerType)
+}
+
+// TestMethodCounts_CountsOneTypePerPackage plants the shapes the counter has to
+// tell apart, rather than trusting the live tree to contain them: a type past
+// the cap is reported, two same-named types in different packages are counted
+// separately, and pointer, value and generic receivers all count as the same
+// type's methods.
+//
+// Planting is what makes the cap a check rather than a claim — the live tree has
+// nothing near the limit, so a counter that always returned zero would pass the
+// test above forever.
+func TestMethodCounts_CountsOneTypePerPackage(t *testing.T) {
+	t.Parallel()
+	root := t.TempDir()
+	writeMethods(t, filepath.Join(root, "wide", "wide.go"), "package wide", "Big", maxMethodsPerType+1)
+	writeMethods(t, filepath.Join(root, "a", "a.go"), "package a", "Same", 3)
+	writeMethods(t, filepath.Join(root, "b", "b.go"), "package b", "Same", 4)
+	require.NoError(t, os.WriteFile(filepath.Join(root, "a", "gen.go"),
+		[]byte("package a\n\ntype Same[T any] struct{}\n\n"+
+			"func (s Same[T]) byValue() {}\n\nfunc (s *Same[T]) byPointer() {}\n"), 0o600))
+	require.NoError(t, os.WriteFile(filepath.Join(root, "a", "a_test.go"),
+		[]byte("package a\n\nfunc (s Same) inATestFile() {}\n"), 0o600))
+	require.NoError(t, os.WriteFile(filepath.Join(root, "a", "embed.go"),
+		[]byte("package a\n\ntype Host struct{ Same }\n\n"+
+			"func (h Host) own1() {}\n\nfunc (h Host) own2() {}\n"), 0o600))
+
+	counts := methodCounts(t, root)
+
+	assert.Equal(t, maxMethodsPerType+1, counts["wide.Big"], "a type past the cap is counted whole")
+	assert.Equal(t, 5, counts["a.Same"],
+		"value, pointer and generic receivers are one type; a test file's method is not counted")
+	assert.Equal(t, 4, counts["b.Same"], "a same-named type in another package is counted apart")
+	assert.Equal(t, 2, counts["a.Host"],
+		"an embedded type's methods belong to the type that declares them, not to the one embedding it")
+}
+
+// methodCounts maps "<dir>.<Type>" to the number of methods production files
+// under root declare on it.
+//
+// The key carries the directory because a type name alone is not an identity: a
+// walker named Scope in two packages is two types, and merging them would report
+// a limit neither reached.
+func methodCounts(t *testing.T, root string) map[string]int {
+	t.Helper()
+	counts := map[string]int{}
+	err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if d.IsDir() {
+			return skipUninterestingDir(root, p, d)
+		}
+		if !isProductionGoFile(d.Name()) {
+			return nil
+		}
+		f, parseErr := parser.ParseFile(token.NewFileSet(), p, nil, parser.SkipObjectResolution)
+		if parseErr != nil {
+			return parseErr
+		}
+		dir := filepath.Base(filepath.Dir(p))
+		for _, decl := range f.Decls {
+			if name, ok := receiverType(decl); ok {
+				counts[dir+"."+name]++
+			}
+		}
+		return nil
+	})
+	require.NoError(t, err)
+	return counts
+}
+
+// receiverType returns the name of the type decl is a method on.
+//
+// It unwraps a pointer receiver and a generic one, in that order, because both
+// spellings declare a method on the same type and counting them apart would let
+// a type split its surface across receiver forms and stay under any cap.
+func receiverType(decl ast.Decl) (string, bool) {
+	fn, isFunc := decl.(*ast.FuncDecl)
+	if !isFunc || fn.Recv == nil || len(fn.Recv.List) != 1 {
+		return "", false
+	}
+	expr := fn.Recv.List[0].Type
+	if star, isPtr := expr.(*ast.StarExpr); isPtr {
+		expr = star.X
+	}
+	switch v := expr.(type) {
+	case *ast.IndexExpr: // Same[T]
+		expr = v.X
+	case *ast.IndexListExpr: // Same[K, V]
+		expr = v.X
+	}
+	id, isIdent := expr.(*ast.Ident)
+	if !isIdent {
+		return "", false
+	}
+	return id.Name, true
+}
+
+// sortedKeys returns m's keys in order, so a failure reads the same on every
+// run.
+func sortedKeys(m map[string]int) []string {
+	out := make([]string, 0, len(m))
+	for k := range m {
+		out = append(out, k)
+	}
+	sort.Strings(out)
+	return out
+}
+
+// writeMethods plants a package declaring typ with n methods on it.
+func writeMethods(t *testing.T, file, pkg, typ string, n int) {
+	t.Helper()
+	require.NoError(t, os.MkdirAll(filepath.Dir(file), 0o755))
+	src := pkg + "\n\ntype " + typ + " struct{}\n"
+	for i := range n {
+		src += fmt.Sprintf("\nfunc (x %s) m%d() {}\n", typ, i)
+	}
+	require.NoError(t, os.WriteFile(file, []byte(src), 0o600))
+}

ir/irverify/refs.go

@@ -156,55 +156,79 @@ func checkReferentialIntegrity(doc *ir.Document) []Violation {
 // callers can surface a too-deep document instead of silently
 // under-checking it.
 func walkValues(root any, visit func(v reflect.Value, path string) bool) bool {
-	seen := map[uintptr]bool{}
-	truncated := false
-	var walk func(v reflect.Value, path string, depth int)
-	descend := func(child reflect.Value, path string, depth int) {
-		if depth > maxWalkDepth {
-			truncated = true
+	w := valueWalk{seen: map[uintptr]bool{}, visit: visit}
+	w.walk(reflect.ValueOf(root), "doc", 0)
+	return w.truncated
+}
+
+// valueWalk is one walk's state: the pointers already followed, the visitor, and
+// whether the depth cap cut the walk short.
+//
+// It is a value being walked rather than a walker being configured, so it is
+// built at the entry point and discarded with it. Nothing here is reentrant and
+// nothing outside this file holds one.
+type valueWalk struct {
+	seen      map[uintptr]bool
+	visit     func(v reflect.Value, path string) bool
+	truncated bool
+}
+
+// walk visits v and then descends into whatever it holds, stopping wherever the
+// visitor says it has seen enough.
+func (w *valueWalk) walk(v reflect.Value, path string, depth int) {
+	if !v.IsValid() || !w.visit(v, path) {
+		return
+	}
+	w.children(v, path, depth)
+}
+
+// descend continues into a child unless that would pass the depth cap, which it
+// records rather than reports: the caller decides whether a truncated walk is a
+// finding, and it is the only one that knows what was being checked.
+func (w *valueWalk) descend(child reflect.Value, path string, depth int) {
+	if depth > maxWalkDepth {
+		w.truncated = true
+		return
+	}
+	w.walk(child, path, depth)
+}
+
+// children descends into every child of v. It is where the walk's shape lives —
+// what counts as a child of a pointer, an interface, a struct, a sequence and a
+// map, and which path each child is addressed by.
+func (w *valueWalk) children(v reflect.Value, path string, depth int) {
+	switch v.Kind() {
+	case reflect.Pointer:
+		if v.IsNil() {
 			return
 		}
-		walk(child, path, depth)
-	}
-	walk = func(v reflect.Value, path string, depth int) {
-		if !v.IsValid() || !visit(v, path) {
+		p := v.Pointer()
+		if w.seen[p] {
 			return
 		}
-		switch v.Kind() {
-		case reflect.Pointer:
-			if v.IsNil() {
-				return
-			}
-			p := v.Pointer()
-			if seen[p] {
-				return
-			}
-			seen[p] = true
-			descend(v.Elem(), path, depth+1)
-		case reflect.Interface:
-			if !v.IsNil() {
-				descend(v.Elem(), path, depth+1)
-			}
-		case reflect.Struct:
-			for i := range v.NumField() {
-				descend(v.Field(i), fieldPath(path, v.Type().Field(i)), depth+1)
-			}
-		case reflect.Slice, reflect.Array:
-			if v.Type().Elem().Kind() == reflect.Uint8 {
-				return // byte sequences hold nothing any visitor looks for
-			}
-			for i := range v.Len() {
-				descend(v.Index(i), fmt.Sprintf("%s[%d]", path, i), depth+1)
-			}
-		case reflect.Map:
-			for _, e := range orderedEntries(v) {
-				descend(e.key, fmt.Sprintf("%s[%s].key", path, e.label), depth+1)
-				descend(e.value, fmt.Sprintf("%s[%s]", path, e.label), depth+1)
-			}
+		w.seen[p] = true
+		w.descend(v.Elem(), path, depth+1)
+	case reflect.Interface:
+		if !v.IsNil() {
+			w.descend(v.Elem(), path, depth+1)
+		}
+	case reflect.Struct:
+		for i := range v.NumField() {
+			w.descend(v.Field(i), fieldPath(path, v.Type().Field(i)), depth+1)
+		}
+	case reflect.Slice, reflect.Array:
+		if v.Type().Elem().Kind() == reflect.Uint8 {
+			return // byte sequences hold nothing any visitor looks for
+		}
+		for i := range v.Len() {
+			w.descend(v.Index(i), fmt.Sprintf("%s[%d]", path, i), depth+1)
+		}
+	case reflect.Map:
+		for _, e := range orderedEntries(v) {
+			w.descend(e.key, fmt.Sprintf("%s[%s].key", path, e.label), depth+1)
+			w.descend(e.value, fmt.Sprintf("%s[%s]", path, e.label), depth+1)
 		}
 	}
-	walk(reflect.ValueOf(root), "doc", 0)
-	return truncated
 }
 
 // fieldPath extends path with f's name, except for an embedded field, which

ir/irverify/refs_test.go

@@ -243,3 +243,31 @@ func TestWalkValues_ByteSequencesAreNotDescendedInto(t *testing.T) {
 	assert.Less(t, visits, payloadBytes,
 		"walking %d bytes of payload one value at a time is what the skip exists to avoid", payloadBytes)
 }
+
+// TestWalkValues_APointerReachedTwiceIsDescendedIntoOnce holds the cycle guard.
+//
+// Nothing in the corpus reaches it: a compiled Document is a flat registry of
+// values referenced by ID, so no two fields point at one struct and no field
+// points back. That is exactly why it needs planting — the guard is what keeps
+// the walk terminating on a document that does share a pointer, and without a
+// document that does, removing it changes nothing any test observes.
+//
+// Visiting once is also what makes the walk's paths deterministic: a value
+// reached twice would be reported at whichever path the walk happened to take
+// first (invariant 7).
+func TestWalkValues_APointerReachedTwiceIsDescendedIntoOnce(t *testing.T) {
+	t.Parallel()
+	shared := &ir.Naming{Source: "shared"}
+	doc := struct{ A, B *ir.Naming }{A: shared, B: shared}
+
+	var sources int
+	truncated := walkValues(&doc, func(v reflect.Value, _ string) bool {
+		if v.Kind() == reflect.String && v.String() == "shared" {
+			sources++
+		}
+		return true
+	})
+
+	require.False(t, truncated, "two fields are not deep")
+	assert.Equal(t, 1, sources, "the second field finds the pointer already seen and stops there")
+}