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

docs/architecture.md

@@ -106,10 +106,11 @@ provenance model, and IR are built for all eight from day one.
 Small, composable, order-explicit transformations that both the engine and users (via config)
 can enable:
 
-- **validate** — referential integrity (every `TypeRef` targets a registered type), discriminator
-  mappings point at actual variants, wire-name uniqueness within a model, binding completeness
-  (every operation parameter is bound exactly once per binding). Structural errors here are
-  fatal; style issues are warnings.
+- **validate** — referential integrity (every typed-ID reference resolves to something the document
+  declares: a `TypeRef` target against the type registry, an `OpID` against the operations the
+  service tree declares, and so on for every ID class), discriminator mappings point at actual
+  variants, wire-name uniqueness within a model, binding completeness (every operation parameter
+  is bound exactly once per binding). Structural errors here are fatal; style issues are warnings.
 - **link** — resolve cross-document references when multiple specs are parsed into one document
   (multi-service, spec-stitching).
 - **dedup** — structurally identical anonymous types are merged (by content hash), with ID

ir/irverify/doc.go

@@ -1,7 +1,7 @@
 // Package irverify checks a compiled ir.Document against the structural
-// invariants every compiler must uphold (stable IDs, no dangling references,
-// neutral naming, routable Unmodeled entries, in-range provenance). Its findings
-// are Violation values — our own compiler bugs, deliberately a separate channel
-// from ir.Diagnostic, which reports problems in the source spec. Verify is pure
-// and imports only ir.
+// invariants every compiler must uphold (stable IDs, no two nodes claiming one
+// identity, no dangling references, neutral naming, routable Unmodeled entries,
+// in-range provenance). Its findings are Violation values — our own compiler
+// bugs, deliberately a separate channel from ir.Diagnostic, which reports
+// problems in the source spec. Verify is pure and imports only ir.
 package irverify

ir/irverify/duplicates.go

@@ -0,0 +1,76 @@
+package irverify
+
+import (
+	"reflect"
+
+	"github.com/dexpace/morphic/ir"
+)
+
+// propIDType is the reflect.Type of the one ID class checkDuplicateIDs does not
+// hold to being declared once; see there for why.
+var propIDType = reflect.TypeFor[ir.PropID]()
+
+// identity is one declared ID together with the class it belongs to, which is
+// the pair that has to be unique: an OpID and a TypeID spelling the same string
+// name two different nodes.
+type identity struct {
+	class reflect.Type
+	id    string
+}
+
+// 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.
+//
+// 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
+// Service→OperationGroup tree and a service sits in a slice, so two of either
+// sharing an ID made every reference to it resolve to whichever the reader
+// reaches first, with nothing in the document saying which that is.
+//
+// A duplicate is a Violation rather than an ir.Diagnostic because an ID is
+// derived from the source pointer of the defining occurrence, so two nodes
+// sharing one means a compiler minted the same pointer twice — our bug, not
+// something a spec author wrote or can fix.
+//
+// ir.PropID is outside the claim, because a repeated PropID is usually not a
+// second declaration. A response declared once in components and referenced by
+// three operations materializes into all three — responses are embedded by value,
+// not interned — so the header property it declares appears at three paths under
+// the one ID its defining occurrence derives
+// (testdata/conformance/openapi/component-reuse.yaml). That the ID stays the
+// declaration's rather than the use site's is what #107 fixed, so the repeat is
+// invariant 3 holding, not breaking: the copies are one property and a lookup for
+// that ID is unambiguous.
+//
+// The skip is wider than that reason, and deliberately provisional rather than
+// settled: two genuinely *different* properties minted at one PropID go
+// unreported with them, which is the same defect this check exists for. Telling
+// the two apart needs a fingerprint of the node rather than its ID alone —
+// GitHub #280 carries it, and this comment is the debt until it lands.
+//
+// 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))
+	var vs []Violation
+	for _, d := range decls {
+		if d.Class == propIDType {
+			continue
+		}
+		key := identity{class: d.Class, id: d.ID}
+		at, taken := first[key]
+		if !taken {
+			first[key] = d.Path
+			continue
+		}
+		vs = append(vs, Violation{
+			Code:    "ir/duplicate-" + ir.RefNoun(d.Class) + "-id",
+			Message: "id " + d.ID + " is declared here and at " + at,
+			Path:    d.Path,
+		})
+	}
+	return vs, truncated
+}

ir/irverify/duplicates_test.go

@@ -0,0 +1,181 @@
+package irverify
+
+import (
+	"go/ast"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/dexpace/morphic/ir"
+)
+
+// duplicateViolations runs the duplicate check and drops the truncation flag,
+// which the cases below assert nothing about; TestWalkChecks_EachReportsTruncation
+// holds that half.
+func duplicateViolations(doc *ir.Document) []Violation {
+	vs, _ := checkDuplicateIDs(doc)
+	return vs
+}
+
+// docWithOperations wraps the given operations in one service and group.
+func docWithOperations(ops ...ir.Operation) *ir.Document {
+	return &ir.Document{Services: []ir.Service{{
+		ID:     "s/x",
+		Groups: []ir.OperationGroup{{Operations: ops}},
+	}}}
+}
+
+func TestCheckDuplicateIDs_TwoOperationsOnOneID(t *testing.T) {
+	doc := docWithOperations(ir.Operation{ID: "op/x"}, ir.Operation{ID: "op/x"})
+
+	got := duplicateViolations(doc)
+	require.Len(t, got, 1, "the first declaration stands and only the second is reported")
+	assert.Equal(t, "ir/duplicate-op-id", got[0].Code)
+	assert.Equal(t, "doc.Services[0].Groups[0].Operations[1]", got[0].Path)
+	assert.Contains(t, got[0].Message, "doc.Services[0].Groups[0].Operations[0]",
+		"the message names the declaration this one collides with")
+}
+
+func TestCheckDuplicateIDs_ThreeOperationsOnOneIDReportTwo(t *testing.T) {
+	doc := docWithOperations(
+		ir.Operation{ID: "op/x"}, ir.Operation{ID: "op/x"}, ir.Operation{ID: "op/x"})
+
+	got := duplicateViolations(doc)
+	require.Len(t, got, 2, "n nodes on one ID are n-1 violations, not n")
+	assert.Equal(t, "doc.Services[0].Groups[0].Operations[1]", got[0].Path)
+	assert.Equal(t, "doc.Services[0].Groups[0].Operations[2]", got[1].Path)
+}
+
+func TestCheckDuplicateIDs_TwoServicesOnOneID(t *testing.T) {
+	doc := &ir.Document{Services: []ir.Service{{ID: "s/x"}, {ID: "s/x"}}}
+
+	got := duplicateViolations(doc)
+	require.Len(t, got, 1)
+	assert.Equal(t, "ir/duplicate-service-id", got[0].Code)
+	assert.Equal(t, "doc.Services[1]", got[0].Path)
+}
+
+// TestCheckDuplicateIDs_TwoRegistryEntriesOnOneNodeID drives a map-keyed class.
+// Two entries cannot share a key, but they can hold nodes claiming one ID, and
+// then a reference to it resolves to whichever the reader reaches first.
+func TestCheckDuplicateIDs_TwoRegistryEntriesOnOneNodeID(t *testing.T) {
+	doc := &ir.Document{Types: ir.TypeRegistry{
+		"t/x/A": &ir.Any{TypeCommon: ir.TypeCommon{ID: "t/x/Clash"}},
+		"t/x/B": &ir.Any{TypeCommon: ir.TypeCommon{ID: "t/x/Clash"}},
+	}}
+
+	got := duplicateViolations(doc)
+	require.Len(t, got, 1)
+	assert.Equal(t, "ir/duplicate-type-id", got[0].Code)
+	assert.Contains(t, got[0].Message, "t/x/Clash")
+}
+
+// TestCheckDuplicateIDs_DistinctIDsAreClean is the other half of the proof: a
+// check that cannot stay silent is no better than one that cannot fire. The
+// operations differ only in their IDs, so nothing but the ID can be what the
+// check reads.
+func TestCheckDuplicateIDs_DistinctIDsAreClean(t *testing.T) {
+	assert.Empty(t, duplicateViolations(
+		docWithOperations(ir.Operation{ID: "op/x"}, ir.Operation{ID: "op/y"})))
+}
+
+// TestCheckDuplicateIDs_RepeatedPropIDIsClean pins the carve-out. A response
+// declared once in components and referenced by two operations materializes into
+// both — responses are embedded by value, not interned — so the header property
+// it declares appears at two paths under the one ID its defining occurrence
+// derives. The copies are the same property, and reporting them would fail every
+// document that reuses a component (see checkDuplicateIDs).
+func TestCheckDuplicateIDs_RepeatedPropIDIsClean(t *testing.T) {
+	header := ir.Property{ID: "p/x/Listed/headers/X-Rate", Name: ir.Naming{Source: "X-Rate"}}
+	shared := ir.Response{Name: ir.Naming{Source: "Listed"}, Headers: []ir.Property{header}}
+	doc := docWithOperations(
+		ir.Operation{ID: "op/a", Responses: []ir.Response{shared}},
+		ir.Operation{ID: "op/b", Responses: []ir.Response{shared}},
+	)
+
+	assert.Empty(t, duplicateViolations(doc))
+}
+
+// TestVerify_ReportsDuplicateIDs pins that Verify runs the check, not just the
+// test: an ambiguous identity has to reach a caller that only ever calls Verify.
+func TestVerify_ReportsDuplicateIDs(t *testing.T) {
+	doc := docWithOperations(ir.Operation{ID: "op/x"}, ir.Operation{ID: "op/x"})
+
+	codes := make([]string, 0, 4)
+	for _, v := range Verify(doc) {
+		codes = append(codes, v.Code)
+	}
+	assert.Contains(t, codes, "ir/duplicate-op-id")
+}
+
+// identityClasses classifies every named string type the ir package declares by
+// whether it is an identity — a class of ID that references resolve against —
+// and, where it is, by what resolves those references and what holds the class to
+// being declared once.
+//
+// Both checkers reach a reference by its Go type, so a class nothing resolves
+// against goes unchecked in silence rather than failing: that is how every OpID
+// and ServiceID reference in the IR went unchecked entirely (GitHub #50). Nothing
+// derives the classification. An ID-keyed map on Document is recognizable from
+// Document's own shape, but a class with no map is indistinguishable from a class
+// nobody has got to yet, and a named string type that is an enum is
+// indistinguishable from one that is an identity. This is where that judgement is
+// 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",
+	"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",
+	"OpID":      "identity, no map: ir.Registries.WithDeclarations resolves references against the operations the document declares, checkDuplicateIDs holds them unique",
+	"ServiceID": "identity, no map: resolved and held as OpID is, against the services the document declares",
+	"PropID":    "identity, model-scoped: pass.Validate resolves references (checkPropIDRefs, checkEncodingKeys); not yet held unique, because a component's property is copied into every position referencing it — provisional, GitHub #280, see checkDuplicateIDs",
+
+	"BigVal":          "arbitrary-precision decimal, not an identity",
+	"PrimKind":        "primitive leaf kind; ir.PrimTypeID derives an ID from it, but the kind is not one",
+	"TypeKind":        "sum-type tag on TypeDef, not an identity",
+	"ValueKind":       "sum-type tag on Value, not an identity",
+	"AuthKind":        "auth scheme class, not an identity",
+	"AdditionalMode":  "open/closed/constrained additional-property mode, not an identity",
+	"PresenceKind":    "wire-presence discipline, not an identity",
+	"Lifecycle":       "visibility lifecycle class; an alias of string, so no reflect.Type of its own",
+	"StreamingMode":   "streaming direction, not an identity",
+	"IdempotencyKind": "idempotency class, not an identity",
+	"PageStrategy":    "pagination mechanism, not an identity",
+	"HTTPLocation":    "HTTP wire location of a parameter, not an identity",
+	"MsgDirection":    "send/receive perspective, not an identity",
+	"UnmodeledReason": "why an Unmodeled entry was kept, not an identity",
+	"Severity":        "diagnostic severity, not an identity",
+}
+
+// TestIdentityClasses_AreAllClassified fails when ir declares a named string type
+// identityClasses does not account for. A new class of ID is invisible to both
+// checkers' reflection walks until something resolves it, and nothing else would
+// notice one going unresolved.
+func TestIdentityClasses_AreAllClassified(t *testing.T) {
+	t.Parallel()
+	found := declaredStringTypes(t)
+	require.NotEmpty(t, found, "the ir package must declare named string types")
+	for _, name := range found {
+		assert.Contains(t, identityClasses, name,
+			"ir.%s is a named string type identityClasses does not classify: "+
+				"say whether it is a class of ID (and resolve references to it) or not", name)
+	}
+	assert.Len(t, identityClasses, len(found),
+		"identityClasses classifies a type the ir package no longer declares")
+}
+
+// declaredStringTypes returns the name of every type the ir package declares as a
+// string, defined or aliased.
+func declaredStringTypes(t *testing.T) []string {
+	t.Helper()
+	var names []string
+	for name, expr := range typeDecls(t) {
+		ident, isIdent := expr.(*ast.Ident)
+		if isIdent && ident.Name == "string" {
+			names = append(names, name)
+		}
+	}
+	return names
+}

ir/irverify/irverify.go

@@ -53,6 +53,7 @@ func Verify(doc *ir.Document) []Violation {
 func walkChecks() []func(*ir.Document) ([]Violation, bool) {
 	return []func(*ir.Document) ([]Violation, bool){
 		checkReferentialIntegrity,
+		checkDuplicateIDs,
 		checkNaming,
 		checkRawPayloads,
 		checkProvenance,

ir/irverify/refs.go

@@ -44,14 +44,33 @@ func collectRefs(doc *ir.Document, regs ir.Registries) ([]refSite, bool) {
 // 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.
+// The registries Document declares maps for are not all of them. An ir.Operation
+// is declared in the Service→OperationGroup tree and an ir.Service in a slice, so
+// neither class has a map to resolve against and every OpID and ServiceID
+// reference resolved against nothing (GitHub #50); ir.Registries.WithDeclarations
+// supplies both from the identities the document's own nodes declare.
+//
+// Those two classes are dropped when the declaration walk truncates. A registry
+// derived from a walk that saw a subset of the document answers "not declared"
+// for a node it simply never reached, so a reference to a legitimate operation
+// buried past the cap would be reported as dangling — a false violation, where
+// the registries Document declares maps for can only ever under-report. The
+// ir/walk-truncated violation Verify folds this flag into says why nothing is
+// claimed for them.
+//
+// One ID class stays out. ir.PropID names a position inside its model rather than
+// a document-level identity, 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 — 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)
 	regs := ir.DocumentRegistries(doc)
+	if !declTruncated {
+		regs = regs.WithDeclarations(decls)
+	}
 	sites, truncated := collectRefs(doc, regs)
+	truncated = truncated || declTruncated
 	var vs []Violation
 	for _, s := range sites {
 		reg := regs[s.idType]