From ee795bbb84755305469f917aa02cc15e9ded7323 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 07:56:31 +0300 Subject: [PATCH 1/2] fix(compilers/openapi): name a node from its declaration A $ref can spell a pointer inside another declaration's body, and both lowerings reach that coordinate: the declaration through its own structure, the reference through the pointer it names. Intern builds the node for whichever arrives first, so the node's Naming.Hint was decided by which one that was. #353 closed this under /components/schemas by teaching the reference's pointer walk to replay what the structural lowering composes. That derivation is not total: a position under /paths takes its enclosing hint from an operationId, a response or a media-type key, and the pointer records none of them. Components lower before paths, so the reference always interned first there and the position was named "items" rather than "response_item" -- deterministically, but decided by whether some unrelated schema pointed at it. A name belongs to the declaration that owns the coordinate, never to a reference to it. hoistSubSchema now lowers under a context marking its names as placeholders, and every schema position replaces the placeholder at its own coordinate as it lowers. The marking covers the subtree rather than the referenced coordinate alone, since a reference to an object body interns its children too and their names hang off the enclosing one. The replacement sits at the schema entry point rather than only at intern because a position whose node already exists resolves to it and returns before interning anything -- which is why marking the alias alone was not enough. --- compilers/compile/compile_test.go | 87 +++++++++++++++++++ compilers/compile/types.go | 54 ++++++++++++ .../openapi/internal/lowering/lowering.go | 31 +++++++ .../internal/lowering/lowering_test.go | 17 ++++ compilers/openapi/internal/schema/hoist.go | 13 ++- compilers/openapi/internal/schema/resolve.go | 14 +++ .../openapi/internal/schema/schema_test.go | 34 ++++---- 7 files changed, 232 insertions(+), 18 deletions(-) diff --git a/compilers/compile/compile_test.go b/compilers/compile/compile_test.go index 52084c43..516e50b0 100644 --- a/compilers/compile/compile_test.go +++ b/compilers/compile/compile_test.go @@ -286,3 +286,90 @@ func TestTypes_RefusesANamespaceUsedBothWays(t *testing.T) { }) } } + +// named returns a model at id carrying hint as its only name, for the +// provisional-naming tests below. +func named(id ir.TypeID, hint string) ir.TypeDef { + return &ir.Model{TypeCommon: ir.TypeCommon{ID: id, Name: ir.Naming{Hint: hint}}} +} + +// provisionalPointer is the coordinate the provisional-naming tests below use: +// an inline position inside another declaration's body, which is the shape a +// reference can name and a declaration also owns. +const provisionalPointer = "/a/items" + +// hintAt reads the hint of the node interned at provisionalPointer. +func hintAt(t *testing.T, types *compile.Types) string { + t.Helper() + td, ok := types.NodeAt(provisionalPointer) + require.True(t, ok, "a node is interned at %s", provisionalPointer) + return td.Common().Name.Hint +} + +// TestTypes_DeclarationReplacesAProvisionalName is the property that makes a +// node's name independent of lowering order: a reference naming a coordinate +// inside another declaration's body builds the node, and the declaration names +// it whenever it arrives. +func TestTypes_DeclarationReplacesAProvisionalName(t *testing.T) { + t.Parallel() + types := compile.NewTypes(0) + + types.InternProvisional("/a/items", "t/anon/a/items", func() ir.TypeDef { + return named("t/anon/a/items", "items") + }) + assert.Equal(t, "items", hintAt(t, types), "the reference names it first") + + types.NameFromDeclaration("/a/items", "a_item") + assert.Equal(t, "a_item", hintAt(t, types), "the declaration replaces that name") + + // And only once: a placeholder that has been replaced is no longer one, so a + // second declaration at the same coordinate — which claimID is what refuses — + // cannot rename it from here. + types.NameFromDeclaration("/a/items", "something_else") + assert.Equal(t, "a_item", hintAt(t, types)) +} + +// TestTypes_NameFromDeclarationLeavesADeclaredNameAlone holds the other +// direction: a coordinate the declaration reached first carries no placeholder, +// so a reference arriving later cannot have marked it and the name stands. +func TestTypes_NameFromDeclarationLeavesADeclaredNameAlone(t *testing.T) { + t.Parallel() + types := compile.NewTypes(0) + + types.Intern("/a/items", "t/anon/a/items", func() ir.TypeDef { + return named("t/anon/a/items", "a_item") + }) + types.InternProvisional("/a/items", "t/anon/a/items", func() ir.TypeDef { + return named("t/anon/a/items", "items") + }) + assert.Equal(t, "a_item", hintAt(t, types), + "the second call interns nothing, so it names nothing") + + types.NameFromDeclaration("/a/items", "a_item") + assert.Equal(t, "a_item", hintAt(t, types)) +} + +// TestTypes_NameFromDeclarationIgnoresAnUninternedCoordinate covers the ordinary +// case: every declaration calls this at its own coordinate, and almost none of +// them are replacing anything. +func TestTypes_NameFromDeclarationIgnoresAnUninternedCoordinate(t *testing.T) { + t.Parallel() + types := compile.NewTypes(0) + require.NotPanics(t, func() { types.NameFromDeclaration("/nothing/here", "x") }) + assert.Empty(t, types.Violations()) +} + +// TestTypes_RefusedProvisionalInternIsNotNamed holds the guard on the marking: a +// build yielding nothing leaves no node, so there is nothing for a later +// declaration to rename and no coordinate left mapped. +func TestTypes_RefusedProvisionalInternIsNotNamed(t *testing.T) { + t.Parallel() + types := compile.NewTypes(0) + + types.InternProvisional("/a/items", "t/anon/a/items", func() ir.TypeDef { return nil }) + _, ok := types.NodeAt("/a/items") + require.False(t, ok, "a refused intern leaves the coordinate unmapped") + assert.NotEmpty(t, types.Violations()) + + require.NotPanics(t, func() { types.NameFromDeclaration("/a/items", "a_item") }) +} diff --git a/compilers/compile/types.go b/compilers/compile/types.go index 4a10bdf9..cf3210e7 100644 --- a/compilers/compile/types.go +++ b/compilers/compile/types.go @@ -25,6 +25,10 @@ type Types struct { // byID is byPointer read backwards, so a derivation that maps two distinct // coordinates onto one ID is caught rather than silently overwriting. byID map[ir.TypeID]string + // provisional holds the coordinates whose node was named by a lowering that + // reached them through a reference rather than through the declaration that + // owns them. See InternProvisional. + provisional map[string]bool } // refuse records why an entry was rejected. The registry declines to hold it @@ -54,6 +58,8 @@ func NewTypes(src int) *Types { src: src, spaces: make(map[Space]bool), byID: make(map[ir.TypeID]string), + + provisional: make(map[string]bool), } } @@ -144,6 +150,54 @@ func (t *Types) Intern(pointer string, id ir.TypeID, build func() ir.TypeDef) ir return id } +// InternProvisional is Intern for a lowering that reached pointer through a +// reference naming it rather than through the declaration that owns it, and +// records that the name the node is being given is a placeholder. +// +// A reference can name a coordinate inside another declaration's body, and both +// lowerings reach it: the declaration through its own structure, the reference +// through the pointer it spells. Intern calls build for whichever arrives first, +// so the node's name used to be decided by declaration order — silently, since +// either spelling is a valid name and nothing compared them. +// +// Only the *name* is a question the declaration answers better; the node itself +// is the same one either way. So the reference still builds it, and +// NameFromDeclaration replaces the name when the declaration arrives — in +// whichever order the two happen. +// +// A coordinate already interned is not marked: the declaration may have been +// there first, and a name it settled is not a placeholder. +func (t *Types) InternProvisional(pointer string, id ir.TypeID, build func() ir.TypeDef) ir.TypeID { + _, before := t.byPointer[pointer] + interned := t.Intern(pointer, id, build) + if _, after := t.byPointer[pointer]; after && !before { + t.provisional[pointer] = true + } + return interned +} + +// NameFromDeclaration gives the node at pointer the hint its declaration +// derives, replacing a placeholder a reference left there first. +// +// It is a no-op for a coordinate that is not carrying a placeholder, which is +// every coordinate the declaration reached first — there the name is already the +// one this would write. That is also what makes a second declaration at one +// coordinate silent here rather than last-write-wins: two declarations claiming +// one coordinate is what claimID refuses, and re-reporting it as a naming +// problem would name the symptom instead of the cause. +func (t *Types) NameFromDeclaration(pointer, hint string) { + if !t.provisional[pointer] { + return + } + delete(t.provisional, pointer) + // The coordinate resolves and its node is present: a coordinate is marked + // provisional only once Intern has recorded both, and Intern is what removes + // the pair again when a build yields nothing — so there is no state here in + // which one exists without the other, and a branch for one would be + // untestable (the reasoning NodeAt states). + t.reg[t.byPointer[pointer]].Common().Name.Hint = hint +} + // Register records td under id without associating it with any source // coordinate. // diff --git a/compilers/openapi/internal/lowering/lowering.go b/compilers/openapi/internal/lowering/lowering.go index 5445f606..c7f440ed 100644 --- a/compilers/openapi/internal/lowering/lowering.go +++ b/compilers/openapi/internal/lowering/lowering.go @@ -88,6 +88,12 @@ type Ctx struct { // holds the struct to. schemas map[string]bool + // namesByReference marks a lowering running under a $ref that named a + // coordinate, whose names are placeholders. Unexported and read through + // NamesByReference so it can only be set by NamingByReference, which is what + // keeps it scoped to the subtree that copy is threaded through. + namesByReference bool + // auth is the document's declared security schemes, keyed by the IDs a // requirement names. It is unexported, and read through a predicate rather // than handed back, for the reason schemas is. @@ -181,6 +187,31 @@ func (c Ctx) WithAuth(auth map[ir.AuthID]ir.AuthScheme) Ctx { return c } +// NamingByReference returns a copy of c marking everything lowered under it as +// reached through a reference naming a coordinate rather than through the +// declaration that owns it. +// +// A $ref can spell a pointer inside another declaration's body, and the node +// interned there is then named by whichever of the two lowerings arrives first. +// A name belongs to a declaration rather than to a reference to it, so a lowering +// running under this context names provisionally and the declaration replaces the +// name when it arrives (GitHub #372). +// +// It marks the whole subtree, not just the referenced coordinate: a reference to +// an object body interns its children too, and their names are derived from the +// enclosing one, so they are placeholders for the same reason. +// +// A copy, not a fresh context: everything below still needs the document, its +// identity and index, and the declared-name index. +func (c Ctx) NamingByReference() Ctx { + c.namesByReference = true + return c +} + +// NamesByReference reports whether names minted under c are placeholders a +// declaration replaces. See NamingByReference. +func (c Ctx) NamesByReference() bool { return c.namesByReference } + // declaredSchemaNames collects the names under components/schemas, or nil when // the document declares none. func declaredSchemaNames(doc *soa.OpenAPI) map[string]bool { diff --git a/compilers/openapi/internal/lowering/lowering_test.go b/compilers/openapi/internal/lowering/lowering_test.go index dcba0fe8..d19578fa 100644 --- a/compilers/openapi/internal/lowering/lowering_test.go +++ b/compilers/openapi/internal/lowering/lowering_test.go @@ -309,3 +309,20 @@ func TestProvenanceAt_NamesTheOverlayForThePositionsItIntroduced(t *testing.T) { c.DiagAt(ir.SeverityWarning, "x", "/info/description", "m").Provenance, "a diagnostic is stamped through the same question") } + +// TestCtx_NamingByReferenceIsScopedToTheCopy holds what makes the flag safe to +// thread: it is set on a copy, so a lowering that descends under a $ref cannot +// leak the marking back to the caller that is still lowering a declaration. +func TestCtx_NamingByReferenceIsScopedToTheCopy(t *testing.T) { + t.Parallel() + declaring := lowering.Ctx{} + assert.False(t, declaring.NamesByReference(), "a context names by declaration by default") + + referencing := declaring.NamingByReference() + assert.True(t, referencing.NamesByReference()) + assert.False(t, declaring.NamesByReference(), + "the caller's context is unchanged, so the marking cannot escape the subtree") + + assert.True(t, referencing.NamingByReference().NamesByReference(), + "and marking an already-marked context is a no-op rather than a toggle") +} diff --git a/compilers/openapi/internal/schema/hoist.go b/compilers/openapi/internal/schema/hoist.go index dbaba28a..aea3362d 100644 --- a/compilers/openapi/internal/schema/hoist.go +++ b/compilers/openapi/internal/schema/hoist.go @@ -42,7 +42,18 @@ func internNode(c lowering.Ctx, ts *compile.Types, pointer, hint string, build func(common ir.TypeCommon) ir.TypeDef, ) ir.TypeID { id := ids.ForPointer(pointer) - return ts.Intern(pointer, id, func() ir.TypeDef { return build(commonFor(c, id, pointer, hint)) }) + mint := func() ir.TypeDef { return build(commonFor(c, id, pointer, hint)) } + if c.NamesByReference() { + // A $ref named this coordinate, so the name is a placeholder until the + // declaration that owns it arrives (GitHub #372). + return ts.InternProvisional(pointer, id, mint) + } + interned := ts.Intern(pointer, id, mint) + // The declaration reaching its own coordinate. On a first visit this is the + // name the node was just built with; on a later one it is what replaces a + // placeholder a reference left here. + ts.NameFromDeclaration(pointer, hint) + return interned } // commonFor builds the TypeCommon shared by every hoisted node at pointer. A diff --git a/compilers/openapi/internal/schema/resolve.go b/compilers/openapi/internal/schema/resolve.go index 5c33879f..f1d5b6f2 100644 --- a/compilers/openapi/internal/schema/resolve.go +++ b/compilers/openapi/internal/schema/resolve.go @@ -46,6 +46,13 @@ func schemaRefHomed(c lowering.Ctx, ts *compile.Types, anchors *AnchorIndex, dep return ts.PrimRef(ir.PrimAny), []ir.Diagnostic{c.DiagAt(ir.SeverityError, diag.DegradedConstruct, pointer, "schema nesting exceeds %d; lowered as any", maxSchemaDepth)} } + if !c.NamesByReference() { + // This is the declaration reaching its own coordinate, so its hint is the + // node's name even when a $ref interned the node there first (GitHub #372). + // It sits here rather than only at intern because a position whose node + // already exists resolves to it and returns before interning anything. + ts.NameFromDeclaration(pointer, hint) + } if js == nil { return ts.PrimRef(ir.PrimAny), nil } @@ -184,6 +191,13 @@ func hoistSubSchema(c lowering.Ctx, ts *compile.Types, anchors *AnchorIndex, dep if s.Node == nil { return "", false, nil } + // Everything lowered from here is reached through a $ref that named this + // coordinate, not through the declaration that owns it, so the names minted + // below are placeholders the declaration replaces (GitHub #372). It covers the + // subtree rather than this coordinate alone: a reference to an object body + // interns its children too, and their names hang off this one. + c = c.NamingByReference() + hint := subSchemaHint(decl, pointer) ref, diags := Ref(c, ts, anchors, depth, decl, pointer, hint) if owned, ok := ts.Lookup(pointer); ok { diff --git a/compilers/openapi/internal/schema/schema_test.go b/compilers/openapi/internal/schema/schema_test.go index f8208f31..0c625104 100644 --- a/compilers/openapi/internal/schema/schema_test.go +++ b/compilers/openapi/internal/schema/schema_test.go @@ -2228,17 +2228,16 @@ func TestInlinePosition_HintIsTheSameInBothOrders(t *testing.T) { } } -// TestInlinePosition_UnderPathsTakesTheWeakerName pins what GitHub #353 did not -// close, so the remainder is a recorded state rather than a silent one. +// TestInlinePosition_UnderPathsIsNamedByItsDeclaration is what GitHub #353 could +// not reach and #372 closed. // // The enclosing hint under /paths comes from a response, an operationId or a -// media-type key, none of which the pointer records, so the pointer walk cannot -// replay it and the position keeps the last-segment fallback. What is left is -// not an order dependence — components lower before paths, so the reference -// interns first in either spelling of the document — but a name that depends on -// whether an unrelated schema points at the position: "response_item" without -// the reference, "items" with it. GitHub #372 holds it. -func TestInlinePosition_UnderPathsTakesTheWeakerName(t *testing.T) { +// media-type key, none of which the pointer records, so no pointer-derived +// spelling reproduces it and the reference could only offer the last segment. +// The name is now taken from the declaration rather than from whichever lowering +// interned the node first, so the position is "response_item" whether or not an +// unrelated schema points at it, and in either declaration order. +func TestInlinePosition_UnderPathsIsNamedByItsDeclaration(t *testing.T) { t.Parallel() const id = ir.TypeID("t/anon/paths/~1x/get/responses/200/content/application~1json/schema/items") const op = `paths: @@ -2259,18 +2258,19 @@ func TestInlinePosition_UnderPathsTakesTheWeakerName(t *testing.T) { refFirst, diags := parseFull(t, "openapi: 3.1.0\ninfo: {title: O, version: \"1.0.0\"}\n"+outsider+op) openapitest.RequireNoErrorDiags(t, diags) - assert.Equal(t, "items", ownerFirst.Types[id].Common().Name.Hint, - "the reference names it, whichever order the two blocks are written in") - assert.Equal(t, "items", refFirst.Types[id].Common().Name.Hint, - "so the document is deterministic; it is the name that is weak") + // The reference lowers first in both spellings — components lower before + // paths — so both of these used to read "items". + assert.Equal(t, "response_item", ownerFirst.Types[id].Common().Name.Hint, + "the declaration names it, whichever order the two blocks are written in") + assert.Equal(t, "response_item", refFirst.Types[id].Common().Name.Hint, + "including when the reference is the lowering that interned the node") - // Without the reference the structural lowering names it, which is what the - // two above are being compared against: an unrelated $ref elsewhere in the - // document is what costs the position its enclosing context. + // And the same name with nothing pointing at the position at all, which is + // the claim: an unrelated $ref elsewhere in the document costs it nothing. unreferenced, diags := parseFull(t, "openapi: 3.1.0\ninfo: {title: O, version: \"1.0.0\"}\n"+op) openapitest.RequireNoErrorDiags(t, diags) assert.Equal(t, "response_item", unreferenced.Types[id].Common().Name.Hint, - "the structural lowering composes the enclosing response's hint") + "which is what the two above are being held to") } // TestInlinePosition_OutsideRefDoesNotMoveTheHome is the regression for the From 035a94316714fa2c15263aaab05529f049845b43 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 13 Aug 2026 15:04:27 +0300 Subject: [PATCH 2/2] fix(compile): neutralize the hint a declaration replaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NameFromDeclaration wrote the caller's hint straight onto the node, where interning the same hint goes through NamingHint and neutralizes it. A coordinate a reference reached first therefore ended up holding "A" where one the declaration reached first held "a" — so the name depended on which lowering arrived, which is the dependence this path was added to remove. It is worse than a wrong spelling. Two documents differing only in the order they declare a component and a reference to one of its inline positions compiled to different IR, which invariant 7 forbids and the order-invariance oracle rejects: allof-oneof-cooccurrence in the corpus failed it, and TestOneOf_CoDeclaredDistributionIsOrderIndependent and TestComposition_BranchAliasIsOrderIndependent with it. Write the field the way interning writes it. The regression case asserts the two paths agree rather than asserting a literal, since what matters is that they match and not what the grammar produces; restoring the raw assignment reddens it, both order tests, and the corpus sweep. --- compilers/compile/compile_test.go | 47 +++++++++++++++++++++++++++---- compilers/compile/types.go | 8 +++++- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/compilers/compile/compile_test.go b/compilers/compile/compile_test.go index 516e50b0..2d060455 100644 --- a/compilers/compile/compile_test.go +++ b/compilers/compile/compile_test.go @@ -289,14 +289,19 @@ func TestTypes_RefusesANamespaceUsedBothWays(t *testing.T) { // named returns a model at id carrying hint as its only name, for the // provisional-naming tests below. -func named(id ir.TypeID, hint string) ir.TypeDef { - return &ir.Model{TypeCommon: ir.TypeCommon{ID: id, Name: ir.Naming{Hint: hint}}} +func named(hint string) ir.TypeDef { + return &ir.Model{TypeCommon: ir.TypeCommon{ID: provisionalID, Name: ir.Naming{Hint: hint}}} } // provisionalPointer is the coordinate the provisional-naming tests below use: // an inline position inside another declaration's body, which is the shape a // reference can name and a declaration also owns. -const provisionalPointer = "/a/items" +const ( + provisionalPointer = "/a/items" + // provisionalID is the ID that coordinate derives, spelled once so the helper + // below can mint a node without every caller repeating it. + provisionalID ir.TypeID = "t/anon/a/items" +) // hintAt reads the hint of the node interned at provisionalPointer. func hintAt(t *testing.T, types *compile.Types) string { @@ -315,7 +320,7 @@ func TestTypes_DeclarationReplacesAProvisionalName(t *testing.T) { types := compile.NewTypes(0) types.InternProvisional("/a/items", "t/anon/a/items", func() ir.TypeDef { - return named("t/anon/a/items", "items") + return named("items") }) assert.Equal(t, "items", hintAt(t, types), "the reference names it first") @@ -329,6 +334,36 @@ func TestTypes_DeclarationReplacesAProvisionalName(t *testing.T) { assert.Equal(t, "a_item", hintAt(t, types)) } +// TestTypes_NameFromDeclarationNeutralizesTheHint pins that a replacement writes +// the field the way interning writes it. NamingHint neutralizes on the way in, so +// a raw hint here would leave the node holding the caller's spelling — and the +// name would then depend on whether a reference reached the coordinate first, +// which is the dependence this path exists to remove. +// +// The two halves are asserted against each other rather than against a literal: +// what matters is that they agree, not what the grammar happens to produce. +func TestTypes_NameFromDeclarationNeutralizesTheHint(t *testing.T) { + t.Parallel() + const raw = "A" + + declaredFirst := compile.NewTypes(0) + declaredFirst.Intern(provisionalPointer, provisionalID, func() ir.TypeDef { + return &ir.Scalar{TypeCommon: ir.TypeCommon{ID: provisionalID, Name: compile.NamingHint(raw)}} + }) + interned := hintAt(t, declaredFirst) + + referencedFirst := compile.NewTypes(0) + referencedFirst.InternProvisional(provisionalPointer, provisionalID, func() ir.TypeDef { + return named("placeholder") + }) + referencedFirst.NameFromDeclaration(provisionalPointer, raw) + replaced := hintAt(t, referencedFirst) + + assert.Equal(t, interned, replaced, + "one hint must name the node the same whether it was interned or replaced") + assert.NotEqual(t, raw, replaced, "and the stored hint carries no casing of its own") +} + // TestTypes_NameFromDeclarationLeavesADeclaredNameAlone holds the other // direction: a coordinate the declaration reached first carries no placeholder, // so a reference arriving later cannot have marked it and the name stands. @@ -337,10 +372,10 @@ func TestTypes_NameFromDeclarationLeavesADeclaredNameAlone(t *testing.T) { types := compile.NewTypes(0) types.Intern("/a/items", "t/anon/a/items", func() ir.TypeDef { - return named("t/anon/a/items", "a_item") + return named("a_item") }) types.InternProvisional("/a/items", "t/anon/a/items", func() ir.TypeDef { - return named("t/anon/a/items", "items") + return named("items") }) assert.Equal(t, "a_item", hintAt(t, types), "the second call interns nothing, so it names nothing") diff --git a/compilers/compile/types.go b/compilers/compile/types.go index cf3210e7..4f8c924c 100644 --- a/compilers/compile/types.go +++ b/compilers/compile/types.go @@ -195,7 +195,13 @@ func (t *Types) NameFromDeclaration(pointer, hint string) { // the pair again when a build yields nothing — so there is no state here in // which one exists without the other, and a branch for one would be // untestable (the reasoning NodeAt states). - t.reg[t.byPointer[pointer]].Common().Name.Hint = hint + // + // Neutralized on the way in, because this writes the field NamingHint would + // have written and has to write it the same way. A raw hint here would leave + // the node holding the caller's spelling — "A" where interning the same hint + // gives "a" — so the name would depend on whether a reference got there + // first, which is the dependence this whole path exists to remove. + t.reg[t.byPointer[pointer]].Common().Name.Hint = neutralHint(hint) } // Register records td under id without associating it with any source