Skip to content

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

Open
OmarAlJarrah wants to merge 1 commit into
fix/openapi-inline-position-hintfrom
fix/openapi-declaration-names-the-node
Open

fix(compilers/openapi): name a node from its declaration#380
OmarAlJarrah wants to merge 1 commit into
fix/openapi-inline-position-hintfrom
fix/openapi-declaration-names-the-node

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Stacks on PR #375 (fix/openapi-inline-position-hint, for #353), which is this
PR's base. Merge #375 first.

Summary

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 calls build for whichever arrives first, so
a 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:

# no reference to it
t/anon/paths/~1x/get/responses/200/content/application~1json/schema/items  hint=response_item

# with an unrelated component pointing at that pointer
t/anon/paths/~1x/get/responses/200/content/application~1json/schema/items  hint=items

Components lower before paths, so the reference always interned first there. The
document stayed deterministic; what moved was the name, decided by whether some
unrelated schema happened to point at the position.

The fix

A name belongs to the declaration that owns the coordinate, never to a reference
to it.
That is the rule, and it needs no agreement between two namers to hold.

  • compile.Types gains InternProvisional, which records that the name a node is
    being given is a placeholder, and NameFromDeclaration, which replaces it. 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.
  • lowering.Ctx gains NamingByReference(), a copy-constructor in the same shape as
    WithAuth. hoistSubSchema lowers under it, so the marking covers the whole
    subtree the reference interns: a reference to an object body interns its children
    too, and their names hang off the enclosing one.
  • The replacement runs in schemaRefHomed, the entry point every schema position
    flows through. It has to be there rather than only at intern, because a position
    whose node already exists resolves to it and returns before interning anything
    — marking the alias alone left the test still green, which is how I found it.

A second declaration at one coordinate is silent here rather than last-write-wins:
that is what claimID refuses, and re-reporting it as a naming problem would name
the symptom instead of the cause.

Test plan

  • TestInlinePosition_UnderPathsIsNamedByItsDeclaration replaces the test that
    pinned this as a known gap. It now asserts response_item in all three cases —
    referenced with the paths block first, referenced with the components block first,
    and not referenced at all.
  • Four tests in compilers/compile for the registry half: a placeholder replaced, a
    placeholder replaced only once, a declared name left alone when the declaration
    interned first, an uninterned coordinate ignored, and a refused intern leaving
    nothing to rename.
  • TestCtx_NamingByReferenceIsScopedToTheCopy holds what makes the flag safe to
    thread — it is set on a copy, so it cannot leak back to a caller still lowering a
    declaration.
  • Watched red, both halves separately: dropping c = c.NamingByReference() fails
    with expected "response_item", actual "items"; dropping the
    NameFromDeclaration call in schemaRefHomed fails the same test.
  • No golden moved.

One thing I checked and deliberately did not do

#372 predicted this would subsume branchPointerHint and structuralPointerHint.
I measured it: the entire suite passes with structuralPointerHint removed, so
nothing in the corpus needs it any more. I have kept it anyway. That measurement
bounds the corpus, not the input space — it shows no committed fixture reaches a
coordinate the declaration never walks, not that no document can — and the pointer
walk is what names such a node if one exists. branchPointerHint is still held by
its own unit test and predates this work (#181, #281), so removing it would settle a
recorded decision as a side effect of an unrelated change.

Full gate green: gofmt, go vet ./..., golangci-lint run (0 issues),
go build ./..., ./scripts/check-coverage.sh (100% of statements; the absolute
count is unstable across runs, see #369).

Closes #372

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant