From 3c28d20e23c390bd3efde8a6aeff86bd9687be7f Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Sun, 19 Apr 2026 15:03:47 +0200 Subject: [PATCH 1/4] fix(cli): print line:col instead of byte offset in parse errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All 8 CLI parse-error sites printed ParseError.offset (a byte offset) as if it were a line number — so a syntax error 90 bytes into a 4-line file was reported as "line 90". Side-note reported in issues #125, #126, #127. Adds `offset_to_line_col(source, offset) -> (u32, u32)` in spar-base-db returning 1-based line and column (columns counted in chars for multibyte safety). Updates all 8 eprintln sites in spar-cli/src/main.rs to render `file:line:col: msg`. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/spar-base-db/src/lib.rs | 41 ++++++++++++++ crates/spar-cli/src/main.rs | 24 ++++++--- crates/spar-cli/tests/parse_error_line_col.rs | 54 +++++++++++++++++++ 3 files changed, 111 insertions(+), 8 deletions(-) create mode 100644 crates/spar-cli/tests/parse_error_line_col.rs diff --git a/crates/spar-base-db/src/lib.rs b/crates/spar-base-db/src/lib.rs index a999220..dd261fe 100644 --- a/crates/spar-base-db/src/lib.rs +++ b/crates/spar-base-db/src/lib.rs @@ -73,6 +73,20 @@ pub struct ParseError { pub offset: usize, } +/// Convert a byte offset into source text to a 1-based `(line, column)` pair. +/// +/// Lines are separated by `\n`. Column counts Unicode scalar values (chars), +/// not bytes, so multibyte characters don't inflate the column number. If +/// `offset` exceeds `source.len()` it is clamped to the end. +pub fn offset_to_line_col(source: &str, offset: usize) -> (u32, u32) { + let clamped = offset.min(source.len()); + let prefix = &source[..clamped]; + let line = prefix.bytes().filter(|&b| b == b'\n').count() as u32 + 1; + let line_start = prefix.rfind('\n').map(|i| i + 1).unwrap_or(0); + let col = source[line_start..clamped].chars().count() as u32 + 1; + (line, col) +} + /// A parsed annex within a file. #[derive(Clone, Debug, PartialEq, Eq)] pub struct AnnexResult { @@ -207,6 +221,33 @@ end ErrorLib; assert_eq!(result.annexes()[0].name, "EMV2"); } + #[test] + fn offset_to_line_col_basic() { + let src = "abc\ndef\nghi"; + assert_eq!(offset_to_line_col(src, 0), (1, 1)); + assert_eq!(offset_to_line_col(src, 2), (1, 3)); + assert_eq!(offset_to_line_col(src, 3), (1, 4)); // column after "abc" + assert_eq!(offset_to_line_col(src, 4), (2, 1)); // 'd' + assert_eq!(offset_to_line_col(src, 8), (3, 1)); // 'g' + assert_eq!(offset_to_line_col(src, 10), (3, 3)); // 'i' + } + + #[test] + fn offset_to_line_col_clamps_past_end() { + let src = "one"; + assert_eq!(offset_to_line_col(src, 9999), (1, 4)); + } + + #[test] + fn offset_to_line_col_multibyte() { + // 'é' is 2 bytes in UTF-8; offsets must land on char boundaries. + let src = "héllo\nworld"; + let e_offset = 3; // byte offset after "hé" (1 + 2) + assert_eq!(offset_to_line_col(src, e_offset), (1, 3)); + let w_offset = src.find('w').unwrap(); + assert_eq!(offset_to_line_col(src, w_offset), (2, 1)); + } + #[test] fn incremental_reparse() { use salsa::Setter; diff --git a/crates/spar-cli/src/main.rs b/crates/spar-cli/src/main.rs index e99a3d1..6bea5d6 100644 --- a/crates/spar-cli/src/main.rs +++ b/crates/spar-cli/src/main.rs @@ -124,7 +124,8 @@ fn cmd_parse(args: &[String]) { } else { has_errors = true; for err in parsed.errors() { - eprintln!("{}:{}: {}", file_path, err.offset, err.msg); + let (line, col) = spar_base_db::offset_to_line_col(&source, err.offset); + eprintln!("{}:{}:{}: {}", file_path, line, col, err.msg); } } } @@ -439,7 +440,8 @@ fn cmd_analyze(args: &[String]) { let parsed = spar_syntax::parse(&source); if !parsed.ok() { for err in parsed.errors() { - eprintln!("{}:{}: {}", file_path, err.offset, err.msg); + let (line, col) = spar_base_db::offset_to_line_col(&source, err.offset); + eprintln!("{}:{}:{}: {}", file_path, line, col, err.msg); } eprintln!("Cannot analyze: parse errors in {}", file_path); process::exit(1); @@ -586,7 +588,8 @@ fn cmd_allocate(args: &[String]) { let parsed = spar_syntax::parse(&source); if !parsed.ok() { for err in parsed.errors() { - eprintln!("{}:{}: {}", file_path, err.offset, err.msg); + let (line, col) = spar_base_db::offset_to_line_col(&source, err.offset); + eprintln!("{}:{}:{}: {}", file_path, line, col, err.msg); } eprintln!("Cannot allocate: parse errors in {}", file_path); process::exit(1); @@ -1287,7 +1290,8 @@ fn cmd_verify(args: &[String]) { let parsed = spar_syntax::parse(&source); if !parsed.ok() { for err in parsed.errors() { - eprintln!("{}:{}: {}", file_path, err.offset, err.msg); + let (line, col) = spar_base_db::offset_to_line_col(&source, err.offset); + eprintln!("{}:{}:{}: {}", file_path, line, col, err.msg); } eprintln!("Cannot verify: parse errors in {}", file_path); process::exit(1); @@ -1581,7 +1585,8 @@ fn cmd_codegen(args: &[String]) { let parsed = spar_syntax::parse(&source); if !parsed.ok() { for err in parsed.errors() { - eprintln!("{}:{}: {}", file_path, err.offset, err.msg); + let (line, col) = spar_base_db::offset_to_line_col(&source, err.offset); + eprintln!("{}:{}:{}: {}", file_path, line, col, err.msg); } eprintln!("Cannot codegen: parse errors in {}", file_path); process::exit(1); @@ -1715,7 +1720,8 @@ fn cmd_sysml2_parse(args: &[String]) { } else { has_errors = true; for err in parsed.errors() { - eprintln!("{}:{}: {}", file_path, err.offset, err.msg); + let (line, col) = spar_base_db::offset_to_line_col(&source, err.offset); + eprintln!("{}:{}:{}: {}", file_path, line, col, err.msg); } } } @@ -1768,7 +1774,8 @@ fn cmd_sysml2_lower(args: &[String]) { let parsed = spar_sysml2::parse(&all_source); if !parsed.ok() { for err in parsed.errors() { - eprintln!("parse error at {}: {}", err.offset, err.msg); + let (line, col) = spar_base_db::offset_to_line_col(&all_source, err.offset); + eprintln!("parse error at {}:{}: {}", line, col, err.msg); } process::exit(1); } @@ -1838,7 +1845,8 @@ fn cmd_sysml2_extract(args: &[String]) { let parsed = spar_sysml2::parse(&all_source); if !parsed.ok() { for err in parsed.errors() { - eprintln!("parse error at {}: {}", err.offset, err.msg); + let (line, col) = spar_base_db::offset_to_line_col(&all_source, err.offset); + eprintln!("parse error at {}:{}: {}", line, col, err.msg); } process::exit(1); } diff --git a/crates/spar-cli/tests/parse_error_line_col.rs b/crates/spar-cli/tests/parse_error_line_col.rs new file mode 100644 index 0000000..ee3b3a1 --- /dev/null +++ b/crates/spar-cli/tests/parse_error_line_col.rs @@ -0,0 +1,54 @@ +use std::env; +use std::fs; +use std::process::Command; + +fn spar() -> Command { + Command::new(env!("CARGO_BIN_EXE_spar")) +} + +/// Regression test for GitHub issues #125/#126/#127 side-note: +/// `spar parse` must print 1-based `line:col` in error messages, not a raw +/// byte offset. +#[test] +fn parse_error_prints_line_and_col() { + // AADL with a deliberate syntax error on line 3 (`let` is not valid AADL). + let src = "package Broken\n\ +public\n\ +let x;\n\ +end Broken;\n"; + + let path = env::temp_dir().join(format!( + "spar_parse_error_{}_{}.aadl", + std::process::id(), + line!() + )); + fs::write(&path, src).expect("write temp AADL"); + + let output = spar() + .arg("parse") + .arg(&path) + .output() + .expect("failed to run spar"); + + let stderr = String::from_utf8_lossy(&output.stderr); + + // Error must be reported on line 3 (where `let x;` is), with a column. + // Format: "::: " + let expected_prefix = format!("{}:3:", path.display()); + assert!( + stderr.contains(&expected_prefix), + "expected stderr to contain {:?} (line:col for line-3 error); got:\n{}", + expected_prefix, + stderr + ); + + // Guard against the pre-fix regression where a raw byte offset (>= file len) + // was printed as the line number. + assert!( + !stderr.contains(":20:") && !stderr.contains(":30:") && !stderr.contains(":40:"), + "stderr looks like it still contains a raw byte offset (file is ~38 bytes):\n{}", + stderr + ); + + let _ = fs::remove_file(&path); +} From d67b4b558517a2777491a143d18b02318936abf9 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Mon, 20 Apr 2026 07:38:31 +0200 Subject: [PATCH 2/4] fix(parser): property-set grammar gaps (#125, #126, #127) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three AS5506B productions in property sets were rejected: #125 — enumeration definition followed by another property definition. The property_set loop tested `at(IDENT)` to continue parsing definitions, but property names that happen to be AADL keywords (e.g. `Binding: aadlstring ...`) lex as the keyword token and the loop exited early. Switch to `at_name()` to accept ` :` as a name context. #126 — inline `units (...)` on `aadlreal`/`aadlinteger`. AS5506B §11.3 permits an inline units_designator on a numeric property type; only the named-classifier form (`units My_Units`) was accepted. Extracted the existing units-body parser into `units_designator_body` and reused it for the inline case. #127 — binary arithmetic in property values (e.g. `5 * 1000 ps`). AS5506B §11.2.5 admits `numeric_term` with binary operators. Split `property_expression` into an outer wrapper that consumes left-associative `* + -` chains and an inner `property_expression_primary`. Division is intentionally omitted — no SLASH token exists in the lexer, and no reporter asked for it. Regression fixtures added under test-data/parser/. Full workspace tests pass (225 parser-test cases, +3). Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/spar-parser/src/grammar/properties.rs | 93 ++++++++++++------- crates/spar-syntax/tests/parser_tests.rs | 19 ++++ .../parser/property_set_enum_sequenced.aadl | 11 +++ .../parser/property_set_inline_units.aadl | 18 ++++ .../parser/property_value_arithmetic.aadl | 20 ++++ 5 files changed, 129 insertions(+), 32 deletions(-) create mode 100644 test-data/parser/property_set_enum_sequenced.aadl create mode 100644 test-data/parser/property_set_inline_units.aadl create mode 100644 test-data/parser/property_value_arithmetic.aadl diff --git a/crates/spar-parser/src/grammar/properties.rs b/crates/spar-parser/src/grammar/properties.rs index 25ea26f..8537dac 100644 --- a/crates/spar-parser/src/grammar/properties.rs +++ b/crates/spar-parser/src/grammar/properties.rs @@ -86,8 +86,24 @@ fn property_value(p: &mut Parser) { } } -/// Parse a property expression. +/// Parse a property expression, including binary operator chains. +/// +/// AS5506B §11.2.5 admits `numeric_term` with binary operators (e.g. +/// `5 * 1000 ps`). Precedence is flat/left-associative — that is enough +/// to parse the common unit-scaling idiom without a full expression +/// grammar. fn property_expression(p: &mut Parser) { + property_expression_primary(p); + while matches!( + p.current(), + SyntaxKind::STAR | SyntaxKind::PLUS | SyntaxKind::MINUS + ) { + p.bump_any(); + property_expression_primary(p); + } +} + +fn property_expression_primary(p: &mut Parser) { match p.current() { SyntaxKind::INTEGER_LIT => { let m = p.start(); @@ -312,7 +328,7 @@ pub(crate) fn property_set(p: &mut Parser) { // property definitions and constants while !p.at(SyntaxKind::END_KW) && !p.at_end() { - if p.at(SyntaxKind::IDENT) { + if p.at_name() { property_definition_or_constant(p); } else { break; @@ -377,19 +393,14 @@ fn property_type(p: &mut Parser) { p.bump(SyntaxKind::AADLINTEGER_KW); if p.at(SyntaxKind::UNITS_KW) { p.bump(SyntaxKind::UNITS_KW); - // units reference - if p.at(SyntaxKind::IDENT) { - super::classifier_ref(p); - } + numeric_units_designator(p); } } SyntaxKind::AADLREAL_KW => { p.bump(SyntaxKind::AADLREAL_KW); if p.at(SyntaxKind::UNITS_KW) { p.bump(SyntaxKind::UNITS_KW); - if p.at(SyntaxKind::IDENT) { - super::classifier_ref(p); - } + numeric_units_designator(p); } } SyntaxKind::AADLSTRING_KW => p.bump(SyntaxKind::AADLSTRING_KW), @@ -432,29 +443,7 @@ fn property_type(p: &mut Parser) { SyntaxKind::UNITS_KW => { // units type: units (base, derived => base * factor, ...) p.bump(SyntaxKind::UNITS_KW); - p.expect(SyntaxKind::L_PAREN); - if p.at(SyntaxKind::IDENT) { - p.bump(SyntaxKind::IDENT); - while p.eat(SyntaxKind::COMMA) { - if p.at(SyntaxKind::IDENT) { - p.bump(SyntaxKind::IDENT); - // Optional `=> base * factor` - if p.eat(SyntaxKind::FAT_ARROW) { - if p.at(SyntaxKind::IDENT) { - p.bump(SyntaxKind::IDENT); - } - if p.eat(SyntaxKind::STAR) { - if p.at(SyntaxKind::INTEGER_LIT) { - p.bump(SyntaxKind::INTEGER_LIT); - } else if p.at(SyntaxKind::REAL_LIT) { - p.bump(SyntaxKind::REAL_LIT); - } - } - } - } - } - } - p.expect(SyntaxKind::R_PAREN); + units_designator_body(p); } SyntaxKind::CLASSIFIER_KW => { p.bump(SyntaxKind::CLASSIFIER_KW); @@ -484,6 +473,46 @@ fn property_type(p: &mut Parser) { m.complete(p, SyntaxKind::PROPERTY_TYPE); } +/// Parse `(uA, mA => uA * 1000, ...)` — body of a `units` designator. +/// +/// Called after the `units` keyword has been consumed. Shared between the +/// standalone `units (...)` property type (AS5506B §11.3) and inline use +/// on `aadlreal`/`aadlinteger` (`aadlreal units (...)`). +fn units_designator_body(p: &mut Parser) { + p.expect(SyntaxKind::L_PAREN); + if p.at(SyntaxKind::IDENT) { + p.bump(SyntaxKind::IDENT); + while p.eat(SyntaxKind::COMMA) { + if p.at(SyntaxKind::IDENT) { + p.bump(SyntaxKind::IDENT); + if p.eat(SyntaxKind::FAT_ARROW) { + if p.at(SyntaxKind::IDENT) { + p.bump(SyntaxKind::IDENT); + } + if p.eat(SyntaxKind::STAR) { + if p.at(SyntaxKind::INTEGER_LIT) { + p.bump(SyntaxKind::INTEGER_LIT); + } else if p.at(SyntaxKind::REAL_LIT) { + p.bump(SyntaxKind::REAL_LIT); + } + } + } + } + } + } + p.expect(SyntaxKind::R_PAREN); +} + +/// On `aadlreal`/`aadlinteger`, accept either a named units classifier +/// (`units My_Units`) or an inline `units (...)` block (AS5506B §11.3). +fn numeric_units_designator(p: &mut Parser) { + if p.at(SyntaxKind::L_PAREN) { + units_designator_body(p); + } else if p.at(SyntaxKind::IDENT) { + super::classifier_ref(p); + } +} + fn applies_to_category(p: &mut Parser) { if p.at(SyntaxKind::ALL_KW) { p.bump(SyntaxKind::ALL_KW); diff --git a/crates/spar-syntax/tests/parser_tests.rs b/crates/spar-syntax/tests/parser_tests.rs index 8e9d18d..1d1b1ed 100644 --- a/crates/spar-syntax/tests/parser_tests.rs +++ b/crates/spar-syntax/tests/parser_tests.rs @@ -863,6 +863,25 @@ fn test_data_with_clauses() { check_file_no_errors("../../test-data/parser/with_clauses.aadl"); } +// Regression for #125: enumeration property definition followed by another +// property must not break the property-set loop. +#[test] +fn test_data_property_set_enum_sequenced() { + check_file_no_errors("../../test-data/parser/property_set_enum_sequenced.aadl"); +} + +// Regression for #126: inline `units (...)` on aadlreal/aadlinteger. +#[test] +fn test_data_property_set_inline_units() { + check_file_no_errors("../../test-data/parser/property_set_inline_units.aadl"); +} + +// Regression for #127: binary arithmetic in property values. +#[test] +fn test_data_property_value_arithmetic() { + check_file_no_errors("../../test-data/parser/property_value_arithmetic.aadl"); +} + // ==================================================================== // OSATE2 test files // ==================================================================== diff --git a/test-data/parser/property_set_enum_sequenced.aadl b/test-data/parser/property_set_enum_sequenced.aadl new file mode 100644 index 0000000..cd5645c --- /dev/null +++ b/test-data/parser/property_set_enum_sequenced.aadl @@ -0,0 +1,11 @@ +-- Regression test for GitHub issue #125: +-- an `enumeration` property definition followed by another property +-- definition must parse cleanly. Before the fix, `Binding:` on the +-- next line was lexed as BINDING_KW and the property-set loop broke. +property set Test_Props is + + Radio: enumeration (ThreadMesh, LoRa, BLE) applies to (bus); + + Binding: aadlstring applies to (device); + +end Test_Props; diff --git a/test-data/parser/property_set_inline_units.aadl b/test-data/parser/property_set_inline_units.aadl new file mode 100644 index 0000000..ea07b46 --- /dev/null +++ b/test-data/parser/property_set_inline_units.aadl @@ -0,0 +1,18 @@ +-- Regression test for GitHub issue #126: +-- AS5506B §11.3 permits an inline `units (...)` designator on a numeric +-- property type (`aadlreal` / `aadlinteger`). The parser must accept +-- both the named-classifier form and the inline form. +property set Test_Units is + + -- Inline units on aadlreal + Power: aadlreal units (uA, mA => uA * 1000, A => mA * 1000) applies to (device); + + -- Inline units on aadlinteger + Capacity: aadlinteger units (mAh, Ah => mAh * 1000) applies to (device); + + -- Named-classifier form must still work (pre-existing path). + Bandwidth: aadlreal units Test_Units::Data_Rate_Units applies to (bus); + + Data_Rate_Units: type units (bps, Kbps => bps * 1000); + +end Test_Units; diff --git a/test-data/parser/property_value_arithmetic.aadl b/test-data/parser/property_value_arithmetic.aadl new file mode 100644 index 0000000..20c54d4 --- /dev/null +++ b/test-data/parser/property_value_arithmetic.aadl @@ -0,0 +1,20 @@ +-- Regression test for GitHub issue #127: +-- property values admit binary arithmetic per AS5506B §11.2.5. +-- Unit suffix applies to the final operand (`5 * 1000 ps` = 5000 ps). +package Test_Arithmetic +public + + processor P + properties + -- multiplication with trailing unit + Clock_Period => 5 * 1000 ps; + -- addition / subtraction + Priority => 10 + 5; + Offset => 100 - 50; + -- chained + Window => 2 * 3 * 4 ms; + -- signed operand on the left of a binary op + Slack => -3 + 10; + end P; + +end Test_Arithmetic; From e96619c3e35a3261305c54d2c2c36dc93c675149 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Mon, 20 Apr 2026 07:51:46 +0200 Subject: [PATCH 3/4] fix(instance): resolve `applies to ` eagerly onto target (#128, #129) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Property associations declared with `applies to fw.firmware` (AS5506B §8.3 / §10.6 dotted-path form) were silently dropped when converting PropertyAssociationItem → PropertyValue during instantiation. Two user-visible consequences: #128 — `binding_rules` reported threads as missing Actual_Processor_Binding even when the binding was declared at the enclosing system level with `applies to fw.firmware`. #129 — `spar instance --format json` emitted no `properties` at all, because (a) the InstanceNode serializer never carried a `properties` field, and (b) even if it had, the target-level map was empty for any `applies to …`-declared value. Design: eager target resolution at instance build (option 1). * New `pending_applies_to` queue on the Builder captures any PropertyAssociationItem whose `applies_to` path is non-empty, alongside the declaring component idx. * `Builder::resolve_pending_applies_to` runs after the recursive `instantiate_component` returns, walks each pending path through `components[owner].children` (case-insensitive name match), and attaches the PropertyValue to the resolved target's property_maps. Unresolvable paths keep the property on the owner and emit an InstanceDiagnostic. * `InstanceNode` gains a `properties: BTreeMap` field, populated in `build_node` from `inner.properties_for(idx)`. Fields are rendered as `PropSet::Name` when qualified, plain `Name` otherwise. Serde-skipped when empty so existing consumers that don't look at properties are unaffected. Tests: * `crates/spar-cli/tests/applies_to_nested.rs` — integration test running `spar analyze` and `spar instance --format json` on the #128 reproducer, asserting the binding is recognized and the property appears in the JSON output. * Workspace tests: 2,452 passed, 0 failed. Trade-offs considered: * Lazy resolution in `properties_for()` was the alternative; rejected because #129 needs the JSON serializer to emit resolved values, which is the same cost as eager attachment. * `PropertyValue` struct was intentionally NOT extended with an `applies_to` field — that would ripple through ~40 construction sites across the workspace. The queue approach keeps blast radius inside `instance.rs` + the new field in `InstanceNode`. * Feature-targeted `applies to sub.port_out` is not yet supported — features don't carry property maps today. Paths that walk into a feature name are reported as unresolved via InstanceDiagnostic. Separate work. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/spar-cli/tests/applies_to_nested.rs | 125 ++++++++++++++++ crates/spar-hir-def/src/instance.rs | 158 ++++++++++++++++----- crates/spar-hir/src/lib.rs | 29 ++++ 3 files changed, 276 insertions(+), 36 deletions(-) create mode 100644 crates/spar-cli/tests/applies_to_nested.rs diff --git a/crates/spar-cli/tests/applies_to_nested.rs b/crates/spar-cli/tests/applies_to_nested.rs new file mode 100644 index 0000000..4e19197 --- /dev/null +++ b/crates/spar-cli/tests/applies_to_nested.rs @@ -0,0 +1,125 @@ +//! Integration test for GitHub issues #128 and #129: +//! +//! #128 — `binding_rules` analyzer missed `Actual_Processor_Binding` +//! declared with `applies to fw.firmware` (dotted path). +//! #129 — `spar instance --format json` omitted all `properties`. +//! +//! Both have the same root cause: property associations with a non-empty +//! `applies to ` clause were dropped when converting +//! PropertyAssociationItem → PropertyValue during instantiation. The fix +//! attaches them eagerly to the resolved target instance. + +use std::env; +use std::fs; +use std::process::Command; + +fn spar() -> Command { + Command::new(env!("CARGO_BIN_EXE_spar")) +} + +/// AS5506B §8.3 reproducer from issue #128: `applies to fw.firmware` +/// where `fw` is a process subcomponent containing thread `firmware`. +const MODEL: &str = "\ +package Test_Applies_To +public + processor NRF52840 + end NRF52840; + + thread DoorFirmware + end DoorFirmware; + + process DoorFirmwareProcess + end DoorFirmwareProcess; + + process implementation DoorFirmwareProcess.Impl + subcomponents + firmware: thread DoorFirmware; + end DoorFirmwareProcess.Impl; + + system DoorNode + end DoorNode; + + system implementation DoorNode.Battery + subcomponents + mcu: processor NRF52840; + fw: process DoorFirmwareProcess.Impl; + properties + Actual_Processor_Binding => (reference (mcu)) applies to fw.firmware; + end DoorNode.Battery; +end Test_Applies_To; +"; + +fn write_model() -> std::path::PathBuf { + let path = env::temp_dir().join(format!( + "spar_applies_to_nested_{}.aadl", + std::process::id() + )); + fs::write(&path, MODEL).expect("write temp AADL"); + path +} + +/// #128: binding_rules must see the binding on the thread instance. +#[test] +fn issue_128_binding_rules_accepts_nested_applies_to() { + let path = write_model(); + let output = spar() + .arg("analyze") + .arg("--root") + .arg("Test_Applies_To::DoorNode.Battery") + .arg(&path) + .output() + .expect("failed to run spar"); + + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + let combined = format!("{stdout}\n{stderr}"); + + assert!( + !combined.contains("missing required Actual_Processor_Binding"), + "binding_rules still reports the thread as unbound — #128 regression.\n\ + combined output:\n{combined}" + ); + + let _ = fs::remove_file(&path); +} + +/// #129: `spar instance --format json` must emit the property on the +/// target instance, not the declaring system. +#[test] +fn issue_129_instance_json_includes_applies_to_properties() { + let path = write_model(); + let output = spar() + .arg("instance") + .arg("--root") + .arg("Test_Applies_To::DoorNode.Battery") + .arg("--format") + .arg("json") + .arg(&path) + .output() + .expect("failed to run spar"); + + assert!( + output.status.success(), + "spar instance failed; stderr:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + + let stdout = String::from_utf8_lossy(&output.stdout); + // The property is attached to the thread instance `firmware` that + // lives under `fw`. A serialized snippet would look like: + // "name": "firmware", ..., "properties": { "Actual_Processor_Binding": ... } + // We do a shape-insensitive assertion against the raw text. + assert!( + stdout.contains("Actual_Processor_Binding"), + "instance JSON does not contain the expected property name — #129 regression.\n\ + stdout:\n{stdout}" + ); + // Bonus guard: the reference text should be present too. + assert!( + stdout.contains("reference") && stdout.contains("mcu"), + "instance JSON does not contain the bound target — #129 regression.\n\ + stdout:\n{stdout}" + ); + + let _ = fs::remove_file(&path); +} diff --git a/crates/spar-hir-def/src/instance.rs b/crates/spar-hir-def/src/instance.rs index 582f788..b5b961b 100644 --- a/crates/spar-hir-def/src/instance.rs +++ b/crates/spar-hir-def/src/instance.rs @@ -207,6 +207,7 @@ impl SystemInstance { mode_transition_instances: Arena::default(), diagnostics: Vec::new(), property_maps: FxHashMap::default(), + pending_applies_to: Vec::new(), depth: 0, max_depth: 100, }; @@ -232,6 +233,10 @@ impl SystemInstance { None, ); + // Resolve properties declared with `applies to ` onto their + // target component instances (AS5506B §8.3/§10.6; fixes #128, #129). + builder.resolve_pending_applies_to(); + let mut instance = SystemInstance { root: root_idx, components: builder.components, @@ -1284,6 +1289,17 @@ struct Builder<'a> { mode_transition_instances: Arena, diagnostics: Vec, property_maps: FxHashMap, + /// Property associations with a non-empty `applies to ` clause. + /// + /// These are collected during instantiation and eagerly attached to + /// their resolved target components in a post-pass, so that downstream + /// analyses (and JSON export) see the property on the target instance + /// rather than on the declaring ancestor (fixes #128, #129). + pending_applies_to: Vec<( + ComponentInstanceIdx, + String, + crate::properties::PropertyValue, + )>, depth: u32, max_depth: u32, } @@ -2061,12 +2077,12 @@ impl<'a> Builder<'a> { for (tree_idx, pa_idx) in type_props { if let Some(tree) = self.scope.tree(tree_idx) { let pa = &tree.property_associations[pa_idx]; - map.add(crate::properties::PropertyValue { - name: pa.name.clone(), - value: pa.value.clone(), - typed_expr: pa.typed_value.clone(), - is_append: pa.is_append, - }); + Self::push_property_association( + pa, + idx, + &mut map, + &mut self.pending_applies_to, + ); } } } @@ -2079,12 +2095,12 @@ impl<'a> Builder<'a> { for (tree_idx, pa_idx) in impl_props { if let Some(tree) = self.scope.tree(tree_idx) { let pa = &tree.property_associations[pa_idx]; - map.add(crate::properties::PropertyValue { - name: pa.name.clone(), - value: pa.value.clone(), - typed_expr: pa.typed_value.clone(), - is_append: pa.is_append, - }); + Self::push_property_association( + pa, + idx, + &mut map, + &mut self.pending_applies_to, + ); } } } @@ -2096,12 +2112,7 @@ impl<'a> Builder<'a> { let sub = &tree.subcomponents[sub_idx]; for &pa_idx in &sub.property_associations { let pa = &tree.property_associations[pa_idx]; - map.add(crate::properties::PropertyValue { - name: pa.name.clone(), - value: pa.value.clone(), - typed_expr: pa.typed_value.clone(), - is_append: pa.is_append, - }); + Self::push_property_association(pa, idx, &mut map, &mut self.pending_applies_to); } } @@ -2130,12 +2141,12 @@ impl<'a> Builder<'a> { for (tree_idx, pa_idx) in type_props { if let Some(tree) = self.scope.tree(tree_idx) { let pa = &tree.property_associations[pa_idx]; - map.add(crate::properties::PropertyValue { - name: pa.name.clone(), - value: pa.value.clone(), - typed_expr: pa.typed_value.clone(), - is_append: pa.is_append, - }); + Self::push_property_association( + pa, + idx, + &mut map, + &mut self.pending_applies_to, + ); } } } @@ -2145,12 +2156,7 @@ impl<'a> Builder<'a> { let sub = &tree.subcomponents[sub_idx]; for &pa_idx in &sub.property_associations { let pa = &tree.property_associations[pa_idx]; - map.add(crate::properties::PropertyValue { - name: pa.name.clone(), - value: pa.value.clone(), - typed_expr: pa.typed_value.clone(), - is_append: pa.is_append, - }); + Self::push_property_association(pa, idx, &mut map, &mut self.pending_applies_to); } } @@ -2172,12 +2178,7 @@ impl<'a> Builder<'a> { let sub = &tree.subcomponents[sub_idx]; for &pa_idx in &sub.property_associations { let pa = &tree.property_associations[pa_idx]; - map.add(crate::properties::PropertyValue { - name: pa.name.clone(), - value: pa.value.clone(), - typed_expr: pa.typed_value.clone(), - is_append: pa.is_append, - }); + Self::push_property_association(pa, idx, &mut map, &mut self.pending_applies_to); } } @@ -2186,6 +2187,91 @@ impl<'a> Builder<'a> { } } + /// Add a property association either to `map` (no `applies to` clause) + /// or to the pending queue (for post-pass eager target resolution). + /// + /// `owner` is the component on which the association is declared; the + /// pending queue later walks the dotted `applies_to` path starting from + /// `owner` to find the target instance. + fn push_property_association( + pa: &crate::item_tree::PropertyAssociationItem, + owner: ComponentInstanceIdx, + map: &mut PropertyMap, + pending: &mut Vec<( + ComponentInstanceIdx, + String, + crate::properties::PropertyValue, + )>, + ) { + let prop = crate::properties::PropertyValue { + name: pa.name.clone(), + value: pa.value.clone(), + typed_expr: pa.typed_value.clone(), + is_append: pa.is_append, + }; + match pa.applies_to.as_deref() { + Some(path) if !path.trim().is_empty() => { + pending.push((owner, path.to_string(), prop)); + } + _ => map.add(prop), + } + } + + /// Post-instantiation pass: attach properties declared with + /// `applies to ` to their target component instances. + /// + /// AS5506B §8.3 / §10.6: a `Actual_Processor_Binding => ... applies to + /// fw.firmware;` declared at a system implementation must bind the + /// target thread instance, not the declaring system. This is resolved + /// eagerly so that downstream analyses and the JSON instance exporter + /// (#129) see the property on the target. + /// + /// If the path cannot be resolved (bad name, or walks into a feature + /// rather than a subcomponent), the property stays on the declaring + /// component and a diagnostic is recorded. + fn resolve_pending_applies_to(&mut self) { + let pending = std::mem::take(&mut self.pending_applies_to); + for (owner, path, prop) in pending { + match self.resolve_applies_to_path(owner, &path) { + Some(target) => { + self.property_maps.entry(target).or_default().add(prop); + } + None => { + // Unresolvable path: keep on owner (prior behavior) and + // emit a diagnostic so the author notices. + self.property_maps.entry(owner).or_default().add(prop); + self.diagnostics.push(InstanceDiagnostic { + message: format!( + "applies_to path '{path}' could not be resolved to a component instance" + ), + path: vec![self.components[owner].name.clone()], + }); + } + } + } + } + + /// Walk a dotted path (`fw.firmware`) from `owner` down through + /// subcomponent children, matching names case-insensitively. Returns + /// the resolved target component index, or `None` if any segment fails. + fn resolve_applies_to_path( + &self, + owner: ComponentInstanceIdx, + path: &str, + ) -> Option { + let mut current = owner; + for segment in path.split('.').map(str::trim).filter(|s| !s.is_empty()) { + let child = self.components[current].children.iter().find(|&&ci| { + self.components[ci] + .name + .as_str() + .eq_ignore_ascii_case(segment) + })?; + current = *child; + } + Some(current) + } + /// STPA-REQ-010: Validate that connection endpoint array indices are within bounds. /// /// For each connection owned by `owner`, if an endpoint references a specific diff --git a/crates/spar-hir/src/lib.rs b/crates/spar-hir/src/lib.rs index 09b0ea0..7a3b4f6 100644 --- a/crates/spar-hir/src/lib.rs +++ b/crates/spar-hir/src/lib.rs @@ -380,6 +380,12 @@ pub struct InstanceNode { pub array_index: Option, pub features: Vec, pub connections: Vec, + /// Resolved property values on this instance — the raw value text keyed + /// by the (optional) property-set qualifier and property name, rendered + /// as `PropSet::Name` or just `Name`. Includes both type-level / impl + /// inheritance and values attached via `applies to `. + #[serde(skip_serializing_if = "std::collections::BTreeMap::is_empty", default)] + pub properties: std::collections::BTreeMap, pub children: Vec, pub diagnostics: Vec, } @@ -544,6 +550,22 @@ impl Instance { .map(|&child_idx| self.build_node(child_idx)) .collect(); + // Resolved property values for this instance (fixes #129). + let properties = { + let mut out = std::collections::BTreeMap::new(); + let pmap = self.inner.properties_for(idx); + for (_key, values) in pmap.iter() { + let Some(pv) = values.first() else { continue }; + let name = pv.name.property_name.as_str(); + let key = match pv.name.property_set.as_ref() { + Some(ps) if !ps.as_str().is_empty() => format!("{}::{}", ps.as_str(), name), + _ => name.to_string(), + }; + out.insert(key, pv.value.clone()); + } + out + }; + InstanceNode { name: comp.name.as_str().to_string(), category: comp.category, @@ -553,6 +575,7 @@ impl Instance { array_index: comp.array_index, features, connections, + properties, children, diagnostics: vec![], } @@ -2193,6 +2216,7 @@ mod serde_round_trip_tests { source: Some("cpu1.data_out".to_string()), destination: Some("cpu2.data_in".to_string()), }], + properties: std::collections::BTreeMap::new(), children: vec![ InstanceNode { name: "cpu1".to_string(), @@ -2208,6 +2232,7 @@ mod serde_round_trip_tests { array_index: None, }], connections: vec![], + properties: std::collections::BTreeMap::new(), children: vec![], diagnostics: vec![], }, @@ -2225,6 +2250,7 @@ mod serde_round_trip_tests { array_index: Some(2), }], connections: vec![], + properties: std::collections::BTreeMap::new(), children: vec![], diagnostics: vec!["unresolved classifier".to_string()], }, @@ -2253,6 +2279,7 @@ mod serde_round_trip_tests { array_index: None, }], connections: vec![], + properties: std::collections::BTreeMap::new(), children: vec![], diagnostics: vec![], }; @@ -2266,6 +2293,7 @@ mod serde_round_trip_tests { array_index: None, features: vec![], connections: vec![], + properties: std::collections::BTreeMap::new(), children: vec![thread], diagnostics: vec![], }; @@ -2279,6 +2307,7 @@ mod serde_round_trip_tests { array_index: None, features: vec![], connections: vec![], + properties: std::collections::BTreeMap::new(), children: vec![process], diagnostics: vec![], }; From 8c521e32e4713564bb7353bb43ac4871f36093ae Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Mon, 20 Apr 2026 19:19:44 +0200 Subject: [PATCH 4/4] test: cover unresolvable applies_to fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `applies_to_unresolvable_path_emits_diagnostic` to exercise the `None` branch of `resolve_applies_to_path` — the fallback that keeps the property on the owner and emits an InstanceDiagnostic when a path segment matches no subcomponent. Raises patch coverage to clear the 88.58% codecov/patch gate. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/spar-cli/tests/applies_to_nested.rs | 51 ++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/crates/spar-cli/tests/applies_to_nested.rs b/crates/spar-cli/tests/applies_to_nested.rs index 4e19197..47a102a 100644 --- a/crates/spar-cli/tests/applies_to_nested.rs +++ b/crates/spar-cli/tests/applies_to_nested.rs @@ -83,6 +83,57 @@ fn issue_128_binding_rules_accepts_nested_applies_to() { let _ = fs::remove_file(&path); } +/// Unresolvable-path fallback: a property association whose `applies to` +/// path does not resolve to a real subcomponent must not crash the +/// pipeline. The property stays on the declaring component and a +/// diagnostic is emitted; `spar instance` still completes. +#[test] +fn applies_to_unresolvable_path_emits_diagnostic() { + let src = "\ +package Test_Unresolvable +public + processor Proc + end Proc; + + system Sys + end Sys; + + system implementation Sys.Impl + subcomponents + cpu: processor Proc; + properties + Actual_Processor_Binding => (reference (cpu)) applies to no_such_sub.no_such_thread; + end Sys.Impl; +end Test_Unresolvable; +"; + let path = env::temp_dir().join(format!("spar_applies_to_bad_{}.aadl", std::process::id())); + fs::write(&path, src).expect("write temp AADL"); + + let output = spar() + .arg("instance") + .arg("--root") + .arg("Test_Unresolvable::Sys.Impl") + .arg(&path) + .output() + .expect("failed to run spar"); + + assert!( + output.status.success(), + "spar instance must not crash on unresolvable applies_to; stderr:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("applies_to path") + && stderr.contains("no_such_sub.no_such_thread") + && stderr.contains("could not be resolved"), + "expected unresolved-path diagnostic in stderr, got:\n{stderr}" + ); + + let _ = fs::remove_file(&path); +} + /// #129: `spar instance --format json` must emit the property on the /// target instance, not the declaring system. #[test]