From b64c680d3afeabae3e9065fd0637ded5d1d28eb6 Mon Sep 17 00:00:00 2001 From: Maciej Myszkiewicz Date: Sun, 11 Oct 2026 22:36:00 +0200 Subject: [PATCH] fix(model): three entry constructs were dropped in silence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. * `` 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 `` whose `` is a `` fell through a chain that only looks for ``, 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. --- SUPPORTED.md | 9 +- crates/xtce-model/src/lower.rs | 123 +++++++++++++++++++++----- crates/xtce-model/src/xml.rs | 1 + crates/xtce-model/tests/composites.rs | 70 +++++++++++++++ 4 files changed, 178 insertions(+), 25 deletions(-) diff --git a/SUPPORTED.md b/SUPPORTED.md index 8f05ef6..a56ad23 100644 --- a/SUPPORTED.md +++ b/SUPPORTED.md @@ -86,12 +86,13 @@ case for commands. | `BaseContainer` + `RestrictionCriteria` | Decodable | | | `Comparison`, `ComparisonList` | Decodable | all six operators, in every accepted spelling | | `BooleanExpression` (`Condition`, `ANDedConditions`, `ORedConditions`) | Decodable | required by `contrived_inheritance_structure.xml` | -| `LocationInContainerInBits` | Decodable | `previousEntry` and `containerStart` | +| `LocationInContainerInBits` | Decodable | `previousEntry` and `containerStart`; reported as unsupported when the entry it sits on names an array or an aggregate, because that entry becomes one field per leaf and the offset is not carried onto them | | `LocationInContainerInBits` with `containerEnd` / `nextEntry` | Represented | needs a container size that is only known once decoding finishes; reported at decode time | -| `RepeatEntry` with a fixed `Count` | Decodable | | -| `RepeatEntry` with `DynamicValue` | Represented | entry decodes once, then reports unsupported | +| `RepeatEntry` with a fixed `Count` | Decodable | reported as unsupported on an entry that names an array or an aggregate, for the same reason as the row above | +| `RepeatEntry` with `DynamicValue` | Represented | `EntryKind::Unsupported`, reported at load. The count is in the packet and this reader does not read it, so decoding the entry once would be a container one field long where the packet has *n* | +| `IncludeCondition` | Represented | `EntryKind::Unsupported`, reported at load. Whether the entry is there at all is decided at decode time; reading it unconditionally would claim bits the packet may not have given it and shift every entry behind it | | `IndirectParameterRefEntry` | Represented | `EntryKind::Unsupported` | -| `ArrayParameterRefEntry` | Represented | `EntryKind::Unsupported` | +| `ArrayParameterRefEntry` | Decodable | expanded into one entry per leaf, the same as `ArrayParameterType` — see the code-generation table below, which says so too | | `CustomAlgorithm` in `RestrictionCriteria` | Represented | `MatchCriteria::Unsupported`; never silently matches | ## Calibration diff --git a/crates/xtce-model/src/lower.rs b/crates/xtce-model/src/lower.rs index 06ee4b0..16188da 100644 --- a/crates/xtce-model/src/lower.rs +++ b/crates/xtce-model/src/lower.rs @@ -1342,6 +1342,19 @@ impl<'d> Lowering<'d> { parameters: &mut Vec, out: &mut Vec, ) -> Result<(), XtceError> { + // Parsed before the match rather than after it, because the composite arm returns + // early: an array entry carrying either of these used to drop it on the way past. + let location = entry_location(element); + let repeat = entry_repeat(element); + + // What this reader cannot act on, reported rather than passed over. An entry whose + // bits are claimed when they should not be does not go missing — it shifts every + // entry behind it, and the values that come out are wrong rather than absent. + if let Some(reason) = entry_out_of_scope(element, repeat) { + self.push_unsupported_entry(element, reason, location, repeat, out); + return Ok(()); + } + let kind = match element.tag() { Tag::ParameterRefEntry | Tag::ArrayParameterRefEntry @@ -1384,6 +1397,18 @@ impl<'d> Lowering<'d> { // here means the interpreter, the code generator and the flight encoder all // see ordinary fields and none of them has to know arrays exist. if let Some(expanded) = self.expand_composite(element, id, types, parameters)? { + if location.is_some() || repeat.is_some() { + self.push_unsupported_entry( + element, + "a LocationInContainerInBits or a RepeatEntry on an entry that \ + names an array or an aggregate; the entry becomes one field per \ + leaf here, and neither is carried onto the leaves", + location, + repeat, + out, + ); + return Ok(()); + } out.extend(expanded); return Ok(()); } @@ -1416,27 +1441,6 @@ impl<'d> Lowering<'d> { } }; - let location = element - .child(Tag::LocationInContainerInBits) - .and_then(|node| { - let offset = node - .child(Tag::FixedValue) - .and_then(Element::text) - .or_else(|| node.text()) - .and_then(|text| text.trim().parse::().ok())?; - Some(Location { - reference: location_reference(node.attr(AttrKey::ReferenceLocation)), - offset_in_bits: offset, - }) - }); - - let repeat = element - .child(Tag::RepeatEntry) - .and_then(|repeat| repeat.child(Tag::Count)) - .and_then(|count| count.child(Tag::FixedValue)) - .and_then(Element::text) - .and_then(|text| text.trim().parse::().ok()); - out.push(Entry { kind, location, @@ -2007,6 +2011,30 @@ impl<'d> Lowering<'d> { } } + /// Records `reason` against `element` and puts an unsupported entry in its place. + /// + /// The entry is pushed rather than dropped: it keeps the place the definition gave it, so + /// a caller reading the container sees something it cannot decode where the bits are, + /// instead of a container that is quietly one entry shorter than the packet. + fn push_unsupported_entry( + &mut self, + element: Element<'d>, + reason: &'static str, + location: Option, + repeat: Option, + out: &mut Vec, + ) { + let name = element.name().to_owned(); + self.note_unsupported(&name, &element.path(), reason); + out.push(Entry { + kind: EntryKind::Unsupported { + element: self.interner.intern(&name), + }, + location, + repeat, + }); + } + fn note_unsupported(&mut self, element: &str, path: &str, reason: &'static str) { self.unsupported.push(Unsupported { element: element.to_owned(), @@ -2016,6 +2044,59 @@ impl<'d> Lowering<'d> { } } +/// The `` an entry carries, if any. +fn entry_location(element: Element<'_>) -> Option { + element + .child(Tag::LocationInContainerInBits) + .and_then(|node| { + let offset = node + .child(Tag::FixedValue) + .and_then(Element::text) + .or_else(|| node.text()) + .and_then(|text| text.trim().parse::().ok())?; + Some(Location { + reference: location_reference(node.attr(AttrKey::ReferenceLocation)), + offset_in_bits: offset, + }) + }) +} + +/// The repetition count an entry carries, when it is a plain ``. +/// +/// `None` covers both "no ``" and "a `` whose count comes from the +/// packet". [`entry_out_of_scope`] is what tells those two apart, because they are not the +/// same thing and decoding once is only right for the first. +fn entry_repeat(element: Element<'_>) -> Option { + element + .child(Tag::RepeatEntry) + .and_then(|repeat| repeat.child(Tag::Count)) + .and_then(|count| count.child(Tag::FixedValue)) + .and_then(Element::text) + .and_then(|text| text.trim().parse::().ok()) +} + +/// Why an entry cannot be read as written, if it cannot. +/// +/// Both of these used to be dropped without a word, and dropping them 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 the offset that assumes them. The values that come out are wrong +/// rather than missing, and nothing says so. +fn entry_out_of_scope(element: Element<'_>, repeat: Option) -> Option<&'static str> { + if element.child(Tag::IncludeCondition).is_some() { + return Some( + "an IncludeCondition, which decides at decode time whether the entry is present \ + at all; reading it unconditionally claims bits the packet may not have given it", + ); + } + if element.child(Tag::RepeatEntry).is_some() && repeat.is_none() { + return Some( + "a RepeatEntry whose Count is not a FixedValue; the count is in the packet and \ + this reader does not read it, so the entry would decode exactly once", + ); + } + None +} + #[derive(Clone, Copy)] enum Definition { Type, diff --git a/crates/xtce-model/src/xml.rs b/crates/xtce-model/src/xml.rs index acb19d0..44cca58 100644 --- a/crates/xtce-model/src/xml.rs +++ b/crates/xtce-model/src/xml.rs @@ -183,6 +183,7 @@ symbols! { OffsetFrom => "OffsetFrom", RepeatEntry => "RepeatEntry", Count => "Count", + IncludeCondition => "IncludeCondition", ServiceSet => "ServiceSet", MessageSet => "MessageSet", StreamSet => "StreamSet", diff --git a/crates/xtce-model/tests/composites.rs b/crates/xtce-model/tests/composites.rs index fa50dd5..9dd1921 100644 --- a/crates/xtce-model/tests/composites.rs +++ b/crates/xtce-model/tests/composites.rs @@ -376,3 +376,73 @@ fn the_ceiling_counts_leaves_across_the_whole_nesting() { "the refusal should name the ceiling: {error}" ); } + +/// An entry that is only sometimes there cannot be read as if it always is. +/// +/// Dropping the `` is worse than it sounds: the entry keeps its place, so +/// its bits are claimed anyway and every entry behind it is read at an offset that assumes +/// them. Wrong values rather than missing ones, and in silence. +#[test] +fn an_entry_with_an_include_condition_is_reported_rather_than_read_unconditionally() { + let xml = definition( + "", + r#""#, + r#" + + + + + "#, + ); + let db = XtceDb::from_xml(&xml).expect("the definition loads"); + assert!( + db.unsupported() + .iter() + .any(|item| item.reason.contains("IncludeCondition")), + "an IncludeCondition has to be reported: {:?}", + db.unsupported() + ); +} + +/// A count that lives in the packet is not the same as no count at all. +#[test] +fn a_repeat_entry_whose_count_is_dynamic_is_reported_rather_than_decoded_once() { + let xml = definition( + "", + r#""#, + r#" + + + + + "#, + ); + let db = XtceDb::from_xml(&xml).expect("the definition loads"); + assert!( + db.unsupported() + .iter() + .any(|item| item.reason.contains("RepeatEntry")), + "a dynamic RepeatEntry has to be reported: {:?}", + db.unsupported() + ); +} + +/// A fixed count still works, so the refusal above is not a blanket one. +#[test] +fn a_repeat_entry_with_a_fixed_count_is_still_decodable() { + let xml = definition( + "", + r#""#, + r#" + 3 + "#, + ); + let db = XtceDb::from_xml(&xml).expect("the definition loads"); + assert!( + !db.unsupported() + .iter() + .any(|item| item.reason.contains("RepeatEntry")), + "a fixed count is handled: {:?}", + db.unsupported() + ); +}