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..3261c6a 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, ir.DocumentPath, func(v reflect.Value, path string) bool { if v.Kind() != reflect.Struct { return true } @@ -49,7 +53,7 @@ func checkIndices(doc *ir.Document) []Violation { } return true // a service still owns the operations nested below it }) - return vs + return vs, truncated } // appendServerIndexViolations appends to vs a violation per entry of a Servers 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..b2dc1bd 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: ir.DocumentPath, + }) +} + // 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..2a8dfc3 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, ir.DocumentPath, 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..30e8e34 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, ir.DocumentPath, 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..23d6847 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, ir.DocumentPath, 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..58f5258 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, ir.DocumentPath, 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..839a0b8 --- /dev/null +++ b/ir/registries.go @@ -0,0 +1,89 @@ +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 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 +} + +// 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. +// +// 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 { + 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..a0c4572 --- /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") +} + +// 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"}}, + }} + + 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..d883550 --- /dev/null +++ b/ir/walk.go @@ -0,0 +1,196 @@ +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" + +// 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. 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 +// 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. +// +// 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..78eab2d --- /dev/null +++ b/ir/walk_test.go @@ -0,0 +1,224 @@ +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") + assert.NotContains(t, paths, "doc.Contact.Name", "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..ba9fcdf 100644 --- a/pass/validate.go +++ b/pass/validate.go @@ -43,23 +43,44 @@ 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, ir.DocumentPath, 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", "document nests deeper than the bounded reference walk; some references went unchecked", - "doc")) + ir.DocumentPath)) } 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)) @@ -164,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)] { @@ -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", @@ -628,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