fix(compilers/openapi): agree on an inline position's hint - #375
Open
OmarAlJarrah wants to merge 1 commit into
Open
fix(compilers/openapi): agree on an inline position's hint#375OmarAlJarrah wants to merge 1 commit into
OmarAlJarrah wants to merge 1 commit into
Conversation
A node hoisted at an inline structural position -- items, additionalProperties, a patternProperties entry, a prefixItems slot -- took its Naming.Hint from whichever lowering interned it first. The structural lowering composes the enclosing node's hint with the position's role; hoistSubSchema, reached through an outside $ref naming that same pointer, fell through to the pointer's last segment. Both are valid hints and nothing compared them, so the same components compiled in two declaration orders produced two documents, silently. structuralPointerHint is branchPointerHint's counterpart for these four: it peels roles off the pointer's tail and rebuilds the composition from what is left. Under /components/schemas that reproduces the structural answer exactly, because the enclosing hint there is the enclosing pointer's own last segment -- a component's name, or a property's key -- and the walk replays the whole chain, so items under items composes as the lowering does. The structural spelling wins rather than the pointer one, which is the naming decision the agreement needed: the last segment named the node after the keyword holding it, the pattern text, or the slot ordinal, none of which distinguish it from the same position on any other schema. That is the direction #181 already chose in preferring variant_0 to 0. It is confined to /components/schemas because the 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. That leaves no order dependence -- 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. #372 holds that remainder and a test pins all three values. orderInvariantIR no longer excludes ir.Naming.Hint at all, which is what turns TestInlinePosition_OutsideRefDoesNotMoveTheHome into this fix's regression: it already permuted all four positions, and the exclusion was the only thing standing between it and this bug.
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.
Summary
A node hoisted at an inline structural position —
items,additionalProperties,a
patternPropertiesentry, aprefixItemsslot — took itsNaming.Hintfromwhichever lowering interned it first. The structural lowering composes the
enclosing node's hint with the position's role (
compile.SubHint(hint, "item"));hoistSubSchema, reached through an outside$refnaming that same pointer, fellthrough
subSchemaHintto the pointer's last segment. Both are valid hints andnothing compared them, so the same components compiled in two declaration orders
produced two different documents — silently, with no diagnostic on either side.
Measured on
main@dbf0054, compiling both orders through the CLI:$ref…/A/itemsA_itemitems…/A/additionalPropertiesA_valueadditionalProperties…/A/patternProperties/^xA_pattern^x…/A/prefixItems/0A_00…/A/items/itemsA_item_itemitemsstructuralPointerHintisbranchPointerHint's counterpart for these four: itpeels roles off the pointer's tail and rebuilds the composition from what is left.
Under
/components/schemasthat reproduces the structural answer exactly, becausethe enclosing hint there is the enclosing pointer's own last segment — a
component's name, or a property's key — and because it replays the whole chain
rather than one step,
itemsunderitemscomposes as the lowering does. The walkis bounded by construction: each step consumes at least one segment.
The naming decision
The structural spelling wins, not the pointer one. Agreement alone would have been
satisfied by both namers producing the weaker name, so this is the part that had to
be decided rather than derived: the last segment names the node after the keyword
holding it, the pattern text, or the slot ordinal, none of which distinguish the
position from the same position on any other schema. That is the direction #181
already took in preferring
variant_0to0.What is deliberately not closed
The derivation is confined to
/components/schemas, because it is not total. Aposition under
/pathstakes its enclosing hint from an operationId, a response, ora media-type key, and the pointer records none of them — the same
itemspositionis
response_itemto the structural lowering, with no pointer spelling thatreproduces it. Answering those with a pointer-derived name would replace one
disagreement with a different one.
Probing that case turned up something worth stating precisely: it is not an
order dependence. Components lower before paths, so a reference from one always
interns first, and both declaration orders give
items. What remains is a name thatdepends on whether an unrelated schema points at the position —
response_itemwithout the reference,
itemswith it. Filed as #372, stated in the code, andpinned by a test that asserts all three values.
Test plan
TestInlinePosition_HintIsTheSameInBothOrderspins the exact hint each of the sixshapes takes in both declaration orders — the four positions, one nested, and
one rooted at a property rather than at the component.
TestInlinePosition_UnderPathsTakesTheWeakerNamepins the residue above.orderInvariantIR()no longer excludesir.Naming.Hintat all. That exclusionwas, in the issue's words, "exactly what stands between that test and this bug", so
removing it is what turns the existing
TestInlinePosition_OutsideRefDoesNotMoveTheHome— which already permutes all four positions and diffs whole documents — into this
fix's regression. I first tried narrowing the exclusion to
/pathsnodes and foundthe suite passes without any exclusion, so none is kept.
structuralPointerHintcall fromsubSchemaHintfails all six pinned shapes (
expected "A_item", actual "items", and so on) andTestInlinePosition_OutsideRefDoesNotMoveTheHome, confirming both halves bite.No golden moved, which is itself the finding: no committed fixture has an outside
$refto an inline structural position, which is how this survived the corpus.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, filed as #369).
Closes #353