Skip to content

refactor(compilers/openapi): extract the lowering context - #218

Merged
OmarAlJarrah merged 1 commit into
mainfrom
refactor/lowering-context
Aug 1, 2026
Merged

refactor(compilers/openapi): extract the lowering context#218
OmarAlJarrah merged 1 commit into
mainfrom
refactor/lowering-context

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

The immutable context every lowering reads moves out of compilers/openapi into compilers/openapi/internal/lowering, as an exported Ctx. This is the prerequisite for #176: the four files that walk schemas reference the context 83 times, and it was declared in a file that stays behind, so extracting them without moving it first would have required the new package to import its own importer.

Two couplings surfaced the moment the type moved. Both are resolved here rather than carried forward.

Ctx held the compiler's public Options. Moving it would have dragged a published type into an internal package for the sake of the one field lowering actually reads — Grouping, at a single site. GroupingStrategy and its constants move down to lowering instead, and options.go names them rather than restating them:

type GroupingStrategy = lowering.GroupingStrategy
const GroupByTags = lowering.GroupByTags

openapi.GroupingStrategy, openapi.GroupByTags and openapi.GroupByPathPrefix keep their exact public spelling and their own documentation, while there stays exactly one declaration of the vocabulary. The alternative considered was a second enum in lowering with a projection function, mirroring how loadOptions projects onto load.Options. That precedent fits a bool and not an open-ended enum: two declarations of one strategy set can drift, and a strategy only the public half knew about would fall through to the default with nothing reporting it. Options itself, whose shape ir-design §10 fixes, stays where it is.

The constructor took the loader's Document. lowering.New now takes (srcIndex, *soa.OpenAPI, ir.SourceInfo, GroupingStrategy), so this package reaches internal/load for the version grammar alone. That is also what lets a test build a context without performing a load — which is what resolves the six internal tests that reached the unexported schemas field directly. Each now says what it means through a document that declares those components (docDeclaring), and the empty-name case gets a context of its own instead of mutating a shared one.

Along the way the source index the loader and the lowering must agree on is named, as rootSrcIndex, rather than written as a bare 0 at each site.

Test plan

  • gofmt, go vet, golangci-lint run (0 issues), go build ./..., ./scripts/check-coverage.sh — all pass; coverage 4188/4188 statements, including 100% of the new package from its own tests.
  • The diagnostic stream is byte-identical to main over all 163 fixtures in testdata/, compared field by field (severity / code / source / pointer / message, in order). Goldens cannot see a diagnostic that moved, was added or was dropped, because none of that changes the IR.
  • The bulk of the diff is proven mechanical, not eyeballed. Applying the same rename to main's copy of each of the ten converted files and diffing against this branch leaves exactly two intentional changes: c.Opts.Groupingc.Grouping, and one comment that named the old constructor.
  • Both new seams were mutation-tested. Dropping the grouping policy in the projection reddens TestParse_ExplicitOptions and TestGrouping_ByPathPrefixInferred; disagreeing with the loader about the source index reddens the goldens and the conformance corpus.
  • The new archtest entry was proven to reach, by planting a loweringengine import and watching TestImportGraph_LayeringHolds fail on it.
  • An AST sweep for diagnostic-returning calls whose diagnostics are discarded reports 0 sites across 138 such functions; the sweep itself was verified by planting a dropped return and watching it get reported.

Notes

  • Not a breaking change: GroupingStrategy becoming an alias is source-compatible, and Options is untouched.
  • docDeclaring now exists in both helpers_test.go and the new package's tests. Duplicating an eight-line test helper is the cost of the package boundary; there is no production API for "pretend this component is declared", which is the point.

@OmarAlJarrah

Copy link
Copy Markdown
Member Author

Force-pushed after a deeper review pass. Two real gaps, both found by mutation rather than by reading, plus two comment fixes. The stack was rebased onto this and every branch re-verified.

1. TestWithAuth_ExtendsACopy did not hold the copy claim. Replacing the method with

func (c Ctx) WithAuth(auth map[ir.AuthID]ir.AuthScheme) Ctx { return Ctx{auth: auth} }

— discarding the document, its identity and index, and the declared-name index derived at entry — passed the whole suite. All three of the test's assertions were about DeclaresAuth, so a WithAuth that built a fresh context around the map satisfied every one of them while handing the service walk a context with nothing in it. The test now pins what survives, including DeclaresSchema, which no field comparison would reach because the index is unexported. The mutation now fails on three assertions.

2. ExclusiveBoundIsBoolean panicked on a context with no document, while the two accessors beside it answer on one — and half this package's tests drive it from a Ctx literal. It reads the version through GetOpenAPI() now, which is nil-safe and is what the compiler already uses at 9 of its 11 reads of c.Doc; the direct field read was the outlier. There is a test for it, matching the two the sibling accessors already have. Reverting to the field read makes that test panic.

Neither changes behaviour for any document: the stream over all 163 fixtures is still byte-identical to main, and coverage is unchanged at 4188/4188 (GetOpenAPI() is the same statement).

Also: Ctx.Grouping now documents that it arrives as the caller wrote it — normalized or not — and that an unrecognized strategy groups by tags, which is what makes the unnormalized zero value harmless rather than a second spelling of the default. And WithAuth's "the phases above it" is now "the phases that run before it", which is what was meant.

Considered and declined: reordering New's parameters to match the struct's field order (doc, src, srcIndex, grouping). The current order puts the source index early, which matches load.Load(ctx, srcIndex, src, opts) — there are two conventions here and this follows one of them, so it is a preference rather than a defect, and it would have rippled through every branch above for no gain.

The immutable context every lowering reads moves to
compilers/openapi/internal/lowering as an exported Ctx. It has to move
before the schema walk can become a package of its own: the four files
that walk schemas reference it 83 times, and it was declared in a file
that stays, so an extraction would have needed the new package to import
its own importer.

Two couplings surfaced with it, both resolved here rather than carried:

Ctx held the compiler's public Options, which would have dragged a
published type into an internal package for the one field lowering reads.
GroupingStrategy moves down to lowering instead, and options.go names it
rather than restating it, so openapi.GroupingStrategy, GroupByTags and
GroupByPathPrefix keep their exact spelling and their documentation while
there stays exactly one declaration of the vocabulary. Options itself,
whose shape ir-design §10 fixes, stays where it is.

The constructor took the loader's Document for two of its fields. It now
takes the document and its identity, so lowering reaches load for the
version grammar alone, and a test can build a context without a load —
which is what unblocks the six internal tests that reached the unexported
schemas field. Each now says what it means through a document that
declares those components, and the empty-name case gets a context of its
own rather than mutating a shared one.

The source index the loader and the lowering must agree on is named at
last, as rootSrcIndex, rather than written as a bare 0 at each site.

Behaviour is unchanged: the compiler's diagnostic stream over all 163
fixtures is byte-identical to main, and the goldens are untouched.
@OmarAlJarrah
OmarAlJarrah force-pushed the refactor/lowering-context branch from d20e419 to ad701ae Compare August 1, 2026 23:04
@OmarAlJarrah
OmarAlJarrah merged commit 81ac131 into main Aug 1, 2026
1 check passed
@OmarAlJarrah
OmarAlJarrah deleted the refactor/lowering-context branch August 1, 2026 23:31
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