Skip to content

feat(api): expose recorded identifier and nested-value provenance - #326

Merged
e54-bot merged 5 commits into
mainfrom
feat/identifier-provenance-324
Sep 29, 2026
Merged

e54-bot merged 5 commits into
mainfrom
feat/identifier-provenance-324

Conversation

@e54-bot

@e54-bot e54-bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #324. Fixes #325.

Exposes identifier provenance through new public Program accessors 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_span return the declared-name span of each declaration: an attached name_span when a provider supplies one via set_*_spans, else the recorded declaration span, which raw parses store as the name itself.
  • action_identifier_span returns the recorded span of the variable or subroutine an action names: a set/modify/for target or a Call Subroutine callee. Indexed ... Variable At Index writes lower to calls and record the name on their variable argument, reachable through action_argument_value_span.
  • rule_name_span returns the span of the name inside a rule's rule("name") string; rule_event_name_span returns the subroutine name a Subroutine event binding names.
  • condition_value_span / action_argument_value_span return the recorded span of a value nested inside a condition or action argument, addressed by a child path into the public Value tree; variable and subroutine references return their recorded identifier span.
  • ProgramProvenance keeps a ValueProvenance tree per condition and action argument plus an identifier per action; to_wir writes nested spans and identifiers back so diagnostics and dumps keep them.
  • The raw parser records identifier spans for declarations, set/modify/for targets, Call Subroutine/Start Rule callees, Subroutine event bindings, ... At Index name arguments, and Global.name/Global Variable(name)/Event Player.name/bare-name reads; wir::Event::Subroutine carries name_span, variable actions carry target_span/callee_span, and ValueNode carries an identifier slot validated like every other span.
  • SourceMap format is unchanged; nested-value and action-identifier mappings remain outside mapped-text-v1, and no set_* method exists for identifier-level provenance.

Test plan

  • cargo fmt --all --check
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo test --workspace --all-targets
  • cargo run -p workshop-rs --bin workshop-catalog-gen -- check
  • git diff --check
  • program_model integration test slices source text through every accessor: declarations, set/modify/for targets, infix and indexed writes, Call Subroutine/Start Rule/Subroutine event names, Global Variable()/Player Variable()/bare-name/parenthesized reads, multi-word names, and string/comment lookalikes that must not resolve as identifiers

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 e54-bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. program.rs:378-387 + docs/source-preservation.md — action_identifier_span enumerates "set/modify and for-variable actions" as identifier-bearing, but for-variable actions can never return a span: ForGlobalVariable.target_span is never populated by the parser and ForPlayerVariable has no such field. The None enumeration omits them, leaving the coverage ambiguous between "names an identifier" and "other action forms always return None."

Test design

All four new tests verdict keep — each protects a distinct contract at the smallest surface. Findings:

  1. Vector child paths 0/1/2 documented but never asserted — a transposed or missing Vector arm in wir_value_children would pass.
  2. PlayerVariable player child traversed but never observed — [0,0,0]→None is satisfied even if the PlayerVariable arm is omitted.
  3. Set Player Variable(Event Player, playerScore, 7) in IDENTIFIER_SOURCE has no assertion — a distinct recording boundary (standard player form) left unobserved.
  4. Control-flow provenance plumbing untested — Disabled wrapping is the trickiest path (public position ↔ inner action provenance) and nothing exercises it.
  5. Out-of-range argument index uncovered.
  6. 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.
@e54-bot

e54-bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Review findings addressed and pushed:

  • ee50ca2 — test coverage: Vector component path [2] → "3", PlayerVariable player child [0,0] → "Event Player", standard Set Player Variable boundary None, disabled Modify Global Variable(...) provenance through the wrapper → "playerScore", out-of-range argument index None; recording-boundary None assertions now marked as current parser coverage pending Record identifier spans in the raw Workshop parser #325.
  • 657dcda — docs: action_identifier_span and docs/source-preservation.md now list For variable loops among the forms that record no identifier.

Verified: cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --test integration program_model (17/17) all green. Ready for follow-up review.

…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.
@e54-bot
e54-bot merged commit b083e78 into main Sep 29, 2026
10 checks passed
@e54-bot
e54-bot deleted the feat/identifier-provenance-324 branch September 29, 2026 17:23
Teakowa added a commit that referenced this pull request Sep 30, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Record identifier spans in the raw Workshop parser Expose identifier provenance for variable and subroutine declarations and uses

2 participants