diff --git a/compilers/compile/compile_test.go b/compilers/compile/compile_test.go index 52084c4..2d06045 100644 --- a/compilers/compile/compile_test.go +++ b/compilers/compile/compile_test.go @@ -286,3 +286,125 @@ 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(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" + // 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 { + 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("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_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. +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("a_item") + }) + types.InternProvisional("/a/items", "t/anon/a/items", func() ir.TypeDef { + return named("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 4a10bdf..4f8c924 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,60 @@ 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). + // + // 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 // coordinate. // diff --git a/compilers/openapi/internal/lowering/lowering.go b/compilers/openapi/internal/lowering/lowering.go index 5445f60..c7f440e 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 dcba0fe..d19578f 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 dbaba28..aea3362 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 5c33879..f1d5b6f 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 f8208f3..0c62510 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