diff --git a/crates/spar-hir-def/src/property_eval.rs b/crates/spar-hir-def/src/property_eval.rs index f542487..46d4ddf 100644 --- a/crates/spar-hir-def/src/property_eval.rs +++ b/crates/spar-hir-def/src/property_eval.rs @@ -112,11 +112,25 @@ pub fn lookup_property( None } -/// Evaluate a numeric property expression to an `f64` value. +/// Evaluate a numeric property expression to an `f64` **magnitude only**. /// -/// Handles `Integer`, `Real`, `UnitValue` (which wraps a numeric inner -/// expression), and `BinaryOp` (arithmetic on sub-expressions). -/// Returns `None` for non-numeric expressions. +/// # Units are silently stripped +/// +/// This function returns the bare magnitude of the expression with NO unit +/// normalization: `5 ms` and `5 ns` both evaluate to `5.0`, and a binary +/// expression's result has no unit attached. Do NOT use this for timing, +/// bandwidth, memory, or any other unit-bearing property — downstream +/// analyses that do will silently operate on wrong values (10³–10⁹× factor +/// errors in the timing case). +/// +/// For unit-aware evaluation, use `crates/spar-analysis/src/property_accessors.rs` +/// (`extract_time_ps`, `extract_size_bits`) or `crate::property_value::convert_units`, +/// both of which carry proper unit-factor tables. +/// +/// This helper is retained only for the narrow case of evaluating an +/// explicitly-unitless numeric property (e.g. a priority, a count) where the +/// caller has already confirmed unit-irrelevance. +#[doc(hidden)] pub fn eval_numeric(expr: &PropertyExpr) -> Option { match expr { PropertyExpr::Integer(v, _unit) => Some(*v as f64), @@ -142,12 +156,21 @@ pub fn eval_numeric(expr: &PropertyExpr) -> Option { } } -/// Compute a numeric property expression, returning both the value -/// and its unit (if any). +/// Compute a numeric property expression, returning the value and unit. +/// +/// # Unit propagation through arithmetic is NOT implemented +/// +/// For leaf `Integer` / `Real` / `UnitValue` the returned unit is correct. +/// For `BinaryOp` (e.g. `5 ms + 3 ms`) the returned unit is `None` — this +/// helper does not propagate units through arithmetic, even when both +/// operands agree. Any caller that expects `5 ms + 3 ms` to yield +/// `(8.0, Some("ms"))` will get a unit-stripped result instead. /// -/// For `Integer(v, Some(u))` returns `Some((v, Some(u)))`. -/// For `UnitValue(inner, u)` returns `Some((numeric(inner), Some(u)))`. -/// For bare `Integer(v, None)` returns `Some((v, None))`. +/// For timing / bandwidth / memory extraction, use the unit-aware helpers +/// in `crates/spar-analysis/src/property_accessors.rs` +/// (`extract_time_ps`, `extract_size_bits`) instead — they consult a +/// full unit-factor table and return a single canonical scale. +#[doc(hidden)] pub fn numeric_with_unit(expr: &PropertyExpr) -> Option<(f64, Option<&Name>)> { match expr { PropertyExpr::Integer(v, unit) => Some((*v as f64, unit.as_ref())), diff --git a/crates/spar-hir-def/src/resolver.rs b/crates/spar-hir-def/src/resolver.rs index 659c204..de1cd3a 100644 --- a/crates/spar-hir-def/src/resolver.rs +++ b/crates/spar-hir-def/src/resolver.rs @@ -356,21 +356,42 @@ impl GlobalScope { return result; } - // Check classifier and feature group renames in the originating package + // Check classifier and feature group renames in the originating package. + // + // AS-5506D §4.2: a classifier alias names a type and can be used + // anywhere that type can, including in `alias.impl_name` form. The + // prior guard `impl_name.is_none()` skipped this branch whenever an + // implementation suffix was present, so `MyAlias.i` resolved to + // Unresolved despite `MyAlias renames system A::Target;`. Drop the + // `impl_name.is_none()` gate and preserve `impl_name` through the + // rewrite. if reference.package.is_none() - && reference.impl_name.is_none() && let Some(from_scope) = self.packages.get(&from_key) { let type_key = CiName::new(&reference.type_name); - // Classifier renames: alias → (package, type_name) + // Classifier renames: alias → (package, type_name). If the + // reference carries `.impl_name`, preserve it through the + // rewrite so `MyAlias.i` resolves to `orig_pkg::orig_type.i`. if let Some((orig_pkg, orig_type)) = from_scope.classifier_renames.get(&type_key) { - let aliased_ref = ClassifierRef::qualified(orig_pkg.clone(), orig_type.clone()); + let aliased_ref = match &reference.impl_name { + Some(impl_name) => ClassifierRef::implementation( + Some(orig_pkg.clone()), + orig_type.clone(), + impl_name.clone(), + ), + None => ClassifierRef::qualified(orig_pkg.clone(), orig_type.clone()), + }; return self.resolve_classifier(from_package, &aliased_ref); } - // Feature group renames: alias → (package, fgt_name) - if let Some((orig_pkg, orig_fgt)) = from_scope.feature_group_renames.get(&type_key) { + // Feature group renames: alias → (package, fgt_name). Feature + // group types do not have implementations, so the `.impl` form + // should not be rewritten against a feature-group alias — fall + // through if impl_name is present. + if reference.impl_name.is_none() + && let Some((orig_pkg, orig_fgt)) = from_scope.feature_group_renames.get(&type_key) + { let aliased_ref = ClassifierRef::qualified(orig_pkg.clone(), orig_fgt.clone()); return self.resolve_classifier(from_package, &aliased_ref); } @@ -1144,6 +1165,81 @@ mod tests { ); } + #[test] + fn classifier_renames_resolves_impl_reference() { + // AS-5506D §4.2: classifier alias usable wherever the type is — + // including `alias.impl_name` form. Prior to the fix, + // resolve_classifier gated rename handling on `impl_name.is_none()`, + // so `MyAlias.i` returned Unresolved despite a valid rename. + let mut tree = ItemTree::default(); + + let ct = tree.component_types.alloc(ComponentTypeItem { + name: Name::new("OriginalSensor"), + category: ComponentCategory::System, + is_public: true, + extends: None, + features: Vec::new(), + flow_specs: Vec::new(), + modes: Vec::new(), + mode_transitions: Vec::new(), + prototypes: Vec::new(), + property_associations: Vec::new(), + requires_modes: false, + }); + let ci = tree.component_impls.alloc(ComponentImplItem { + type_name: Name::new("OriginalSensor"), + impl_name: Name::new("i"), + category: ComponentCategory::System, + is_public: true, + extends: None, + subcomponents: Vec::new(), + connections: Vec::new(), + end_to_end_flows: Vec::new(), + flow_impls: Vec::new(), + modes: Vec::new(), + mode_transitions: Vec::new(), + prototypes: Vec::new(), + call_sequences: Vec::new(), + property_associations: Vec::new(), + requires_modes: false, + }); + tree.packages.alloc(Package { + name: Name::new("A"), + with_clauses: Vec::new(), + public_items: vec![ItemRef::ComponentType(ct), ItemRef::ComponentImpl(ci)], + private_items: Vec::new(), + renames: Vec::new(), + }); + + let renames_idx = tree.renames.alloc(RenamesItem { + alias: Name::new("MySensor"), + original: Name::new("A::OriginalSensor"), + kind: RenamesKind::Classifier, + }); + tree.packages.alloc(Package { + name: Name::new("B"), + with_clauses: vec![Name::new("A")], + public_items: Vec::new(), + private_items: Vec::new(), + renames: vec![renames_idx], + }); + + let scope = GlobalScope::from_trees(vec![Arc::new(tree)]); + + // `MySensor.i` from B should resolve to A::OriginalSensor.i + let reference = ClassifierRef::implementation(None, Name::new("MySensor"), Name::new("i")); + let result = scope.resolve_classifier(&Name::new("B"), &reference); + assert!( + matches!( + result, + ResolvedClassifier::ComponentImpl { ref package, .. } + if package.as_str() == "A" + ), + "classifier rename should resolve MySensor.i to A::OriginalSensor.i: {:?}", + result + ); + } + #[test] fn feature_group_renames_resolves_unqualified_reference() { // Package A has feature group type "OriginalBusIface". diff --git a/crates/spar-parser/src/grammar/connections.rs b/crates/spar-parser/src/grammar/connections.rs index cff86a2..990fd21 100644 --- a/crates/spar-parser/src/grammar/connections.rs +++ b/crates/spar-parser/src/grammar/connections.rs @@ -25,12 +25,15 @@ fn connection(p: &mut Parser) { p.bump_any(); // name (IDENT or keyword-as-name) p.expect(SyntaxKind::COLON); - // Optional `refined to` + // Optional `refined to` — refined connections may legitimately omit + // endpoints (they inherit them from the refined declaration). + let mut is_refined = false; if p.at(SyntaxKind::REFINED_KW) { let r = p.start(); p.bump(SyntaxKind::REFINED_KW); p.expect(SyntaxKind::TO_KW); r.complete(p, SyntaxKind::REFINED_TO); + is_refined = true; } // Connection kind keyword @@ -94,7 +97,15 @@ fn connection(p: &mut Parser) { } }; - // Source and destination elements (optional for refined connections) + // Source and destination elements. + // + // AS-5506B §9.2: a non-refined connection declaration MUST have a + // source reference, arrow (`->` or `<->`), and destination reference. + // A refined connection MAY omit endpoints (the refinement inherits + // them). Before this check the parser silently accepted + // `c1 : port ;` — zero endpoints, no arrow — because the guard only + // entered on IDENT/keyword/SELF_KW. The instance-level validator + // caught it downstream, but the diagnostic locality was poor. if p.at(SyntaxKind::IDENT) || p.current().is_keyword() || p.at(SyntaxKind::SELF_KW) { connected_element(p); @@ -105,6 +116,8 @@ fn connection(p: &mut Parser) { // Destination element connected_element(p); + } else if !is_refined { + p.error("connection must have source and destination (AS-5506B §9.2); use `refined to` to inherit endpoints"); } // Optional property block diff --git a/crates/spar-parser/src/grammar/features.rs b/crates/spar-parser/src/grammar/features.rs index efa51e5..99ddb96 100644 --- a/crates/spar-parser/src/grammar/features.rs +++ b/crates/spar-parser/src/grammar/features.rs @@ -63,11 +63,23 @@ fn feature(p: &mut Parser) { // Determine feature type match p.current() { SyntaxKind::IN_KW | SyntaxKind::OUT_KW => { - // Direction + // Direction. AS-5506B §8.1 grammar: + // feature_direction ::= in | out | in out + // Only the `in out` combination is legal — `out in`, `in in`, + // and `out out` are not. The HIR lowering normalizes unknown + // direction text to `None`, so these used to parse silently + // and leave downstream direction_rules to skip the feature; + // reject at the syntax layer for better locality of diagnosis + // and spec-conformance. let d = p.start(); + let first_was_in = p.at(SyntaxKind::IN_KW); p.bump_any(); // in or out - if p.at(SyntaxKind::OUT_KW) || p.at(SyntaxKind::IN_KW) { - p.bump_any(); // second part of `in out` + // Only `in` may be followed by `out` to form `in out`. + if first_was_in && p.at(SyntaxKind::OUT_KW) { + p.bump(SyntaxKind::OUT_KW); + } else if p.at(SyntaxKind::IN_KW) || p.at(SyntaxKind::OUT_KW) { + p.error("feature direction must be `in`, `out`, or `in out` (AS-5506B §8.1)"); + p.bump_any(); // consume the offending keyword so the parser recovers } d.complete(p, SyntaxKind::DIRECTION); diff --git a/crates/spar-syntax/tests/parser_tests.rs b/crates/spar-syntax/tests/parser_tests.rs index 1ffd930..b80259c 100644 --- a/crates/spar-syntax/tests/parser_tests.rs +++ b/crates/spar-syntax/tests/parser_tests.rs @@ -882,6 +882,136 @@ fn test_data_property_value_arithmetic() { check_file_no_errors("../../test-data/parser/property_value_arithmetic.aadl"); } +// AS-5506B §9.2: a non-refined connection must have source and destination. +// Previously `c1 : port ;` parsed cleanly (the instance-level check caught it +// later, but the diagnostic locality was poor). +#[test] +fn connection_without_endpoints_is_rejected() { + let src = "\ +package P +public + system S end S; + system implementation S.i + connections + c1 : port ; + end S.i; +end P; +"; + let result = parse(src); + assert!( + result + .errors() + .iter() + .any(|e| e.msg.contains("source and destination")), + "expected `source and destination` error, got: {:?}", + result.errors() + ); +} + +// `refined to` connections may legitimately omit endpoints — the refinement +// inherits them. Keep this form working. +#[test] +fn refined_connection_may_omit_endpoints() { + let src = "\ +package P +public + system S end S; + system implementation S.Base + connections + c1 : port ; + end S.Base; + system implementation S.Extended extends S.Base + connections + c1 : refined to port ; + end S.Extended; +end P; +"; + let result = parse(src); + // S.Base's connection is still invalid (no `refined to`), so we expect + // one error from S.Base. The S.Extended refinement must NOT add a + // second error. + let source_dest_errors: Vec<_> = result + .errors() + .iter() + .filter(|e| e.msg.contains("source and destination")) + .collect(); + assert_eq!( + source_dest_errors.len(), + 1, + "refined-to connection without endpoints must parse cleanly; \ + got {} source/destination errors: {:?}", + source_dest_errors.len(), + source_dest_errors + ); +} + +// AS-5506B §8.1: feature_direction ::= in | out | in out — order-specific. +// `out in` / `in in` / `out out` are not legal; the parser must reject +// at the syntax layer rather than relying on HIR normalization to drop +// the direction to None later. +#[test] +fn out_in_feature_direction_is_rejected() { + let src = "\ +package P +public + abstract A + features + p : out in data port; + end A; +end P; +"; + let result = parse(src); + assert!( + result + .errors() + .iter() + .any(|e| e.msg.contains("feature direction")), + "expected direction error, got: {:?}", + result.errors() + ); +} + +#[test] +fn in_in_feature_direction_is_rejected() { + let src = "\ +package P +public + abstract A + features + p : in in data port; + end A; +end P; +"; + let result = parse(src); + assert!( + result + .errors() + .iter() + .any(|e| e.msg.contains("feature direction")), + "expected direction error, got: {:?}", + result.errors() + ); +} + +#[test] +fn in_out_feature_direction_still_accepted() { + let src = "\ +package P +public + abstract A + features + p : in out data port; + end A; +end P; +"; + let result = parse(src); + assert!( + result.errors().is_empty(), + "`in out` must still parse cleanly, got: {:?}", + result.errors() + ); +} + // 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