Repository navigation
pass: pin Validate's diagnostic order wherever it ranges a map #821
Description
Problem
pass.Validate sorts a map's keys before every check that ranges one, so diagnostic order does not follow Go's randomized map iteration (invariant 7). Nothing sorts the combined list afterwards, so each check's order is the order callers get, and engine merges these diagnostics into the document it persists.
Three of those sorts are pinned by a test: TestValidate_ServerIndexDiagnosticsAreDeterministic, TestValidate_ResponseIndexDiagnosticsAreDeterministic and TestValidate_EncodingKeyDiagnosticsAreDeterministic. The other four are not. At d06a55c, iterating the keys unsorted (slices.Collect(maps.Keys(m)) in place of the sorted list) at any one of these sites leaves go test -count=5 ./pass/ ./engine/ ./compilers/openapi/internal/schema/ green:
Function in pass |
Map ranged | Diagnostics whose order it sets |
|---|---|---|
forEachPayload |
doc.Messages |
ir/payload-required-outside-request and ir/encoding-key-unknown-property on message payloads |
liveTypeIDs |
doc.Types |
pass/discriminator-missing-variant, pass/discriminator-unknown-property, pass/duplicate-wire-name, pass/args-outside-graphql |
checkNilTypes |
doc.Types |
ir/nil-type |
checkMapping |
Discriminator.Mapping |
pass/discriminator-missing-variant within one discriminator |
No other check reaches these orderings. internal/harness compiles without running pass.Validate, so its determinism and order-invariance oracles never see a Validate diagnostic.
The sorts at these four sites are therefore held only by reading the code. A change that touches them, such as #806's rewrite of the key-sorting helper, can drop one without reddening anything.
Proposed change
Add a determinism test per site, in the shape of the existing three: plant five offending entries whose keys are out of order (z, b, q, c, m), require the sorted order on the first run, and assert that eight more runs repeat it.
liveTypeIDsserves three checks. One of them is enough to pin the helper, andpass/args-outside-graphqlwith fiveargModels is the smallest fixture.checkMappinglocates every diagnostic at the union, so assert the messages, which name the mapping key, rather than the locations.
This pins today's sites only. A check added later that ranges a map needs its own test, and nothing will notice one that lacks it.
Acceptance
- Each new test passes on
mainas is. - Iterating unsorted at any one of the four sites reddens that site's test. A throwaway probe built this way did so at d06a55c, one site per test.
Activity
OmarAlJarrah commentedon Oct 19, 2026
Re-run at
c387e25c, with the same results:forEachPayload,liveTypeIDs,checkNilTypesorcheckMapping, one at a time, leavesgo test -count=5 ./pass/ ./engine/ ./compilers/openapi/...green.main. ForliveTypeIDsandcheckMapping, a planted spec throughmorphic validatediffers in 10 of 10 runs.So a green suite here means "unpinned", not "unreachable".
test(pass): hold a discriminator mapping's reports to sorted order
OmarAlJarrah commentedon Oct 29, 2026
#1147 rewrites
checkMapping's sort, so it pins that site:TestValidate_MappingDiagnosticsAreDeterministicmaps five keys out of order onto no variant and requires their reports, which name the key, in key order across nine runs. With the sort removed it reddens. The other three sites,forEachPayload's message range,liveTypeIDsandcheckNilTypes, are unchanged by it and still unpinned.