diff --git a/Cargo.lock b/Cargo.lock index 5f91b24..4012961 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1229,9 +1229,9 @@ checksum = "f18aa187839b2bdb1ad2fa35ead8c4c2976b64e4363c386d45ac0f7ee85c9233" [[package]] name = "thin-vec" -version = "0.2.14" +version = "0.2.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "144f754d318415ac792f9d69fc87abbbfc043ce2ef041c60f16ad828f638717d" +checksum = "259cdf8ed4e4aca6f1e9d011e10bd53f524a2d0637d7b28450f6c64ac298c4c6" [[package]] name = "toml" diff --git a/crates/spar-hir-def/src/item_tree/lower.rs b/crates/spar-hir-def/src/item_tree/lower.rs index 68bdfea..3cc78f5 100644 --- a/crates/spar-hir-def/src/item_tree/lower.rs +++ b/crates/spar-hir-def/src/item_tree/lower.rs @@ -1986,8 +1986,20 @@ fn lower_range_from_tokens(node: &SyntaxNode) -> Option { }; PropertyExpr::Real(display, min_unit) } else { - let val = parse_aadl_integer(&num_text).unwrap_or(0) * min_sign; - PropertyExpr::Integer(val, min_unit) + // Prior code used `unwrap_or(0)` here, silently substituting + // `0` for any integer literal that didn't fit in i64 — a range + // like `-9999999999999999999 .. 10` would lower to a range + // starting at 0 with zero diagnostic. Preserve the original + // literal as Opaque so analyses can detect the overflow + // instead of consuming a forged bound. + match parse_aadl_integer(&num_text).and_then(|v| v.checked_mul(min_sign)) { + Some(val) => PropertyExpr::Integer(val, min_unit), + None => PropertyExpr::Opaque(format!( + "{}{}", + if min_sign < 0 { "-" } else { "" }, + num_text + )), + } } }; @@ -2977,6 +2989,90 @@ end Pkg; } } +#[cfg(test)] +mod range_lowering_tests { + use super::*; + use spar_syntax::{SyntaxKind, parse}; + + /// Regression for the `unwrap_or(0)` silent-substitution bug in + /// `lower_range_from_tokens`. An AADL range whose min literal + /// exceeds i64 must NOT lower to `Integer(0, _)` — a forged bound + /// that downstream analyses would consume as legitimate. + #[test] + fn overflowing_range_min_does_not_silently_become_zero() { + // -9999999999999999999 is magnitude ~1e19, well above i64::MAX. + let src = r#" +package P +public + processor Proc + properties + Compute_Execution_Time => -9999999999999999999 ms .. 10 ms; + end Proc; +end P; +"#; + let parsed = parse(src); + // Walk to the RANGE_VALUE node and lower it directly. + let range = parsed + .syntax_node() + .descendants() + .find(|n| n.kind() == SyntaxKind::RANGE_VALUE) + .expect("range value node"); + let lowered = lower_range_from_tokens(&range).expect("range lowers"); + match lowered { + PropertyExpr::Range { min, .. } => match *min { + PropertyExpr::Integer(0, _) => { + panic!( + "range min silently became 0 on i64 overflow; \ + author wrote -9999999999999999999" + ); + } + PropertyExpr::Integer(_, _) => { + // Unexpected: should not fit in i64. A non-zero i64 + // would mean saturation or wrap — also wrong, but + // not the regression we're guarding against here. + panic!("range min fit in i64 unexpectedly"); + } + PropertyExpr::Opaque(text) => { + assert!( + text.contains("9999999999999999999"), + "Opaque min must preserve the original literal; got {text:?}" + ); + } + other => panic!("unexpected min shape: {other:?}"), + }, + other => panic!("expected Range, got {other:?}"), + } + } + + /// In-range positive integer range-min still lowers correctly. + #[test] + fn in_range_positive_min_lowers_correctly() { + let src = r#" +package P +public + processor Proc + properties + Compute_Execution_Time => 5 ms .. 10 ms; + end Proc; +end P; +"#; + let parsed = parse(src); + let range = parsed + .syntax_node() + .descendants() + .find(|n| n.kind() == SyntaxKind::RANGE_VALUE) + .expect("range value node"); + let lowered = lower_range_from_tokens(&range).expect("range lowers"); + match lowered { + PropertyExpr::Range { min, .. } => match *min { + PropertyExpr::Integer(5, _) => {} + other => panic!("expected Integer(5, _), got {other:?}"), + }, + other => panic!("expected Range, got {other:?}"), + } + } +} + #[cfg(test)] mod text_fallback_tests { use super::*; diff --git a/crates/spar-parser/src/grammar/components.rs b/crates/spar-parser/src/grammar/components.rs index 2fc1620..90566ca 100644 --- a/crates/spar-parser/src/grammar/components.rs +++ b/crates/spar-parser/src/grammar/components.rs @@ -158,17 +158,65 @@ fn impl_extension(p: &mut Parser) { m.complete(p, SyntaxKind::IMPL_EXTENSION); } +/// Track which distinct sections have been seen so far inside a single +/// component type or implementation body. +/// +/// AS-5506B §4.5 (component_type) and §5.1 (component_implementation) +/// grammar uses `?` (zero-or-one) on each section — not `*`. A component +/// declaration with two `features` clauses (or two `modes`, two +/// `properties`, …) is therefore a syntax error. Without this guard the +/// parser silently accepted duplicates, and lowering's `.find(|c| c.kind() +/// == FEATURE_SECTION)` call at item_tree/lower.rs silently dropped the +/// second section, so a reviewer who saw one interface in source ended up +/// with a different interface in the instance model. +#[derive(Default)] +struct SeenSections { + seen: Vec<(SyntaxKind, &'static str)>, +} + +impl SeenSections { + /// Returns true on the first hit, false (and emits an error) on a repeat. + fn check(&mut self, p: &mut Parser, kind: SyntaxKind, label: &'static str) -> bool { + if self.seen.iter().any(|(k, _)| *k == kind) { + p.error(format!("duplicate `{label}` section")); + false + } else { + self.seen.push((kind, label)); + true + } + } +} + fn component_type_sections(p: &mut Parser) { + let mut seen = SeenSections::default(); loop { match p.current() { - SyntaxKind::PROTOTYPES_KW => prototype_section(p), - SyntaxKind::FEATURES_KW => super::features::feature_section(p), - SyntaxKind::FLOWS_KW => super::flows::flow_spec_section(p), - SyntaxKind::MODES_KW => super::modes::mode_section(p), + SyntaxKind::PROTOTYPES_KW => { + seen.check(p, SyntaxKind::PROTOTYPES_KW, "prototypes"); + prototype_section(p); + } + SyntaxKind::FEATURES_KW => { + seen.check(p, SyntaxKind::FEATURES_KW, "features"); + super::features::feature_section(p); + } + SyntaxKind::FLOWS_KW => { + seen.check(p, SyntaxKind::FLOWS_KW, "flows"); + super::flows::flow_spec_section(p); + } + SyntaxKind::MODES_KW => { + seen.check(p, SyntaxKind::MODES_KW, "modes"); + super::modes::mode_section(p); + } SyntaxKind::REQUIRES_KW if p.nth(1) == SyntaxKind::MODES_KW => { + seen.check(p, SyntaxKind::MODES_KW, "modes"); super::modes::mode_section(p); } - SyntaxKind::PROPERTIES_KW => super::properties::property_section(p), + SyntaxKind::PROPERTIES_KW => { + seen.check(p, SyntaxKind::PROPERTIES_KW, "properties"); + super::properties::property_section(p); + } + // AS-5506B §4.5: an annex subclause may appear multiple times + // (one per annex language), so do not dedup on ANNEX_KW here. SyntaxKind::ANNEX_KW => super::annexes::annex_subclause(p), _ => break, } @@ -176,22 +224,50 @@ fn component_type_sections(p: &mut Parser) { } fn component_impl_sections(p: &mut Parser) { + let mut seen = SeenSections::default(); loop { match p.current() { - SyntaxKind::PROTOTYPES_KW => prototype_section(p), - SyntaxKind::SUBCOMPONENTS_KW => subcomponent_section(p), - SyntaxKind::INTERNAL_KW => internal_features_section(p), + SyntaxKind::PROTOTYPES_KW => { + seen.check(p, SyntaxKind::PROTOTYPES_KW, "prototypes"); + prototype_section(p); + } + SyntaxKind::SUBCOMPONENTS_KW => { + seen.check(p, SyntaxKind::SUBCOMPONENTS_KW, "subcomponents"); + subcomponent_section(p); + } + SyntaxKind::INTERNAL_KW => { + seen.check(p, SyntaxKind::INTERNAL_KW, "internal features"); + internal_features_section(p); + } SyntaxKind::PROCESSOR_KW if p.nth(1) == SyntaxKind::FEATURES_KW => { + seen.check(p, SyntaxKind::PROCESSOR_KW, "processor features"); processor_features_section(p); } - SyntaxKind::CONNECTIONS_KW => super::connections::connection_section(p), - SyntaxKind::CALLS_KW => call_section(p), - SyntaxKind::FLOWS_KW => super::flows::flow_impl_section(p), - SyntaxKind::MODES_KW => super::modes::mode_section(p), + SyntaxKind::CONNECTIONS_KW => { + seen.check(p, SyntaxKind::CONNECTIONS_KW, "connections"); + super::connections::connection_section(p); + } + SyntaxKind::CALLS_KW => { + seen.check(p, SyntaxKind::CALLS_KW, "calls"); + call_section(p); + } + SyntaxKind::FLOWS_KW => { + seen.check(p, SyntaxKind::FLOWS_KW, "flows"); + super::flows::flow_impl_section(p); + } + SyntaxKind::MODES_KW => { + seen.check(p, SyntaxKind::MODES_KW, "modes"); + super::modes::mode_section(p); + } SyntaxKind::REQUIRES_KW if p.nth(1) == SyntaxKind::MODES_KW => { + seen.check(p, SyntaxKind::MODES_KW, "modes"); super::modes::mode_section(p); } - SyntaxKind::PROPERTIES_KW => super::properties::property_section(p), + SyntaxKind::PROPERTIES_KW => { + seen.check(p, SyntaxKind::PROPERTIES_KW, "properties"); + super::properties::property_section(p); + } + // AS-5506B §5.1: annex subclauses may repeat. SyntaxKind::ANNEX_KW => super::annexes::annex_subclause(p), _ => break, } diff --git a/crates/spar-parser/src/grammar/properties.rs b/crates/spar-parser/src/grammar/properties.rs index 8537dac..3532fa2 100644 --- a/crates/spar-parser/src/grammar/properties.rs +++ b/crates/spar-parser/src/grammar/properties.rs @@ -245,10 +245,16 @@ fn property_expression_primary(p: &mut Parser) { } } SyntaxKind::PLUS | SyntaxKind::MINUS => { - // Signed numeric value + // Signed numeric value. Recurse into *primary* (not the outer + // wrapper), otherwise the binary-op loop would greedily consume + // the following `+`/`-`/`*` operators into the signed operand — + // causing `-1 + 2` to parse as `-(1+2)` instead of `(-1)+2`. + // AS-5506B §11.2.5: `numeric_term ::= [sign] numeric_literal` + // — the sign is part of the signed literal, not a prefix over + // the additive expression. let m = p.start(); p.bump_any(); - property_expression(p); + property_expression_primary(p); m.complete(p, SyntaxKind::PROPERTY_EXPRESSION); } _ => { diff --git a/crates/spar-syntax/tests/parser_tests.rs b/crates/spar-syntax/tests/parser_tests.rs index 1d1b1ed..1ffd930 100644 --- a/crates/spar-syntax/tests/parser_tests.rs +++ b/crates/spar-syntax/tests/parser_tests.rs @@ -882,6 +882,151 @@ fn test_data_property_value_arithmetic() { check_file_no_errors("../../test-data/parser/property_value_arithmetic.aadl"); } +// Regression for the unary-sign operator-precedence bug introduced when +// property_expression was split into outer-binary-loop + _primary: the +// PLUS/MINUS arm was still recursing into the outer function, so the sign +// greedily consumed the following additive tail. +#[test] +fn test_data_property_value_signed_arithmetic() { + check_file_no_errors("../../test-data/parser/property_value_signed_arithmetic.aadl"); +} + +// Regression for the duplicate-section silent-acceptance bug: AS-5506B +// §4.5 / §5.1 use `?` on each component section, so a second `features`, +// `modes`, `properties`, etc. inside the same component must be rejected. +// The prior `loop { match }` dispatcher had no seen-tracking; worse, +// lowering used `.find()` (not `.filter()`) and silently dropped the +// second section, so a reviewer could see ports in source that never +// reached the instance model. +#[test] +fn duplicate_features_section_is_rejected() { + let src = "\ +package P +public + system S + features + p1 : in data port; + features + p2 : out data port; + end S; +end P; +"; + let result = parse(src); + let errors = result.errors(); + assert!( + errors + .iter() + .any(|e| e.msg.contains("duplicate") && e.msg.contains("features")), + "expected `duplicate features section` error, got: {:?}", + errors + ); +} + +#[test] +fn duplicate_modes_section_is_rejected() { + let src = "\ +package P +public + system S + modes + m1 : initial mode; + modes + m2 : mode; + end S; +end P; +"; + let result = parse(src); + assert!( + result + .errors() + .iter() + .any(|e| e.msg.contains("duplicate") && e.msg.contains("modes")), + "expected `duplicate modes section` error, got: {:?}", + result.errors() + ); +} + +#[test] +fn duplicate_properties_section_in_impl_is_rejected() { + let src = "\ +package P +public + system S end S; + system implementation S.i + properties + Priority => 5; + properties + Priority => 7; + end S.i; +end P; +"; + let result = parse(src); + assert!( + result + .errors() + .iter() + .any(|e| e.msg.contains("duplicate") && e.msg.contains("properties")), + "expected `duplicate properties section` error, got: {:?}", + result.errors() + ); +} + +// Multiple annex subclauses must still be allowed — AS-5506B §4.5 / §5.1 +// don't dedup on annex_subclause, and real models routinely attach EMV2 +// alongside Behavior Annex. +#[test] +fn multiple_annex_subclauses_are_allowed() { + let src = "\ +package P +public + system S + annex EMV2 {** **}; + annex BLESS {** **}; + end S; +end P; +"; + let result = parse(src); + assert!( + !result.errors().iter().any(|e| e.msg.contains("duplicate")), + "annex subclauses must not be flagged as duplicate; got errors: {:?}", + result.errors() + ); +} + +// Shape-level assertion: in `-1 + 2` the signed PROPERTY_EXPRESSION node +// must cover only `-1`, not the entire tail. +#[test] +fn unary_sign_binds_to_literal_only_not_to_additive_tail() { + let src = "\ +package P +public + processor PP + properties + Foo::Bar => -1 + 2; + end PP; +end P; +"; + let result = parse(src); + assert!( + result.errors().is_empty(), + "expected clean parse, got: {:?}", + result.errors() + ); + let root = result.syntax_node(); + let signed = root + .descendants() + .find(|n| { + n.kind() == SyntaxKind::PROPERTY_EXPRESSION + && n.text().to_string().trim_start().starts_with('-') + }) + .expect("signed-literal PROPERTY_EXPRESSION node"); + let text = signed.text().to_string(); + assert!( + !text.contains('+'), + "unary `-` greedy-ate the additive tail: signed node text = {text:?}" + ); +} + // ==================================================================== // OSATE2 test files // ==================================================================== diff --git a/supply-chain/config.toml b/supply-chain/config.toml index 5eb1b8f..5c936e4 100644 --- a/supply-chain/config.toml +++ b/supply-chain/config.toml @@ -465,7 +465,7 @@ version = "1.1.1" criteria = "safe-to-deploy" [[exemptions.thin-vec]] -version = "0.2.14" +version = "0.2.16" criteria = "safe-to-deploy" [[exemptions.toml]] diff --git a/test-data/parser/property_value_signed_arithmetic.aadl b/test-data/parser/property_value_signed_arithmetic.aadl new file mode 100644 index 0000000..054656e --- /dev/null +++ b/test-data/parser/property_value_signed_arithmetic.aadl @@ -0,0 +1,23 @@ +-- Regression test: unary sign must bind only to the adjacent numeric +-- literal, not to the whole following additive expression. Prior to the +-- fix, the PLUS/MINUS primary arm recursed into the outer +-- property_expression (which runs a left-associative binary-op loop), +-- so `-1 + 2` parsed as `-(1+2)` instead of `(-1)+2` — silently +-- inverting the sign of every downstream arithmetic property. +-- AS-5506B §11.2.5: `numeric_term ::= [sign] numeric_literal`. +package Test_Signed_Arithmetic +public + + processor P + properties + -- leading negative + additive tail + Slack => -1 + 2; + -- leading positive + additive tail + Bias => +3 + 4; + -- subtractive tail after leading negative + Adjust => -10 - 3; + -- mixed with multiplication + Scale => -2 * 5 + 1; + end P; + +end Test_Signed_Arithmetic;