From 982785dccbb6670c06e8cd4193d7f45e1debf4bf Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 13:59:34 +0300 Subject: [PATCH 1/4] refactor(irverify): read declarations once per Verify run checkReferentialIntegrity and checkDuplicateIDs each called ir.DeclaredIDs, so Verify walked the whole document twice to build the same slice. On a 500-operation document that was ~4ms of ~27ms, a sixth of the run. runWalkChecks reads the declarations once and hands them to every walking check. The four that do not need them take the parameter anyway: one signature is what lets walkChecks be a list, and the drift guard in walkchecks_test.go keys on the result types, which are unchanged. checkDuplicateIDs no longer walks for itself, so it passes on the flag of the walk its input rests on. Both the walkChecks contract and its own doc comment now say that rather than "its own walk". --- ir/irverify/duplicates.go | 14 ++++++------ ir/irverify/duplicates_test.go | 4 ++-- ir/irverify/indices.go | 2 +- ir/irverify/indices_test.go | 2 +- ir/irverify/irverify.go | 42 +++++++++++++++++++++++++++++----- ir/irverify/naming.go | 2 +- ir/irverify/provenance.go | 2 +- ir/irverify/rawpayloads.go | 2 +- ir/irverify/refs.go | 9 ++++---- ir/irverify/refs_test.go | 4 ++-- ir/irverify/walkchecks_test.go | 5 ++-- 11 files changed, 59 insertions(+), 29 deletions(-) diff --git a/ir/irverify/duplicates.go b/ir/irverify/duplicates.go index f8383af..ef22096 100644 --- a/ir/irverify/duplicates.go +++ b/ir/irverify/duplicates.go @@ -19,8 +19,9 @@ type identity struct { } // checkDuplicateIDs asserts no two nodes declare the same identity (invariant -// #3). It reports whether the bounded walk was cut short; Verify folds that into -// the document's one ir/walk-truncated violation. +// #3). It reads the declarations rather than walking for them, and passes on +// whether the walk that produced them was cut short; Verify folds that into the +// document's one ir/walk-truncated violation. // // Uniqueness was enforced only by the registry maps, and they cannot express it // for a class they do not hold: an operation nests inside the @@ -52,11 +53,10 @@ type identity struct { // The first declaration in walk order stands and every later one is reported, so // n nodes on one ID yield n-1 violations rather than n. Walk order is // deterministic (invariant 7), so which one stands does not vary between runs. -func checkDuplicateIDs(doc *ir.Document) ([]Violation, bool) { - decls, truncated := ir.DeclaredIDs(doc) - first := make(map[identity]string, len(decls)) +func checkDuplicateIDs(_ *ir.Document, decls declarations) ([]Violation, bool) { + first := make(map[identity]string, len(decls.ids)) var vs []Violation - for _, d := range decls { + for _, d := range decls.ids { if d.Class == propIDType { continue } @@ -72,5 +72,5 @@ func checkDuplicateIDs(doc *ir.Document) ([]Violation, bool) { Path: d.Path, }) } - return vs, truncated + return vs, decls.truncated } diff --git a/ir/irverify/duplicates_test.go b/ir/irverify/duplicates_test.go index 0fc13c1..da4f8fa 100644 --- a/ir/irverify/duplicates_test.go +++ b/ir/irverify/duplicates_test.go @@ -14,7 +14,7 @@ import ( // which the cases below assert nothing about; TestWalkChecks_EachReportsTruncation // holds that half. func duplicateViolations(doc *ir.Document) []Violation { - vs, _ := checkDuplicateIDs(doc) + vs, _ := checkDuplicateIDs(doc, readDeclarations(doc)) return vs } @@ -124,7 +124,7 @@ func TestVerify_ReportsDuplicateIDs(t *testing.T) { // written down, and the test below fails when ir grows a named string type it // does not account for. var identityClasses = map[string]string{ - "TypeID": "identity: Document.Types keys it; checkReferentialIntegrity resolves references, checkRegistryKeys holds each key to its node's own ID", + "TypeID": "identity: Document.Types keys it; checkReferentialIntegrity resolves references, checkRegistryKeys holds each key to its node's own ID and checkDuplicateIDs holds no two nodes to one ID", "ChannelID": "identity: Document.Channels keys it; resolved and held as TypeID is", "MessageID": "identity: Document.Messages keys it; resolved and held as TypeID is", "AuthID": "identity: Document.Auth keys it; resolved and held as TypeID is", diff --git a/ir/irverify/indices.go b/ir/irverify/indices.go index 3261c6a..cd060de 100644 --- a/ir/irverify/indices.go +++ b/ir/irverify/indices.go @@ -36,7 +36,7 @@ var ( // // 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) { +func checkIndices(doc *ir.Document, _ declarations) ([]Violation, bool) { declared := len(doc.Servers) var vs []Violation truncated := ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool { diff --git a/ir/irverify/indices_test.go b/ir/irverify/indices_test.go index 8e376bb..671691b 100644 --- a/ir/irverify/indices_test.go +++ b/ir/irverify/indices_test.go @@ -23,7 +23,7 @@ func docWithServers() *ir.Document { // cases below assert nothing about; TestWalkChecks_EachReportsTruncation holds // that half. func indexViolations(doc *ir.Document) []Violation { - vs, _ := checkIndices(doc) + vs, _ := checkIndices(doc, readDeclarations(doc)) return vs } diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index 70801af..b2e4b7b 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -47,11 +47,33 @@ func Verify(doc *ir.Document) []Violation { return vs } +// declarations is the identities a document's nodes declare, plus whether the +// walk that read them was cut short. +// +// Reading them costs a full walk of the document, and two checks need them — +// checkReferentialIntegrity to resolve the classes Document keys no map by, and +// checkDuplicateIDs to hold each one to being declared once. Deriving it in each +// was the same walk twice, about a sixth of Verify's work on a large document, so +// it is read once per run and handed down. The checks that do not need it still +// take it, because one signature is what lets walkChecks be a list at all. +type declarations struct { + ids []ir.IDDeclaration + truncated bool +} + +// readDeclarations reads the identities doc's nodes declare, once for the run. +func readDeclarations(doc *ir.Document) declarations { + ids, truncated := ir.DeclaredIDs(doc) + return declarations{ids: ids, truncated: truncated} +} + // 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){ +// the document. Each returns whether the walk its result rests on was cut short, +// so the flag is part of the signature rather than a value a check can quietly +// drop — whether the check runs that walk itself or reads a declarations value +// walked once for the run. +func walkChecks() []func(*ir.Document, declarations) ([]Violation, bool) { + return []func(*ir.Document, declarations) ([]Violation, bool){ checkReferentialIntegrity, checkDuplicateIDs, checkNaming, @@ -71,11 +93,19 @@ func walkChecks() []func(*ir.Document) ([]Violation, bool) { // 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. +// +// The seed is decls.truncated rather than false because the declaration walk runs +// here, and a function that walks owns its own flag. Both checks that read the +// declarations return it too, so seeding from false reports the same thing today +// — planting that mutation leaves the suite green. It is written this way anyway, +// for the reason above: relying on a callee to hand back the flag for a walk this +// function performed is the same dependence on a coincidence that #55 was. func runWalkChecks(doc *ir.Document) []Violation { + decls := readDeclarations(doc) var vs []Violation - truncated := false + truncated := decls.truncated for _, check := range walkChecks() { - found, cut := check(doc) + found, cut := check(doc, decls) vs = append(vs, found...) truncated = truncated || cut } diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index ad5c27c..9899a11 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -58,7 +58,7 @@ var nameOptional = map[reflect.Type]bool{ // 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, bool) { +func checkNaming(doc *ir.Document, _ declarations) ([]Violation, bool) { var vs []Violation optional := map[string]bool{} truncated := ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool { diff --git a/ir/irverify/provenance.go b/ir/irverify/provenance.go index 30e8e34..80ff5ae 100644 --- a/ir/irverify/provenance.go +++ b/ir/irverify/provenance.go @@ -19,7 +19,7 @@ var provenanceType = reflect.TypeFor[ir.Provenance]() // 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) { +func checkProvenance(doc *ir.Document, _ declarations) ([]Violation, bool) { var vs []Violation declared := len(doc.Sources) truncated := ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool { diff --git a/ir/irverify/rawpayloads.go b/ir/irverify/rawpayloads.go index 23d6847..d591ccd 100644 --- a/ir/irverify/rawpayloads.go +++ b/ir/irverify/rawpayloads.go @@ -28,7 +28,7 @@ var ( // 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) { +func checkRawPayloads(doc *ir.Document, _ declarations) ([]Violation, bool) { var vs []Violation truncated := ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool { if v.Kind() != reflect.Map { diff --git a/ir/irverify/refs.go b/ir/irverify/refs.go index fef830b..8e18c8c 100644 --- a/ir/irverify/refs.go +++ b/ir/irverify/refs.go @@ -63,14 +63,13 @@ func collectRefs(doc *ir.Document, regs ir.Registries) ([]refSite, bool) { // values a document declares and looking the ID up among them, which // pass.Validate's checkPropIDRefs does — beside checkEncodingKeys, which makes // the tighter model-scoped claim for the keys of ir.Content.Encoding. -func checkReferentialIntegrity(doc *ir.Document) ([]Violation, bool) { - decls, declTruncated := ir.DeclaredIDs(doc) +func checkReferentialIntegrity(doc *ir.Document, decls declarations) ([]Violation, bool) { regs := ir.DocumentRegistries(doc) - if !declTruncated { - regs = regs.WithDeclarations(decls) + if !decls.truncated { + regs = regs.WithDeclarations(decls.ids) } sites, truncated := collectRefs(doc, regs) - truncated = truncated || declTruncated + truncated = truncated || decls.truncated var vs []Violation for _, s := range sites { reg := regs[s.idType] diff --git a/ir/irverify/refs_test.go b/ir/irverify/refs_test.go index e057e0e..b0fd052 100644 --- a/ir/irverify/refs_test.go +++ b/ir/irverify/refs_test.go @@ -48,7 +48,7 @@ func TestCollectRefs_SkipsEmptyIDs(t *testing.T) { // the cases below assert nothing about; TestWalkChecks_EachReportsTruncation // holds that half. func refViolations(doc *ir.Document) []Violation { - vs, _ := checkReferentialIntegrity(doc) + vs, _ := checkReferentialIntegrity(doc, readDeclarations(doc)) return vs } @@ -210,7 +210,7 @@ func TestCheckReferentialIntegrity_DanglingOpRefWithNoOperationDeclared(t *testi // not off the walk. func TestCheckReferentialIntegrity_TruncatedWalkClaimsNoDeclarations(t *testing.T) { deep, target := opDocNested(ir.MaxWalkDepth) - vs, truncated := checkReferentialIntegrity(deep) + vs, truncated := checkReferentialIntegrity(deep, readDeclarations(deep)) assert.True(t, truncated, "the walk must report that it was cut short") assert.Empty(t, vs, "%s is declared past the cap, not undeclared", target) diff --git a/ir/irverify/walkchecks_test.go b/ir/irverify/walkchecks_test.go index 230409b..181bdb5 100644 --- a/ir/irverify/walkchecks_test.go +++ b/ir/irverify/walkchecks_test.go @@ -42,8 +42,9 @@ func deepDoc() *ir.Document { func TestWalkChecks_EachReportsTruncation(t *testing.T) { t.Parallel() doc := deepDoc() + decls := readDeclarations(doc) for _, check := range walkChecks() { - _, truncated := check(doc) + _, truncated := check(doc, decls) assert.True(t, truncated, "%s walked a document nested past the cap without reporting it", checkName(check)) } @@ -103,7 +104,7 @@ func TestWalkChecks_NoWalkDropsItsTruncationFlag(t *testing.T) { } // checkName is a walkChecks entry's function name, without its package path. -func checkName(check func(*ir.Document) ([]Violation, bool)) string { +func checkName(check func(*ir.Document, declarations) ([]Violation, bool)) string { full := runtime.FuncForPC(reflect.ValueOf(check).Pointer()).Name() return full[strings.LastIndex(full, ".")+1:] } From 80d7fa2413734d13f20fbe21234c640ddb4d1355 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 13:59:49 +0300 Subject: [PATCH 2/4] docs(ir): correct the claims the ID-reference change left stale DeclaredIDs said a node carrying an empty ID is "reported where the node's registry key is". That holds for the classes Document keys a map by and not for the two the same change added, which have no key: an operation or a service with an empty ID is reported by nothing. Filed as #289; the comment now states the gap instead of asserting it away. Three comments in validate_idrefs_test.go describe a set that has since grown. idRefSite named the classes it covers, TestValidate_DanglingTypedIDRef said it plants "channel, message or auth" references, and the location order above sortedIDRefPointers said "all three reference classes" while the literal below it gained six entries. The enumerations are replaced with the rule that derives them, so the next class to arrive cannot leave them wrong. identityClasses says per identity what holds it unique, and the TypeID entry named only checkRegistryKeys. checkDuplicateIDs holds it too, which is what TestCheckDuplicateIDs_TwoRegistryEntriesOnOneNodeID exists to show. --- ir/registries.go | 12 +++++++++--- pass/validate_idrefs_test.go | 27 +++++++++++++++++---------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/ir/registries.go b/ir/registries.go index b97455a..9abd000 100644 --- a/ir/registries.go +++ b/ir/registries.go @@ -205,9 +205,15 @@ type IDDeclaration struct { // each of them twice: a document with nothing wrong with it would read as one // where every type ID is declared twice. // -// An empty ID declares no identity and is skipped. A node carrying one is its own -// defect, reported where the node's registry key is, and calling several of them -// duplicates of each other would name the wrong problem. +// An empty ID declares no identity and is skipped: nothing can reference one, and +// treating several nodes that carry one as duplicates of each other would name +// the wrong defect. +// +// Whether the empty ID is itself reported is a separate claim, and one this +// derivation does not make. A class Document keys a map by is covered — an empty +// or disagreeing key is what irverify.checkRegistryKeys reads — but an Operation +// and a Service have no key for it to read, so an empty ID on either goes +// unreported (GitHub #289). func DeclaredIDs(doc *Document) ([]IDDeclaration, bool) { var decls []IDDeclaration truncated := WalkValues(doc, DocumentPath, func(v reflect.Value, path string) bool { diff --git a/pass/validate_idrefs_test.go b/pass/validate_idrefs_test.go index 6e95417..5e0e31a 100644 --- a/pass/validate_idrefs_test.go +++ b/pass/validate_idrefs_test.go @@ -11,13 +11,16 @@ import ( "github.com/dexpace/morphic/pass" ) -// idRefSite is one field carrying a typed-ID reference other than an ir.TypeID — -// the class Validate resolved nowhere before checkDanglingRefs. +// idRefSite is one field carrying a typed-ID reference of a class other than +// ir.TypeID — the classes Validate resolved nowhere before checkDanglingRefs. // // The set below is derived from the ir package's own declarations: every field -// whose type mentions ChannelID, MessageID or AuthID. Fields holding a node's own -// ID (Channel.ID, Message.ID, AuthScheme.ID) and registry keys resolve against -// their own entry by construction, so only the cross-references are listed. +// whose type is one of those classes. A field holding a node's own ID +// (Channel.ID, Operation.ID, Service.ID and their siblings) and a registry key +// resolve against their own declaration by construction, so only the +// cross-references are listed. Deriving the set is the point — enumerating the +// classes here instead would leave a class added to the IR silently untested, +// which is how the operation and service classes went unchecked (GitHub #50). type idRefSite struct { name string code string @@ -204,10 +207,14 @@ func withCode(diags []ir.Diagnostic, code string) []ir.Diagnostic { return out } -// TestValidate_DanglingTypedIDRef plants one dangling channel, message or auth -// reference per field that carries one and requires Validate to report it, at -// that field's location. Before checkDanglingRefs, only the auth case produced a -// diagnostic at all — Validate resolved no ChannelID or MessageID anywhere. +// TestValidate_DanglingTypedIDRef plants one dangling reference per field in +// idRefSites and requires Validate to report it, at that field's location. +// +// Two rounds of silence are pinned here. Before checkDanglingRefs, only the auth +// case produced a diagnostic at all — Validate resolved no ChannelID or MessageID +// anywhere. Before ir.Registries.WithDeclarations, the operation and service +// cases resolved against nothing either, because neither class has a map on +// Document for a registry to be derived from (GitHub #50). func TestValidate_DanglingTypedIDRef(t *testing.T) { t.Parallel() for _, tc := range idRefSites() { @@ -262,7 +269,7 @@ func TestValidate_DanglingOpRefWithNoOperationDeclared(t *testing.T) { } // sortedIDRefPointers is the location order every run must produce: ascending by -// pointer, across all three reference classes rather than grouped by class. +// pointer, across every reference class rather than grouped by class. // // It is written out rather than captured from a first run for the reason // sortedRefPointers is (validate_refs_test.go): a captured order proves only From d3b062e44f60fac772eba1f09b01d38942ab2c20 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 14:09:05 +0300 Subject: [PATCH 3/4] docs(irverify): mark the redundant fold and the invalid zero value Two things a reader can only find by planting a mutation, so both are now written down beside the code. checkReferentialIntegrity folds decls.truncated into the flag it returns beside collectRefs' own. Neither of those walks prunes, so they truncate together and dropping either half is invisible to the suite. The fold stays for the reason the seed in runWalkChecks does, and now says so. The declarations zero value is not "nothing worth mentioning": it says the document declares no identity, which makes every OpID and ServiceID reference in it report as dangling. The suite does catch a check being handed one, but the type cannot, so the doc comment names it. --- ir/irverify/irverify.go | 5 +++++ ir/irverify/refs.go | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index b2e4b7b..ab911f9 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -56,6 +56,11 @@ func Verify(doc *ir.Document) []Violation { // was the same walk twice, about a sixth of Verify's work on a large document, so // it is read once per run and handed down. The checks that do not need it still // take it, because one signature is what lets walkChecks be a list at all. +// +// The zero value is not a stand-in for "no declarations to speak of": it says the +// document declares none, which makes every OpID and ServiceID reference in it +// resolve against an empty registry and report as dangling. Read one with +// readDeclarations from the document being checked. type declarations struct { ids []ir.IDDeclaration truncated bool diff --git a/ir/irverify/refs.go b/ir/irverify/refs.go index 8e18c8c..e2421ab 100644 --- a/ir/irverify/refs.go +++ b/ir/irverify/refs.go @@ -63,6 +63,13 @@ func collectRefs(doc *ir.Document, regs ir.Registries) ([]refSite, bool) { // values a document declares and looking the ID up among them, which // pass.Validate's checkPropIDRefs does — beside checkEncodingKeys, which makes // the tighter model-scoped claim for the keys of ir.Content.Encoding. +// +// The returned flag folds in decls.truncated beside collectRefs' own. Neither of +// those two walks prunes, so today they reach equally far and truncate together, +// and dropping either half reports the same thing — planting that mutation leaves +// the suite green. It is folded anyway: "the other walk trips the cap first" is +// the coincidence GitHub #55 was, and a visitor here that began pruning, as other +// checks' visitors already do, would end it without anything saying so. func checkReferentialIntegrity(doc *ir.Document, decls declarations) ([]Violation, bool) { regs := ir.DocumentRegistries(doc) if !decls.truncated { From 1df07c58433d68be319e28a5cc0a89978e7bcca3 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 14:11:02 +0300 Subject: [PATCH 4/4] docs(irverify): drop a drifting ratio and a memoization implication The declarations comment gave the saving as a fraction of Verify's cost, which is a figure that goes stale as the other checks change; the reason that cannot drift is that it was the same walk twice, and the measurement belongs in the pull request rather than the source. readDeclarations said it reads the identities "once for the run", which describes what runWalkChecks does with it and reads as though the function memoizes. It does not, and the tests call it per case. --- ir/irverify/irverify.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index ab911f9..ec779b6 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -53,9 +53,9 @@ func Verify(doc *ir.Document) []Violation { // Reading them costs a full walk of the document, and two checks need them — // checkReferentialIntegrity to resolve the classes Document keys no map by, and // checkDuplicateIDs to hold each one to being declared once. Deriving it in each -// was the same walk twice, about a sixth of Verify's work on a large document, so -// it is read once per run and handed down. The checks that do not need it still -// take it, because one signature is what lets walkChecks be a list at all. +// was the same walk twice over, so it is read once per run and handed down. The +// checks that do not need it still take it, because one signature is what lets +// walkChecks be a list at all. // // The zero value is not a stand-in for "no declarations to speak of": it says the // document declares none, which makes every OpID and ServiceID reference in it @@ -66,7 +66,8 @@ type declarations struct { truncated bool } -// readDeclarations reads the identities doc's nodes declare, once for the run. +// readDeclarations reads the identities doc's nodes declare. It memoizes +// nothing; runWalkChecks is what calls it once and shares the result. func readDeclarations(doc *ir.Document) declarations { ids, truncated := ir.DeclaredIDs(doc) return declarations{ids: ids, truncated: truncated}