fix(compilers/openapi): keep raw timestamp and binary spelling - #246
Merged
Conversation
The channel whose documented promise is verbatim preservation rewrote the two YAML scalars JSON has no type for, so a source could not be recovered from what reached the IR. A timestamp was rendered as the RFC 3339 instant it resolves to, giving `2021-1-1` a padding, a time and a zone the source never wrote. A !!binary was rendered as its decoded bytes in a JSON string, which lost the base64 spelling on every value and lost the data itself on any byte that is not valid UTF-8: `/w==` is 0xFF, and encoding/json substitutes U+FFFD for it, leaving nothing to tell it from a source that wrote U+FFFD. Both now keep the text the source wrote. The resolved form is derivable from the spelling and the spelling is not derivable from the resolved form, which is the same reasoning the ir.Value channel already applies to a timestamp; the two channels agreed on nothing here before. The decode stays as the tag check it has always been, so a scalar that does not satisfy the type it declares is still refused and what a source gets back for one is unchanged. Whether such a scalar should instead survive as text is a question about what the walk accepts rather than what it preserves, and is left open as #245. Both conformance fixtures that reach these sites were blind to this: no golden held a normalized timestamp and no !!binary reached the raw channel at all, so the corpus could not have caught the rewrite and cannot show the fix without new cases. Reverting the change reddens both. Closes #242
Three corrections to the raw-scalar work, found reviewing the branch as it will be merged. The corpus grouped 2021-01-01 10:20:30 under a comment calling it a spelling the old decode reproduced exactly, while rawDivergences two declarations above recorded it as a divergence — the old decode rewrote it to a T and a Z. The block-form !!binary sat under a header reserved for rows the old conversion refused, which it is not. Both rows now sit with the scalars they belong to, under a comment that says which kind is which and why the equivalence ones earn their place. Nesting had no unit row. Every raw site that holds a structure rather than a bare scalar reaches the walk that way, so a mapping whose members diverge is now recorded with both spellings; reverting the fix reddens it alongside the scalar rows. Also restores the fractional-seconds row dropped when that table was rewritten, and drops a stale count from a test doc that said "the two preconditions" after a third was added.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
ir.UnmodeledEntry.Valueis the channel whose documented promise is verbatimpreservation, and it rewrote the two YAML scalars JSON has no type for. Neither
source could be recovered from what reached the IR.
A timestamp was rendered as the RFC 3339 instant it resolves to, so
2021-1-1arrived as
"2021-01-01T00:00:00Z"— a padding, a time and a zone the sourcenever wrote. A
!!binarywas rendered as its decoded bytes in a JSON string,which lost the base64 spelling on every value and lost the data outright on any
byte that is not valid UTF-8:
/w==is0xFF,encoding/jsonsubstitutesU+FFFD for it, and nothing downstream can tell that from a source that really
wrote U+FFFD.
Every raw-preservation site reaches this through the one function,
RawFromNode,so all of them carried it. To read the list off the tree rather than trust a
recital of it:
which is
x-*extensions, the §4.7 validation-only keywords,openapi:allOf/<i>branch preservation,
openapi:items-after-prefix, andcontentSchema.Both scalars now keep the text the source wrote. The resolved form is derivable
from the spelling and the spelling is not derivable from the resolved form,
which is the reasoning the sibling
ir.Valuechannel already applies to atimestamp — the two channels disagreed for the same source scalar until now.
ir.UnmodeledEntry.Value's doc comment states the rule, since that comment iswhere a consumer learns what the field guarantees.
Deliberately out of scope
The decode stays, as the tag check it has always been: a scalar that does not
satisfy the type it declares (
!!timestamp notadate,!!binary "###") is stillrefused and still costs the caller the whole construct. Only the kept spelling
moved, so nothing about what the compiler accepts changed here.
That refusal is inherited rather than chosen — it exists because the conversion
used to decode these scalars to produce its output — and it is arguably wrong
now that nothing consumes the decoded value. That is a question about what the
walk accepts rather than what it preserves, so it is filed as #245 and the
code comment on the arm points there rather than settling it as a side effect.
Test plan
space-separated datetime, a written zone, an explicit
!!timestamp, flow andblock
!!binary, and the0xFFcase.losing the rows that now diverge.
rawDivergencesrecords each one with bothspellings — the new one because it is the preservation this change exists for,
the old one so a row that quietly stopped diverging cannot leave the oracle
asserting nothing. A companion test fails if a recorded row is not in the
corpus, so a divergence cannot be parked in the map and never evaluated.
Two timestamp spellings the old decode already reproduced exactly are kept as
equivalence rows, which is what keeps the divergence set tight.
timestamp and no
!!binaryreached the raw channel anywhere, so the corpuscould not have caught the rewrite — the fixture named for dates covered only
the
ir.Valuechannel and stayed green throughout. Both fixtures grew themissing sites: raw-preserved dates in a vendor extension and under the §4.7
carve-out, and
!!binaryextensions in flow and block form.which is how every raw site holding a structure rather than a bare scalar
reaches the walk.
new unit row, so the regenerated goldens are not vacuous.
assertYAMLTimestampScalarspreviously required zero diagnostics; it nowrequires that the only notice is the §4.7 carve-out's
info, so a warningfrom a dropped or degraded date still reddens it.
gofmtclean,go vetclean,golangci-lint0 issues,go build ./..., coverage 100% (4509/4509 statements).Closes #242