refactor(irverify): read declarations once per Verify run

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
 }

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",

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 {

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
 }
 

ir/irverify/irverify.go

@@ -47,11 +47,39 @@ 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 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
+// 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
+}
+
+// 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}
+}
+
 // 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 +99,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
 	}

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 {

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 {

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 {

ir/irverify/refs.go

@@ -63,14 +63,20 @@ 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)
+//
+// 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 !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]

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)
 

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:]
 }

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 {

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