fix(compilers/openapi): name a node from its declaration

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") })
+}

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.
 //

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 {

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")
+}

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

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 {

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