refactor(ir): give the shared document walk one home

compilers/compile/types.go

@@ -2,7 +2,6 @@ package compile
 
 import (
 	"fmt"
-	"reflect"
 
 	"github.com/dexpace/morphic/ir"
 )
@@ -28,17 +27,6 @@ type Types struct {
 	byID map[ir.TypeID]string
 }
 
-// isNilTypeDef reports whether td is a nil TypeDef — an untyped nil interface or
-// a typed nil pointer. A typed nil satisfies a type switch case, so a caller
-// cannot screen one by kind.
-func isNilTypeDef(td ir.TypeDef) bool {
-	if td == nil {
-		return true
-	}
-	rv := reflect.ValueOf(td)
-	return rv.Kind() == reflect.Pointer && rv.IsNil()
-}
-
 // refuse records why an entry was rejected. The registry declines to hold it
 // rather than returning an error, because the caller is mid-walk with no useful
 // recovery — but declining silently would make this type the one place able to
@@ -144,7 +132,7 @@ func (t *Types) Intern(pointer string, id ir.TypeID, build func() ir.TypeDef) ir
 	t.claimID(id, pointer)
 	t.byPointer[pointer] = id
 	td := build() // may recurse; a self-reference hits byPointer above
-	if isNilTypeDef(td) {
+	if ir.IsNilTypeDef(td) {
 		// Leaving the coordinate mapped would be the one state NodeAt's contract
 		// rules out: a pointer that resolves to an ID holding no node.
 		delete(t.byPointer, pointer)
@@ -178,7 +166,7 @@ func (t *Types) Register(id ir.TypeID, td ir.TypeDef) {
 		t.refuse("register rejected: empty type id")
 		return
 	}
-	if isNilTypeDef(td) {
+	if ir.IsNilTypeDef(td) {
 		t.refuse("register rejected: nil type definition for id=%q", id)
 		return
 	}

ir/doc.go

@@ -6,6 +6,13 @@
 // entities live in flat, ID-keyed registries on [Document] and reference each
 // other by ID. The whole Document round-trips through JSON deterministically.
 //
+// Besides the nodes, it owns the traversal every consumer inspecting a whole
+// document needs: [WalkValues] is the one bounded, cycle-guarded,
+// deterministically-ordered reflection walk, and [DocumentRegistries] derives
+// what counts as a resolvable reference from Document's own shape. Both live
+// here because a second copy of either is a second answer to keep in step with
+// the first.
+//
 // This package imports only the standard library. It contains no parsing, no
 // generation, and no I/O. All types are plain data and safe for concurrent
 // reads; nothing in this package mutates package-level state.

ir/irverify/ids.go

@@ -22,7 +22,7 @@ import (
 func checkIDs(doc *ir.Document) []Violation {
 	var vs []Violation
 	for id, td := range doc.Types {
-		if isNilTypeDef(td) {
+		if ir.IsNilTypeDef(td) {
 			continue // checkRegistryKeys reports the nil entry itself
 		}
 		vs = appendIDViolations(vs, ir.IDKindType, string(id),
@@ -57,7 +57,7 @@ func checkIDs(doc *ir.Document) []Violation {
 func checkPrimIDs(doc *ir.Document) []Violation {
 	var vs []Violation
 	for id, td := range doc.Types {
-		if isNilTypeDef(td) {
+		if ir.IsNilTypeDef(td) {
 			continue // checkRegistryKeys reports the nil entry itself
 		}
 		path := "types[" + string(id) + "]"

ir/irverify/indices.go

@@ -20,22 +20,26 @@ var (
 //
 // Nothing in an int's Go type marks it as a reference, so collectRefs cannot
 // reach these the way it reaches typed IDs — the carriers are named here
-// instead, and a new integer-index reference has to be named here too.
-// Provenance.Source, the third such index, has its own check in provenance.go.
+// instead, and a new integer-index reference has to be named here too. This is
+// the one thing ir.DocumentRegistries cannot derive and so the one list left
+// written by hand. Provenance.Source, the third such index, has its own check in
+// provenance.go.
 //
 // Naming a carrier means reaching its fields by name, which the Go compiler
 // cannot check: renaming or retyping ir.Service.Servers leaves FieldByName
 // returning the zero reflect.Value, and the Len() below panics on it. The
 // guarantee this package makes — that Verify never crashes on a malformed
 // document — is unaffected, since no input can rename a field, but the coupling
-// is real and is guarded the way this package guards its other hand-written
-// couplings: indexCarrierFields (indices_test.go) fails the moment one of these
-// names or shapes drifts, the same role TestStringTypes_AreAllClassified plays
-// for refKindByType.
-func checkIndices(doc *ir.Document) []Violation {
+// is real and is guarded: indexCarrierFields (indices_test.go) fails the moment
+// one of these names or shapes drifts, and integerFields beside it fails when
+// the IR grows an integer field nobody has classified.
+//
+// The bool reports whether the bounded walk was cut short; Verify folds that
+// into the document's one ir/walk-truncated violation.
+func checkIndices(doc *ir.Document) ([]Violation, bool) {
 	declared := len(doc.Servers)
 	var vs []Violation
-	walkValues(doc, func(v reflect.Value, path string) bool {
+	truncated := ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool {
 		if v.Kind() != reflect.Struct {
 			return true
 		}
@@ -49,7 +53,7 @@ func checkIndices(doc *ir.Document) []Violation {
 		}
 		return true // a service still owns the operations nested below it
 	})
-	return vs
+	return vs, truncated
 }
 
 // appendServerIndexViolations appends to vs a violation per entry of a Servers

ir/irverify/indices_test.go

@@ -19,11 +19,19 @@ func docWithServers() *ir.Document {
 	return &ir.Document{Servers: []ir.Server{{URLTemplate: "https://api.example.com"}}}
 }
 
+// indexViolations runs the index check and drops the truncation flag, which the
+// cases below assert nothing about; TestWalkChecks_EachReportsTruncation holds
+// that half.
+func indexViolations(doc *ir.Document) []Violation {
+	vs, _ := checkIndices(doc)
+	return vs
+}
+
 func TestCheckIndices_ServiceServerIndexOutOfRange(t *testing.T) {
 	doc := docWithServers()
 	doc.Services = []ir.Service{{ID: "s/x", Servers: []int{-1, 1}}}
 
-	got := checkIndices(doc)
+	got := indexViolations(doc)
 	require.Len(t, got, 2, "both the negative index and the one past the end are reported")
 	assert.Equal(t, "ir/server-index-out-of-range", got[0].Code)
 	assert.Equal(t, "doc.Services[0].Servers[0]", got[0].Path)
@@ -34,7 +42,7 @@ func TestCheckIndices_ChannelServerIndexOutOfRange(t *testing.T) {
 	doc := docWithServers()
 	doc.Channels = map[ir.ChannelID]ir.Channel{"c/x": {ID: "c/x", Servers: []int{2}}}
 
-	got := checkIndices(doc)
+	got := indexViolations(doc)
 	require.Len(t, got, 1)
 	assert.Equal(t, "ir/server-index-out-of-range", got[0].Code)
 	assert.Contains(t, got[0].Message, "none of the 1 declared servers")
@@ -43,7 +51,7 @@ func TestCheckIndices_ChannelServerIndexOutOfRange(t *testing.T) {
 func TestCheckIndices_ServerIndexWithNoDeclaredServers(t *testing.T) {
 	doc := &ir.Document{Services: []ir.Service{{ID: "s/x", Servers: []int{0}}}}
 
-	got := checkIndices(doc)
+	got := indexViolations(doc)
 	require.Len(t, got, 1, "a document declaring no servers can satisfy no index")
 	assert.Contains(t, got[0].Message, "none of the 0 declared servers")
 }
@@ -53,7 +61,7 @@ func TestCheckIndices_ServerIndexInRangeIsClean(t *testing.T) {
 	doc.Services = []ir.Service{{ID: "s/x", Servers: []int{0}}}
 	doc.Channels = map[ir.ChannelID]ir.Channel{"c/x": {ID: "c/x", Servers: []int{0}}}
 
-	assert.Empty(t, checkIndices(doc))
+	assert.Empty(t, indexViolations(doc))
 }
 
 // docWithSuccessStatus wraps one operation declaring a single response and one
@@ -72,18 +80,18 @@ func docWithSuccessStatus(status map[int]int) *ir.Document {
 }
 
 func TestCheckIndices_ResponseIndexOutOfRange(t *testing.T) {
-	got := checkIndices(docWithSuccessStatus(map[int]int{-1: 200}))
+	got := indexViolations(docWithSuccessStatus(map[int]int{-1: 200}))
 	require.Len(t, got, 1)
 	assert.Equal(t, "ir/response-index-out-of-range", got[0].Code)
 	assert.Contains(t, got[0].Message, "none of the 1 declared responses")
 
-	got = checkIndices(docWithSuccessStatus(map[int]int{1: 202}))
+	got = indexViolations(docWithSuccessStatus(map[int]int{1: 202}))
 	require.Len(t, got, 1)
 	assert.Equal(t, "doc.Services[0].Groups[0].Operations[0].Bindings.HTTP[0].SuccessStatus[1]", got[0].Path)
 }
 
 func TestCheckIndices_ResponseIndexInRangeIsClean(t *testing.T) {
-	assert.Empty(t, checkIndices(docWithSuccessStatus(map[int]int{0: 200})))
+	assert.Empty(t, indexViolations(docWithSuccessStatus(map[int]int{0: 200})))
 }
 
 // TestVerify_ReportsOutOfRangeIndices pins that Verify runs the check, not just
@@ -152,9 +160,12 @@ func TestIndexCarrierFields_MatchTheIRShape(t *testing.T) {
 // type-driven walk can recognize and which therefore needs an explicit bounds
 // check — or a magnitude that addresses nothing. The distinction is invisible to
 // both checkers' reflection walks, so it is recorded here and this test fails
-// when the ir package grows an integer field that nobody has classified. That is
-// the drift guard the index checks need, in the same spirit as the one over
-// refKindByType.
+// when the ir package grows an integer field that nobody has classified.
+//
+// This is the last hand-written classification of a reference class either
+// checker keeps, and it stays hand-written because nothing can derive it: an
+// ID-keyed registry is recognizable from Document's own shape
+// (ir.DocumentRegistries), while an index is an int like any other.
 var integerFields = map[string]string{
 	"Service.Servers":           "index into Document.Servers; bounds-checked by checkIndices",
 	"Channel.Servers":           "index into Document.Servers; bounds-checked by checkIndices",
@@ -168,6 +179,11 @@ var integerFields = map[string]string{
 	// state, so it is left out rather than guessed at.
 	"Discriminator.Index": "tuple element position; addresses no one slice, see comment",
 
+	// The ir package's own traversal machinery, which is not IR data at all: the
+	// scan reaches every integer field the package declares, and classifying one
+	// costs less than a filter that could hide a real index behind it.
+	"valueWalk.seen": "pointer identities the walk has already followed, not positions",
+
 	"Value.Bytes":           "byte-string payload, not a sequence of positions",
 	"TypeCommon.Usage":      "bitset of usage sites (UsageFlags), not a position",
 	"Property.WireID":       "protobuf field number / thrift id, not a position",
@@ -251,15 +267,14 @@ func integerFieldsOf(t *testing.T, decls map[string]ast.Expr, ts *ast.TypeSpec)
 // mentionsInteger reports whether a field's type expression is built from an
 // integer type, looking through pointers, slices, both halves of a map, and the
 // ir package's own declarations. Following declarations is what makes the guard
-// total, exactly as underlyingIsString does for the string guard
-// (refkinds_test.go): `type Ordinal int` is an integer field as much as a plain
-// int is, and matching only the builtin spelling would let a named index type
-// into the IR unclassified.
+// total: `type Ordinal int` is an integer field as much as a plain int is, and
+// matching only the builtin spelling would let a named index type into the IR
+// unclassified.
 //
 // A declaration whose underlying type comes from another package (a selector
-// such as json.RawMessage) is not followed, for the same reason the string guard
-// does not: resolving it needs go/types rather than a parse. The ir package
-// imports only encoding/json, and declares no integer type through it.
+// such as json.RawMessage) is not followed: resolving that needs go/types rather
+// than a parse. The ir package imports only the standard library, and declares
+// no integer type through any of it.
 //
 // depth bounds the walk through declarations (the bounded-recursion rule). Go
 // forbids a cycle among type declarations except through a pointer, slice or

ir/irverify/irsource_test.go

@@ -0,0 +1,77 @@
+package irverify
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// maxTypeChain bounds the walk through defined types and aliases (the
+// bounded-recursion rule). Go forbids a cycle among type declarations, so
+// exceeding this means the parse went wrong, not that the IR grew deep.
+const maxTypeChain = 16
+
+// typeDecls maps every type name the ir package's production sources declare at
+// package level to the expression it is declared as. Function-local types are not
+// package-level declarations and so cannot name a reference class.
+func typeDecls(t *testing.T) map[string]ast.Expr {
+	t.Helper()
+	out := map[string]ast.Expr{}
+	for _, path := range irSourceFiles(t) {
+		f, err := parser.ParseFile(token.NewFileSet(), path, nil, parser.SkipObjectResolution)
+		require.NoError(t, err, "parsing %s", path)
+		for _, decl := range f.Decls {
+			gd, isGen := decl.(*ast.GenDecl)
+			if !isGen || gd.Tok != token.TYPE {
+				continue
+			}
+			for _, spec := range gd.Specs {
+				ts, isType := spec.(*ast.TypeSpec)
+				require.True(t, isType, "type decl spec is not a TypeSpec: %#v", spec)
+				out[ts.Name.Name] = ts.Type
+			}
+		}
+	}
+	require.NotEmpty(t, out, "the ir package must declare types")
+	return out
+}
+
+// irSourceFiles lists the ir package's non-test Go files.
+func irSourceFiles(t *testing.T) []string {
+	t.Helper()
+	return goSourceFiles(t, packageDir(t, ".."))
+}
+
+// packageDir resolves rel against this test file's own directory, so a result
+// does not depend on the working directory the suite runs from.
+func packageDir(t *testing.T, rel string) string {
+	t.Helper()
+	_, self, _, ok := runtime.Caller(0)
+	require.True(t, ok, "runtime.Caller must report this test's path")
+	return filepath.Join(filepath.Dir(self), rel)
+}
+
+// goSourceFiles lists dir's non-test Go files.
+func goSourceFiles(t *testing.T, dir string) []string {
+	t.Helper()
+	entries, err := os.ReadDir(dir)
+	require.NoError(t, err)
+
+	var out []string
+	for _, e := range entries {
+		name := e.Name()
+		if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") {
+			continue
+		}
+		out = append(out, filepath.Join(dir, name))
+	}
+	require.NotEmpty(t, out, "%s must hold production Go sources", dir)
+	return out
+}