fix: batch close #125, #126, #127, #128, #129 (parser + instance)#130
Merged
fix: batch close #125, #126, #127, #128, #129 (parser + instance)#130
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This was referenced Apr 20, 2026
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) <noreply@anthropic.com>
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 `<keyword> :` 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) <noreply@anthropic.com>
…129) 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<String, String>` 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) <noreply@anthropic.com>
avrabe
force-pushed
the
fix/issue-batch-125-129
branch
from
2067f77 to
fa1cc03
Compare
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) <noreply@anthropic.com>
avrabe
force-pushed
the
fix/issue-batch-125-129
branch
from
fa1cc03 to
8c521e3
Compare
This was referenced Apr 21, 2026
Closed
Closed
avrabe
added a commit
that referenced
this pull request
Apr 22, 2026
* fix(parser): unary sign must bind only to adjacent literal Regression from PR #130. When property_expression was split into an outer binary-op wrapper + an inner _primary for #127, the PLUS/MINUS arm in the primary kept recursing into the outer wrapper. The outer's binary-op loop then greedily consumed the following `+`/`-`/`*` operators into the signed operand — so `-1 + 2` parsed as `-(1+2)` instead of `(-1)+2`, silently inverting the sign of every additive tail downstream. AS-5506B §11.2.5 defines `numeric_term ::= [sign] numeric_literal` — the sign is atomic with the literal, not a prefix over the additive expression. Fix: one-line change at properties.rs:251, recurse into `property_expression_primary` in the signed-numeric arm. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(parser): reject duplicate component sections AS-5506B §4.5 (component_type) and §5.1 (component_implementation) grammar uses `?` on each section clause — `features`, `modes`, `properties`, `subcomponents`, `connections`, `prototypes`, `calls`, `flows`, `internal features`, `processor features`. Before this fix the `loop { match }` section dispatcher had no seen-tracking, so a component with two `features` blocks parsed cleanly. Lowering then silently dropped the duplicate because `item_tree/lower.rs` walks the CST with `.find(|c| c.kind() == FEATURE_SECTION)` — first match wins — so a reviewer who saw ports in both sections ended up with ONLY the first set in the instance model. Adds a `SeenSections` guard consulted before each arm. On a repeat we emit `p.error("duplicate `<label>` section")` and still consume the section so the parser recovers cleanly. Annex subclauses are intentionally exempt — real models routinely stack EMV2 and Behavior Annex on the same component. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hir-def): preserve overflowing range min as Opaque instead of silent 0 `lower_range_from_tokens` had `parse_aadl_integer(&num_text).unwrap_or(0) * min_sign`. On a literal larger than i64::MAX, `parse_aadl_integer` returns None, `unwrap_or(0)` substitutes 0, and the RANGE_VALUE was lowered as `Integer(0, _)` with zero diagnostic. Downstream analyses (scheduling, latency, resource budgets) would then consume a forged bound of `0` in place of the author's actual literal — a silent HIR drift. Replace with `and_then(.checked_mul).unwrap_or(Opaque(<original-text>))` so overflow falls back to preserving the original literal as Opaque text, letting property-typed analyses detect the overflow rather than silently using a fake value. The sibling helper at line 1688 already used `?` — this site was the outlier. Maps to hazard H-5 (HIR drift); UCA-4 (Lowering assigns wrong property value). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(deps): bump thin-vec to 0.2.16 to clear RUSTSEC-2026-0103 `thin-vec 0.2.14` has a Use-After-Free / Double Free in `IntoIter::drop` and `ThinVec::clear` when an element's Drop panics, reachable from safe Rust (confirmed via Miri / AddressSanitizer per the RustSec advisory). 0.2.16 ships the fix; bumping via `cargo update -p thin-vec` is a patch-level bump through salsa's indirect dependency. Unblocks `Cargo Deny` + `Security Audit (RustSec)` CI checks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(vet): update thin-vec exemption to 0.2.16 Match the Cargo.lock bump in the previous commit. cargo-vet was failing Supply Chain because the exemption still pointed at the pre-RUSTSEC-2026-0103 version. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five reported issues, three commits, one branch. All user-visible, all with regression tests.
Closes
units (...)onaadlreal/aadlinteger5 * 1000 ps)binding_rules: nested-pathActual_Processor_Bindingnot recognized (applies to fw.firmware)spar instance --format jsonomits all property valuesPlus a diagnostic fix that surfaced across three reports:
line:col.Commits (in branch order)
ff428eafix(cli): print line:col instead of byte offset in parse errorsef1e405fix(parser): property-set grammar gaps (#125, #126, #127)6f7c8f3fix(instance): resolve applies to <path> eagerly onto target (#128, #129)Design notes
PropertyAssociationItem.applies_towas dropped when converting toPropertyValue. Fixed by queueingapplies_to-bearing associations during instantiation and attaching them to the resolved target component in a post-pass. AS5506B §8.3 / §10.6 semantics; matches OSATE's eager-resolution model.InstanceNodegains aproperties: BTreeMap<String, String>field, serde-skipped when empty, so downstream JSON consumers that ignore properties keep working.PropertyValuewas intentionally not extended with anapplies_tofield — that would ripple through ~40 construction sites across the workspace. The side-queue approach keeps the blast radius insideinstance.rs.at(IDENT)check that broke on keyword-as-name in property-set loops (Binding:lexes asBINDING_KW). Switched toat_name().units_designator_bodyhelper reused by the inline numeric form and the standalone form.property_expressioninto an outer binary-op wrapper and an innerproperty_expression_primary. Division intentionally omitted — noSLASHtoken exists and no reporter asked.Test plan
cargo test --workspace→ 2,452 passed, 0 failedcargo clippy --workspace --tests -- -D warnings→ cleancargo fmt --check→ cleancrates/spar-base-db/src/lib.rs— 3 unit tests foroffset_to_line_colcrates/spar-cli/tests/parse_error_line_col.rs— CLI integration (spar parseline:col)crates/spar-syntax/tests/parser_tests.rs— 3 file-based fixture teststest-data/parser/property_{set_enum_sequenced,set_inline_units,value_arithmetic}.aadlcrates/spar-cli/tests/applies_to_nested.rs— end-to-end binding_rules: nested-path Actual_Processor_Binding not recognized (applies to fw.firmware) #128 + Instance JSON output omits all property values (subcomponent bindings and type-level both) #129 viaspar analyzeandspar instance --format jsonNot in this PR
stpa-yamlsource format not expanding shorthand (controller: X) into canonicallinks:entries. Fix is either upstream (rivet) or full artifact migration — both out of scope for this batch.🤖 Generated with Claude Code