Skip to content

fix: derive naming hints neutrally and consistently - #354

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/naming-hint-neutral-and-consistent
Open

fix: derive naming hints neutrally and consistently#354
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/naming-hint-neutral-and-consistent

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

ir.Naming has two channels that carry a name for an emitter to render: Canonical for a name the
source declared, Hint for one derived from the position an entity occupies. Only Canonical was
held to anything.

What Hint is for. It is not a presentation affordance sitting outside invariant 4. A hoisted
anonymous type has no other name, so Hint is exactly what an emitter renders that type's
identifier from — the job Canonical does for a declared name — and ir-design.md §3.2 has always
spelled its example in neutral words (connection_domain). What makes it leak is that a hint is
nearly always derived from something a source did spell: a component key, an operationId, a
header name, a $ref target. Passing that context through put precisely the casing and punctuation
the IR rules out into the one channel nothing was checking. So Hint is held to the same content
rules as Canonical — uncased, a word sequence, segmented — and the compilers derive hints through
the grammar. It is not held to the derivation rule (ir/naming-not-derived), because it has no
Source beside it to be recomputed from; that is the whole reason the channel exists. §3.2 now
states this outright.

The two halves and the sweep:

  • compile.NamingHint and compile.SubHint run the hint through ir.CanonicalWords. That is the
    choke point — they are the only production code that fills Naming.Hint — so every derivation
    site is covered without each one remembering. SubHint neutralizes both halves, since either can
    arrive from a source spelling, and two neutral words joined by _ are neutral again, which is
    what lets a composed hint be the parent of the next one.
  • irverify.checkNaming runs the three content rules over Hint as well as Canonical, naming the
    channel in the message.
  • The architecture test that kept a second copy of the grammar from being written now sweeps both
    name channels, not just Canonical.
  • 19 of the 60 distinct hints in the committed goldens carried casing and 2 carried punctuation, so
    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 $ref naming that same branch pointer derives variant_<index>. Only the first
lowering 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, the
resolution #181 took at the sites it swept, so both paths agree whichever arrives first.

Two notes on scope:

Merge ordering. This regenerates every golden that contains a hint, so it should land after the
open PRs that touch compilers/openapi/internal/schema and 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) will
need their hint derivation to go through the framework when they rebase, the same way their
canonical derivation already does.

Test plan

  • ir/irverify: TestVerify_CasedOrPunctuatedHintIsAViolation drives the rule with five values the
    compilers really emitted into the goldens plus a letter/digit run; TestVerify_NeutralHintIsClean
    and TestVerify_HintIsNotDerivedFromTheSource hold the other direction and the boundary of what
    the hint channel is held to.
  • compilers/compile: TestNamingHint_NeutralizesTheContextItWasDerivedFrom,
    TestSubHint_NeutralizesBothHalves, and TestSubHint_IsItselfANeutralHint for the composition
    property the callers depend on.
  • compilers/openapi/internal/schema: TestNullCollapse_BranchHintIsOrderIndependent compiles both
    declaration 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.
  • The corpus reaches both fixes rather than only the unit tests. nullable-31-ref gains the collapse
    with an outside $ref at its branch pointer, which puts it under internal/harness's
    corpus-wide two-order oracle — that oracle compares hints with nothing excluded.
  • Every test was written first and confirmed red. Each fix was then reverted on its own with the
    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 irverify half reddens its unit tests; reverting the hint derivation reddens
    TestVerify_Corpus on 15 real specs, which is what shows the new rule reaches compiler output and
    not just fixtures. Planting ir.Naming{Hint: strings.ToLower(...)} in compilers/openapi/meta.go
    reddens the extended architecture sweep.
  • Deleting the new BranchRef component from nullable-31-ref.yaml reddens the conformance suite,
    so the regeneration is not a broken -update.
  • Full gate green: gofmt, go vet, golangci-lint (cache cleaned), go build,
    scripts/check-coverage.sh at 100%.

Closes #54
Closes #281

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.

openapi: the {X, null} collapse hints a branch pointer differently from a $ref to it irverify: cased Naming.Hint passes the neutrality check

1 participant