Skip to content

fix(ir): resolve every class of ID reference a document carries - #271

Merged
OmarAlJarrah merged 2 commits into
mainfrom
fix/ir-reference-integrity-gaps
Aug 6, 2026
Merged

fix(ir): resolve every class of ID reference a document carries#271
OmarAlJarrah merged 2 commits into
mainfrom
fix/ir-reference-integrity-gaps

Conversation

@OmarAlJarrah

@OmarAlJarrah OmarAlJarrah commented Aug 6, 2026

Copy link
Copy Markdown
Member

Summary

Both of Morphic's reference checkers — pass.Validate and irverify.Verify — decide what counts as
a reference from the ID-keyed maps hanging off ir.Document. Two ID classes have no such map: an
operation is declared inside the ServiceOperationGroup tree, and a service sits in
Document.Services, a slice. Neither class had a registry, so every OpID and ServiceID
reference in the IR resolved against nothing and was accepted in silence

Operation.OverloadOf, LongRunning.PollingOperation/FinalOperation, Callback.Operations,
ResourceInfo.Lifecycle/InstanceOps/CollectionOps, and Service.Extends.

Separately, ID uniqueness came only from the registry map keys, which cannot express it for a class
they do not hold. Two operations sharing an OpID made every reference to it resolve to whichever
the reader reached first, with nothing in the document saying which that is.

#51 is a direct consequence of the same walker gap — GraphQL reachability was built on the same
incomplete traversal — so the two are one change over one walk rather than two PRs editing it. The
reachability half turned out to be already fixed by the traversal consolidation in #249; this PR
adds the regression cover it never got.

What the state actually is

#50 lists four classes of missed reference. Each was re-checked by building an ir.Document in Go
that plants the specific defect and running both checkers over it. Two of the four had already
been closed by #249, which replaced the four hand-written typed walkers with the single
reflection-driven ir.WalkValues:

Class from #50 Before this PR Evidence
1 — ~18 unwalked TypeRef field classes already closed 30 planted dangling TypeRefs, one per site the issue names → 30 ir/dangling-type-ref diagnostics and 30 violations
2 — OpID references checked nowhere open 7 planted dangling OpIDs → 0 diagnostics, 0 violations
3 — duplicate IDs outside the map registries open duplicate OpID + duplicate PropID → 0 diagnostics, 0 violations
4 — index references never bounds-checked already closed 4 planted out-of-range indices (2 server, 1 response, 1 provenance) → all reported
#51 — subscription false positive already closed the subscription document the issue describes yields no pass/args-outside-graphql; removing its GraphQL binding does yield one, so the check reaches

Class 3 is the one place the PR does less than the issue asks. See "The PropID carve-out" below.

Changes

ir.DeclaredIDs reads the identities a document's own nodes declare — a node's ID field — off
the same bounded walk both checkers already share. Only a struct that declares the field itself
counts: every type node embeds TypeCommon and promotes its ID, so counting promoted fields would
read a sound document as one where every type ID is declared twice.

ir.idClasses answers which classes there are, from the IR's type graph rather than from one
document's contents — a bounded walk over reflect.Type seeded with Document and each concrete
TypeDef, applying to a type the same rule declaredID applies to a value. Reading the class set
off the declarations instead would leave a class no node happens to declare with no registry at
all, and a site whose class has no registry is not an unresolved reference but no reference: the
same silence, reached by a different route. A Smithy resource names its lifecycle operations, so a
document holding no ir.Operation still carries OpID references.

ir.Registries.WithDeclarations turns those into a Registry for each class Document holds no
map for. A class that does have a map keeps it — irverify already holds every entry to being keyed
by its own node's ID, so a second answer derived here could only disagree with the first. A
declaration-derived registry the receiver already carries is replaced rather than added to, so
extending one call's result cannot mutate it.

Both checkers build their registry map from DocumentRegistries(doc).WithDeclarations(decls), so
one defect reports under one ir/dangling-<noun>-ref code whichever checker a caller runs — the
consistency ir.RefNoun exists for.

Truncation is the direction a derived registry can be wrong in. A registry built from a walk
that saw part of the document answers "not declared" for a node it never reached, so an operation
buried past ir.MaxWalkDepth would make a valid reference to it report as dangling. That is a false
error, where a map-keyed registry can only ever under-report. Both checkers drop the
declaration-derived classes when the declaration walk truncates and leave ir/walk-truncated to say
why nothing is claimed for them, as checkArgsOutsideGraphQL already does with reachability it
cannot trust.

irverify.checkDuplicateIDs holds every declared identity to being declared once, reporting
ir/duplicate-<noun>-id for each declaration after the first in walk order. It is a Violation
rather than an ir.Diagnostic because an ID derives from the source pointer of its defining
occurrence: two nodes sharing one means a compiler minted the same pointer twice, which is our bug,
not something a spec author wrote or can fix.

irverify.identityClasses is the drift guard. It classifies every named string type the ir
package declares as an identity or not, and says for each identity what resolves references to it and
what holds it unique. TestIdentityClasses_AreAllClassified fails when ir grows a named string
type nobody has accounted for; this is the string-typed counterpart to the existing integerFields
guard, which does the same job for index references.

The PropID carve-out

#50 asks for two properties sharing a PropID to be reported. Running that check over the existing
corpus showed the premise does not hold for the common case:
testdata/conformance/openapi/component-reuse.yaml declares components/responses/Listed once and
$refs it from three operations. Responses are embedded by value rather than interned, so the header
property that response declares materializes at three paths under the one PropID its defining
occurrence derives. That the ID stays the declaration's rather than the use site's is what #107
fixed, so the repeat is invariant 3 holding rather than breaking: the three are the same property,
a lookup for that ID is unambiguous, and reporting it would fail every document that reuses a
component.

The skip is nonetheless wider than that reason, and is deliberately provisional. Two genuinely
different properties minted at one PropID go unreported with the copies, which is the same defect
this check exists for. Telling them apart needs a fingerprint of the node rather than its ID alone;
that is filed as #280, and checkDuplicateIDs and the identityClasses entry both name it so
the exemption reads as the debt it is. TestCheckDuplicateIDs_RepeatedPropIDIsClean pins the case
that is settled.

PropID also stays out of WithDeclarations: pass.Validate already resolves those references
against the properties a document declares (checkPropIDRefs) and against the parts of the model a
content names (checkEncodingKeys), and adding a document-wide answer would report one defect twice
under two codes.

#51

graphqlReachableTypes now runs on the reflection walk, so it reaches ResponseStream.Events,
Property.Args parameter types and template instantiation arguments — the three sites #51 names.
This PR adds TestValidate_GraphQLReachabilityFollowsEveryReference, which drives all three and, for
each, asserts both halves: no diagnostic with the GraphQL binding present, and a diagnostic once it is
removed. Without the second half the case would pass on a reachability walk that reaches nothing at
all. Two test comments naming the deleted walkModelRefs/walkOperationRefs/walkPayloadRefs
walkers are corrected while in the file.

Test plan

Gate run in the CI order: gofmt clean, go vet clean, golangci-lint 0 issues, go build clean,
./scripts/check-coverage.sh at 100%.

Each new behavioural assertion was proved able to fail by planting the defect back and watching it go
red. Mutations planted and killed, each named at the site it was planted:

Mutation Went red
checkDanglingRefs (pass) drops .WithDeclarations TestValidate_DanglingTypedIDRef — all 8 new subtests, …_DanglingOpRefWithNoOperationDeclared, and TestValidate_TypedIDDiagnosticOrderIsDeterministic
checkReferentialIntegrity (irverify) drops .WithDeclarations TestCheckReferentialIntegrity_DanglingOpRef, …_DanglingServiceRef, …_DanglingOpRefWithNoOperationDeclared
WithDeclarations takes its classes from the declarations again TestWithDeclarations_ResolvesAClassNothingDeclares, and the …WithNoOperationDeclared case in both checkers
either checker trusts a truncated declaration walk TestCheckDanglingRefs_TruncatedWalkClaimsNoDeclarations, TestCheckReferentialIntegrity_TruncatedWalkClaimsNoDeclarations
checkDuplicateIDs removed from walkChecks TestVerify_ReportsDuplicateIDs, TestWalkChecks_NoWalkDropsItsTruncationFlag
checkDuplicateIDs drops the PropID skip TestCheckDuplicateIDs_RepeatedPropIDIsClean, TestVerify_Corpus/component-reuse.yaml, TestVerify_EngineOutput, TestHarness_InRepoCorpus
declaredID drops the len(f.Index) != 1 promoted-field guard TestDeclaredIDs_ReachesEveryIDBearingNodeOnce, plus the whole irverify corpus
WithDeclarations mutates its receiver instead of copying TestWithDeclarations_LeavesTheReceiverAlone, …_LeavesAnEarlierResultAlone
WithDeclarations writes into a set an earlier call handed out TestWithDeclarations_LeavesAnEarlierResultAlone
one entry deleted from identityClasses TestIdentityClasses_AreAllClassified
graphqlReachableTypes seeds from op.Params/op.Responses instead of op …FollowsEveryReference/subscription_response_stream
appendTypeIDs drops sites under .Args and .Instantiation …FollowsEveryReference/nested_field-argument_type, …/template_instantiation_argument

TestIdentityClasses_AreAllClassified was also driven the other way, by adding a type FooID string
to ir and confirming it fails on a class nobody has accounted for.

The new OpID check was confirmed to reach live compiler output rather than only synthetic
documents: compiling testdata/conformance/openapi/component-reuse.yaml emits a
Callback.Operations entry
(op/openapi/paths/~1orders/post/callbacks/onShipped/{$request.body#~1callbackUrl}/post), which the
new registry resolves, and the corpus stays clean.

No IR shape changed, so ir.IRVersion is unmoved.

Closes #50
Closes #51

Both reference checkers build their registry map from ir.DocumentRegistries,
which reads the ID-keyed maps hanging off Document. Two ID classes have no such
map: an operation is declared inside the Service->OperationGroup tree and a
service sits in a slice. Neither class had a registry, so every OpID and
ServiceID reference in the IR resolved against nothing and was silently
accepted - Operation.OverloadOf, LongRunning.PollingOperation/FinalOperation,
Callback.Operations, ResourceInfo.Lifecycle/InstanceOps/CollectionOps and
Service.Extends alike.

ir.DeclaredIDs reads the identities a document's own nodes declare off the
shared bounded walk, and ir.Registries.WithDeclarations turns the classes with
no map into registries from them. Both checkers pick those up, so the defect
reports under the same ir/dangling-<noun>-ref code whichever one runs.

Uniqueness came only from the registry map keys, which cannot express it for a
class they do not hold: two operations on one OpID made every reference to it
resolve to whichever the reader reached first. checkDuplicateIDs holds every
declared identity to being declared once. It is a Violation rather than a
Diagnostic because an ID derives from the source pointer of its defining
occurrence, so two nodes sharing one is a compiler bug.

PropID stays out of both. A response declared once in components and referenced
by several operations materializes into each of them, so its header property
appears at several paths under one ID - the copies are the same property, and a
lookup for it is unambiguous.

identityClasses (irverify) classifies every named string type ir declares as an
identity or not, and fails when the package grows one nobody has accounted for.
A class nothing resolves against goes unchecked in silence rather than failing,
which is what let these two go unnoticed.
WithDeclarations read the classes it answered for out of the declarations it
was handed, so a class nothing declared got no registry at all. A site whose
class has no registry is not collected, and an uncollected site is not an
unresolved reference but no reference — the same silence #50 is about, reached
by a different route. A Smithy resource names its lifecycle operations, so a
document holding no ir.Operation still carries OpID references, and all of them
went unchecked: three planted dangling OpIDs and one dangling ServiceID
produced no diagnostic and no violation in either checker.

The classes now come from idClasses, a bounded walk of the IR's type graph
seeded with Document and each concrete TypeDef, applying the rule declaredID
applies to a value: a field named ID the struct declares itself, of a named
string type. A class with no map gets a registry whether or not this document
fills it, so the reference is resolved rather than skipped.

Truncation is the other direction. A registry derived from a walk that saw part
of the document answers "not declared" for a node it never reached, so an
operation buried past the cap made a valid reference to it report as dangling —
a false error, where a map-keyed registry can only under-report. Both checkers
now drop the declaration-derived classes when the walk truncates and leave
ir/walk-truncated to say why, as checkArgsOutsideGraphQL already does with
reachability it cannot trust.

WithDeclarations also replaces a declaration-derived registry it is handed
instead of writing into its set, so extending one call's result cannot mutate
it.

The PropID skip in checkDuplicateIDs stands for the case it names — a component
response materializes into each operation referencing it, carrying the one ID
#107 derives from the declaration — but it is wider than that reason: two
different properties minted at one PropID go unreported with the copies. That
is #280, and the comment now reads as the debt it is rather than a settled
carve-out.
@OmarAlJarrah
OmarAlJarrah merged commit f8efb61 into main Aug 6, 2026
1 check passed
@OmarAlJarrah
OmarAlJarrah deleted the fix/ir-reference-integrity-gaps branch August 6, 2026 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant