Repository navigation
fix(model): three entry constructs were dropped in silence - #5
Merged
Merged
Conversation
This crate's whole claim is that a construct outside its subset is refused by name rather than quietly skipped. Three of them were not. * `<IncludeCondition>` was never read — the string did not appear anywhere in the crate. An entry present only when a condition holds was read as if it were always there. * A `<RepeatEntry>` whose `<Count>` is a `<DynamicValue>` fell through a chain that only looks for `<FixedValue>`, leaving `repeat: None`, which is also what "no RepeatEntry at all" leaves. The entry decoded exactly once. * An entry naming an array or an aggregate returns early to expand into one field per leaf, and the early return happened *before* the location and the repeat were parsed. Both were discarded on the way past. Dropping these is worse than it sounds. The entry keeps its place in the container, so its bits are still claimed and every entry behind it is still read at an offset that assumes them: the values that come out are wrong rather than missing, and nothing says so. All three are `EntryKind::Unsupported` now, reported at load and pushed in place rather than left out, so a caller sees something undecodable where the bits are instead of a container quietly one entry short. SUPPORTED.md had four rows that disagreed with the code — including `ArrayParameterRefEntry` listed as unsupported on line 94 and as handled on line 176 — and now matches it. 182 tests, all 9 differential cases still agree with the reference, fmt and clippy clean, and this time the 1.85 floor was checked too.
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.
This crate's whole claim is that a construct outside its subset is refused by name, never
quietly skipped. Three of them were not.
<IncludeCondition><RepeatEntry>with a<DynamicValue>count<FixedValue>, leavingrepeat: None— which is also what no RepeatEntry at all leaves. The entry decoded exactly once.RepeatEntryorLocationInContainerInBitson an array/aggregate entryDropping these is worse than it sounds. The entry keeps its place in the container, so its bits
are still claimed and every entry behind it is still read at an offset that assumes them — the
values that come out are wrong rather than missing, and nothing says so. This is precisely
the failure the
Unsupportedmachinery exists to prevent.All three are
EntryKind::Unsupportednow, reported at load, and pushed in place ratherthan left out, so a caller sees something undecodable where the bits are instead of a container
quietly one entry short.
SUPPORTED.mdhad four rows that disagreed with the code — includingArrayParameterRefEntrylisted as unsupported on line 94 and as handled on line 176, which contradicted itself — and
now matches.
Why the differential suite never caught any of it: no bundled definition uses one.
rg RepeatEntry testdata/returns nothing, andIncludeConditionappears in none of the nine.This is the shape of gap that a test suite built from real mission files cannot close.
182 tests, all 9 differential cases still agree with the reference, fmt and clippy clean, and
the 1.85 floor was checked locally this time rather than discovered in CI.