fix(internal/harness): exclude permutations that change meaning - #243
Open
OmarAlJarrah wants to merge 1 commit into
Open
fix(internal/harness): exclude permutations that change meaning#243OmarAlJarrah wants to merge 1 commit into
OmarAlJarrah wants to merge 1 commit into
Conversation
The two-order oracle compiles a source as written and again with every
mapping reversed, and reports what the permutation changed. That rests on
the rewrite being meaning-preserving. reverseMappings documented one case
where it is not — a duplicate mapping key, whose meaning depends on the
order being changed — and excluded it. Three more went unexcluded, and in
each the two compiles differ because of the rewrite rather than because of
a lowering, so the oracle blamed the compiler.
yaml.Marshal does not preserve a flow-style implicit null: "{A}" comes
back with an empty string for its value, so the second compile read a
string where the first read null. Both arms are now compiled from the
encoder's output, leaving declaration order as the only difference.
Reversing a mapping can carry an alias above the anchor it names, which
YAML forbids, so the permuted source no longer parsed at all and the parse
failure was reported as order dependence. The rewrite now re-parses what
it produced and declines a source whose permutation does not survive it,
which covers any later ordering rule of the same kind without enumerating
it.
diagnosticSet identified a finding partly by Provenance.Pointer, which is
documented to hold either a structural pointer or line:col, and a
permutation moves a construct to a different line by design. A positional
pointer is now replaced rather than dropped, so a permutation that changes
how many findings were reported still shows.
All three were latent: the committed corpus reaches none of them and the
sweep was green. Each is pinned by a case that fails without its own fix
and by nothing else. The reachability guard now covers both arms, since
the baseline can go quiet on its own.
TestReverseMappings_AliasIsNotFollowed asserted the anchor-above-alias
output as correct; its fixture moves to a sequence, whose order the rewrite
leaves alone, so it still pins that an anchored mapping reverses once.
Closes #241
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
The two-order oracle compiles a source as written and again with every mapping
reversed, and reports what the permutation changed. Its correctness rests on the
rewrite being meaning-preserving.
reverseMappingsdocumented one case where itis not — a duplicate mapping key, whose meaning depends on the order being
changed — and excluded it. Three more went unexcluded, and in each the two
compiles differ because of the rewrite rather than because of a lowering, so the
oracle reported a defect in the compiler that was not there.
yaml.Marshalre-emits{A}with an empty string for its value, so the second compile read a string where
the first read null. Both arms are now compiled from the encoder's output,
which leaves declaration order as the only difference between them.
anchor to be defined before it is referenced, so the permuted source did not
parse and the parse failure was reported as order dependence. The rewrite now
re-parses what it produced and declines a source whose permutation does not
survive it, which covers any later ordering rule of the same kind without
enumerating it.
diagnosticSetidentified a finding partly byProvenance.Pointer, whichir/provenance.godocuments as holding either a structural pointer orline:col. A permutation moves a construct to a different line by design. Apositional pointer is now replaced rather than dropped, so a permutation that
changes how many findings were reported still shows.
All three were latent. No committed spec reaches any of them and the corpus
sweep was green; they surface once a spec using one of these spellings is
committed, or the oracle is driven by generated input.
TestReverseMappings_AliasIsNotFollowedasserted the anchor-above-alias outputas correct — invalid YAML that the oracle was handing to the compiler. Its
fixture moves into a sequence, whose order the rewrite leaves alone, so it still
pins that an anchored mapping reverses once rather than once per alias; the shape
it used to assert is now covered by a test that requires it to be refused.
Closes #241
Test plan
passes with it. Reverting the fixes one at a time reddens exactly one case
each, so no test is standing in for another:
flow-style implicit nullan alias reordered above its anchorandTestReverseMappings_AliasAboveItsAnchorIsRefuseda diagnostic located by line and columnthe sweep noticing, so reach was measured rather than assumed: 82 corpus specs
still reach the comparison, and the new baseline skip fires on none of them.
The seven that are newly declined are the deliberately-invalid anchor and
duplicate-key fixtures, which stop at an earlier oracle and never reached this
one.
TestOrderInvariant_ReachesTheCorpusnow guards both arms rather than therewrite alone. Forcing either arm to decline reddens the suite.
gofmt,go vet,golangci-lint run(0 issues),
go build ./..., and./scripts/check-coverage.shat 100% of4373 statements.