Skip to content

refactor(compilers/openapi): rehome reference-entry resolution - #220

Merged
OmarAlJarrah merged 1 commit into
mainfrom
refactor/resolve-referenced-entries
Aug 2, 2026
Merged

refactor(compilers/openapi): rehome reference-entry resolution#220
OmarAlJarrah merged 1 commit into
mainfrom
refactor/resolve-referenced-entries

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Stacked on #219, which is stacked on #218. Review those first; this PR's diff against #219 is the change under review.

Summary

The generic walk over speakeasy's Referenced* aliases — what a reference-or-inline entry stands for, and which declaration it resolves to — moves out of operations.go and into internal/resolve, as resolve.Object and resolve.ObjectAt.

Two reasons, and the second is why it happens now:

  1. It is reference resolution, and internal/resolve is the package whose whole job is answering what a $ref names. It was in operations.go only because path items were the first thing to need it.
  2. It blocks compilers/openapi: extract operations, parameters and content #175. auth.go resolves security schemes through it and does not move with the operation walk, so a package built around operations.go would end up imported by a file it imports. Pulling the helpers below both breaks that before it forms.

The move removes a dependency rather than adding one. resolveRefAt took a whole lowering.Ctx and read exactly one thing from it — c.RefScope(). ObjectAt takes that Scope directly, so internal/resolve still reaches nothing that lowers, and its allowlist gains only the references package the interface is written in terms of.

MaxRefChain is exported with them. Only a real compile can build a chain long enough to cross the bound — the resolution info each hop reads is built by speakeasy's resolver, not by any value a test can construct — so the test that holds the walk to that bound has to be able to say "one hop past it" without restating the number, which is the thing CLAUDE.md asks not to maintain by hand.

Two doc comments are corrected on the way, both stale before this PR: one named cycles.go, which no longer exists (that refusal lives in internal/scan), and one described the alternative as "one resolveX per aliased type", using a name the package has not had for some time.

Test plan

  • gofmt, go vet, golangci-lint run (0 issues), go build ./..., ./scripts/check-coverage.sh — all pass; 4188/4188 statements, including 100% of internal/resolve from its own tests.
  • The diagnostic stream is byte-identical to main over all 163 fixtures.
  • The tests moved with the code. The five ObjectAt behaviour tests now live in internal/resolve, renamed to follow the function. They drive whole documents through the compiler rather than calling ObjectAt directly, which is not a shortcut — a chain of component aliases only exists once the resolver has built each hop's resolution info — so they live in package resolve_test, the only place that may import the compiler.
  • The last uncovered statement got a test that says what it is for. Object's fallback from the inline object to the resolved one was reached only incidentally, by an unrelated auth fixture with a dangling $ref. It now has a direct test: an entry that is a $ref to a component the document does not declare has no object, and must answer nil rather than a zero value the caller would lower as though it were written.
  • No test was lost. Comparing top-level Test/Fuzz/Benchmark names before and after: five are renames, one (TestResolvers_NilInputs) split into the two things it was checking, and two are new. Net 623 → 625.
  • The AST sweep for discarded diagnostics reports 0 sites across 138 diagnostic-returning functions.

Notes

  • paramKey's half of the old TestResolvers_NilInputs stayed in compilers/openapi and got a name and a comment of its own: a nil parameter cannot key anything, and answering with the zero key would merge every such entry onto one another.

@OmarAlJarrah

Copy link
Copy Markdown
Member Author

Force-pushed after a deeper review pass.

TestObject_NilEntryIsNil did not hold the guard it was named for. Removing if ref == nil { return nil } from Object left it green: every speakeasy Referenced* alias tolerates a nil receiver, so all eight instantiations answer nil either way. The test pinned the behaviour, not the branch.

Nothing about the constraint promises that tolerance, and Object explicitly declines to rely on it two lines further down — the resolved-object fallback stays explicit "rather than coupling this compiler to that undocumented nil-tolerance". A fixture whose getters do dereference their receiver tells the two apart, and now does. Removing the guard fails it.

The other three tests in this PR were mutation-checked too: inventing an object for an unresolved reference, dropping the use-site pointer on a cross-document chain, and raising MaxRefChain are each caught.

@OmarAlJarrah
OmarAlJarrah force-pushed the refactor/internal-schema branch from cb7362d to 435ed4d Compare August 1, 2026 23:11
@OmarAlJarrah
OmarAlJarrah force-pushed the refactor/resolve-referenced-entries branch from aeeb25d to 1295c79 Compare August 1, 2026 23:11
@OmarAlJarrah
OmarAlJarrah force-pushed the refactor/internal-schema branch from 435ed4d to c3ba2fd Compare August 1, 2026 23:40
@OmarAlJarrah
OmarAlJarrah force-pushed the refactor/resolve-referenced-entries branch from 1295c79 to 2353f67 Compare August 1, 2026 23:40
@OmarAlJarrah
OmarAlJarrah force-pushed the refactor/internal-schema branch from c3ba2fd to 3262829 Compare August 1, 2026 23:44
@OmarAlJarrah
OmarAlJarrah force-pushed the refactor/resolve-referenced-entries branch from 2353f67 to 22f2234 Compare August 1, 2026 23:44
Base automatically changed from refactor/internal-schema to main August 1, 2026 23:50
The generic walk over speakeasy's Referenced* aliases — what a
reference-or-inline entry stands for, and which declaration it resolves
to — moves from operations.go into internal/resolve, as Object and
ObjectAt.

It is reference resolution, and internal/resolve is the package that
answers what a $ref names, so this is where it belonged already. Leaving
it in operations.go also blocks the operation extraction (#175): auth.go
resolves security schemes through it and stays behind, so a package built
around operations.go would be imported by a file it imports.

The move removes a dependency rather than adding one. ObjectAt read
exactly one thing from the lowering context — the reference scope — so it
now takes that scope directly, and this package still reaches nothing
that lowers.

The hop bound stays unexported. Only a real compile can build a chain
long enough to cross it, so the test that holds the walk to the bound has
to say "one hop past" rather than restate the number — it reaches the
constant through export_test.go, which keeps a value no production caller
reads out of the package's API.

Two doc comments are corrected on the way: one named cycles.go, which no
longer exists — that refusal lives in internal/scan — and one described
the alternative as "one resolveX per aliased type" using a name the
package no longer has.

Behaviour is unchanged: the compiler's diagnostic stream over all 163
fixtures is byte-identical to main.

Object's nil guard is held by a fixture of its own. Every speakeasy alias
tolerates a nil receiver, so the test listing all eight passes with or
without the guard — it pins the behaviour and not the branch. An entry
whose getters do dereference their receiver tells the two apart, which is
what the guard is for, since nothing about the constraint promises that
tolerance and the resolved-object fallback two lines down already
declines to rely on it.
@OmarAlJarrah
OmarAlJarrah force-pushed the refactor/resolve-referenced-entries branch from 22f2234 to 8671a77 Compare August 1, 2026 23:56
@OmarAlJarrah

Copy link
Copy Markdown
Member Author

Rebased onto the new main (the #219 squash), which clears the conflict — my branch was still sitting on the pre-squash commit. Also a deeper review pass, with one change.

MaxRefChain was exported for a test and nothing else. Counting consumers rather than trusting my own rationale: zero production reads, two test reads. My justification for exporting it — that the test must say "one hop past the bound" rather than restate 32, which CLAUDE.md forbids maintaining by hand — is still right, but exporting was the wrong way to get it. The constant is unexported again and reaches the external test through export_test.go, the standard idiom: resolve.MaxRefChain exists during go test and is absent from the package's API otherwise (go doc confirms). The bound test is unchanged and still fails when the bound is raised.

Everything else probed clean:

  • The cut is pure. operations.go and params.go differ by exactly the four removed declarations, eight call-site rewrites, one added import and one removed one — nothing else.
  • No over-export elsewhere. Scope, InternedID, IsRefSite, TargetSchema, Object and ObjectAt all have production consumers. Referenced has none, but it appears in Object's exported signature, so unexporting it would make that signature unreadable rather than smaller — it stays.
  • ObjectAt taking Scope is right, not merely sufficient. It uses the InternalPointer method, not just the self-path behind it, so narrowing the parameter to a string would mean re-implementing or exposing sameFile.
  • Every branch in ObjectAt is now held, not just covered: the nil guard, the resolved-object fallback, the terminator adopting the candidate, the advance along the chain, the candidate update, the cross-document break, and the bound. Three mutations had to be redone in compiling form first — a mutant that does not compile proves nothing.
  • 100% coverage of the package from its own tests; the diagnostic stream over all 163 fixtures is byte-identical to main; no stale references to the old names remain anywhere in the repo.

@OmarAlJarrah
OmarAlJarrah merged commit 0b9cea6 into main Aug 2, 2026
1 check passed
@OmarAlJarrah
OmarAlJarrah deleted the refactor/resolve-referenced-entries branch August 2, 2026 00:01
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