Uh oh!
There was an error while loading. Please reload this page.
refactor: remove duplicated code and scaffolding across the repository - #97
Merged
Conversation
The working tree accumulated several untracked directories that 'git add -A' would have staged: .claude/ holds seven cloned reference repositories, each of which git stages as a 160000 gitlink pointing at a commit that exists in no clone of this repo, breaking checkout for everyone else. .crush/ and .idea/ are similarly local-only, and out.json, logs.txt and pets.json are ad hoc run artifacts that land in the repo root. Ignore all of them. The .idea/ and .vscode/ entries were already present but commented out. Nothing is deleted from disk - out.json and pets.json were built from inputs that are not in the repo.
CLAUDE.md still declared that the repository 'contains only design documents under docs/ - there is no Go code, no go.mod, and no build/test tooling yet', and that the standard Go commands 'None of these work today'. Both have been false for several milestones: the ir package, the OpenAPI compiler, the validation pass, the engine and two CLIs all ship and are gated in CI. Point at README.md for the milestone table rather than maintaining a second copy. The architecture.md §3 package tree was also missing three packages that exist: ir/irverify, internal/harness and cmd/morphic-harness.
The gate ran 'go test ./...' as its own step and then ran check-coverage.sh, which runs the same full suite again with coverage instrumentation. The standalone step passed no flag the coverage run lacks, so it was pure duplicated wall-clock. Also pin golangci-lint-action to an exact release instead of the floating major tag, so an upstream release cannot turn the gate red with no change in this repo.
NewWithRegistry had no production callers. Its only purpose was to let tests reach newEngine's register-error branch, which required a whole internal test file to exist alongside the external one. NewWith(fronts ...compilers.Compiler) covers the same ground without handing the caller a mutable registry to retain, and it can be exercised from the external test package, so engine_internal_test.go goes away entirely. Registry.Formats likewise had no callers outside its own two tests, and was the only reason the compilers package imported sort. The engine's lookup-miss error does not enumerate formats and the CLI has one subcommand, so nothing needed it.
…ce table Three separate one-per-kind repetitions collapse: - Common() was defined eleven times, once per concrete TypeDef kind, each body identical. Promote it from the embedded *TypeCommon instead. The unexported typeDef() marker stays per-kind - moving that to the exported TypeCommon would let any embedder join the sum and break the seal. - 'does any diagnostic carry error severity' was written five times across three packages. Add ir.HasError and ir.FirstError; both are needed because several call sites use the predicate in expression position and cannot take a two-value return. - irverify spelled its four-registry list four times over (the ID-type vars, registryFor, resolves, and singular). One keyed table replaces them, so a reference class can no longer be dropped from the oracle by someone updating three of the four lists. The fields are keyed rather than positional because two adjacent string fields would let a future row silently emit 'ir/dangling-types-ref'. The resolves accessor keeps its nil guard: Verify is documented as a report-only oracle that never crashes on a malformed document. irverify's corpus test is deleted - internal/harness already applies the identical oracle to a strict superset of the same specs. irtest now spells the golden encoding rule once, so the writer and the comparer cannot disagree.
lowerSecurityScheme and lowerMeta both appended their extension diagnostics inside the same 'if len(ext) > 0' guard that gated the assignment. That guard is false in exactly the case the diagnostic exists to describe: when every x-* extension on the node fails to serialize, ext comes back empty and the warning was dropped silently. A scheme carrying a single unserializable extension compiled clean. Route both through a lowerer.extensions helper that records the diagnostic unconditionally, and fold the four other copies of the same idiom onto it. The rest of this change is mechanical deduplication of the lowering path with no behavior change: - Eight resolveX reference unwrappers had byte-identical bodies because all eight argument types are aliases of one Reference[T,V,C]. One generic resolveRef replaces them. - The 'adopt if unset' blocks in mergeConstraints and reconcileProperty become cmp.Or, except the Min/Max pairs that co-adopt an exclusivity flag and Examples, whose emptiness test is len() on a non-comparable slice. Note that this folds thirty tracked coverage obligations out of existence, which leaves the two TestMergeConstraints tests as the only thing pinning per-keyword adoption. - The four single-caller conflict predicates fold into their detail-returning twins; lowerDiscriminator and modelDiscriminator merge; internNode becomes the single hoisting entry point; variantRef is deleted as lowerTyped with a TypeRef wrapper around it. - cycles.go's unescapePointer was a byte-identical copy of ids.go's unescapeSegment, in a file that already owns pointer construction and its inverse. loaded.Format was assigned and never read. Eight nolint:unused directives deferring to 'later compiler files' outlived those files.
Three spec fixtures were defined twice each across six test files under identical content but different names (brokenSpec/badSpec, tinySpec/tinySpec, minimalSpec/okSpec). Hoist the three constants into internal/testspec. The package holds constants only, deliberately: a statement-bearing package with no tests reports 0.0% and fails the per-package coverage gate, while a const-only package registers as having no test files and is skipped. The four writeSpec helpers are left where they are - hoisting those made fifteen call sites more verbose to save five lines. pass's four count-then-assert blocks share one countCode helper.
componentSpec landed in an earlier change but the tests written before it kept hand-writing the five-line YAML preamble it emits - thirty-six of them, byte for byte. assertHasCode sat in cycles_test.go with three callers while roughly twenty-five other sites hand-rolled the same scan loop. The helpers were never retrofitted to the call sites that predated them. Move the diagnostic assertions into helpers_test.go alongside a pathsSpec companion to componentSpec, add firstOp, indexBy and propsByWire, and migrate the call sites. Only the purely existential and purely counting assertions are converted. Thirteen sites assert a property of every match, or AND a message substring with a code, and an existential helper would silently weaken them. fuzz_test.go's predicate is a fuzz-target skip gate rather than an assertion and keeps its call-site semantics. The petstore spec existed both as a 104-line Go constant and as the on-disk golden input, byte-identical, with nothing forcing them to stay in step: editing the fixture and regenerating left the end-to-end test asserting against a stale copy. It now reads the file the golden already uses. TestMappingPairs_Cases becomes a table like its five siblings, keeping the three subtests whose assertions the shared one would erase and moving their rationale into the doc comment.
The package carried 138 test functions of the form TestX_ZeroValueShape and TestX_PopulatedRoundTrip whose bodies were a single helper call each - the fixture was the only content and the wrapper cost four to six lines around it. One assertJSONContract runs both halves as subtests, so the failure isolation that assertRoundTrip's require.NoError provides today is preserved per half. Fifty-seven of the sixty-three paired types merge. Six do not: Idempotency, StatusRange, TemplateArg, VersionedBool and Visibility have table-driven round-trips over several fixtures that a single populated value would quietly drop - Visibility's three-state table is what pins optionality against nullability - and Operation merges to sixty-three lines against a seventy-line cap. The eleven TypeDef kinds round-trip through a pointer and stay as they are. Also: the eight constant-spelling tests share one generic helper, threading the empty-state name as a parameter rather than assuming 'unspecified', which contradicts IdempotencyKind's own documented spelling. The eleven closures in the Common aliasing test become a loop over the kind list, reading the mutation back through json.Marshal - asserting through Common() instead would pass against a shared-singleton implementation, since it reads back through the method it wrote through. Four tests that hand-rolled the round-trip and determinism helpers now use them. The map-determinism tests gain the five cases that were missing: Message.Bindings, Content.ItemEncoding, and Document's Channels, Messages and Auth registries.
…emptyConstraints drift
Three gaps from the simplification audit's unverified leads:
- emptyConstraints was missing Precision, Scale, and PatternMessage from its
field check. These are scalar constraints that constraintsFromSchema could
populate but weren't guarded against, so adding any of them would silently
leak a non-nil *Constraints when it should be nil.
- pickBool plus three nil-safe accessors (schemaReadOnly, schemaWriteOnly,
schemaDeprecated) followed an identical shape across exactly three call
sites. Replaced with one pickFlag(ref, tgt, accessor) that captures the
nil-safe "use-site overrides referent" pattern once.
- 17 of 19 provenance-stamping sites hand-wrote the same
append+diagf+Provenance{Source: l.srcIndex} triple. Extracted into an
l.diag method so the stamping rule is stated once. The two remaining raw
sites are correct exceptions: operations.go uses op.Provenance (not
l.srcIndex), and preserveRaw now delegates through l.diag itself.
💘 Generated with Crush
Assisted-by: Crush:qwen3.7-max
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 maintenance sweep across the whole repository, removing duplicated code and
scaffolding that had accumulated as helpers landed after the call sites that
should have used them. Net −965 lines (1,788 added, 2,753 removed) across 73
files, with one real bug fixed along the way.
No IR field name, struct shape, JSON tag, diagnostic code, or golden file
changes. The pipeline's observable behavior is unchanged apart from the
diagnostic fix below.
The one behavior change
lowerSecuritySchemeandlowerMetaappended their extension diagnostics insidethe same
if len(ext) > 0guard that gated the assignment. That guard is falsein exactly the case the diagnostic describes: when every
x-*extension on anode fails to serialize,
extcomes back empty and the warning was droppedsilently, so a security scheme carrying a single unserializable extension
compiled clean. Both sites now route through a
lowerer.extensionshelper thatrecords the diagnostic unconditionally. Regression tests cover both.
What drove the duplication
Four patterns accounted for most of it:
componentSpecwas added in an earlier change; 36 tests still hand-wrote thebyte-identical preamble it emits.
assertHasCodehad three callers while ~25sites hand-rolled the same scan loop.
ir/carried 138 test functions whose bodieswere a single helper call, the fixture being the only content.
resolveXunwrappers were identical because all eight argument types arealiases of one
Reference[T,V,C].Common()was written eleven times.irverifyspelled its four-registry list four separate times.NewWithRegistryandRegistry.Formatshad no production callers; eightnolint:unuseddirectivesdeferred to "later compiler files" that had since arrived.
Deliberately not done
Several tempting consolidations were costed and rejected because they make the
code worse or do not actually save anything:
pass/validate.go's typed walker intoirverify's reflectionwalk. They are separate channels by design —
ir.Diagnosticreports problemsin the source spec,
irverify.Violationreports our own compiler bugs.Retiring the typed walker also forces
graphqlReachableTypesonto a collectorthat widens reachability to every
TypeID, and sinceProperty.Args[].Typeisitself a
TypeRef, the fieldcheckArgsOutsideGraphQLpolices becomes an edgethat legalizes what it reaches. The line count inverts to positive.
MarshalJSONadapters. They arefour-line adapters over one
marshalWithKind, and the recursion-avoiding typealias they depend on is not expressible inside a generic function.
lines, and it only fits the function-size cap by dropping the per-case
rationale comments.
cycles.go. 452 code lines across 36 functions; the remaining ~290 are therationale for the cycle-detection work and are load-bearing. The pre-parse gate
is permanent — it guards against stack exhaustion, which
recovercannotcatch, so it must run before unmarshalling rather than around it.
Repository hygiene
git add -Apreviously staged seven cloned reference repositories under.claude/as gitlinks pointing at commits that exist in no clone of this repo,which breaks checkout for anyone else, plus a 91 MB scratch file. Those are now
ignored; nothing was deleted from disk.
CLAUDE.mdalso still declared that therepository contained no Go code and no build tooling.
Test plan
gofmt -l .— cleango vet ./...— cleangolangci-lint run— 0 issuesgo test ./...— all 12 packages passscripts/check-coverage.sh— total 100.0%, every package at 100.0%-updaterun,git statusclean undertestdata/meta.goto theguarded form fails the new test, restoring it passes.