From 04182106c91053ef6abcdd6176e1ae18bcba60a6 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 4 Aug 2026 15:25:26 +0300 Subject: [PATCH 1/2] refactor(ir): give the shared document walk one home MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bounded, cycle-guarded reflection walk over an ir.Document was written twice — once in pass/refs.go, once in ir/irverify/refs.go — with two of its helpers byte-for-byte identical and the rest the same logic spelled twice: the pointer/struct/sequence/map arms, the embedded-field path rule, the depth cap and its rationale, and the byte-slice skip. Both comments already cross-referenced the other to say the two must agree, which is agreement maintained by hand. ir is the package below both — Layer 0, stdlib-only, and already the owner of traversal — so the walk lives there now as ir.WalkValues, with its bound as ir.MaxWalkDepth and its map-key marker as ir.MapKeySuffix. irverify's visit(v, path) bool signature is the one that survived: pass's isRef predicate and per-struct hook are closures over it, so neither package keeps a walker of its own. Two smaller duplications go with it: - isNilTypeDef existed three times (pass, ir/irverify, compilers/compile) with identical bodies. It asks a question about ir.TypeDef, so it is now ir.IsNilTypeDef. - irverify.refKindByType enumerated the registry-bearing ID classes by hand and needed TestStringTypes_AreAllClassified to stop it drifting, while pass derived the same set from Document's own shape. That derivation moves to ir.DocumentRegistries and both checkers use it; the hand-written table and its guard test are deleted, and a registry added to Document is checked by both the moment it exists. The integer-index carriers stay written by hand in both checkers, because nothing can derive them: an index is an int like any other and reflection has nothing to key on. irverify's integerFields guard, which fails when the IR grows an integer field nobody has classified, is what keeps that list honest. Codes, messages, paths and ordering are unchanged; no golden or conformance fixture moved. This also fixes the walking checks that discarded their truncation flag. checkNaming dropped it outright, and checkIndices, checkProvenance and checkRawPayloads dropped it too — only the reference walk reported one. That was benign by coincidence: a pruning walk reaches a subset of what the unpruned reference walk does, so the reference walk trips the cap first, and the coincidence holds only until it gains pruning of its own. Every walking check now returns the flag and Verify folds them into a single ir/walk-truncated violation, a too-deep document being one fact about the document rather than one per walk that noticed it. A drift guard holds the shape from both sides: a function that calls ir.WalkValues must hand a bool back, and one shaped like a walking check must be in the list Verify runs. Closes #247 Closes #55 --- compilers/compile/types.go | 16 +- ir/doc.go | 7 + ir/irverify/ids.go | 4 +- ir/irverify/indices.go | 22 +-- ir/irverify/indices_test.go | 49 ++++-- ir/irverify/irsource_test.go | 77 ++++++++++ ir/irverify/irverify.go | 67 +++++--- ir/irverify/naming.go | 11 +- ir/irverify/provenance.go | 15 +- ir/irverify/rawpayloads.go | 10 +- ir/irverify/refkinds_test.go | 184 ---------------------- ir/irverify/refs.go | 273 +++++---------------------------- ir/irverify/refs_test.go | 145 +++-------------- ir/irverify/walkchecks_test.go | 176 +++++++++++++++++++++ ir/registries.go | 84 ++++++++++ ir/registries_test.go | 79 ++++++++++ ir/typedef.go | 17 ++ ir/typedef_test.go | 13 ++ ir/walk.go | 189 +++++++++++++++++++++++ ir/walk_test.go | 226 +++++++++++++++++++++++++++ pass/refs.go | 267 +++----------------------------- pass/refs_internal_test.go | 99 ++---------- pass/validate.go | 53 ++++--- 23 files changed, 1107 insertions(+), 976 deletions(-) create mode 100644 ir/irverify/irsource_test.go delete mode 100644 ir/irverify/refkinds_test.go create mode 100644 ir/irverify/walkchecks_test.go create mode 100644 ir/registries.go create mode 100644 ir/registries_test.go create mode 100644 ir/walk.go create mode 100644 ir/walk_test.go diff --git a/compilers/compile/types.go b/compilers/compile/types.go index 98dfc17..4a10bdf 100644 --- a/compilers/compile/types.go +++ b/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 } diff --git a/ir/doc.go b/ir/doc.go index c5c1c8a..006848d 100644 --- a/ir/doc.go +++ b/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. diff --git a/ir/irverify/ids.go b/ir/irverify/ids.go index b77e60a..86f4da5 100644 --- a/ir/irverify/ids.go +++ b/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) + "]" diff --git a/ir/irverify/indices.go b/ir/irverify/indices.go index 3d49c3b..91e1162 100644 --- a/ir/irverify/indices.go +++ b/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, "doc", 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 diff --git a/ir/irverify/indices_test.go b/ir/irverify/indices_test.go index b34044a..8e376bb 100644 --- a/ir/irverify/indices_test.go +++ b/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 diff --git a/ir/irverify/irsource_test.go b/ir/irverify/irsource_test.go new file mode 100644 index 0000000..4743a99 --- /dev/null +++ b/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 +} diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index 64daaaa..5dbf4b6 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -1,7 +1,6 @@ package irverify import ( - "reflect" "sort" "strconv" "unicode/utf8" @@ -32,12 +31,8 @@ func Verify(doc *ir.Document) []Violation { vs := checkRegistryKeys(doc) vs = append(vs, checkIDs(doc)...) vs = append(vs, checkPrimIDs(doc)...) - vs = append(vs, checkReferentialIntegrity(doc)...) - vs = append(vs, checkNaming(doc)...) vs = append(vs, checkDiagnostics(doc)...) - vs = append(vs, checkRawPayloads(doc)...) - vs = append(vs, checkProvenance(doc)...) - vs = append(vs, checkIndices(doc)...) + vs = append(vs, runWalkChecks(doc)...) // Stable: two violations can share a (Code, Path) — an embedded field // contributes no path segment, so two promoted fields of the same name would @@ -52,14 +47,59 @@ func Verify(doc *ir.Document) []Violation { return vs } +// walkChecks are the checks that reach their subject through a bounded walk of +// the document. Each returns whether its own walk was cut short, so the flag is +// part of the signature rather than a value a check can quietly drop. +func walkChecks() []func(*ir.Document) ([]Violation, bool) { + return []func(*ir.Document) ([]Violation, bool){ + checkReferentialIntegrity, + checkNaming, + checkRawPayloads, + checkProvenance, + checkIndices, + } +} + +// runWalkChecks runs every walking check and folds their truncation flags into +// one ir/walk-truncated violation. +// +// Truncation is a fact about the document, not about the check that noticed it, +// so it is reported once here rather than once per walk that noticed it — which +// would give one document a violation per walking check, all sharing a code and +// a path. Not reporting it at all is what left the pruning walks silently +// under-checking a too-deep document (GitHub #55): a pruned walk reaches a subset +// of what the unpruned reference walk does, so today that one trips the cap +// first, and depending on that coincidence is exactly what the flag replaces. +func runWalkChecks(doc *ir.Document) []Violation { + var vs []Violation + truncated := false + for _, check := range walkChecks() { + found, cut := check(doc) + vs = append(vs, found...) + truncated = truncated || cut + } + if !truncated { + return vs + } + return append(vs, Violation{ + Code: "ir/walk-truncated", + Message: "document nests deeper than the bounded verifier walk; part of it went unchecked", + Path: "doc", + }) +} + // checkRegistryKeys asserts every entry of each flat, ID-keyed registry // (Types, Channels, Messages, Auth) is keyed by its own node ID and that the key // is non-empty (invariant #3). Each registry contributes symmetric empty-*-id and // *-id-mismatch violations. +// +// A nil type definition is reported rather than dereferenced — Common() panics +// on one — which is what keeps Verify a report-only oracle that never crashes on +// a malformed document. The walk-based checks already tolerate nil entries. func checkRegistryKeys(doc *ir.Document) []Violation { var vs []Violation for id, td := range doc.Types { - if isNilTypeDef(td) { + if ir.IsNilTypeDef(td) { vs = append(vs, Violation{ Code: "ir/nil-type", Message: "types registry has a nil type definition", @@ -102,19 +142,6 @@ func registryKey(vs []Violation, noun, reg, key, nodeID string) []Violation { return vs } -// isNilTypeDef reports whether td is a nil TypeDef — either an untyped nil -// interface or a typed nil pointer. Calling Common() on either panics, so -// checkRegistryKeys reports the entry as an ir/nil-type violation instead of -// dereferencing it, keeping Verify a report-only oracle that never crashes on a -// malformed document (the walk-based checks already tolerate nil entries). -func isNilTypeDef(td ir.TypeDef) bool { - if td == nil { - return true - } - rv := reflect.ValueOf(td) - return rv.Kind() == reflect.Pointer && rv.IsNil() -} - // checkDiagnostics asserts every diagnostic message is well-formed UTF-8 // (invariant #7). A message carrying an ill-formed byte run — as a third-party // validator emits when it truncates a multibyte rune — reaches diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index 6ebe899..eada76b 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -12,17 +12,18 @@ var namingType = reflect.TypeFor[ir.Naming]() // checkNaming asserts every Naming.Canonical is what invariant #4 promises: a // neutral lower_snake word sequence, carrying no casing an emitter should own -// and no character that is not part of a word. It reuses walkValues to reach -// every ir.Naming value in the document. +// and no character that is not part of a word. It reuses the shared bounded walk +// to reach every ir.Naming value in the document, and reports whether that walk +// was cut short so a name past the cap cannot go unchecked in silence. // // Only Canonical is checked. Naming.Hint — the generated-name channel for // anonymous types — is held to none of these rules, so casing and punctuation // still reach the IR through it. That is GitHub #54, left open deliberately: // closing it means changing how the compilers derive hints and regenerating // every golden, which is a different change from tightening this checker. -func checkNaming(doc *ir.Document) []Violation { +func checkNaming(doc *ir.Document) ([]Violation, bool) { var vs []Violation - walkValues(doc, func(v reflect.Value, path string) bool { + truncated := ir.WalkValues(doc, "doc", func(v reflect.Value, path string) bool { if v.Kind() != reflect.Struct || v.Type() != namingType { return true } @@ -30,7 +31,7 @@ func checkNaming(doc *ir.Document) []Violation { v.FieldByName("Source").String(), v.FieldByName("Canonical").String(), path) return false // Naming holds no references or nested Naming to descend into }) - return vs + return vs, truncated } // appendNamingViolations reports the ways canon can break neutrality. They are diff --git a/ir/irverify/provenance.go b/ir/irverify/provenance.go index c8d39f7..ff564a7 100644 --- a/ir/irverify/provenance.go +++ b/ir/irverify/provenance.go @@ -12,16 +12,17 @@ var provenanceType = reflect.TypeFor[ir.Provenance]() // checkProvenance asserts every Provenance.Source addresses a declared entry of // Document.Sources. The index is a reference like any typed ID — a stale or // off-by-one one makes a report point at a file the document never loaded — -// but Sources is a slice rather than an ID-keyed registry, so it cannot ride -// refKindByType and gets its own check. +// but Sources is a slice rather than an ID-keyed registry, so no derived +// registry resolves it and it gets its own check. // // It is document-wide rather than scoped to any one carrier: the defect reads -// the same on a type, a diagnostic, or an Unmodeled entry, and walkValues -// reaches all of them for one traversal. -func checkProvenance(doc *ir.Document) []Violation { +// the same on a type, a diagnostic, or an Unmodeled entry, and one walk reaches +// all of them. The bool reports whether that walk was cut short; Verify folds it +// into the document's one ir/walk-truncated violation. +func checkProvenance(doc *ir.Document) ([]Violation, bool) { var vs []Violation declared := len(doc.Sources) - walkValues(doc, func(v reflect.Value, path string) bool { + truncated := ir.WalkValues(doc, "doc", func(v reflect.Value, path string) bool { if v.Kind() != reflect.Struct || v.Type() != provenanceType { return true } @@ -36,7 +37,7 @@ func checkProvenance(doc *ir.Document) []Violation { } return false // Provenance holds no references and no nested Provenance }) - return vs + return vs, truncated } // sourceOutOfRange reports whether index fails to address one of declared diff --git a/ir/irverify/rawpayloads.go b/ir/irverify/rawpayloads.go index b523694..cceb12d 100644 --- a/ir/irverify/rawpayloads.go +++ b/ir/irverify/rawpayloads.go @@ -25,10 +25,12 @@ var ( // compiler will write protocol bindings. // // Entries need no ordering first: each violation carries its key in Path, and -// Verify orders the whole result by (Code, Path) before returning it. -func checkRawPayloads(doc *ir.Document) []Violation { +// Verify orders the whole result by (Code, Path) before returning it. The bool +// reports whether the bounded walk was cut short; Verify folds that into the +// document's one ir/walk-truncated violation. +func checkRawPayloads(doc *ir.Document) ([]Violation, bool) { var vs []Violation - walkValues(doc, func(v reflect.Value, path string) bool { + truncated := ir.WalkValues(doc, "doc", func(v reflect.Value, path string) bool { if v.Kind() != reflect.Map { return true } @@ -42,7 +44,7 @@ func checkRawPayloads(doc *ir.Document) []Violation { } return false // entries hold no references and no nested payload map }) - return vs + return vs, truncated } // appendEntries appends check's verdict on every entry of the payload map m to diff --git a/ir/irverify/refkinds_test.go b/ir/irverify/refkinds_test.go deleted file mode 100644 index 82f1a5f..0000000 --- a/ir/irverify/refkinds_test.go +++ /dev/null @@ -1,184 +0,0 @@ -package irverify - -import ( - "go/ast" - "go/parser" - "go/token" - "os" - "path/filepath" - "runtime" - "slices" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// notReferences classifies every named string type in ir that addresses no -// document-level registry, and says why each addresses none. -// -// Classifying the whole population rather than filtering it to what looks like an -// ID is what makes the guard total, in the same spirit as integerFields in -// indices_test.go. A filter on the ID suffix admits a reference class spelled -// without one; a filter on the literal underlying ident `string` admits one -// defined from another ID type. Neither can be admitted here, because every named -// string type has to be written down one way or the other. -var notReferences = map[string]string{ - // Identities the IR does have, but that no flat, document-level registry - // declares: these nodes are reached by position inside their owner (refKind). - "OpID": "an operation is a position inside its group, not a registry entry", - "ServiceID": "a service is a position in Document.Services", - "PropID": "a property is a position inside its model", - - // Closed enums and scalar payloads: values, never identities. - "AdditionalMode": "openness of a model's property set", - "AuthKind": "kind of auth scheme", - "BigVal": "arbitrary-precision numeric literal", - "HTTPLocation": "where a parameter sits on the wire", - "IdempotencyKind": "idempotency guarantee", - "MsgDirection": "publish or subscribe", - "PageStrategy": "pagination strategy", - "PresenceKind": "how a property represents absence", - "PrimKind": "which built-in scalar", - "Severity": "how bad a diagnostic is", - "StreamingMode": "shape of a streaming exchange", - "TypeKind": "TypeDef sum tag", - "UnmodeledReason": "why a construct is kept verbatim rather than modeled", - "ValueKind": "Value sum tag", - - // An alias is not a distinct type at run time, so refKindByType can never key - // on one: reflection reports the type it aliases. Listing it here is the - // conservative answer — the aliased type is classified on its own row. - "Lifecycle": "alias of plain string; reflection sees string, which names no registry", -} - -// TestStringTypes_AreAllClassified fails when ir declares a named string type -// that refKindByType neither resolves nor notReferences accounts for. -// -// collectRefs finds reference-bearing fields by reflection, so no field can be -// missed; refKindByType is the one piece still written by hand, and this is what -// stops a newly added registry from going silently unchecked. -// -// One kind of reference stays invisible here and cannot be given an exclusion -// row: one carried in a plain `string` field. It declares no named type to -// classify, and reflection sees the same `string` every free-form field is. -// -// Docs.Description is one such field — CommonMark that may carry {t:TypeID} -// cross-reference tokens for emitters to resolve — so a token naming a type no -// registry declares is reported by neither reference walk. Content.Encoding's -// keys were another until they were retyped map[PropID]PartEncoding (#134). The -// retype bought an honest shape and compile-time protection against passing a -// TypeID or a bare string, not resolution: PropID addresses no document-level -// registry (its row below), so neither reference walk can resolve a key either -// way. pass.Validate resolves each one by hand instead, against the properties -// of the model the content's type names. -func TestStringTypes_AreAllClassified(t *testing.T) { - t.Parallel() - resolved := map[string]bool{} - for rt := range refKindByType { - resolved[rt.Name()] = true - } - - found := declaredStringTypes(t) - require.NotEmpty(t, found, "the ir package must declare named string types") - for _, name := range found { - assert.True(t, resolved[name] || notReferences[name] != "", - "ir.%s is a named string type that refKindByType does not resolve and "+ - "notReferences does not classify: name the registry it references (and "+ - "teach refKindByType to resolve it), or say why it references none", name) - } -} - -// 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 - -// declaredStringTypes returns, in sorted order, the name of every type the ir -// package declares whose underlying type is string. Each step is followed through -// the package's own declarations, so `type GizmoID TypeID` and -// `type DoodadID = TypeID` both land on string — where matching the literal ident -// `string` sees neither. -// -// A declaration whose underlying type comes from another package (a selector such -// as json.RawMessage) is not followed: resolving that needs go/types rather than a -// parse. The ir package imports only encoding/json, and declares no string type -// through it. -func declaredStringTypes(t *testing.T) []string { - t.Helper() - decls := typeDecls(t) - names := make([]string, 0, len(decls)) - for name := range decls { - if underlyingIsString(t, decls, name) { - names = append(names, name) - } - } - slices.Sort(names) - return names -} - -// underlyingIsString reports whether name resolves to string, following each step -// through decls. -func underlyingIsString(t *testing.T, decls map[string]ast.Expr, name string) bool { - t.Helper() - for range maxTypeChain { - expr, isDeclared := decls[name] - if !isDeclared { - return name == "string" // the chain ended at a builtin - } - id, isIdent := expr.(*ast.Ident) - if !isIdent { - return false // struct, map, slice, interface, func or selector - } - name = id.Name - } - require.Fail(t, "type chain too long", "resolving %s exceeded %d steps", name, maxTypeChain) - return false -} - -// 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, located relative to -// this test's own path so the result does not depend on the working directory. -func irSourceFiles(t *testing.T) []string { - t.Helper() - _, self, _, ok := runtime.Caller(0) - require.True(t, ok, "runtime.Caller must report this test's path") - dir := filepath.Join(filepath.Dir(self), "..") - 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)) - } - return out -} diff --git a/ir/irverify/refs.go b/ir/irverify/refs.go index edbb9e0..7ca5c91 100644 --- a/ir/irverify/refs.go +++ b/ir/irverify/refs.go @@ -1,99 +1,32 @@ package irverify import ( - "fmt" "reflect" - "slices" - "strings" "github.com/dexpace/morphic/ir" ) -// maxWalkDepth bounds the reflection traversal (bounded-recursion rule). Value -// trees (defaults, examples) are the deepest structures reached, and compilers -// cap their nesting (the OpenAPI compiler caps it at 128), so this limit sits -// well above what a validly-bounded document can produce — hitting it signals -// a pathological document, not legitimate nesting. walkValues reports -// truncation and checkReferentialIntegrity surfaces it as ir/walk-truncated. -const maxWalkDepth = 4096 - -// refSite is one discovered ID reference and the reference class (refKind) it -// must resolve in. +// refSite is one discovered ID reference: the class that made it a reference, +// its value, and where in the document it sits. type refSite struct { - id string - kind refKind - path string -} - -// resolves reports whether s.id exists in the registry s.kind names. The -// has == nil guard — rather than calling s.kind.has directly — keeps Verify a -// report-only oracle that never crashes on a malformed document, even if a -// refSite is ever built with a kind this package does not recognize. -func (s refSite) resolves(doc *ir.Document) bool { - if s.kind.has == nil { - return false - } - return s.kind.has(doc, s.id) -} - -// refKind is one class of typed-ID reference this checker resolves: a -// registry label (for violation paths and messages), a diagnostic-code -// singular, and a resolves-in-registry test. refKindByType is the single -// source of truth for what counts as a "reference" here, so a class can't be -// silently dropped by missing it from one of several parallel lists. -// -// PropID and ServiceID are intentionally absent: neither lives in a -// document-level flat registry, so neither can be resolved by a -// registry-driven walk. -// -// PropID stays out here rather than growing a second implementation. Resolving -// one means collecting the ir.Property values a document declares and looking -// the ID up among them, which pass.Validate's checkPropIDRefs does; a copy of -// that in this checker would be a second answer to maintain in step with the -// first, for a class this walk's own mechanism cannot reach either way. -type refKind struct { - registry string - singular string - has func(doc *ir.Document, id string) bool -} - -// refKindByType maps a typed-ID reflect.Type to its refKind. Literals below -// are keyed, not positional: registry and singular are both plain strings, -// and a positional row could silently swap them, emitting -// "ir/dangling-types-ref" instead of "ir/dangling-type-ref". -var refKindByType = map[reflect.Type]refKind{ - reflect.TypeFor[ir.TypeID](): {registry: "types", singular: "type", has: func(doc *ir.Document, id string) bool { - _, ok := doc.Types[ir.TypeID(id)] - return ok - }}, - reflect.TypeFor[ir.AuthID](): {registry: "auth", singular: "auth", has: func(doc *ir.Document, id string) bool { - _, ok := doc.Auth[ir.AuthID(id)] - return ok - }}, - reflect.TypeFor[ir.ChannelID](): {registry: "channels", singular: "channel", has: func(doc *ir.Document, id string) bool { - _, ok := doc.Channels[ir.ChannelID(id)] - return ok - }}, - reflect.TypeFor[ir.MessageID](): {registry: "messages", singular: "message", has: func(doc *ir.Document, id string) bool { - _, ok := doc.Messages[ir.MessageID(id)] - return ok - }}, + idType reflect.Type + id string + path string } -// collectRefs walks doc and returns every non-empty typed-ID reference plus -// whether the bounded walk was truncated. It inspects struct fields, slice/array -// elements, and both map keys and values: most map keys are an entry's own ID -// and resolve trivially, but some — Service.Renames's map[TypeID]Naming keys — -// are genuine references into a registry that must resolve, so keys are -// collected too. -func collectRefs(doc *ir.Document) ([]refSite, bool) { +// collectRefs walks doc and returns every non-empty typed-ID reference regs +// recognizes, plus whether the bounded walk was truncated. It inspects both map +// keys and values: most keys are an entry's own ID and resolve trivially, but +// some — Service.Renames's map[TypeID]Naming keys — are genuine references into +// a registry that must resolve. +func collectRefs(doc *ir.Document, regs ir.Registries) ([]refSite, bool) { var sites []refSite - truncated := walkValues(doc, func(v reflect.Value, path string) bool { - if v.Kind() != reflect.String { + truncated := ir.WalkValues(doc, "doc", func(v reflect.Value, path string) bool { + if v.Kind() != reflect.String || v.String() == "" { return true } - if k, ok := refKindByType[v.Type()]; ok && v.String() != "" { - sites = append(sites, refSite{id: v.String(), kind: k, path: path}) + if _, isRef := regs[v.Type()]; isRef { + sites = append(sites, refSite{idType: v.Type(), id: v.String(), path: path}) } return true }) @@ -101,165 +34,35 @@ func collectRefs(doc *ir.Document) ([]refSite, bool) { } // checkReferentialIntegrity asserts every discovered reference resolves in its -// registry, emitting one dangling-*-ref Violation per unresolved reference. -func checkReferentialIntegrity(doc *ir.Document) []Violation { +// registry, emitting one dangling-*-ref Violation per unresolved reference. It +// reports whether the bounded walk was truncated; Verify folds that into the +// document's one ir/walk-truncated violation. +// +// What counts as a reference comes from Document's own shape rather than a table +// written here: a registry added to Document is checked the moment it exists, +// under a code spelled from the ID type it is keyed by — the same derivation +// pass.Validate reports the identical defect under, so one defect reads as one +// code whichever checker a caller runs. +// +// Two ID classes are outside what a registry-driven walk can resolve, and both +// stay out rather than growing a second implementation here. ir.ServiceID names +// a position in Document.Services; ir.PropID names a position inside its model, +// and resolving one means collecting the ir.Property values a document declares +// and looking the ID up among them, which pass.Validate's checkPropIDRefs does. +func checkReferentialIntegrity(doc *ir.Document) ([]Violation, bool) { + regs := ir.DocumentRegistries(doc) + sites, truncated := collectRefs(doc, regs) var vs []Violation - sites, truncated := collectRefs(doc) - if truncated { - vs = append(vs, Violation{ - Code: "ir/walk-truncated", - Message: "document nests deeper than the bounded verifier walk; some references and names went unchecked", - Path: "doc", - }) - } for _, s := range sites { - if s.resolves(doc) { + reg := regs[s.idType] + if reg.Has(s.id) { continue } vs = append(vs, Violation{ - Code: "ir/dangling-" + s.kind.singular + "-ref", - Message: "reference " + s.id + " does not resolve in " + s.kind.registry, + Code: "ir/dangling-" + ir.RefNoun(s.idType) + "-ref", + Message: "reference " + s.id + " does not resolve in " + reg.Label, Path: s.path, }) } - return vs -} - -// walkValues performs a bounded, cycle-guarded reflection traversal of root, -// calling visit on every value it reaches; returning false from visit skips -// that value's children. Map keys are walked too, not just values — see -// collectRefs for why that matters. -// -// A value reached through an unexported field is read-only, and Interface() -// panics on one where FieldByName does not, so a visitor reads the fields it -// needs rather than converting the value back to its Go type. That is what keeps -// Verify an oracle that never crashes on a malformed document. -// -// Map entries are visited in rendered-key order rather than Go's randomized map -// order. A pointer reachable from two entries is descended into at whichever the -// walk reaches first, so a random order yields a different path for it — and so a -// different violation set, not merely a different order — on each run, which -// Verify's final sort cannot repair (invariant 7). -// -// Byte sequences are skipped, matching pass.refWalk.walkSequence. Unmodeled and -// RawConfig payloads are json.RawMessage and are the largest values a document -// holds, while a uint8 element is none of the things a visitor here looks for — -// no typed ID, no Unmodeled map, no Provenance, no index carrier. Descending one -// costs a reflect.Value and a formatted path per byte for nothing: Verify over a -// document holding one 256 KB payload measured 88ms without this skip against -// 22µs with it, for the same result. Since the result is the same either way, a -// test asserting it cannot notice the skip going missing — each walk is guarded -// by one that counts what it reaches instead -// (TestWalkValues_ByteSequencesAreNotDescendedInto here, walkSequence's in pass). -// -// The bool return is true only when the depth cap truncated the walk, so -// 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 { - 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 - } - p := v.Pointer() - if w.seen[p] { - return - } - 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) - } - } -} - -// fieldPath extends path with f's name, except for an embedded field, which -// contributes no segment: JSON inlines it and Go promotes its fields, so -// "….TypeCommon.Examples[0]" names a step neither encoding has — the example is -// reached as "….Examples[0]" in both. -func fieldPath(path string, f reflect.StructField) string { - if f.Anonymous { - return path - } - return path + "." + f.Name -} - -// mapEntry is one map entry paired with its rendered key, which both spells the -// entry's path and orders the walk. -type mapEntry struct { - label string - key reflect.Value - value reflect.Value -} - -// orderedEntries returns v's entries ordered by rendered key. Ordering by the -// same rendering the path uses keeps the two in step, and it is a total order for -// every key type the IR declares: named string types, plain strings and ints all -// render distinct keys distinctly. -func orderedEntries(v reflect.Value) []mapEntry { - entries := make([]mapEntry, 0, v.Len()) - for iter := v.MapRange(); iter.Next(); { - k := iter.Key() - entries = append(entries, mapEntry{label: fmt.Sprintf("%v", k), key: k, value: iter.Value()}) - } - slices.SortFunc(entries, func(a, b mapEntry) int { return strings.Compare(a.label, b.label) }) - return entries + return vs, truncated } diff --git a/ir/irverify/refs_test.go b/ir/irverify/refs_test.go index 023fcb4..4836469 100644 --- a/ir/irverify/refs_test.go +++ b/ir/irverify/refs_test.go @@ -26,10 +26,10 @@ func docWithRef(target ir.TypeID) *ir.Document { func TestCollectRefs_FindsTypeRefTarget(t *testing.T) { doc := docWithRef("t/x/Missing") - sites, _ := collectRefs(doc) + sites, _ := collectRefs(doc, ir.DocumentRegistries(doc)) var found bool for _, s := range sites { - if s.id == "t/x/Missing" && s.kind.registry == "types" { + if s.id == "t/x/Missing" && s.idType == reflect.TypeFor[ir.TypeID]() { found = true } } @@ -38,22 +38,31 @@ func TestCollectRefs_FindsTypeRefTarget(t *testing.T) { func TestCollectRefs_SkipsEmptyIDs(t *testing.T) { doc := docWithRef("") // empty target must not be collected - sites, _ := collectRefs(doc) + sites, _ := collectRefs(doc, ir.DocumentRegistries(doc)) for _, s := range sites { assert.NotEqual(t, "", s.id) } } +// refViolations runs the reference check and drops the truncation flag, which +// the cases below assert nothing about; TestWalkChecks_EachReportsTruncation +// holds that half. +func refViolations(doc *ir.Document) []Violation { + vs, _ := checkReferentialIntegrity(doc) + return vs +} + func TestCheckReferentialIntegrity_DanglingTypeRef(t *testing.T) { - got := checkReferentialIntegrity(docWithRef("t/x/Missing")) + got := refViolations(docWithRef("t/x/Missing")) require.Len(t, got, 1) assert.Equal(t, "ir/dangling-type-ref", got[0].Code) + assert.Contains(t, got[0].Message, "does not resolve in types") } func TestCheckReferentialIntegrity_ResolvedRefIsClean(t *testing.T) { doc := docWithRef("t/x/Target") doc.Types["t/x/Target"] = &ir.Any{TypeCommon: ir.TypeCommon{ID: "t/x/Target"}} - assert.Empty(t, checkReferentialIntegrity(doc)) + assert.Empty(t, refViolations(doc)) } func TestCheckReferentialIntegrity_DanglingAuthRef(t *testing.T) { @@ -66,9 +75,10 @@ func TestCheckReferentialIntegrity_DanglingAuthRef(t *testing.T) { }}, }}, } - got := checkReferentialIntegrity(doc) + got := refViolations(doc) require.NotEmpty(t, got) assert.Equal(t, "ir/dangling-auth-ref", got[0].Code) + assert.Contains(t, got[0].Message, "does not resolve in auth") } func TestCheckReferentialIntegrity_DanglingMessageRef(t *testing.T) { @@ -79,9 +89,10 @@ func TestCheckReferentialIntegrity_DanglingMessageRef(t *testing.T) { doc := &ir.Document{ Channels: map[ir.ChannelID]ir.Channel{ch.ID: ch}, } - got := checkReferentialIntegrity(doc) + got := refViolations(doc) require.NotEmpty(t, got) assert.Equal(t, "ir/dangling-message-ref", got[0].Code) + assert.Contains(t, got[0].Message, "does not resolve in messages") } func TestCheckReferentialIntegrity_DanglingChannelRef(t *testing.T) { @@ -97,9 +108,10 @@ func TestCheckReferentialIntegrity_DanglingChannelRef(t *testing.T) { }}, }}, } - got := checkReferentialIntegrity(doc) + got := refViolations(doc) require.NotEmpty(t, got) assert.Equal(t, "ir/dangling-channel-ref", got[0].Code) + assert.Contains(t, got[0].Message, "does not resolve in channels") } func TestCheckReferentialIntegrity_DanglingRenameKey(t *testing.T) { @@ -112,66 +124,12 @@ func TestCheckReferentialIntegrity_DanglingRenameKey(t *testing.T) { Renames: map[ir.TypeID]ir.Naming{"t/x/ghost": {Source: "Ghost"}}, }}, } - got := checkReferentialIntegrity(doc) + got := refViolations(doc) require.Len(t, got, 1) assert.Equal(t, "ir/dangling-type-ref", got[0].Code) assert.Equal(t, "t/x/ghost", refIDInMessage(got[0].Message)) } -func TestCheckReferentialIntegrity_DeepValueTreeReportsTruncation(t *testing.T) { - // A Value tree nested past maxWalkDepth must not be silently under-checked: the - // bounded walk reports truncation rather than claiming the document is clean. - v := ir.Value{Kind: ir.ValueNull} - for range 2 * maxWalkDepth { - v = ir.Value{Kind: ir.ValueList, List: []ir.Value{v}} - } - deep := v - m := &ir.Model{TypeCommon: ir.TypeCommon{ - ID: "t/x/M", - Examples: []ir.Example{{Value: &deep}}, - }} - doc := &ir.Document{Types: ir.TypeRegistry{m.ID: m}} - - got := checkReferentialIntegrity(doc) - require.NotEmpty(t, got) - assert.Equal(t, "ir/walk-truncated", got[0].Code) -} - -func TestRefSite_ResolvesUnknownKindIsUnresolved(t *testing.T) { - // A refSite whose kind is the zero value — as an unrecognized reference - // class would produce, since it never gets an entry in refKindByType — - // falls through the has == nil guard and reports the reference as - // unresolved rather than panicking. - site := refSite{id: "x"} - assert.False(t, site.resolves(&ir.Document{})) -} - -func TestCollectRefs_SharedPointerVisitedOnce(t *testing.T) { - // The same *TypeRef reachable through two template arguments must trip the - // cycle guard: the walk descends into it once and skips the repeat visit, so - // its target is discovered exactly once. - shared := &ir.TypeRef{Target: "t/x/Shared"} - m := &ir.Model{TypeCommon: ir.TypeCommon{ - ID: "t/x/M", - Instantiation: &ir.TemplateInstantiation{Args: []ir.TemplateArg{ - {Type: shared}, - {Type: shared}, - }}, - }} - doc := &ir.Document{Types: ir.TypeRegistry{m.ID: m}} - - sites, truncated := collectRefs(doc) - assert.False(t, truncated) - - var count int - for _, s := range sites { - if s.id == "t/x/Shared" { - count++ - } - } - assert.Equal(t, 1, count, "the shared pointer's target is collected once, not per reference") -} - // refIDInMessage extracts the reference ID from a dangling-ref message of the // form "reference does not resolve in ". func refIDInMessage(msg string) string { @@ -210,64 +168,3 @@ func TestVerify_AliasedPointerIsDeterministic(t *testing.T) { require.Equal(t, want, fmt.Sprintf("%+v", Verify(doc)), "run %d disagrees with the first", i) } } - -// payloadBytes sizes the preserved payload the byte-skip test walks. It is large -// enough that descending it would dominate any plausible visit count for a -// two-node document, and small enough to build inline. -const payloadBytes = 4096 - -// TestWalkValues_ByteSequencesAreNotDescendedInto drives the byte-sequence skip, -// which exists for cost rather than for correctness: a uint8 element is none of -// the things a visitor looks for — no typed ID, no Unmodeled map, no Provenance, -// no index carrier — so collecting nothing from it is the same result either -// way, and only the price differs. Descending one payload spends a reflect.Value -// and a formatted path per byte. -// -// The visit count is what makes that assertable: the walk reports every value it -// reaches, so a document whose payload dwarfs its structure must still be walked -// in a number of steps bounded by the structure. Without the skip the count -// grows past the payload's length instead. -func TestWalkValues_ByteSequencesAreNotDescendedInto(t *testing.T) { - t.Parallel() - doc := &ir.Document{Unmodeled: ir.Unmodeled{"openapi:x-thing": { - Reason: ir.ReasonVendorExtension, - Value: ir.RawValue(`"` + strings.Repeat("t", payloadBytes) + `"`), - }}} - - var visits int - truncated := walkValues(doc, func(reflect.Value, string) bool { - visits++ - return true - }) - require.False(t, truncated) - 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") -} diff --git a/ir/irverify/walkchecks_test.go b/ir/irverify/walkchecks_test.go new file mode 100644 index 0000000..230409b --- /dev/null +++ b/ir/irverify/walkchecks_test.go @@ -0,0 +1,176 @@ +package irverify + +import ( + "go/ast" + "go/parser" + "go/token" + "go/types" + "reflect" + "runtime" + "slices" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" +) + +// deepDoc returns a document whose one example value nests past ir.MaxWalkDepth, +// so every walk over it is cut short. The model carries a well-formed name, so +// the naming check has something to reach before the value tree runs the walk +// out of budget. +func deepDoc() *ir.Document { + v := ir.Value{Kind: ir.ValueNull} + for range ir.MaxWalkDepth { + v = ir.Value{Kind: ir.ValueList, List: []ir.Value{v}} + } + m := &ir.Model{TypeCommon: ir.TypeCommon{ + ID: "t/x/M", + Name: ir.Naming{Source: "M", Canonical: "m"}, + Examples: []ir.Example{{Value: &v}}, + }} + return &ir.Document{Types: ir.TypeRegistry{m.ID: m}} +} + +// TestWalkChecks_EachReportsTruncation drives the flag every walking check owes +// its caller. checkNaming used to discard it (GitHub #55), which was benign only +// because the unpruned reference walk reaches at least as far as every pruning +// one and so trips the cap first — a coincidence, not a guarantee, and one that +// hid every name past the cap the day a pruning walk went deeper. +func TestWalkChecks_EachReportsTruncation(t *testing.T) { + t.Parallel() + doc := deepDoc() + for _, check := range walkChecks() { + _, truncated := check(doc) + assert.True(t, truncated, + "%s walked a document nested past the cap without reporting it", checkName(check)) + } +} + +// TestVerify_ReportsTruncationOnce holds the other half: five walks over one +// too-deep document are one fact about the document, so Verify states it once +// rather than once per walk that noticed it. +func TestVerify_ReportsTruncationOnce(t *testing.T) { + t.Parallel() + var truncations int + for _, v := range Verify(deepDoc()) { + if v.Code != "ir/walk-truncated" { + continue + } + truncations++ + assert.Equal(t, "doc", v.Path, "truncation is a fact about the document, not about a node") + } + assert.Equal(t, 1, truncations) +} + +// TestWalkChecks_NoWalkDropsItsTruncationFlag is the drift guard on the list +// itself, and it makes two claims that together leave a walk nowhere to drop the +// flag. A function that calls ir.WalkValues must hand a bool back to its caller, +// so the flag cannot die at the walk site; and a function shaped like a walking +// check must be in walkChecks, so it cannot be wired into Verify by a path that +// never folds the flag into a report. +// +// The two populations overlap without coinciding: checkReferentialIntegrity +// reaches the walk through collectRefs, so it is a check that does not itself +// call ir.WalkValues, and collectRefs is a walker that is no check. Each claim is +// therefore asked of the functions it applies to rather than of one list. +func TestWalkChecks_NoWalkDropsItsTruncationFlag(t *testing.T) { + t.Parallel() + listed := map[string]bool{} + for _, check := range walkChecks() { + listed[checkName(check)] = true + } + + fns := packageFuncs(t) + require.NotEmpty(t, fns, "the package must declare functions") + var walkers int + for _, fn := range fns { + if fn.callsWalkValues { + walkers++ + assert.Equal(t, "bool", last(fn.results), + "%s calls ir.WalkValues but returns no truncation flag", fn.name) + } + if slices.Equal(fn.results, []string{"[]Violation", "bool"}) { + assert.True(t, listed[fn.name], + "%s is shaped like a walking check but is not in walkChecks, "+ + "so nothing folds its truncation flag into a report", fn.name) + } + } + assert.Positive(t, walkers, + "nothing reaches the document through ir.WalkValues, so the claim above proves nothing") +} + +// checkName is a walkChecks entry's function name, without its package path. +func checkName(check func(*ir.Document) ([]Violation, bool)) string { + full := runtime.FuncForPC(reflect.ValueOf(check).Pointer()).Name() + return full[strings.LastIndex(full, ".")+1:] +} + +// last returns the final element of ss, or "" when ss is empty. +func last(ss []string) string { + if len(ss) == 0 { + return "" + } + return ss[len(ss)-1] +} + +// packageFunc is one function this package's production sources declare: its +// name, the types it returns, and whether its body reaches ir.WalkValues. +type packageFunc struct { + name string + results []string + callsWalkValues bool +} + +// packageFuncs returns every function declared in this package's production +// sources, methods included. +func packageFuncs(t *testing.T) []packageFunc { + t.Helper() + var out []packageFunc + for _, path := range goSourceFiles(t, packageDir(t, ".")) { + f, err := parser.ParseFile(token.NewFileSet(), path, nil, parser.SkipObjectResolution) + require.NoError(t, err, "parsing %s", path) + for _, decl := range f.Decls { + fn, isFunc := decl.(*ast.FuncDecl) + if !isFunc { + continue + } + out = append(out, packageFunc{ + name: fn.Name.Name, + results: resultTypes(fn), + callsWalkValues: callsWalkValues(fn), + }) + } + } + return out +} + +// resultTypes renders fn's result types, one entry per result, so a named and an +// unnamed result list read the same. +func resultTypes(fn *ast.FuncDecl) []string { + if fn.Type.Results == nil { + return nil + } + var out []string + for _, field := range fn.Type.Results.List { + out = append(out, slices.Repeat([]string{types.ExprString(field.Type)}, max(1, len(field.Names)))...) + } + return out +} + +// callsWalkValues reports whether fn's body calls ir.WalkValues. +func callsWalkValues(fn *ast.FuncDecl) bool { + var found bool + ast.Inspect(fn, func(node ast.Node) bool { + sel, isSelector := node.(*ast.SelectorExpr) + if !isSelector || sel.Sel.Name != "WalkValues" { + return true + } + pkg, isIdent := sel.X.(*ast.Ident) + found = found || (isIdent && pkg.Name == "ir") + return true + }) + return found +} diff --git a/ir/registries.go b/ir/registries.go new file mode 100644 index 0000000..471042d --- /dev/null +++ b/ir/registries.go @@ -0,0 +1,84 @@ +package ir + +import ( + "reflect" + "strings" +) + +// Registry is one flat, ID-keyed registry a [Document] declares: the entries +// themselves, plus the name a report about them is spelled with. +// +// The zero value declares nothing, which is what a lookup for an ID class the +// document registers no map for yields. [Registry.Has] reports false for it +// rather than indexing an invalid value, so a checker holding a site built +// against another document reports it as unresolved instead of crashing. +type Registry struct { + // Label names the registry as the document spells it — "types", "channels". + Label string + + entries reflect.Value +} + +// Has reports whether the registry declares id. +func (r Registry) Has(id string) bool { + if !r.entries.IsValid() { + return false + } + return r.entries.MapIndex(reflect.ValueOf(id).Convert(r.entries.Type().Key())).IsValid() +} + +// Registries maps each ID type to the [Registry] that declares those IDs. +// +// A value's Go type is what makes it a reference: a [ChannelID]-typed field is a +// reference into Document.Channels wherever it sits — a node's own ID included, +// which resolves against its own entry — so no field has to be listed here and +// none can be forgotten. +// +// Type-driven coverage is not total, and what it misses is a category rather +// than a stray field. A reference carried as an integer index into a slice is an +// int like any other, and reflection has nothing to key on; [PropID] names a +// position inside a model rather than an entry in a document-level map. Both +// classes are resolved by hand where they are checked. +type Registries map[reflect.Type]Registry + +// DocumentRegistries derives doc's registries from Document's own shape: a field +// that is a map keyed by a named string type is an ID-keyed registry, and its key +// type names the reference class it resolves. Deriving them covers a registry +// added to Document the moment it exists, where a hand-written list would drift. +// Document.Unmodeled is the counterexample: keyed by plain string, it keys on a +// source construct's name rather than an identity, and is no registry. +func DocumentRegistries(doc *Document) Registries { + out := Registries{} + if doc == nil { + return out + } + for shape, f := range reflect.ValueOf(doc).Elem().Fields() { + key, isRegistry := registryKeyType(f) + if !isRegistry { + continue + } + out[key] = Registry{Label: strings.ToLower(shape.Name), entries: f} + } + return out +} + +// registryKeyType returns the named string type f is keyed by, and whether f is +// an ID-keyed registry at all. +func registryKeyType(f reflect.Value) (reflect.Type, bool) { + if f.Kind() != reflect.Map { + return nil, false + } + key := f.Type().Key() + if key.Kind() != reflect.String || key.PkgPath() == "" { + return nil, false + } + return key, true +} + +// RefNoun names the reference class an ID type identifies: its type name minus +// the ID suffix, lowercased ("ChannelID" → "channel"). Both of Morphic's +// checkers spell a dangling-reference code with it, so one defect reads under +// one code whichever of them reports it. +func RefNoun(idType reflect.Type) string { + return strings.ToLower(strings.TrimSuffix(idType.Name(), "ID")) +} diff --git a/ir/registries_test.go b/ir/registries_test.go new file mode 100644 index 0000000..18121e4 --- /dev/null +++ b/ir/registries_test.go @@ -0,0 +1,79 @@ +package ir_test + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" +) + +// TestDocumentRegistries_DerivedFromDocumentShape pins the derivation rule that +// replaces a hand-written registry table in each checker: every ID-keyed map on +// Document is a registry, labelled by the field that declares it, and a map keyed +// by plain string — Unmodeled keys on a source construct's name, not an identity +// — is not. +func TestDocumentRegistries_DerivedFromDocumentShape(t *testing.T) { + t.Parallel() + regs := ir.DocumentRegistries(&ir.Document{}) + + want := map[reflect.Type]string{ + reflect.TypeFor[ir.TypeID](): "types", + reflect.TypeFor[ir.ChannelID](): "channels", + reflect.TypeFor[ir.MessageID](): "messages", + reflect.TypeFor[ir.AuthID](): "auth", + } + for idType, label := range want { + reg, isRegistry := regs[idType] + require.True(t, isRegistry, "%s names a Document registry", idType.Name()) + assert.Equal(t, label, reg.Label) + } + _, isRegistry := regs[reflect.TypeFor[string]()] + assert.False(t, isRegistry, "a plain string key is a name, not an identity") + assert.Len(t, regs, len(want), "Document declares exactly these ID-keyed registries") +} + +// TestDocumentRegistries_CoverAWholeRegistry drives resolution both ways: an ID +// the registry declares resolves, one it does not declare dangles. +func TestDocumentRegistries_CoverAWholeRegistry(t *testing.T) { + t.Parallel() + doc := &ir.Document{Types: ir.TypeRegistry{ + "t/x/M": &ir.Any{TypeCommon: ir.TypeCommon{ID: "t/x/M"}}, + }} + + types := ir.DocumentRegistries(doc)[reflect.TypeFor[ir.TypeID]()] + assert.True(t, types.Has("t/x/M")) + assert.False(t, types.Has("t/x/Ghost")) +} + +// TestRegistry_ZeroValueDeclaresNothing holds the report-only guard: a lookup for +// an ID class the document registers no map for yields the zero Registry, and +// asking it beats indexing an invalid reflect.Value and panicking. No site a +// checker collects reaches this, since the classes it collects come from the same +// derivation — which is exactly why it is asserted here rather than left to +// chance. +func TestRegistry_ZeroValueDeclaresNothing(t *testing.T) { + t.Parallel() + assert.False(t, ir.Registry{}.Has("op/x")) + assert.Empty(t, ir.DocumentRegistries(nil), "a nil document declares no registry at all") +} + +// TestRefNoun_NamesTheReferenceClass pins the singular both checkers spell a +// dangling-reference code with, so one defect reads under one code whichever of +// them reports it. PropID is included though it addresses no registry: the noun +// is derived from the type, and pass.Validate reports ir/dangling-prop-ref with +// it. +func TestRefNoun_NamesTheReferenceClass(t *testing.T) { + t.Parallel() + for want, idType := range map[string]reflect.Type{ + "type": reflect.TypeFor[ir.TypeID](), + "channel": reflect.TypeFor[ir.ChannelID](), + "message": reflect.TypeFor[ir.MessageID](), + "auth": reflect.TypeFor[ir.AuthID](), + "prop": reflect.TypeFor[ir.PropID](), + } { + assert.Equal(t, want, ir.RefNoun(idType)) + } +} diff --git a/ir/typedef.go b/ir/typedef.go index 618ee69..99df448 100644 --- a/ir/typedef.go +++ b/ir/typedef.go @@ -1,5 +1,7 @@ package ir +import "reflect" + // TypeKind names one variant of the sealed TypeDef sum (ir-design §4). type TypeKind string @@ -38,6 +40,21 @@ type TypeDef interface { Common() *TypeCommon } +// 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 and a comma-ok assertion alike, so +// matching a kind is no evidence the value is safe to dereference; +// [TypeDef.Kind] and [TypeDef.Common] both panic on one. Every walk over a type +// registry screens entries through this before reading them. +func IsNilTypeDef(td TypeDef) bool { + if td == nil { + return true + } + rv := reflect.ValueOf(td) + return rv.Kind() == reflect.Pointer && rv.IsNil() +} + // UsageFlags is a bitset recording how a type is used across the API surface. It // is computed by a pass and JSON-encoded as a number. type UsageFlags uint32 diff --git a/ir/typedef_test.go b/ir/typedef_test.go index 76ec8a8..6458026 100644 --- a/ir/typedef_test.go +++ b/ir/typedef_test.go @@ -51,3 +51,16 @@ func TestTypeDef_ConcreteTypesImplementInterface(t *testing.T) { assert.Contains(t, allKinds, td.Kind()) } } + +// TestIsNilTypeDef_ScreensBothSpellingsOfNil holds the screen every walk over a +// type registry runs entries through. The typed nil is the one that matters: it +// satisfies a type switch case and a comma-ok assertion alike, so a check that +// matched a kind and read the value would panic on it rather than report it. +func TestIsNilTypeDef_ScreensBothSpellingsOfNil(t *testing.T) { + t.Parallel() + assert.True(t, ir.IsNilTypeDef(nil), "an untyped nil interface holds no definition") + assert.True(t, ir.IsNilTypeDef((*ir.Model)(nil)), "a typed nil pointer holds none either") + for _, td := range allConcreteTypeDefs { + assert.False(t, ir.IsNilTypeDef(td), "%T is a live definition", td) + } +} diff --git a/ir/walk.go b/ir/walk.go new file mode 100644 index 0000000..61413fb --- /dev/null +++ b/ir/walk.go @@ -0,0 +1,189 @@ +package ir + +import ( + "fmt" + "reflect" + "slices" + "strings" +) + +// MaxWalkDepth bounds a [WalkValues] traversal (the bounded-recursion rule). +// Value trees — defaults, examples — nest deepest, and compilers cap their +// nesting far below this (the OpenAPI compiler at 128), so reaching the cap +// signals a pathological document rather than legitimate nesting. The walk +// reports truncation so a caller can say so instead of under-checking in +// silence. +const MaxWalkDepth = 4096 + +// MapKeySuffix ends the path of a value reached as a map key rather than as a +// field or an element, so a caller that treats the two differently can tell +// them apart. +const MapKeySuffix = ".key" + +// WalkValues performs a bounded, cycle-guarded reflection traversal of root, +// calling visit on every value it reaches together with the path it was reached +// by; returning false from visit skips that value's children. It reports +// whether the depth cap cut the walk short. +// +// Deriving what a document holds from the value graph instead of naming fields +// is what makes a check built on it complete: a field added to the IR is covered +// the moment it exists, which a hand-written enumeration cannot promise. Map +// keys are reached as well as values, because some are references in their own +// right — Service.Renames is map[TypeID]Naming, where the key is the reference +// and the value is not. +// +// Paths spell fields joined by ".", slice indices and map keys in brackets, and +// an embedded field contributes no segment of its own: JSON inlines it and Go +// promotes its fields, so "….TypeCommon.Examples[0]" names a step neither +// encoding has — the example is reached as "….Examples[0]" in both. Both +// checkers report a dangling reference under one code, and a caller running both +// can only dedupe them if one defect reads as one location. +// +// A value reached through an unexported field is read-only, and Interface() +// panics on one where FieldByName does not, so a visitor reads the fields it +// needs rather than converting the value back to its Go type. That is what lets +// a caller be an oracle that never crashes on a malformed document. +// +// Map entries are visited in rendered-key order rather than Go's randomized map +// order. A pointer reachable from two entries is descended into at whichever the +// walk reaches first, so a random order yields a different path for it — and so +// a different result set, not merely a different order — on each run, which no +// later sort can repair (invariant 7). +// +// Byte sequences are skipped. Unmodeled and RawConfig payloads are +// json.RawMessage and are the largest values a document holds, while a uint8 +// element is none of the things a visitor looks for — no typed ID, no Unmodeled +// map, no Provenance, no index carrier. Descending one costs a reflect.Value and +// a formatted path per byte for nothing: verifying a document holding one 256 KB +// payload measured 88ms without this skip against 22µs with it, for the same +// result. Since the result is the same either way, a test asserting the result +// cannot notice the skip going missing — one that counts what the walk reaches +// is what holds it. +func WalkValues(root any, path string, visit func(v reflect.Value, path string) bool) bool { + w := valueWalk{seen: map[uintptr]bool{}, visit: visit} + w.walk(reflect.ValueOf(root), path, 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. The invalid zero Value — a nil root, a nil +// interface's dynamic value — is not visited, so no visitor has to guard against +// calling Type() on one. +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. Numbers, bools, strings, +// funcs and channels have none. +func (w *valueWalk) children(v reflect.Value, path string, depth int) { + switch v.Kind() { + case reflect.Pointer: + w.pointer(v, path, depth) + case reflect.Interface: + if !v.IsNil() { + w.descend(v.Elem(), path, depth+1) + } + case reflect.Struct: + t := v.Type() + for i := range v.NumField() { + w.descend(v.Field(i), fieldPath(path, t.Field(i)), depth+1) + } + case reflect.Slice, reflect.Array: + w.sequence(v, path, depth) + case reflect.Map: + for _, e := range orderedEntries(v) { + w.descend(e.key, fmt.Sprintf("%s[%s]%s", path, e.label, MapKeySuffix), depth+1) + w.descend(e.value, fmt.Sprintf("%s[%s]", path, e.label), depth+1) + } + } +} + +// pointer descends through a non-nil pointer once. The seen set terminates +// cyclic graphs — normal input for schema languages, not an edge case — and +// means a value reached through a shared pointer is visited once rather than +// once per reference to it. +// +// Which of an aliased pointer's referrers wins that single visit is decided by +// traversal order, so the walk's order has to be fixed rather than incidental; +// orderedEntries is where that is arranged. +func (w *valueWalk) pointer(v reflect.Value, path string, depth int) { + if v.IsNil() { + return + } + p := v.Pointer() + if w.seen[p] { + return + } + w.seen[p] = true + w.descend(v.Elem(), path, depth+1) +} + +// sequence descends into slice and array elements, skipping byte sequences (see +// [WalkValues] for why they are skipped and how the skip is held). +func (w *valueWalk) sequence(v reflect.Value, path string, depth int) { + if v.Type().Elem().Kind() == reflect.Uint8 { + return + } + for i := range v.Len() { + w.descend(v.Index(i), fmt.Sprintf("%s[%d]", path, i), depth+1) + } +} + +// fieldPath extends path with f's name, except for an embedded field, which +// contributes no segment (see [WalkValues]). +func fieldPath(path string, f reflect.StructField) string { + if f.Anonymous { + return path + } + return path + "." + f.Name +} + +// mapEntry is one map entry paired with its rendered key, which both spells the +// entry's path and orders the walk. +type mapEntry struct { + label string + key reflect.Value + value reflect.Value +} + +// orderedEntries returns v's entries ordered by rendered key. Ordering by the +// same rendering the path uses keeps the two in step, and it is a total order +// for every key type the IR declares: named string types, plain strings and ints +// all render distinct keys distinctly. +func orderedEntries(v reflect.Value) []mapEntry { + entries := make([]mapEntry, 0, v.Len()) + for iter := v.MapRange(); iter.Next(); { + k := iter.Key() + entries = append(entries, mapEntry{label: fmt.Sprintf("%v", k), key: k, value: iter.Value()}) + } + slices.SortFunc(entries, func(a, b mapEntry) int { return strings.Compare(a.label, b.label) }) + return entries +} diff --git a/ir/walk_test.go b/ir/walk_test.go new file mode 100644 index 0000000..2550605 --- /dev/null +++ b/ir/walk_test.go @@ -0,0 +1,226 @@ +package ir_test + +import ( + "reflect" + "slices" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" +) + +// walkPaths returns every path ir.WalkValues reaches from root, in walk order, +// plus whether the depth cap cut the walk short. +func walkPaths(root any) ([]string, bool) { + var paths []string + truncated := ir.WalkValues(root, "doc", func(_ reflect.Value, path string) bool { + paths = append(paths, path) + return true + }) + return paths, truncated +} + +// nestedListValue returns a Value that is depth levels of single-element lists +// wrapping a number — the shape a deeply-nested array default lowers to. +func nestedListValue(depth int) ir.Value { + v := ir.Value{Kind: ir.ValueNumber, Num: ir.BigVal("0")} + for range depth { + v = ir.Value{Kind: ir.ValueList, List: []ir.Value{v}} + } + return v +} + +// TestWalkValues_PathsSpellFieldsIndicesAndKeys pins the three path rules two +// checkers report against: a field is joined by ".", a slice index and a map key +// sit in brackets (a key carrying ir.MapKeySuffix so a caller can tell it from a +// value), and an embedded field contributes no segment of its own — Examples is +// declared on TypeCommon, and is reached below the model rather than below a +// ".TypeCommon" step that neither JSON nor Go has. +func TestWalkValues_PathsSpellFieldsIndicesAndKeys(t *testing.T) { + t.Parallel() + value := ir.Value{Kind: ir.ValueNull} + doc := &ir.Document{ + Name: "api", + Types: ir.TypeRegistry{"t/x/M": &ir.Model{TypeCommon: ir.TypeCommon{ + ID: "t/x/M", + Examples: []ir.Example{{Value: &value}}, + }}}, + } + + paths, truncated := walkPaths(doc) + require.False(t, truncated) + assert.Contains(t, paths, "doc.Name") + assert.Contains(t, paths, "doc.Types[t/x/M]"+ir.MapKeySuffix) + assert.Contains(t, paths, "doc.Types[t/x/M].Examples[0].Value.Kind") + assert.NotContains(t, paths, "doc.Types[t/x/M].TypeCommon.ID", + "an embedded field contributes no path segment") +} + +// TestWalkValues_MapEntriesAreVisitedInRenderedKeyOrder holds the ordering rule +// invariant 7 rests on. Go randomizes map iteration, and a pointer reachable +// from two entries is descended into at whichever entry the walk reaches first — +// so an unordered walk records a different path for it run to run, which is a +// different result set rather than a different order, and no later sort repairs +// that. +func TestWalkValues_MapEntriesAreVisitedInRenderedKeyOrder(t *testing.T) { + t.Parallel() + doc := &ir.Document{Types: ir.TypeRegistry{}} + for _, id := range []ir.TypeID{"t/c", "t/a", "t/d", "t/b"} { + doc.Types[id] = &ir.Any{TypeCommon: ir.TypeCommon{ID: id}} + } + + paths, _ := walkPaths(doc) + var entries []string + for _, p := range paths { + if strings.HasPrefix(p, "doc.Types[") && strings.HasSuffix(p, ir.MapKeySuffix) { + entries = append(entries, p) + } + } + require.Len(t, entries, 4) + assert.True(t, slices.IsSorted(entries), "entries are walked in rendered-key order, got %v", entries) +} + +// TestWalkValues_DeepValueTreeIsTruncated drives the depth cap from both sides: +// a document nested past it must report truncation rather than be under-checked +// in silence, and one nested deep but within the bound must not. +func TestWalkValues_DeepValueTreeIsTruncated(t *testing.T) { + t.Parallel() + docWith := func(v ir.Value) *ir.Document { + return &ir.Document{Types: ir.TypeRegistry{"t/m": &ir.Model{TypeCommon: ir.TypeCommon{ + ID: "t/m", + Examples: []ir.Example{{Value: &v}}, + }}}} + } + + _, truncated := walkPaths(docWith(nestedListValue(ir.MaxWalkDepth))) + assert.True(t, truncated, "a tree nested past the cap must report truncation") + + _, truncated = walkPaths(docWith(nestedListValue(200))) + assert.False(t, truncated, "a tree a compiler can produce must be walked whole") +} + +// TestWalkValues_SharedPointerIsDescendedIntoOnce holds the cycle guard. Nothing +// in a compiled document reaches it — a Document is a flat registry of values +// referenced by ID, so no two fields point at one struct — which is exactly why +// it needs planting: the guard is what keeps the walk terminating on a document +// that does share a pointer, and without one, removing it changes nothing. +func TestWalkValues_SharedPointerIsDescendedIntoOnce(t *testing.T) { + t.Parallel() + shared := &ir.TypeRef{Target: "t/x/Shared"} + doc := &ir.Document{Types: ir.TypeRegistry{"t/m": &ir.Model{ + TypeCommon: ir.TypeCommon{ID: "t/m", Instantiation: &ir.TemplateInstantiation{ + Args: []ir.TemplateArg{{Type: shared}, {Type: shared}}, + }}, + }}} + + paths, truncated := walkPaths(doc) + require.False(t, truncated, "two template arguments are not deep") + var reached int + for _, p := range paths { + if strings.HasSuffix(p, ".Target") { + reached++ + } + } + assert.Equal(t, 1, reached, "the second argument finds the pointer already seen and stops there") +} + +// TestWalkValues_CyclicPointerGraphTerminates states the same guard as a +// liveness claim: a value graph that points back at itself is normal input for a +// schema language, and the walk has to end on one rather than spin until the +// depth cap. +func TestWalkValues_CyclicPointerGraphTerminates(t *testing.T) { + t.Parallel() + type node struct{ Next *node } + loop := &node{} + loop.Next = loop + + paths, truncated := walkPaths(loop) + assert.False(t, truncated, "the cycle guard stops the walk well before the depth cap") + assert.Equal(t, []string{"doc", "doc", "doc.Next"}, paths, + "the pointer, the struct behind it, then the field that leads back to it") +} + +// payloadBytes sizes the preserved payload the byte-skip test walks. It is large +// enough that descending it would dominate any plausible visit count for a +// two-node document, and small enough to build inline. +const payloadBytes = 4096 + +// TestWalkValues_ByteSequencesAreNotDescendedInto drives the byte-sequence skip, +// which exists for cost rather than for correctness: a uint8 element is none of +// the things a visitor looks for, so collecting nothing from it is the same +// result either way and only the price differs. +// +// The visit count is what makes that assertable: since the result is the same +// with or without the skip, a test reading the result cannot notice the skip +// going missing. Without it the count grows past the payload's own length. +func TestWalkValues_ByteSequencesAreNotDescendedInto(t *testing.T) { + t.Parallel() + doc := &ir.Document{Unmodeled: ir.Unmodeled{"openapi:x-thing": { + Reason: ir.ReasonVendorExtension, + Value: ir.RawValue(`"` + strings.Repeat("t", payloadBytes) + `"`), + }}} + + paths, truncated := walkPaths(doc) + require.False(t, truncated) + assert.Less(t, len(paths), payloadBytes, + "walking %d bytes of payload one value at a time is what the skip exists to avoid", payloadBytes) +} + +// TestWalkValues_VisitorPrunesChildren holds the one lever a visitor has over +// the walk: returning false stops the descent at that value, which is how a +// check that has learned all it needs from a node avoids paying for the subtree +// below it. +func TestWalkValues_VisitorPrunesChildren(t *testing.T) { + t.Parallel() + doc := &ir.Document{Name: "api", Types: ir.TypeRegistry{ + "t/m": &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/m"}}, + }} + + var pruned []string + ir.WalkValues(doc, "doc", func(v reflect.Value, path string) bool { + pruned = append(pruned, path) + return v.Type() != reflect.TypeFor[ir.TypeRegistry]() + }) + + assert.Contains(t, pruned, "doc.Types") + for _, p := range pruned { + assert.False(t, strings.HasPrefix(p, "doc.Types["), "%s sits below the pruned registry", p) + } +} + +// TestWalkValues_NothingUnreachableIsVisited covers the three ways the walk +// reaches nothing: an invalid root, a nil pointer, and a nil interface. None may +// be handed to a visitor, which would then have to guard every Type() call +// against the zero reflect.Value. +func TestWalkValues_NothingUnreachableIsVisited(t *testing.T) { + t.Parallel() + paths, truncated := walkPaths(nil) + assert.Empty(t, paths, "an invalid root is not visited") + assert.False(t, truncated) + + // Contact is a nil *Contact and the registry entry is a nil TypeDef, so the + // walk reaches the pointer and the interface but nothing behind either. + doc := &ir.Document{Contact: nil, Types: ir.TypeRegistry{"t/nil": nil}} + paths, _ = walkPaths(doc) + assert.Contains(t, paths, "doc.Contact", "the nil pointer itself is visited") + assert.Contains(t, paths, "doc.Types[t/nil]", "the nil interface itself is visited") + for _, p := range paths { + assert.NotEqual(t, "doc.Contact.Name", p, "nothing behind a nil pointer is reachable") + } +} + +// TestWalkValues_ArrayElementsAreReached pins that a fixed-size array is walked +// like a slice. The IR declares none today, so this states the rule rather than +// guarding a live shape: a kind the walk silently skipped would take references +// with it. +func TestWalkValues_ArrayElementsAreReached(t *testing.T) { + t.Parallel() + holder := &struct{ IDs [2]ir.TypeID }{IDs: [2]ir.TypeID{"t/a", "t/b"}} + + paths, _ := walkPaths(holder) + assert.Contains(t, paths, "doc.IDs[0]") + assert.Contains(t, paths, "doc.IDs[1]") +} diff --git a/pass/refs.go b/pass/refs.go index 7b5f5ca..b1fa35c 100644 --- a/pass/refs.go +++ b/pass/refs.go @@ -1,7 +1,6 @@ package pass import ( - "fmt" "reflect" "slices" "strings" @@ -9,13 +8,6 @@ import ( "github.com/dexpace/morphic/ir" ) -// maxRefWalkDepth bounds the reflection traversal of a document (the -// bounded-recursion rule). Value trees — defaults, examples — nest deepest, and -// compilers cap their nesting far below this (the OpenAPI compiler at 128), so -// reaching the cap signals a pathological document rather than legitimate -// nesting. The walk reports truncation instead of under-checking in silence. -const maxRefWalkDepth = 4096 - // typeIDType is the reflect.Type of ir.TypeID, the one reference class the // reachability analysis in validate.go needs without a document to resolve // against. @@ -31,86 +23,6 @@ var ( propertyType = reflect.TypeFor[ir.Property]() ) -// mapKeySuffix marks a site reached as a map key rather than as a field or an -// element. walkMap spells it and collectPropIDs reads it, so the two cannot -// disagree about which sites are keys. -const mapKeySuffix = ".key" - -// registries maps each ID type to the Document registry that declares those IDs. -// The walk recognizes a reference by its declared Go type, never by field name: -// an ir.ChannelID-typed field is a reference into Document.Channels wherever it -// sits — a node's own ID included, which resolves against its own registry entry -// — so no field has to be listed here and none can be forgotten. -// -// Type-driven coverage is not total, and what it misses is a category rather -// than a stray field: a reference carried as an integer index into a slice is an -// int like any other, and reflection has nothing to key on. The IR has three — -// Service.Servers and Channel.Servers into Document.Servers, and -// HTTPBinding.SuccessStatus's keys into Operation.Responses, both enumerated by -// hand in checkServerIndices and checkResponseIndices; and Provenance.Source into -// Document.Sources, which irverify checks instead. Provenance is left to irverify -// because a stale source index is a compiler bug rather than a spec problem, and -// because this pass stamps ir.NoSource on its own diagnostics, which the engine -// folds into Document.Diagnostics — a document-wide check here would report its -// own previous output. A new integer-index reference has to be added to those -// checks by hand too, because nothing here will find it; irverify's -// integerFields guard is what stops one being added unnoticed. -// -// A second category is visible to the walk but resolvable by no registry: -// ir.PropID names a position inside a model rather than an entry in a -// document-level map, so it is collected by collectPropIDs and resolved against -// the ir.Property values the same traversal saw. -type registries map[reflect.Type]reflect.Value - -// documentRegistries derives doc's registries from Document's own shape: a field -// that is a map keyed by a named string type is an ID-keyed registry, and its key -// type names the reference class it resolves. Deriving them covers a registry -// added to Document the moment it exists, where a hand-written list would drift. -// Document.Unmodeled is the counterexample: keyed by plain string, it keys on a -// source construct's name rather than an identity, and is no registry. -func documentRegistries(doc *ir.Document) registries { - out := registries{} - fields := reflect.ValueOf(doc).Elem() - for _, f := range fields.Fields() { - if f.Kind() != reflect.Map { - continue - } - key := f.Type().Key() - if key.Kind() == reflect.String && key.PkgPath() != "" { - out[key] = f - } - } - return out -} - -// isRef reports whether values of Go type t are references this document can -// resolve. -func (r registries) isRef(t reflect.Type) bool { - _, ok := r[t] - return ok -} - -// resolves reports whether the registry for s.idType declares s.id. The -// unknown-class guard — rather than indexing r directly — keeps the pass -// report-only: collectRefs only builds sites for classes isRef accepted, so an -// unresolvable class means the caller mixed sites from another document, and -// reporting that as dangling beats panicking on a zero reflect.Value. -func (r registries) resolves(s refSite) bool { - entries, ok := r[s.idType] - if !ok { - return false - } - return entries.MapIndex(reflect.ValueOf(s.id).Convert(s.idType)).IsValid() -} - -// refNoun names the reference class an ID type identifies: its type name minus -// the ID suffix, lowercased ("ChannelID" → "channel"). The same noun spells -// irverify's code for the identical defect, so both checkers report a dangling -// reference under one code rather than two. -func refNoun(idType reflect.Type) string { - return strings.ToLower(strings.TrimSuffix(idType.Name(), "ID")) -} - // refSite is one discovered ID reference: the class that made it a reference, // its value, and a human-readable location used for diagnostic provenance. type refSite struct { @@ -119,53 +31,47 @@ type refSite struct { where string } -// refWalk carries the mutable state of one bounded, cycle-guarded traversal. -type refWalk struct { - isRef func(reflect.Type) bool - // onStruct, when set, is called for every struct value the walk reaches, so a - // caller can collect declarations alongside references in the same traversal - // rather than standing up a second bounded walker beside this one. - onStruct func(reflect.Value) - sites []refSite - seen map[uintptr]bool - truncated bool -} - // collectRefs returns every reference reachable from root whose Go type isRef // accepts, sorted by location, and reports whether the depth cap truncated the // walk. // -// Deriving the sites from the value graph instead of naming fields is what makes -// the walk complete: a ref-bearing field added to the IR is covered the moment it -// exists, which a hand-written enumeration cannot promise. -// -// Locations are spelled as ir/irverify's walk spells them — fields joined by ".", -// slice indices and map keys in brackets — because the two checkers report a -// dangling reference under one code (see the package doc) and a caller running -// both can only dedupe them if one defect reads as one location. The integer-index -// checks in validate.go stay rooted at a stable ID rather than at doc: those -// diagnostics are read by a spec author, for whom an ID outlives a position. +// The traversal is ir.WalkValues, which this pass and ir/irverify share so that +// one document has one walk: the same bound, cycle guard, order and path +// spelling, and a reference-bearing field added to the IR covered by both the +// moment it exists. The integer-index checks in validate.go stay rooted at a +// stable ID rather than at doc — those diagnostics are read by a spec author, +// for whom an ID outlives a position. // // The sort alone does not make the result deterministic (invariant 7). Sorting // reorders a site set; it cannot repair one whose *membership* varies, which is // what a randomized traversal of an aliased value graph produces — hence the -// ordered map walk in walkMap. +// ordered map walk ir.WalkValues performs. func collectRefs(root any, path string, isRef func(reflect.Type) bool) ([]refSite, bool) { return collectWalk(root, path, isRef, nil) } -// collectWalk is collectRefs with an optional per-struct hook; see refWalk.onStruct. +// collectWalk is collectRefs with an optional per-struct hook, so a caller can +// collect declarations alongside references in one traversal rather than +// standing up a second bounded walk beside this one. func collectWalk(root any, path string, isRef func(reflect.Type) bool, onStruct func(reflect.Value)) ([]refSite, bool) { - w := refWalk{isRef: isRef, onStruct: onStruct, seen: map[uintptr]bool{}} - w.walk(reflect.ValueOf(root), path, 0) + var sites []refSite + truncated := ir.WalkValues(root, path, func(v reflect.Value, at string) bool { + if v.Kind() == reflect.String && v.String() != "" && isRef(v.Type()) { + sites = append(sites, refSite{idType: v.Type(), id: v.String(), where: at}) + } + if onStruct != nil && v.Kind() == reflect.Struct { + onStruct(v) + } + return true + }) // Distinct sites have distinct paths in every shape the IR declares, but two // embedded structs contributing a same-named promoted field would collide; // a stable sort keeps the deterministic walk order in that case rather than // leaving the pair to sort.Interface's unspecified swap order. - slices.SortStableFunc(w.sites, func(a, b refSite) int { + slices.SortStableFunc(sites, func(a, b refSite) int { return strings.Compare(a.where, b.where) }) - return w.sites, w.truncated + return sites, truncated } // collectTypeIDs returns every ir.TypeID reachable from root. Type reachability @@ -201,133 +107,6 @@ func collectPropIDs(root any, path string) ([]refSite, map[ir.PropID]bool) { // to the same cap as checkDanglingRefs, which reports ir/walk-truncated for // both. return slices.DeleteFunc(sites, func(s refSite) bool { - return strings.HasSuffix(s.where, mapKeySuffix) + return strings.HasSuffix(s.where, ir.MapKeySuffix) }), declared } - -// descend walks child one level deeper, marking the walk truncated at the cap. -func (w *refWalk) descend(child reflect.Value, path string, depth int) { - if depth > maxRefWalkDepth { - w.truncated = true - return - } - w.walk(child, path, depth) -} - -// walk dispatches on v's kind. The invalid zero Value — a nil root, a nil -// interface's dynamic value — falls to the default arm, so nothing here calls -// Type() on it. -func (w *refWalk) walk(v reflect.Value, path string, depth int) { - switch v.Kind() { - case reflect.String: - if v.String() != "" && w.isRef(v.Type()) { - w.sites = append(w.sites, refSite{idType: v.Type(), id: v.String(), where: path}) - } - case reflect.Pointer: - w.walkPointer(v, path, depth) - case reflect.Interface: - if !v.IsNil() { - w.descend(v.Elem(), path, depth+1) - } - case reflect.Struct: - w.walkStruct(v, path, depth) - case reflect.Slice, reflect.Array: - w.walkSequence(v, path, depth) - case reflect.Map: - w.walkMap(v, path, depth) - default: - // Numbers, bools, funcs, channels and the invalid Value hold no typed ID. - } -} - -// walkStruct descends into a struct's fields. An embedded field contributes no -// path segment of its own: JSON inlines it and Go promotes its fields, so -// "….TypeCommon.Examples[0]" names a step that neither encoding has — the -// example is reached as "….Examples[0]" in both. -func (w *refWalk) walkStruct(v reflect.Value, path string, depth int) { - if w.onStruct != nil { - w.onStruct(v) - } - t := v.Type() - for i := range v.NumField() { - f := t.Field(i) - child := path - if !f.Anonymous { - child = path + "." + f.Name - } - w.descend(v.Field(i), child, depth+1) - } -} - -// walkPointer descends through a non-nil pointer once. The seen set terminates -// cyclic type graphs — normal input for schema languages, not an edge case — and -// means a target reached through a shared pointer is reported once rather than -// once per reference to it. -// -// Which of an aliased pointer's referrers wins that single visit is decided by -// traversal order, so the walk's order has to be fixed rather than incidental; -// walkMap is where that is arranged. -func (w *refWalk) walkPointer(v reflect.Value, path string, depth int) { - if v.IsNil() { - return - } - p := v.Pointer() - if w.seen[p] { - return - } - w.seen[p] = true - w.descend(v.Elem(), path, depth+1) -} - -// walkSequence descends into slice and array elements, skipping byte sequences: -// Unmodeled and RawConfig payloads are json.RawMessage and are the largest thing -// in a document, while a byte element can hold no typed ID. The result is the -// same without the skip, so it is guarded by a test that observes the descent -// itself (TestWalkSequence_ByteSequenceIsNotDescendedInto) rather than the sites. -func (w *refWalk) walkSequence(v reflect.Value, path string, depth int) { - if v.Type().Elem().Kind() == reflect.Uint8 { - return - } - for i := range v.Len() { - w.descend(v.Index(i), fmt.Sprintf("%s[%d]", path, i), depth+1) - } -} - -// mapEntry is one map entry paired with its rendered key, which both spells the -// entry's path and orders the walk. -type mapEntry struct { - label string - key reflect.Value - value reflect.Value -} - -// orderedEntries returns v's entries ordered by rendered key. Ordering by the -// same rendering the path uses keeps the two in step, and it is a total order for -// every key type the IR declares: named string types, plain strings and ints all -// render distinct keys distinctly. -func orderedEntries(v reflect.Value) []mapEntry { - entries := make([]mapEntry, 0, v.Len()) - for iter := v.MapRange(); iter.Next(); { - k := iter.Key() - entries = append(entries, mapEntry{label: fmt.Sprintf("%v", k), key: k, value: iter.Value()}) - } - slices.SortFunc(entries, func(a, b mapEntry) int { return strings.Compare(a.label, b.label) }) - return entries -} - -// walkMap descends into keys as well as values: most keys are an entry's own ID, -// but Service.Renames is map[TypeID]Naming, where the key is the reference and -// the value is not. -// -// Entries are visited in rendered-key order, not Go's randomized map order. A -// pointer reachable from two entries is descended into at whichever the walk -// reaches first (walkPointer), so under a random order the two runs record two -// different paths for it — a different site set, not a different order, which no -// later sort can repair. Ordering the walk itself is what keeps map iteration out -// of the diagnostics (invariant 7). -func (w *refWalk) walkMap(v reflect.Value, path string, depth int) { - for _, e := range orderedEntries(v) { - w.descend(e.key, fmt.Sprintf("%s[%s]%s", path, e.label, mapKeySuffix), depth+1) - w.descend(e.value, fmt.Sprintf("%s[%s]", path, e.label), depth+1) - } -} diff --git a/pass/refs_internal_test.go b/pass/refs_internal_test.go index 8c8734c..d2c5953 100644 --- a/pass/refs_internal_test.go +++ b/pass/refs_internal_test.go @@ -1,7 +1,6 @@ -package pass // internal test package — exercises the walk's bounds directly +package pass // internal test package — exercises the collectors directly import ( - "reflect" "testing" "github.com/stretchr/testify/assert" @@ -10,14 +9,15 @@ import ( "github.com/dexpace/morphic/ir" ) -// TestCollectTypeIDs_DeepValueTreeIsTruncated drives the depth cap: a value tree -// nested past it must not be silently under-checked, so the walk reports -// truncation and Validate turns that into a diagnostic rather than claiming the -// document is referentially closed. +// TestCollectTypeIDs_DeepValueTreeIsTruncated drives what this pass does with a +// truncated walk: a value tree nested past the shared cap must not be silently +// under-checked, so the flag comes back out of the collector and Validate turns +// it into a diagnostic rather than claiming the document is referentially closed. +// The bound itself is ir.MaxWalkDepth's to hold; this is the reporting half. func TestCollectTypeIDs_DeepValueTreeIsTruncated(t *testing.T) { t.Parallel() v := ir.Value{Kind: ir.ValueNull} - for range maxRefWalkDepth { + for range ir.MaxWalkDepth { v = ir.Value{Kind: ir.ValueList, List: []ir.Value{v}} } m := &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/m", Examples: []ir.Example{{Value: &v}}}} @@ -32,38 +32,9 @@ func TestCollectTypeIDs_DeepValueTreeIsTruncated(t *testing.T) { assert.Equal(t, ir.SeverityError, diags[0].Severity) } -// TestCollectTypeIDs_SharedPointerVisitedOnce drives the cycle guard: the same -// *TypeRef reached through two template arguments is descended into once, so its -// target is collected once rather than per reference to it. -func TestCollectTypeIDs_SharedPointerVisitedOnce(t *testing.T) { - t.Parallel() - shared := &ir.TypeRef{Target: "t/ghost/shared"} - m := &ir.Model{TypeCommon: ir.TypeCommon{ - ID: "t/m", - Instantiation: &ir.TemplateInstantiation{Args: []ir.TemplateArg{ - {Type: shared}, - {Type: shared}, - }}, - }} - doc := &ir.Document{Types: ir.TypeRegistry{m.ID: m}} - - sites, truncated := collectTypeIDs(doc, "doc") - assert.False(t, truncated) - - var n int - for _, s := range sites { - if s.id == "t/ghost/shared" { - n++ - } - } - assert.Equal(t, 1, n, "the shared pointer's target is collected once") -} - -// TestCollectTypeIDs_UnmodeledBytesYieldNoReference states the result half: a -// preserved JSON blob is opaque data, so bytes that happen to spell a type ID -// are not a reference. This holds with or without the byte-sequence skip — a -// uint8 is not a string and could never be collected — which is why the skip -// itself is driven separately below. +// TestCollectTypeIDs_UnmodeledBytesYieldNoReference states the result the byte +// payloads must produce: a preserved JSON blob is opaque data, so bytes that +// happen to spell a type ID are not a reference. func TestCollectTypeIDs_UnmodeledBytesYieldNoReference(t *testing.T) { t.Parallel() doc := &ir.Document{Unmodeled: ir.Unmodeled{"openapi:x-thing": { @@ -75,29 +46,6 @@ func TestCollectTypeIDs_UnmodeledBytesYieldNoReference(t *testing.T) { assert.Empty(t, sites) } -// TestWalkSequence_ByteSequenceIsNotDescendedInto drives the skip itself, which -// exists for cost: Unmodeled and RawConfig payloads are the largest values a -// document holds, and descending one spends a reflect.Value and a formatted path -// per byte to collect nothing. -// -// Descent is observed through the depth budget, the one effect it has that a -// visitor-less walk exposes. Starting at the cap, every element the walk -// descended into would sit one level past it and mark the walk truncated; with -// the skip nothing below the slice is reached, so it does not. -func TestWalkSequence_ByteSequenceIsNotDescendedInto(t *testing.T) { - t.Parallel() - w := refWalk{isRef: func(reflect.Type) bool { return true }, seen: map[uintptr]bool{}} - w.walkSequence(reflect.ValueOf(ir.RawValue(`"t/ghost/in-bytes"`)), "doc", maxRefWalkDepth) - assert.False(t, w.truncated, "no element of a byte sequence may be descended into") - assert.Empty(t, w.sites) - - // The same walk one level shallower, over a sequence that is not bytes, does - // descend — so the assertion above is the skip and not the depth arithmetic. - deep := refWalk{isRef: func(reflect.Type) bool { return true }, seen: map[uintptr]bool{}} - deep.walkSequence(reflect.ValueOf([]ir.TypeID{"t/x"}), "doc", maxRefWalkDepth) - assert.True(t, deep.truncated, "a non-byte element is descended into and hits the cap") -} - // TestCheckDanglingRefs_NilTypeDefIsNotFollowed pins report-only behaviour on // a malformed registry: a nil entry is skipped rather than dereferenced, so the // pass reports what it can instead of panicking. @@ -106,30 +54,3 @@ func TestCheckDanglingRefs_NilTypeDefIsNotFollowed(t *testing.T) { doc := &ir.Document{Types: ir.TypeRegistry{"t/nil": nil}} assert.Empty(t, checkDanglingRefs(doc)) } - -// TestRegistries_ResolvesUnknownClassIsReportOnly drives the unknown-class guard: -// a site whose ID type this document declares no registry for cannot resolve, and -// saying so beats indexing a zero reflect.Value and panicking. Every site -// collectRefs builds does have a registry, so nothing reaches this in practice — -// which is exactly why it is asserted here rather than left to chance. -func TestRegistries_ResolvesUnknownClassIsReportOnly(t *testing.T) { - t.Parallel() - regs := documentRegistries(&ir.Document{}) - site := refSite{idType: reflect.TypeFor[ir.OpID](), id: "op/x", where: "doc"} - assert.False(t, regs.resolves(site), "an ID class with no registry resolves to nothing") -} - -// TestDocumentRegistries_DerivedFromDocumentShape pins the derivation rule that -// replaces a hand-written registry table: every ID-keyed map on Document is a -// registry, and a map keyed by plain string — Unmodeled keys on a source -// construct's name, not an identity — is not. -func TestDocumentRegistries_DerivedFromDocumentShape(t *testing.T) { - t.Parallel() - regs := documentRegistries(&ir.Document{}) - - for _, id := range []any{ir.TypeID(""), ir.ChannelID(""), ir.MessageID(""), ir.AuthID("")} { - assert.True(t, regs.isRef(reflect.TypeOf(id)), "%T names a Document registry", id) - } - assert.False(t, regs.isRef(reflect.TypeFor[string]()), "a plain string key is a name, not an identity") - assert.Len(t, regs, 4, "Document declares exactly the four ID-keyed registries") -} diff --git a/pass/validate.go b/pass/validate.go index b5bfb5e..ab765e0 100644 --- a/pass/validate.go +++ b/pass/validate.go @@ -43,12 +43,29 @@ func Validate(doc *ir.Document) []ir.Diagnostic { // SchemeUse.Scheme into doc.Auth — wherever it sits. // // The sites and the registries both come from reflection over the document's own -// shape (refs.go) rather than a list of field names: referential integrity is the -// guarantee an emitter relies on, and a hand-written enumeration drifts behind -// the IR without anything failing. +// shape (ir.WalkValues, ir.DocumentRegistries) rather than a list of field names: +// referential integrity is the guarantee an emitter relies on, and a hand-written +// enumeration drifts behind the IR without anything failing. +// +// What that leaves out is a category rather than a stray field. A reference +// carried as an integer index is an int like any other: Service.Servers and +// Channel.Servers into Document.Servers, and HTTPBinding.SuccessStatus's keys +// into Operation.Responses, are enumerated by hand below; Provenance.Source into +// Document.Sources is left to irverify, because a stale source index is a +// compiler bug rather than a spec problem, and because this pass stamps +// ir.NoSource on its own diagnostics, which the engine folds into +// Document.Diagnostics — a document-wide check here would report its own previous +// output. A new integer-index reference has to be added to those checks by hand +// too; irverify's integerFields guard is what stops one being added unnoticed. +// The other class the registries cannot resolve is ir.PropID, which names a +// position inside a model: checkPropIDRefs resolves those against the properties +// the same traversal saw. func checkDanglingRefs(doc *ir.Document) []ir.Diagnostic { - regs := documentRegistries(doc) - sites, truncated := collectRefs(doc, "doc", regs.isRef) + regs := ir.DocumentRegistries(doc) + sites, truncated := collectRefs(doc, "doc", func(t reflect.Type) bool { + _, isRegistry := regs[t] + return isRegistry + }) var diags []ir.Diagnostic if truncated { diags = append(diags, diag(ir.SeverityError, "ir/walk-truncated", @@ -56,10 +73,14 @@ func checkDanglingRefs(doc *ir.Document) []ir.Diagnostic { "doc")) } for _, s := range sites { - if regs.resolves(s) { + // A site whose class this document declares no registry for cannot + // resolve; the zero ir.Registry reports so rather than panicking, which + // keeps the pass report-only if a caller ever mixes in sites collected + // against another document. + if regs[s.idType].Has(s.id) { continue } - noun := refNoun(s.idType) + noun := ir.RefNoun(s.idType) diags = append(diags, diag(ir.SeverityError, "ir/dangling-"+noun+"-ref", fmt.Sprintf("%s reference %q at %s resolves to no %s in the registry", noun, s.id, s.where, noun), s.where)) @@ -268,7 +289,7 @@ func exposedProps(doc *ir.Document, root ir.TypeID) map[ir.PropID]bool { } seen[id] = true td := doc.Types[id] - if isNilTypeDef(td) { + if ir.IsNilTypeDef(td) { continue // undeclared, or the typed nil checkNilTypes reports } queue = appendPartSources(queue, props, td) @@ -312,18 +333,6 @@ func appendCompositionParents(dst []ir.TypeID, m *ir.Model) []ir.TypeID { return dst } -// 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 matching a -// kind says nothing about whether the value is safe to dereference; every walk -// over doc.Types screens entries through this first. -func isNilTypeDef(td ir.TypeDef) bool { - if td == nil { - return true - } - rv := reflect.ValueOf(td) - return rv.Kind() == reflect.Pointer && rv.IsNil() -} - // liveTypeIDs returns the registry's type IDs in sorted order, omitting entries // that hold a nil type definition. // @@ -336,7 +345,7 @@ func liveTypeIDs(doc *ir.Document) []ir.TypeID { ids := sortedKeys(doc.Types) live := make([]ir.TypeID, 0, len(ids)) for _, id := range ids { - if !isNilTypeDef(doc.Types[id]) { + if !ir.IsNilTypeDef(doc.Types[id]) { live = append(live, id) } } @@ -352,7 +361,7 @@ func liveTypeIDs(doc *ir.Document) []ir.TypeID { func checkNilTypes(doc *ir.Document) []ir.Diagnostic { var diags []ir.Diagnostic for _, id := range sortedKeys(doc.Types) { - if !isNilTypeDef(doc.Types[id]) { + if !ir.IsNilTypeDef(doc.Types[id]) { continue } diags = append(diags, diag(ir.SeverityError, "ir/nil-type", From a75ed8d3a650c97fb5058cd25882af8a0e7328cf Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 4 Aug 2026 20:10:24 +0300 Subject: [PATCH 2/2] refactor(ir): spell the document root path in one place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the walk consolidation. The root label "doc" was the one piece of the path grammar still written by hand, in seven places across the two checkers. It is load-bearing for the property the shared walk exists to give — a caller running both checkers can only dedupe one defect if both spell it as one location — so it is now ir.DocumentPath, used at every document-rooted walk and by the truncation reports that name the document itself. Nothing else in the walk was left hand-maintained; a sweep for reflect.ValueOf outside ir confirms the two checkers keep no traversal of their own. Also from the same pass, none of it behavioural: - WalkValues now states that visit is required and what path is for. - Registry.Label says how it is derived (the declaring Document field's name, lowercased), and DocumentRegistries says what a nil document yields and why that is the answer a report-only caller wants. - TestDocumentRegistries_CoverAWholeRegistry named no claim; it is TestRegistry_HasResolvesOnlyDeclaredIDs. - A loop of NotEqual asserting one absent path became NotContains. --- ir/irverify/indices.go | 2 +- ir/irverify/irverify.go | 2 +- ir/irverify/naming.go | 2 +- ir/irverify/provenance.go | 2 +- ir/irverify/rawpayloads.go | 2 +- ir/irverify/refs.go | 2 +- ir/registries.go | 7 ++++++- ir/registries_test.go | 6 +++--- ir/walk.go | 15 +++++++++++---- ir/walk_test.go | 4 +--- pass/validate.go | 8 ++++---- 11 files changed, 31 insertions(+), 21 deletions(-) diff --git a/ir/irverify/indices.go b/ir/irverify/indices.go index 91e1162..3261c6a 100644 --- a/ir/irverify/indices.go +++ b/ir/irverify/indices.go @@ -39,7 +39,7 @@ var ( func checkIndices(doc *ir.Document) ([]Violation, bool) { declared := len(doc.Servers) var vs []Violation - truncated := ir.WalkValues(doc, "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 } diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index 5dbf4b6..b2dc1bd 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -84,7 +84,7 @@ func runWalkChecks(doc *ir.Document) []Violation { return append(vs, Violation{ Code: "ir/walk-truncated", Message: "document nests deeper than the bounded verifier walk; part of it went unchecked", - Path: "doc", + Path: ir.DocumentPath, }) } diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index eada76b..2a8dfc3 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -23,7 +23,7 @@ var namingType = reflect.TypeFor[ir.Naming]() // every golden, which is a different change from tightening this checker. func checkNaming(doc *ir.Document) ([]Violation, bool) { var vs []Violation - truncated := ir.WalkValues(doc, "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 || v.Type() != namingType { return true } diff --git a/ir/irverify/provenance.go b/ir/irverify/provenance.go index ff564a7..30e8e34 100644 --- a/ir/irverify/provenance.go +++ b/ir/irverify/provenance.go @@ -22,7 +22,7 @@ var provenanceType = reflect.TypeFor[ir.Provenance]() func checkProvenance(doc *ir.Document) ([]Violation, bool) { var vs []Violation declared := len(doc.Sources) - truncated := ir.WalkValues(doc, "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 || v.Type() != provenanceType { return true } diff --git a/ir/irverify/rawpayloads.go b/ir/irverify/rawpayloads.go index cceb12d..23d6847 100644 --- a/ir/irverify/rawpayloads.go +++ b/ir/irverify/rawpayloads.go @@ -30,7 +30,7 @@ var ( // document's one ir/walk-truncated violation. func checkRawPayloads(doc *ir.Document) ([]Violation, bool) { var vs []Violation - truncated := ir.WalkValues(doc, "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.Map { return true } diff --git a/ir/irverify/refs.go b/ir/irverify/refs.go index 7ca5c91..58f5258 100644 --- a/ir/irverify/refs.go +++ b/ir/irverify/refs.go @@ -21,7 +21,7 @@ type refSite struct { // a registry that must resolve. func collectRefs(doc *ir.Document, regs ir.Registries) ([]refSite, bool) { var sites []refSite - truncated := ir.WalkValues(doc, "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.String || v.String() == "" { return true } diff --git a/ir/registries.go b/ir/registries.go index 471042d..839a0b8 100644 --- a/ir/registries.go +++ b/ir/registries.go @@ -13,7 +13,8 @@ import ( // rather than indexing an invalid value, so a checker holding a site built // against another document reports it as unresolved instead of crashing. type Registry struct { - // Label names the registry as the document spells it — "types", "channels". + // Label is the name of the Document field that declares the registry, + // lowercased — "types", "channels" — for a report that has to name it. Label string entries reflect.Value @@ -47,6 +48,10 @@ type Registries map[reflect.Type]Registry // added to Document the moment it exists, where a hand-written list would drift. // Document.Unmodeled is the counterexample: keyed by plain string, it keys on a // source construct's name rather than an identity, and is no registry. +// +// A nil doc declares nothing, which is the answer a report-only caller wants: +// every reference then resolves against no registry and is reported, rather than +// the call panicking on the way to saying so. func DocumentRegistries(doc *Document) Registries { out := Registries{} if doc == nil { diff --git a/ir/registries_test.go b/ir/registries_test.go index 18121e4..a0c4572 100644 --- a/ir/registries_test.go +++ b/ir/registries_test.go @@ -35,9 +35,9 @@ func TestDocumentRegistries_DerivedFromDocumentShape(t *testing.T) { assert.Len(t, regs, len(want), "Document declares exactly these ID-keyed registries") } -// TestDocumentRegistries_CoverAWholeRegistry drives resolution both ways: an ID -// the registry declares resolves, one it does not declare dangles. -func TestDocumentRegistries_CoverAWholeRegistry(t *testing.T) { +// TestRegistry_HasResolvesOnlyDeclaredIDs drives resolution both ways: an ID the +// registry declares resolves, one it does not declare dangles. +func TestRegistry_HasResolvesOnlyDeclaredIDs(t *testing.T) { t.Parallel() doc := &ir.Document{Types: ir.TypeRegistry{ "t/x/M": &ir.Any{TypeCommon: ir.TypeCommon{ID: "t/x/M"}}, diff --git a/ir/walk.go b/ir/walk.go index 61413fb..d883550 100644 --- a/ir/walk.go +++ b/ir/walk.go @@ -20,10 +20,19 @@ const MaxWalkDepth = 4096 // them apart. const MapKeySuffix = ".key" +// DocumentPath is the path a walk rooted at a whole [Document] starts from, and +// so the first segment of every location such a walk reports. It is a constant +// rather than each caller's own string literal for the reason the rest of the +// path grammar is one walk: two checkers reporting one defect can only be +// deduped by a caller running both if the defect reads as one location. +const DocumentPath = "doc" + // WalkValues performs a bounded, cycle-guarded reflection traversal of root, // calling visit on every value it reaches together with the path it was reached // by; returning false from visit skips that value's children. It reports -// whether the depth cap cut the walk short. +// whether the depth cap cut the walk short. visit is required, and path is the +// segment every reported location is rooted at — [DocumentPath] for a walk over +// a whole document. // // Deriving what a document holds from the value graph instead of naming fields // is what makes a check built on it complete: a field added to the IR is covered @@ -35,9 +44,7 @@ const MapKeySuffix = ".key" // Paths spell fields joined by ".", slice indices and map keys in brackets, and // an embedded field contributes no segment of its own: JSON inlines it and Go // promotes its fields, so "….TypeCommon.Examples[0]" names a step neither -// encoding has — the example is reached as "….Examples[0]" in both. Both -// checkers report a dangling reference under one code, and a caller running both -// can only dedupe them if one defect reads as one location. +// encoding has — the example is reached as "….Examples[0]" in both. // // A value reached through an unexported field is read-only, and Interface() // panics on one where FieldByName does not, so a visitor reads the fields it diff --git a/ir/walk_test.go b/ir/walk_test.go index 2550605..78eab2d 100644 --- a/ir/walk_test.go +++ b/ir/walk_test.go @@ -207,9 +207,7 @@ func TestWalkValues_NothingUnreachableIsVisited(t *testing.T) { paths, _ = walkPaths(doc) assert.Contains(t, paths, "doc.Contact", "the nil pointer itself is visited") assert.Contains(t, paths, "doc.Types[t/nil]", "the nil interface itself is visited") - for _, p := range paths { - assert.NotEqual(t, "doc.Contact.Name", p, "nothing behind a nil pointer is reachable") - } + assert.NotContains(t, paths, "doc.Contact.Name", "nothing behind a nil pointer is reachable") } // TestWalkValues_ArrayElementsAreReached pins that a fixed-size array is walked diff --git a/pass/validate.go b/pass/validate.go index ab765e0..ba9fcdf 100644 --- a/pass/validate.go +++ b/pass/validate.go @@ -62,7 +62,7 @@ func Validate(doc *ir.Document) []ir.Diagnostic { // the same traversal saw. func checkDanglingRefs(doc *ir.Document) []ir.Diagnostic { regs := ir.DocumentRegistries(doc) - sites, truncated := collectRefs(doc, "doc", func(t reflect.Type) bool { + sites, truncated := collectRefs(doc, ir.DocumentPath, func(t reflect.Type) bool { _, isRegistry := regs[t] return isRegistry }) @@ -70,7 +70,7 @@ func checkDanglingRefs(doc *ir.Document) []ir.Diagnostic { if truncated { diags = append(diags, diag(ir.SeverityError, "ir/walk-truncated", "document nests deeper than the bounded reference walk; some references went unchecked", - "doc")) + ir.DocumentPath)) } for _, s := range sites { // A site whose class this document declares no registry for cannot @@ -185,7 +185,7 @@ func appendSuccessStatusDiags(dst []ir.Diagnostic, status map[int]int, declared // for the same reason a PropID was. Reaching it needs a token parser rather than a // lookup, and a false positive inside prose is noisier than a missing check. func checkPropIDRefs(doc *ir.Document) []ir.Diagnostic { - sites, declared := collectPropIDs(doc, "doc") + sites, declared := collectPropIDs(doc, ir.DocumentPath) var diags []ir.Diagnostic for _, s := range sites { if declared[ir.PropID(s.id)] { @@ -637,7 +637,7 @@ func checkGroupWalkTruncated(doc *ir.Document) []ir.Diagnostic { } return []ir.Diagnostic{diag(ir.SeverityError, "ir/walk-truncated", fmt.Sprintf("operation groups nest deeper than %d; some operations went unchecked", maxGroupDepth), - "doc")} + ir.DocumentPath)} } // checkArgsOutsideGraphQL reports field arguments on models that are not