feat(api): expose recorded identifier and nested-value provenance - #326
Conversation
Refs #324. Add public accessors on Program for the identifier spans the raw parser already records: - global_variable_name_span, player_variable_name_span, and subroutine_name_span return the declared-name span of each declaration (an attached identifier span when a provider supplies one, else the recorded declaration span, which raw parses record as the name). - action_identifier_span returns the span recorded for the variable or subroutine an action names: an infix-assignment/for target or a Call Subroutine callee. - condition_value_span and action_argument_value_span return the recorded span of a value nested inside a condition or action argument, addressed by a path into the public Value tree. Spans the parser never records (standard-form write targets, Call Subroutine callees, Global.name/Global Variable(...) read identifiers) return None; recording them is parser-side follow-up. Nested value spans are also written back through to_wir so diagnostics keep them.
e54-bot
left a comment
There was a problem hiding this comment.
Independent review of 13044ac — rust-engineering pass over the provenance plumbing and API contract, plus a test-design pass over the new tests.
Rust engineering
ValueProvenance.children ↔ public Value child order verified for all 13 wir::Value variants; action_identifier/apply_action_source cover exactly the six identifier-bearing WIR variants; Disabled recursion surfaces inner-action provenance correctly; shape guards still drop displaced spans; source_map apply writes only .span (nested/identifier mappings stay outside mapped-text-v1, documented); name_span.or(span) fallback verified defensible (raw parses store the name in span). One finding:
program.rs:378-387+docs/source-preservation.md—action_identifier_spanenumerates "set/modify and for-variable actions" as identifier-bearing, but for-variable actions can never return a span:ForGlobalVariable.target_spanis never populated by the parser andForPlayerVariablehas no such field. The None enumeration omits them, leaving the coverage ambiguous between "names an identifier" and "other action forms always returnNone."
Test design
All four new tests verdict keep — each protects a distinct contract at the smallest surface. Findings:
Vectorchild paths0/1/2documented but never asserted — a transposed or missingVectorarm inwir_value_childrenwould pass.PlayerVariableplayer child traversed but never observed —[0,0,0]→Noneis satisfied even if thePlayerVariablearm is omitted.Set Player Variable(Event Player, playerScore, 7)inIDENTIFIER_SOURCEhas no assertion — a distinct recording boundary (standard player form) left unobserved.- Control-flow provenance plumbing untested —
Disabledwrapping is the trickiest path (public position ↔ inner action provenance) and nothing exercises it. - Out-of-range
argumentindex uncovered. - Recording-boundary
None/keyword assertions correctly describe current parser coverage but don't mark it as pending #325, so a reader may treat them as permanent contract.
Findings are being addressed in a follow-up commit on this branch.
…ce tests Review findings: assert Vector component and PlayerVariable player child paths, the standard Set Player Variable boundary, disabled-action provenance, and out-of-range argument/path positions; note that the None assertions describe current parser coverage pending #325.
Review finding: for-variable actions were listed among the identifier-bearing forms but can never return a span — ForGlobalVariable's target_span is never populated and ForPlayerVariable has no field.
|
Review findings addressed and pushed:
Verified: |
…nd calls The raw Workshop parser now records the declared identifier at every position that names a variable or subroutine: variable and subroutine declaration sections, Set/Modify/For write targets, Call Subroutine and Start Rule callees, Subroutine event bindings, indexed-write name arguments, and reads written as Global.name, Global/Player Variable(name), Event Player.name, or a bare declared name. ValueNode carries a dedicated identifier slot so a node's own span keeps its expression meaning; Event::Subroutine becomes a struct variant with name_span; For Player Variable gains target_span. The public provenance accessors return the recorded identifier for references, and the WIR validator now checks every recorded span so the every-span-is-valid invariant covers identifier fields too. Fixes #325
…ovenance
Address review findings on the identifier-provenance work:
- `for_group`/`for_player_group` recorded `Variable` (the phrase's last
word) as the action span start; group parsers now take the phrase start
so `For`/`While`/`If` spans cover their keyword.
- `wir::Rule.name_span` is populated (inside the `rule("name")` string
token), carried through `RuleProvenance`, and exposed as
`Program::rule_name_span`.
- `Call Subroutine`/`Start Rule`/subroutine-event failures to resolve a
name now report the recorded name span in the `Unknown` diagnostic.
- `action_identifier_span` documentation no longer claims indexed writes;
those names live on the lowered call's variable argument, and the source
docs note `set_*` does not cover identifier-level provenance.
- Drop redundant `While`/`For` value-provenance reapplication and simplify
the declaration-line span branch that was vacuous.
- Extend the identifier provenance test over indexed writes,
`Global/Player Variable(name)` reads, parenthesized `Event Player`
member reads, multi-word names, chase forms, and `If`/`Else If`
condition reads.
Program::validate resolved every recorded span by scanning the retained source text from the beginning to convert each line/column position into a byte offset. The identifier and nested-value spans added in #326 roughly tripled the number of checked spans, so this repeated full-text scan dominated large real-project workloads: on the 448 KB bastion.ow fixture, Program::validate took ~8.9s in release and ~146s in debug builds. SourceDocument now records each line's byte offset once, and position resolution scans only the addressed line. Span semantics are unchanged: a differential test checks the indexed resolution against the original full-scan algorithm, and a scan-bound test fails if full-document rescans return. The bastion.ow stage benchmark is kept as an ignored performance measurement (release: validate ~10.3ms, element_count ~15.5ms; debug: validate ~119ms). Fixes #331 Co-authored-by: Teakowa <git@teakowa.dev>
Summary
Fixes #324. Fixes #325.
Exposes identifier provenance through new public
Programaccessors and records the previously missing identifier spans in the raw Workshop parser, so every source position that names a variable or subroutine carries an exact span.global_variable_name_span/player_variable_name_span/subroutine_name_spanreturn the declared-name span of each declaration: an attachedname_spanwhen a provider supplies one viaset_*_spans, else the recorded declaration span, which raw parses store as the name itself.action_identifier_spanreturns the recorded span of the variable or subroutine an action names: a set/modify/for target or aCall Subroutinecallee. Indexed... Variable At Indexwrites lower to calls and record the name on their variable argument, reachable throughaction_argument_value_span.rule_name_spanreturns the span of the name inside a rule'srule("name")string;rule_event_name_spanreturns the subroutine name aSubroutineevent binding names.condition_value_span/action_argument_value_spanreturn the recorded span of a value nested inside a condition or action argument, addressed by a child path into the publicValuetree; variable and subroutine references return their recorded identifier span.ProgramProvenancekeeps aValueProvenancetree per condition and action argument plus anidentifierper action;to_wirwrites nested spans and identifiers back so diagnostics and dumps keep them.Call Subroutine/Start Rulecallees,Subroutineevent bindings,... At Indexname arguments, andGlobal.name/Global Variable(name)/Event Player.name/bare-name reads;wir::Event::Subroutinecarriesname_span, variable actions carrytarget_span/callee_span, andValueNodecarries anidentifierslot validated like every other span.SourceMapformat is unchanged; nested-value and action-identifier mappings remain outsidemapped-text-v1, and noset_*method exists for identifier-level provenance.Test plan
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace --all-targetscargo run -p workshop-rs --bin workshop-catalog-gen -- checkgit diff --checkprogram_modelintegration test slices source text through every accessor: declarations, set/modify/for targets, infix and indexed writes,Call Subroutine/Start Rule/Subroutineevent names,Global Variable()/Player Variable()/bare-name/parenthesized reads, multi-word names, and string/comment lookalikes that must not resolve as identifiers