fix: derive naming hints neutrally and consistently - #354
Open
OmarAlJarrah wants to merge 1 commit into
Open
Conversation
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
ir.Naminghas two channels that carry a name for an emitter to render:Canonicalfor a name thesource declared,
Hintfor one derived from the position an entity occupies. OnlyCanonicalwasheld to anything.
What
Hintis for. It is not a presentation affordance sitting outside invariant 4. A hoistedanonymous type has no other name, so
Hintis exactly what an emitter renders that type'sidentifier from — the job
Canonicaldoes for a declared name — andir-design.md§3.2 has alwaysspelled its example in neutral words (
connection_domain). What makes it leak is that a hint isnearly always derived from something a source did spell: a component key, an
operationId, aheader name, a
$reftarget. Passing that context through put precisely the casing and punctuationthe IR rules out into the one channel nothing was checking. So
Hintis held to the same contentrules as
Canonical— uncased, a word sequence, segmented — and the compilers derive hints throughthe grammar. It is not held to the derivation rule (
ir/naming-not-derived), because it has noSourcebeside it to be recomputed from; that is the whole reason the channel exists. §3.2 nowstates this outright.
The two halves and the sweep:
compile.NamingHintandcompile.SubHintrun the hint throughir.CanonicalWords. That is thechoke point — they are the only production code that fills
Naming.Hint— so every derivationsite is covered without each one remembering.
SubHintneutralizes both halves, since either canarrive from a source spelling, and two neutral words joined by
_are neutral again, which iswhat lets a composed hint be the parent of the next one.
irverify.checkNamingruns the three content rules overHintas well asCanonical, naming thechannel in the message.
name channels, not just
Canonical.every golden containing one is regenerated. That is the bulk of the diff.
Separately, the
{X, null}collapse handed the surviving branch the enclosing schema's hint,while an outside
$refnaming that same branch pointer derivesvariant_<index>. Only the firstlowering to arrive interns the node, so the same components in two declaration orders compiled to
two different documents with no diagnostic on either side. It now asks
branchHint's question, theresolution #181 took at the sites it swept, so both paths agree whichever arrives first.
Two notes on scope:
/,{,}#42. That issue was path-derived hints containing/,{,}, and it isgenuinely closed —
grep -rhoE '"hint": *"[^"]*"' testdata/ | grep -E '[/{}]'was empty beforethis change as well as after. The two punctuated hints that remained came from a header name and a
dotted property key, a different origin. The two explicit
ir.CanonicalWordscalls openapi: operations without operationId get a Naming.Hint containing/,{,}#42 added atoperation and response naming become redundant once the framework does it, and are dropped rather
than undone.
positions —
items,additionalProperties, apatternPropertiesentry, aprefixItemsslot —still hint two ways for the same reason. They cannot take this repair: the structural spelling is
built out of the enclosing node's hint, which the pointer walk does not have, and deriving the
chain from the pointer is not even total (a position under
/paths/…takes its hint from anoperationId). Closing them is a naming decision rather than an agreement fix, which is whatcompilers/openapi: an inline union branch's name hint depends on declaration order #181's own text said.
orderInvariantIR()'sNaming.Hintexclusion therefore stays; its doccomment now names what is left and points at openapi: an inline structural position hints differently from a $ref to it #353 instead of describing a gap that has shrunk.
Naming.Aliasesis read by no rule (irverify: Naming.Aliases is held to no rule at all #317) and is out of scope;checkNamingsays so.Merge ordering. This regenerates every golden that contains a hint, so it should land after the
open PRs that touch
compilers/openapi/internal/schemaand the conformance corpus — #333, #345,#346, #348, #351, #342 — rather than making each of them rebase across the golden churn. It also
extends the architecture sweep to
Naming.Hint, so the GraphQL and Protobuf drafts (#20, #21) willneed their hint derivation to go through the framework when they rebase, the same way their
canonical derivation already does.
Test plan
ir/irverify:TestVerify_CasedOrPunctuatedHintIsAViolationdrives the rule with five values thecompilers really emitted into the goldens plus a letter/digit run;
TestVerify_NeutralHintIsCleanand
TestVerify_HintIsNotDerivedFromTheSourcehold the other direction and the boundary of whatthe hint channel is held to.
compilers/compile:TestNamingHint_NeutralizesTheContextItWasDerivedFrom,TestSubHint_NeutralizesBothHalves, andTestSubHint_IsItselfANeutralHintfor the compositionproperty the callers depend on.
compilers/openapi/internal/schema:TestNullCollapse_BranchHintIsOrderIndependentcompiles bothdeclaration orders and diffs the registries with hints included, and asserts the hint directly
in the order that was wrong (the collapse reaching the pointer first). Comparing through
orderInvariantIR()alone would pass on both orders of the bug.nullable-31-refgains the collapsewith an outside
$refat its branch pointer, which puts it underinternal/harness'scorpus-wide two-order oracle — that oracle compares hints with nothing excluded.
tests kept: reverting the collapse fix reddens the unit test, the conformance case and
TestHarness_InRepoCorpus(order-dependent … type registry depends on declaration order);reverting the
irverifyhalf reddens its unit tests; reverting the hint derivation reddensTestVerify_Corpuson 15 real specs, which is what shows the new rule reaches compiler output andnot just fixtures. Planting
ir.Naming{Hint: strings.ToLower(...)}incompilers/openapi/meta.goreddens the extended architecture sweep.
BranchRefcomponent fromnullable-31-ref.yamlreddens the conformance suite,so the regeneration is not a broken
-update.gofmt,go vet,golangci-lint(cache cleaned),go build,scripts/check-coverage.shat 100%.Closes #54
Closes #281