fix: 4 Mythos-hardening fixes (parser spec-conformance + helper footguns)

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<f64> {
     match expr {
         PropertyExpr::Integer(v, _unit) => Some(*v as f64),
@@ -142,12 +156,21 @@ pub fn eval_numeric(expr: &PropertyExpr) -> Option<f64> {
     }
 }
 
-/// 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())),

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".

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

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);
 

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