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..ec779b6 100644 --- a/ir/irverify/irverify.go +++ b/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 } 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..e2421ab 100644 --- a/ir/irverify/refs.go +++ b/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] 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:] } 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