fix(compilers/openapi): name a node from its declaration - #380
Open
OmarAlJarrah wants to merge 1 commit into
Open
fix(compilers/openapi): name a node from its declaration#380OmarAlJarrah wants to merge 1 commit into
OmarAlJarrah wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacks on PR #375 (
fix/openapi-inline-position-hint, for #353), which is thisPR's base. Merge #375 first.
Summary
A
$refcan spell a pointer inside another declaration's body, and both loweringsreach that coordinate: the declaration through its own structure, the reference
through the pointer it names.
Interncallsbuildfor whichever arrives first, soa node's
Naming.Hintwas decided by which one that was.#353 closed this under
/components/schemasby teaching the reference's pointerwalk to replay what the structural lowering composes. That derivation is not total.
A position under
/pathstakes 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. 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.TypesgainsInternProvisional, which records that the name a node isbeing given is a placeholder, and
NameFromDeclaration, which replaces it. Onlythe name is a question the declaration answers better — the node itself is the
same one either way — so the reference still builds it.
lowering.CtxgainsNamingByReference(), a copy-constructor in the same shape asWithAuth.hoistSubSchemalowers under it, so the marking covers the wholesubtree the reference interns: a reference to an object body interns its children
too, and their names hang off the enclosing one.
schemaRefHomed, the entry point every schema positionflows 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
claimIDrefuses, and re-reporting it as a naming problem would namethe symptom instead of the cause.
Test plan
TestInlinePosition_UnderPathsIsNamedByItsDeclarationreplaces the test thatpinned this as a known gap. It now asserts
response_itemin all three cases —referenced with the paths block first, referenced with the components block first,
and not referenced at all.
compilers/compilefor the registry half: a placeholder replaced, aplaceholder 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_NamingByReferenceIsScopedToTheCopyholds what makes the flag safe tothread — it is set on a copy, so it cannot leak back to a caller still lowering a
declaration.
c = c.NamingByReference()failswith
expected "response_item", actual "items"; dropping theNameFromDeclarationcall inschemaRefHomedfails the same test.One thing I checked and deliberately did not do
#372 predicted this would subsume
branchPointerHintandstructuralPointerHint.I measured it: the entire suite passes with
structuralPointerHintremoved, sonothing 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.
branchPointerHintis still held byits 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 absolutecount is unstable across runs, see #369).
Closes #372