fix(compilers/openapi): agree on an inline position's hint

compilers/openapi/internal/schema/resolve.go

@@ -1,11 +1,14 @@
 package schema
 
 import (
+	"strings"
+
 	oas3 "github.com/speakeasy-api/openapi/jsonschema/oas3"
 
 	"github.com/dexpace/morphic/compilers/compile"
 	"github.com/dexpace/morphic/compilers/openapi/internal/annotation"
 	"github.com/dexpace/morphic/compilers/openapi/internal/diag"
+	"github.com/dexpace/morphic/compilers/openapi/internal/ids"
 	"github.com/dexpace/morphic/compilers/openapi/internal/lowering"
 	"github.com/dexpace/morphic/compilers/openapi/internal/resolve"
 	"github.com/dexpace/morphic/ir"
@@ -161,19 +164,26 @@ func hoistSubSchema(c lowering.Ctx, ts *compile.Types, anchors *AnchorIndex, dep
 
 // subSchemaHint names the node a $ref'd sub-schema pointer owns: the target it
 // aliases when the sub-schema is itself a $ref carrying siblings, the branch
-// hint when the pointer addresses a composition branch, the pointer's last
-// segment otherwise.
+// hint when the pointer addresses a composition branch, the structural hint when
+// it addresses an inline structural position, the pointer's last segment
+// otherwise.
 //
-// The first two cases exist because a composition branch can own the same
-// pointer and derives its hint that way (branchHint). Both lowerings reach the
-// pointer — the branch through its composition, this one through an outside $ref
-// naming it — and only the first to arrive interns the node, so a hint derived
+// Every case but the last exists because another lowering can own the same
+// pointer and derives its hint that way. Both lowerings reach the pointer — the
+// enclosing schema through its own body, this one through an outside $ref naming
+// it — and only the first to arrive interns the node, so a hint derived
 // differently here makes the document depend on declaration order.
 //
-// The branch case is the second half of that agreement. Falling through to the
-// last segment named an inline branch after its own ordinal — "0" — which is a
-// hint an emitter cannot build an identifier from, and which disagreed with the
-// composition's "variant_0" (GitHub #181).
+// The branch case is the second half of that agreement for a composition branch.
+// Falling through to the last segment named an inline branch after its own
+// ordinal — "0" — which is a hint an emitter cannot build an identifier from,
+// and which disagreed with the composition's "variant_0" (GitHub #181).
+//
+// The structural case is the same agreement for items, additionalProperties, a
+// patternProperties entry and a prefixItems slot (GitHub #353), where the last
+// segment named the node after the keyword that holds it — "items" — or after
+// the pattern text or the slot ordinal, none of which distinguish it from the
+// same position on any other schema.
 func subSchemaHint(decl *oas3.JSONSchema[oas3.Referenceable], pointer string) string {
 	if decl != nil && decl.IsReference() {
 		if name := refLastSegment(decl.GetRef().String()); name != "" {
@@ -183,9 +193,99 @@ func subSchemaHint(decl *oas3.JSONSchema[oas3.Referenceable], pointer string) st
 	if hint, ok := branchPointerHint(pointer); ok {
 		return hint
 	}
+	if hint, ok := structuralPointerHint(pointer); ok {
+		return hint
+	}
 	return refLastSegment(pointer)
 }
 
+// componentSchemasPrefix is the pointer root under which a structural position's
+// enclosing hint is the enclosing pointer's own last segment. See
+// structuralPointerHint for why the derivation is confined to it.
+const componentSchemasPrefix = "/components/schemas/"
+
+// structuralPointerHint returns the hint the inline structural position at
+// pointer takes, for a caller holding only the pointer, and whether the pointer
+// addresses one it can answer.
+//
+// It is branchPointerHint's counterpart for the four positions whose hint is
+// composed rather than positional: the structural lowering builds them as
+// compile.SubHint(enclosing, role), so answering requires the enclosing node's
+// hint, which a bare pointer walk does not carry. Under /components/schemas it
+// does: the enclosing hint there is the enclosing pointer's own last segment —
+// the component's name, or a property's key — so the composition can be replayed
+// by peeling roles off the tail and rebuilding from what is left.
+//
+// It is confined to that root because the derivation is not total, and the
+// remainder is a naming decision rather than a bug to paper over. 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: the same items position
+// is "response_item" to the structural lowering and has no pointer spelling that
+// reproduces it. Answering those with a pointer-derived name would replace one
+// disagreement with a different one, so they keep the last-segment fallback.
+// That leaves no order dependence there — components lower before paths, so a
+// reference from one always interns first — but it does leave a name that
+// depends on whether an unrelated schema points at the position. GitHub #372
+// holds that remainder.
+//
+// The walk is bounded by construction: each step consumes at least one segment
+// and the loop runs only while segments remain.
+func structuralPointerHint(pointer string) (string, bool) {
+	segments := strings.Split(pointer, "/")
+
+	var roles []string // innermost first
+	for len(segments) > 1 {
+		role, consumed, ok := structuralRole(segments)
+		if !ok {
+			break
+		}
+		roles = append(roles, role)
+		segments = segments[:len(segments)-consumed]
+	}
+	if len(roles) == 0 {
+		return "", false
+	}
+
+	enclosing := strings.Join(segments, "/")
+	if !strings.HasPrefix(enclosing, componentSchemasPrefix) {
+		return "", false
+	}
+
+	hint := ids.UnescapeSegment(segments[len(segments)-1])
+	for i := len(roles) - 1; i >= 0; i-- {
+		hint = compile.SubHint(hint, roles[i])
+	}
+	return hint, true
+}
+
+// structuralRole reports the role the structural lowering names the position at
+// the tail of segments by, and how many segments that position spells. The roles
+// are the suffixes the four compile.SubHint call sites pass, and a change to one
+// of them has to be made here too — TestInlinePosition_HintIsTheSameInBothOrders
+// is what fails when they drift.
+//
+// segments holds at least two entries: its only caller reads the tail of a
+// pointer, which always starts with the empty segment before the first token, and
+// stops looping once one segment is left.
+func structuralRole(segments []string) (role string, consumed int, ok bool) {
+	last := segments[len(segments)-1]
+	switch last {
+	case "items":
+		return "item", 1, true
+	case "additionalProperties":
+		return "value", 1, true
+	}
+	switch segments[len(segments)-2] {
+	case "patternProperties":
+		return "pattern", 2, true
+	case "prefixItems":
+		if isDecimalIndex(last) {
+			return last, 2, true
+		}
+	}
+	return "", 0, false
+}
+
 // refNullable reports whether a $ref usage admits null: the reference site or
 // its resolved target admits null in any spelling. The ref site must recompute
 // this because a target interned at its own ID (a model, a union) discards the

compilers/openapi/internal/schema/schema_test.go

@@ -2094,6 +2094,103 @@ func stolenPositions() []stolenPosition {
 	}
 }
 
+// TestInlinePosition_HintIsTheSameInBothOrders pins the spelling each inline
+// position takes, rather than only that the two orders agree (GitHub #353).
+//
+// Agreement alone is satisfied by both namers producing the weaker name, so the
+// table is what says which one won: the structural spelling, composed from the
+// enclosing node's hint and the position's role. The outside $ref used to name
+// these after the keyword holding them ("items"), the pattern text ("^x") or the
+// slot ordinal ("0") — none of which distinguish the position from the same
+// position on any other schema.
+func TestInlinePosition_HintIsTheSameInBothOrders(t *testing.T) {
+	t.Parallel()
+	for _, tc := range []struct {
+		name  string
+		owner string
+		id    ir.TypeID
+		hint  string
+	}{
+		{"items", "    A: {type: array, items: " + inlineProbeBody + "}\n",
+			"t/anon/components/schemas/A/items", "A_item"},
+		{"additionalProperties", "    A: {type: object, additionalProperties: " + inlineProbeBody + "}\n",
+			"t/anon/components/schemas/A/additionalProperties", "A_value"},
+		{"patternProperties", "    A: {type: object, patternProperties: {\"^x\": " + inlineProbeBody + "}}\n",
+			"t/anon/components/schemas/A/patternProperties/^x", "A_pattern"},
+		{"prefixItems", "    A: {type: array, prefixItems: [" + inlineProbeBody + "]}\n",
+			"t/anon/components/schemas/A/prefixItems/0", "A_0"},
+		// Nested, because the derivation replays the whole chain rather than one
+		// step: the outside $ref used to name this "items", losing both levels.
+		{"items under items", "    A: {type: array, items: {type: array, items: " + inlineProbeBody + "}}\n",
+			"t/anon/components/schemas/A/items/items", "A_item_item"},
+		// Rooted at a property rather than at the component, so the enclosing hint
+		// the walk rebuilds from is the property's key.
+		{"items under a property", "    A: {type: object, properties: {p: {type: array, items: " +
+			inlineProbeBody + "}}}\n",
+			"t/anon/components/schemas/A/properties/p/items", "p_item"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			pos := stolenPosition{name: tc.name, owner: tc.owner, id: tc.id}
+			for _, order := range []struct {
+				name     string
+				refFirst bool
+			}{{"owner declared first", false}, {"reference declared first", true}} {
+				doc, diags := parseFull(t, pos.spec(order.refFirst))
+				requireNoErrorDiags(t, diags)
+				td, ok := doc.Types[tc.id]
+				require.True(t, ok, "%s: the position owns a node at %s", order.name, tc.id)
+				assert.Equal(t, tc.hint, td.Common().Name.Hint, order.name)
+			}
+		})
+	}
+}
+
+// TestInlinePosition_UnderPathsTakesTheWeakerName pins what GitHub #353 did not
+// close, so the remainder is a recorded state rather than a silent one.
+//
+// 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) {
+	t.Parallel()
+	const id = ir.TypeID("t/anon/paths/~1x/get/responses/200/content/application~1json/schema/items")
+	const op = `paths:
+  /x:
+    get:
+      operationId: getX
+      responses:
+        "200":
+          description: ok
+          content:
+            application/json: {schema: {type: array, items: ` + inlineProbeBody + `}}
+`
+	const outsider = "components:\n  schemas:\n" +
+		"    Outsider: {$ref: '#/paths/~1x/get/responses/200/content/application~1json/schema/items'}\n"
+
+	ownerFirst, diags := parseFull(t, "openapi: 3.1.0\ninfo: {title: O, version: \"1.0.0\"}\n"+op+outsider)
+	requireNoErrorDiags(t, diags)
+	refFirst, diags := parseFull(t, "openapi: 3.1.0\ninfo: {title: O, version: \"1.0.0\"}\n"+outsider+op)
+	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")
+
+	// 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.
+	unreferenced, diags := parseFull(t, "openapi: 3.1.0\ninfo: {title: O, version: \"1.0.0\"}\n"+op)
+	requireNoErrorDiags(t, diags)
+	assert.Equal(t, "response_item", unreferenced.Types[id].Common().Name.Hint,
+		"the structural lowering composes the enclosing response's hint")
+}
+
 // TestInlinePosition_OutsideRefDoesNotMoveTheHome is the regression for the
 // second half of the pointer collision. A $ref naming an inline position hoists
 // that position's home before the position itself is reached, and the position
@@ -2131,20 +2228,20 @@ func TestInlinePosition_OutsideRefDoesNotMoveTheHome(t *testing.T) {
 	}
 }
 
-// orderInvariantIR compares two whole IR documents, minus the two fields that
-// differ by construction when the same components are declared in two orders.
+// orderInvariantIR compares two whole IR documents, minus what differs by
+// construction when the same components are declared in two orders.
 //
 // SourceInfo.Hash digests the source bytes, which are the thing being permuted.
-// Naming.Hint is a live gap: the hint a node hoisted at a pointer carries is
-// minted by whichever of the two namers reaches the pointer first — the
-// declaration's context ("A_item") or the reference's last pointer segment
-// ("items") — because intern keeps the first name it is given. That divergence
-// is naming only, and predates the annotation work: a $ref to an object-bodied
-// `items` shows it with no annotations involved at all. Everything else is
-// compared.
+//
+// Naming.Hint used to be excluded too, and no longer is. The hint a node hoisted
+// at a pointer carries is minted by whichever namer reaches the pointer first —
+// the enclosing declaration's context or an outside $ref's pointer walk —
+// because intern keeps the first name it is given, so the two had to agree and
+// did not. They now do at every position either can reach (GitHub #181, #281,
+// #353), which is what lets the field be compared: the tests permuting those
+// positions are the regression only while nothing here hides the difference.
 func orderInvariantIR() []cmp.Option {
 	return []cmp.Option{
-		cmpopts.IgnoreFields(ir.Naming{}, "Hint"),
 		cmpopts.IgnoreFields(ir.SourceInfo{}, "Hash"),
 	}
 }