From c23dc738e8af5994d3bd9c57ba0213bba2f3e868 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Wed, 30 Sep 2026 01:56:53 +0800 Subject: [PATCH 1/3] deps: bump workshop-rs to 1.1.0 for identifier provenance The 1.1.0 release records identifier-level source spans for declarations, action targets and callees, and nested values (wrightkit/workshop-rs#324, wrightkit/workshop-rs#325), which Wright consumes to report exact reference and symbol locations. Refs #433 --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 055d6fc4..67f16568 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2082,9 +2082,9 @@ checksum = "f17a85883d4e6d00e8a97c586de764dabcc06133f7f1d55dce5cdc070ad7fe59" [[package]] name = "workshop-rs" -version = "1.0.0" +version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "505565b6069a1403594a9b217a072e810c85770176b3aeb12bac45373ea8cde5" +checksum = "c69d47bf47502c2d70fdc7b248d0a2ba210466643b81f4fae5af455e3e95fc70" dependencies = [ "serde", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index 96bc8d8c..87ed31a1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,7 +11,7 @@ repository = "https://github.com/wrightkit/wright" [workspace.dependencies] # Canonical Workshop core; SemVer-compatible requirement on the crates.io release. -workshop-rs = "1.0.0" +workshop-rs = "1.1.0" libc = "0.2" serde = "1" serde_json = "1" From 2d7ae8126bdce8905303b6e299c8d06c9b6b8ef6 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Wed, 30 Sep 2026 01:57:03 +0800 Subject: [PATCH 2/3] feat(analyzer): report identifier spans from workshop-rs provenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raw Workshop references and symbol declarations now take their spans from the parser-recorded provenance accessors (global/player/subroutine/rule name spans, action identifier spans, condition and action-argument value paths) instead of re-searching the source text inside a coarse enclosing span. visit_value_tree carries the child-position path the provenance API addresses values by, and analysis findings point at the offending call expression rather than a whole enclosing action. Without recorded provenance — a programmatically built program or a provider-attached program without identifier spans — spans report as unmapped instead of falling back to an enclosing span. The provider-side text search remains only in SemanticIndex::build_with_sources, which resolves provider-file occurrences for OPY/DEL sources. Closes #433 --- .../wright-analyzer/src/canonical/analysis.rs | 63 +++--- .../wright-analyzer/src/canonical/symbols.rs | 123 ++++++----- .../src/canonical/traversal.rs | 33 ++- .../tests/workshop_integration.rs | 191 +++++++++++++++++- 4 files changed, 323 insertions(+), 87 deletions(-) diff --git a/crates/wright-analyzer/src/canonical/analysis.rs b/crates/wright-analyzer/src/canonical/analysis.rs index a7957093..b7ea42f7 100644 --- a/crates/wright-analyzer/src/canonical/analysis.rs +++ b/crates/wright-analyzer/src/canonical/analysis.rs @@ -4,7 +4,7 @@ use workshop_rs::source::Span; use workshop_rs::{Action, Event, ModifyOp, Program, Rule, Value}; use super::cfg::{is_wait, matching_end}; -use super::symbols::{ActionId, RuleId, ValueId, source_occurrence, value_identity_map}; +use super::symbols::{ActionId, RuleId, ValueId, value_identity_map}; use super::traversal::{visit_action_roots, visit_value_tree}; use crate::analysis::{Boundedness, EvidenceClass, Severity}; use crate::registry::LintConfig; @@ -45,24 +45,28 @@ pub fn analyze(program: &Program, config: &LintConfig) -> Vec { } if config.is_enabled("expensive-loop-check") { for (offset, body_action) in body.iter().enumerate() { + let body_action_id = start + offset; let mut expensive = Vec::new(); - visit_action_roots(body_action, &mut |_, value| { - collect_expensive_values(value, &mut expensive) + visit_action_roots(body_action, &mut |argument, value| { + for (path, value) in collect_expensive_values(value) { + expensive.push((argument, path, value)); + } }); - for value in expensive { - let Value::Call { name, .. } = value else { - unreachable!("only expensive calls are collected") - }; + for (argument, path, value) in expensive { + debug_assert!( + matches!(value, Value::Call { .. }), + "only expensive calls are collected" + ); findings.push(Finding { code: "expensive-loop-check".into(), severity: Severity::Info, message: "geometry predicate evaluated inside a loop body may be expensive per iteration" .into(), - span: source_occurrence( - program, - program.action_span(rule_id, start + offset), - name, - false, + span: program.action_argument_value_span( + rule_id, + body_action_id, + argument, + &path, ), rule: rule_id, action: Some(action_id), @@ -202,9 +206,8 @@ fn ongoing_condition_findings( let condition_count = active_conditions.len(); let mut findings = Vec::new(); for (index, &(source_index, condition)) in active_conditions.iter().enumerate() { - let mut expensive = Vec::new(); - collect_expensive_values(&condition.value, &mut expensive); - for value in expensive { + let expensive = collect_expensive_values(&condition.value); + for (path, value) in expensive { let preceding = index; let later = condition_count - index - 1; let evaluation = match preceding { @@ -220,11 +223,7 @@ fn ongoing_condition_findings( if later == 1 { "" } else { "s" } ) }; - let name = match value { - Value::Call { name, .. } => name, - _ => unreachable!("only expensive call values are collected"), - }; - let span = program.condition_span(rule_id, source_index); + let span = program.condition_value_span(rule_id, source_index, &path); findings.push(Finding { code: "ongoing-condition-hot-path".into(), severity: Severity::Info, @@ -232,7 +231,7 @@ fn ongoing_condition_findings( "geometry predicate in an ongoing-rule condition {} of {condition_count} {evaluation}{later_gates}; its cost is heuristic, not measured runtime load", index + 1, ), - span: source_occurrence(program, span, name, false), + span, rule: rule_id, action: None, value: value_ids.get(&(value as *const Value as usize)).copied(), @@ -244,15 +243,17 @@ fn ongoing_condition_findings( findings } -fn collect_expensive_values<'a>(value: &'a Value, out: &mut Vec<&'a Value>) { - visit_value_tree(value, None, &mut |value, _| { +fn collect_expensive_values(value: &Value) -> Vec<(Vec, &Value)> { + let mut out = Vec::new(); + visit_value_tree(value, None, &mut |value, _, path| { if let Value::Call { name, .. } = value { if ["distance", "raycast", "isInLoS"].contains(&name.as_str()) { - out.push(value); + out.push((path.to_vec(), value)); } } 0 }); + out } fn duplicate_condition_findings( @@ -309,7 +310,7 @@ fn repeated_value_findings( collect_value_tree( condition, None, - program.action_argument_span(rule_id, loop_action, 0), + &mut |path| program.action_argument_value_span(rule_id, loop_action, 0, path), &mut values, &mut parents, &mut spans, @@ -332,7 +333,9 @@ fn repeated_value_findings( collect_value_tree( value, None, - program.action_argument_span(rule_id, action_id, argument), + &mut |path| { + program.action_argument_value_span(rule_id, action_id, argument, path) + }, &mut values, &mut parents, &mut spans, @@ -369,16 +372,16 @@ fn repeated_value_findings( fn collect_value_tree<'a>( value: &'a Value, parent: Option, - span: Option, + span_at: &mut impl FnMut(&[usize]) -> Option, values: &mut Vec<&'a Value>, parents: &mut Vec>, spans: &mut Vec>, ) { - visit_value_tree(value, parent, &mut |value, parent| { + visit_value_tree(value, parent, &mut |value, parent, path| { let index = values.len(); values.push(value); parents.push(parent); - spans.push(span); + spans.push(span_at(path)); index }); } @@ -432,7 +435,7 @@ fn duplicated_value_families(values: &[&Value], parents: &[Option]) -> Ve fn value_call_count(value: &Value) -> usize { let mut count = 0; - visit_value_tree(value, None, &mut |value, _| { + visit_value_tree(value, None, &mut |value, _, _| { count += usize::from(matches!(value, Value::Call { .. })); 0 }); diff --git a/crates/wright-analyzer/src/canonical/symbols.rs b/crates/wright-analyzer/src/canonical/symbols.rs index 1970bb21..e53f464a 100644 --- a/crates/wright-analyzer/src/canonical/symbols.rs +++ b/crates/wright-analyzer/src/canonical/symbols.rs @@ -25,12 +25,16 @@ impl SymbolId { fn append_named_symbols<'a>( symbols: &mut Vec, program: &Program, - named: impl Iterator, + named: impl Iterator, ) { - for (kind, name) in named { - let prefix = kind.declaration_prefix(); + for (kind, name, index) in named { + let occurrence = match kind { + SymbolKind::GlobalVariable => program.global_variable_name_span(index), + SymbolKind::PlayerVariable => program.player_variable_name_span(index), + SymbolKind::Subroutine => program.subroutine_name_span(index), + SymbolKind::Rule => None, + }; let id = SymbolId::from_index(symbols.len()); - let occurrence = declaration_span(program, prefix, name); symbols.push(Symbol { id, kind, @@ -126,7 +130,7 @@ pub(super) fn value_identity_map(program: &Program) -> HashMap { let mut identities = HashMap::new(); for rule in &program.rules { for condition in &rule.conditions { - visit_value_tree(&condition.value, None, &mut |value, _| { + visit_value_tree(&condition.value, None, &mut |value, _, _| { let id = identities.len(); identities.insert((value as *const Value) as usize, id); 0 @@ -134,7 +138,7 @@ pub(super) fn value_identity_map(program: &Program) -> HashMap { } for action in &rule.actions { visit_action_roots(action, &mut |_, root| { - visit_value_tree(root, None, &mut |value, _| { + visit_value_tree(root, None, &mut |value, _, _| { let id = identities.len(); identities.insert((value as *const Value) as usize, id); 0 @@ -154,18 +158,27 @@ impl SemanticIndex { program .global_variables .iter() - .map(|variable| (SymbolKind::GlobalVariable, variable.name.as_str())) + .enumerate() + .map(|(index, variable)| { + (SymbolKind::GlobalVariable, variable.name.as_str(), index) + }) .chain( program .player_variables .iter() - .map(|variable| (SymbolKind::PlayerVariable, variable.name.as_str())), + .enumerate() + .map(|(index, variable)| { + (SymbolKind::PlayerVariable, variable.name.as_str(), index) + }), ) .chain( program .subroutines .iter() - .map(|subroutine| (SymbolKind::Subroutine, subroutine.name.as_str())), + .enumerate() + .map(|(index, subroutine)| { + (SymbolKind::Subroutine, subroutine.name.as_str(), index) + }), ), ); for (rule, data) in program.rules.iter().enumerate() { @@ -175,7 +188,9 @@ impl SemanticIndex { kind: SymbolKind::Rule, name: data.name.clone(), span: program.rule_span(rule), - occurrence: program.rule_span(rule), + occurrence: program + .rule_name_span(rule) + .or_else(|| program.rule_span(rule)), rule: Some(rule), }); } @@ -192,7 +207,7 @@ impl SemanticIndex { &value.value, rule, None, - program.condition_span(rule, condition), + ValueRoot::Condition(condition), program, ); } @@ -255,8 +270,16 @@ impl SemanticIndex { .and_then(|action| program.action_span(rule, action)) }); let implicit_modify = reference.value.is_none() - && reference.span.is_some() - && reference.span == action_span; + && reference + .rule + .zip(reference.action) + .and_then(|(rule, action)| { + program + .rules + .get(rule) + .and_then(|rule| rule.actions.get(action)) + }) + .is_some_and(is_modify_action); let current = if implicit_modify { 1 } else { *ordinal }; *ordinal += 1; reference @@ -377,7 +400,7 @@ impl SemanticIndex { Some(rule), None, None, - source_occurrence(program, program.rule_span(rule), name, true), + program.rule_event_name_span(rule), ); } } @@ -389,7 +412,7 @@ impl SemanticIndex { action_id: ActionId, program: &Program, ) { - let span = program.action_span(rule, action_id); + let identifier = program.action_identifier_span(rule, action_id); let variable = match action { Action::SetGlobalVariable { variable, .. } => { Some((SymbolKind::GlobalVariable, variable, false)) @@ -419,7 +442,7 @@ impl SemanticIndex { Some(rule), Some(action_id), None, - source_occurrence(program, span, name, true), + identifier, ); if reads_old_value { self.push( @@ -428,7 +451,7 @@ impl SemanticIndex { Some(rule), Some(action_id), None, - span, + identifier, ); } } @@ -442,7 +465,7 @@ impl SemanticIndex { Some(rule), Some(action_id), None, - span, + identifier, ); } } @@ -457,7 +480,10 @@ impl SemanticIndex { value, rule, Some(action_id), - program.action_argument_span(rule, action_id, argument), + ValueRoot::Argument { + action: action_id, + argument, + }, program, ); }); @@ -467,12 +493,13 @@ impl SemanticIndex { value: &Value, rule: RuleId, action: Option, - span: Option, + root: ValueRoot, program: &Program, ) { - visit_value_tree(value, None, &mut |value, _| { + visit_value_tree(value, None, &mut |value, _, path| { let value_id = self.next_value_id; self.next_value_id += 1; + let span = root.span(program, rule, path); match value { Value::GlobalVariable(name) => { if let Some(symbol) = self.find_symbol(SymbolKind::GlobalVariable, name) { @@ -482,7 +509,7 @@ impl SemanticIndex { Some(rule), action, Some(value_id), - source_occurrence(program, span, name, false), + span, ); } } @@ -494,7 +521,7 @@ impl SemanticIndex { Some(rule), action, Some(value_id), - source_occurrence(program, span, variable, false), + span, ); } } @@ -505,19 +532,32 @@ impl SemanticIndex { } } -fn declaration_span(program: &Program, prefix: &str, name: &str) -> Option { - for file_index in 0..64 { - let file = workshop_rs::source::FileId::from_index(file_index); - let Some(source) = program.source(file) else { - continue; - }; - if let Some(span) = - declaration_span_in_source(file, source.text(), std::slice::from_ref(&prefix), name) - { - return Some(span); +/// The provenance root a value tree hangs from: a rule condition or one action +/// argument. Paths under it address nested values through +/// `Program::condition_value_span` / `Program::action_argument_value_span`. +#[derive(Clone, Copy)] +enum ValueRoot { + Condition(usize), + Argument { action: usize, argument: usize }, +} + +impl ValueRoot { + fn span(self, program: &Program, rule: usize, path: &[usize]) -> Option { + match self { + Self::Condition(condition) => program.condition_value_span(rule, condition, path), + Self::Argument { action, argument } => { + program.action_argument_value_span(rule, action, argument, path) + } } } - None +} + +fn is_modify_action(action: &Action) -> bool { + match action { + Action::ModifyGlobalVariable { .. } | Action::ModifyPlayerVariable { .. } => true, + Action::Disabled { action } => is_modify_action(action), + _ => false, + } } fn declaration_span_in_sources( @@ -672,18 +712,3 @@ fn is_code_position(chars: &[char], position: usize) -> bool { } !quoted } - -pub(super) fn source_occurrence( - program: &Program, - span: Option, - name: &str, - before_assignment: bool, -) -> Option { - let span = span?; - let Some(source_doc) = program.source(span.file) else { - return Some(span); - }; - Some( - find_occurrence(source_doc.text(), span, name, before_assignment, 0, false).unwrap_or(span), - ) -} diff --git a/crates/wright-analyzer/src/canonical/traversal.rs b/crates/wright-analyzer/src/canonical/traversal.rs index 25f8cd7b..59980f81 100644 --- a/crates/wright-analyzer/src/canonical/traversal.rs +++ b/crates/wright-analyzer/src/canonical/traversal.rs @@ -46,24 +46,43 @@ pub(super) fn visit_action_roots<'a>(action: &'a Action, visit: &mut impl FnMut( } } +/// Visit a value tree in pre-order. `path` passed to `visit` is the child's +/// position chain matching `Program::condition_value_span` / +/// `Program::action_argument_value_span`: `Array`/`Call` children by index, +/// `Vector` components as 0/1/2, a `PlayerVariable` player at 0. pub(super) fn visit_value_tree<'a>( value: &'a Value, parent: Option, - visit: &mut impl FnMut(&'a Value, Option) -> usize, + visit: &mut impl FnMut(&'a Value, Option, &[usize]) -> usize, ) { - let parent = Some(visit(value, parent)); + let mut path = Vec::new(); + visit_value_tree_at(value, parent, &mut path, visit); +} + +fn visit_value_tree_at<'a>( + value: &'a Value, + parent: Option, + path: &mut Vec, + visit: &mut impl FnMut(&'a Value, Option, &[usize]) -> usize, +) { + let parent = Some(visit(value, parent, path)); + let mut visit_child = |index: usize, value: &'a Value| { + path.push(index); + visit_value_tree_at(value, parent, path, visit); + path.pop(); + }; match value { Value::Array(values) | Value::Call { args: values, .. } => { - for value in values { - visit_value_tree(value, parent, visit); + for (index, value) in values.iter().enumerate() { + visit_child(index, value); } } Value::Vector { x, y, z } => { - for value in [x, y, z] { - visit_value_tree(value, parent, visit); + for (index, value) in [x, y, z].iter().enumerate() { + visit_child(index, value); } } - Value::PlayerVariable { player, .. } => visit_value_tree(player, parent, visit), + Value::PlayerVariable { player, .. } => visit_child(0, player), _ => {} } } diff --git a/crates/wright-analyzer/tests/workshop_integration.rs b/crates/wright-analyzer/tests/workshop_integration.rs index f19842e7..68781b15 100644 --- a/crates/wright-analyzer/tests/workshop_integration.rs +++ b/crates/wright-analyzer/tests/workshop_integration.rs @@ -67,6 +67,26 @@ fn id_for(service: &SemanticService<'_>, request: Value, name: &str) -> u32 { .unwrap_or_else(|| panic!("query result has no item named {name}")) as u32 } +fn span_text(source: &str, span: &Value) -> String { + let (start, end) = (&span["start"], &span["end"]); + assert_eq!( + start["line"], end["line"], + "test spans stay on one line: {span}" + ); + let line = source + .lines() + .nth(start["line"].as_u64().unwrap() as usize - 1) + .unwrap(); + let (start_col, end_col) = ( + start["col"].as_u64().unwrap() as usize, + end["col"].as_u64().unwrap() as usize, + ); + line.chars() + .skip(start_col - 1) + .take(end_col - start_col) + .collect() +} + #[test] fn workshop_input_runs_all_semantic_queries() { let service = workshop_service("synthetic/control-flow"); @@ -551,6 +571,175 @@ rule ("argument spans") { ); for (reference, (line, column)) in reads.iter().zip(expected) { assert_eq!(reference["span"]["start"]["line"], line); - assert_eq!(reference["span"]["start"]["col"], column); + // The reported span is the variable identifier, which follows the + // `Global.` qualifier in the authored text. + assert_eq!(reference["span"]["start"]["col"], column + "Global.".len()); + assert_eq!(span_text(source, &reference["span"]), "source"); + } +} + +#[test] +fn workshop_references_slice_to_the_identifier() { + // #433: every declaration, write, and read span reported for a raw + // Workshop symbol slices exactly to the authored identifier — including + // reads nested inside other values (`Add(First Of(Global.cakePos), …)`) + // and `For` targets. + let source = fixture_text("real-world/overpy-cake"); + let service = workshop_service_from_text(&source); + for name in ["cakePos", "i2", "candlePos"] { + let symbol = id_for(&service, serde_json::json!({"op": "listSymbols"}), name); + let references = query( + &service, + serde_json::json!({"op": "findReferences", "symbol": symbol}), + ); + let references = references.as_array().unwrap(); + // `Modify Global Variable(name, …)` reports both the write and the + // implicit old-value read at the same identifier span. + let implicit_modify_reads = source + .match_indices(&format!("Modify Global Variable({name}")) + .count(); + assert_eq!( + references.len(), + source.match_indices(name).count() + implicit_modify_reads, + "every authored {name} occurrence is one reference, no more and no fewer" + ); + assert!( + references + .iter() + .any(|reference| reference["kind"] == "declaration"), + "{name} references include the declaration: {references:?}" + ); + for reference in references { + assert_eq!( + span_text(&source, &reference["span"]), + name, + "reference span slices to the {name} identifier: {reference:?}" + ); + } + } +} + +#[test] +fn workshop_symbols_report_their_declaration_identifier() { + // #433: `symbols` returns the recorded declaration-name span, not a + // null or a search-derived guess. + let source = fixture_text("real-world/overpy-cake"); + let service = workshop_service_from_text(&source); + let symbols = query(&service, serde_json::json!({"op": "listSymbols"})); + let symbols = symbols.as_array().unwrap(); + for name in ["cakePos", "i2", "candlePos"] { + let symbol = symbols + .iter() + .find(|symbol| symbol["name"] == name) + .unwrap_or_else(|| panic!("listSymbols has {name}")); + assert_eq!(symbol["kind"], "globalVariable"); + assert_eq!( + span_text(&source, &symbol["span"]), + name, + "symbol span slices to the declaration name: {symbol:?}" + ); + } +} + +#[test] +fn workshop_reference_spans_ignore_same_name_strings_and_comments() { + // #433: a variable name spelled inside a string literal or comment in + // the same action is not reported as the reference location. + let source = r#" +variables { + global: + 0: needle + 1: target +} +rule ("same-name text") { + event { + Ongoing - Global; + } + actions { + // needle in a comment is not a reference + Big Message(All Players(All Teams), Custom String("needle {0}", Global.needle)); + Set Global Variable(target, Global.needle); // needle + } +} +"#; + let service = workshop_service_from_text(source); + let symbol = id_for(&service, serde_json::json!({"op": "listSymbols"}), "needle"); + let references = query( + &service, + serde_json::json!({"op": "findReferences", "symbol": symbol}), + ); + let references = references.as_array().unwrap(); + // Declaration + the two authored reads; the comment and string + // occurrences are never reported. + assert_eq!(references.len(), 3, "{references:?}"); + for reference in references { + assert_eq!( + span_text(source, &reference["span"]), + "needle", + "reference must slice the real identifier, not the literal: {reference:?}" + ); + } + let (string_line_number, string_line) = source + .lines() + .enumerate() + .find(|(_, line)| line.contains("Custom String")) + .map(|(index, line)| (index + 1, line)) + .unwrap(); + let literal_column = string_line.find("needle").unwrap() + 1; + let read_column = string_line.find("Global.needle").unwrap() + "Global.".len() + 1; + let nested_read = references + .iter() + .find(|reference| { + reference["kind"] == "read" + && reference["span"]["start"]["line"] == string_line_number as u64 + }) + .expect("the read nested in Custom String is reported"); + assert_ne!( + nested_read["span"]["start"]["col"], literal_column, + "the nested read must not point inside the string literal" + ); + assert_eq!(nested_read["span"]["start"]["col"], read_column); +} + +#[test] +fn workshop_references_without_provenance_are_unmapped() { + // #433: a reference without recorded provenance reports an unmapped + // span; it never falls back to an enclosing action or value span. + let mut program = Program::new(); + program.global_variable(Variable::new("unmapped")); + program.rule( + Rule::new("built", Event::Global).action(Action::SetGlobalVariable { + variable: "unmapped".into(), + value: WorkshopValue::GlobalVariable("unmapped".into()), + }), + ); + let program = Box::leak(Box::new(program)); + let service = SemanticService::new(program); + let symbols = query(&service, serde_json::json!({"op": "listSymbols"})); + let symbol = symbols + .as_array() + .unwrap() + .iter() + .find(|symbol| symbol["name"] == "unmapped") + .unwrap(); + assert!( + symbol["span"].is_null(), + "no recorded declaration provenance -> unmapped span: {symbol}" + ); + let references = query( + &service, + serde_json::json!({"op": "findReferences", "symbol": symbol["id"].as_u64().unwrap()}), + ); + let references = references.as_array().unwrap(); + assert_eq!( + references.len(), + 2, + "one write and one read: {references:?}" + ); + for reference in references { + assert!( + reference["span"].is_null(), + "a reference without provenance is unmapped, not the enclosing span: {reference:?}" + ); } } From b48aba49f520d7c7085bd898813af9e2c85fcef3 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Wed, 30 Sep 2026 02:49:43 +0800 Subject: [PATCH 3/3] fix(analyzer): drop the misindexed condition fallback and widen span coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build_with_sources' read fallback passed the ValueId as a condition index to condition_span — wrong or out of range for multi-condition rules. The fallback is removed rather than re-indexed: provider condition reads resolve through the mapped root span, and nested reads without recorded provenance report unmapped. Tests now also slice subroutine declaration/definition/call spans and a nested Event Player variable read to their identifiers. Refs #433 --- .../wright-analyzer/src/canonical/symbols.rs | 17 +---- .../tests/workshop_integration.rs | 72 +++++++++++++++++++ 2 files changed, 75 insertions(+), 14 deletions(-) diff --git a/crates/wright-analyzer/src/canonical/symbols.rs b/crates/wright-analyzer/src/canonical/symbols.rs index e53f464a..1d4dd80b 100644 --- a/crates/wright-analyzer/src/canonical/symbols.rs +++ b/crates/wright-analyzer/src/canonical/symbols.rs @@ -282,20 +282,9 @@ impl SemanticIndex { .is_some_and(is_modify_action); let current = if implicit_modify { 1 } else { *ordinal }; *ordinal += 1; - reference - .span - .or_else(|| { - action_span.or_else(|| { - reference.rule.and_then(|rule| { - reference - .value - .and_then(|value| program.condition_span(rule, value)) - }) - }) - }) - .and_then(|span| { - occurrence_in_sources(sources, span, &symbol.name, false, current) - }) + reference.span.or(action_span).and_then(|span| { + occurrence_in_sources(sources, span, &symbol.name, false, current) + }) } }; reference.span = span; diff --git a/crates/wright-analyzer/tests/workshop_integration.rs b/crates/wright-analyzer/tests/workshop_integration.rs index 68781b15..057eff24 100644 --- a/crates/wright-analyzer/tests/workshop_integration.rs +++ b/crates/wright-analyzer/tests/workshop_integration.rs @@ -701,6 +701,78 @@ rule ("same-name text") { assert_eq!(nested_read["span"]["start"]["col"], read_column); } +#[test] +fn workshop_subroutine_references_slice_to_the_identifier() { + // Declaration (subroutines table), definition (event binding), and call + // each slice to `showStatus`. + let source = fixture_text("synthetic/declarations-rules"); + let service = workshop_service("synthetic/declarations-rules"); + let symbol = id_for( + &service, + serde_json::json!({"op": "listSymbols"}), + "showStatus", + ); + let references = query( + &service, + serde_json::json!({"op": "findReferences", "symbol": symbol}), + ); + let references = references.as_array().unwrap(); + for kind in ["declaration", "definition", "call"] { + assert_eq!( + references + .iter() + .filter(|reference| reference["kind"] == kind) + .count(), + 1, + "one {kind} reference for showStatus: {references:?}" + ); + } + for reference in references { + assert_eq!(span_text(&source, &reference["span"]), "showStatus"); + } +} + +#[test] +fn workshop_player_variable_reads_slice_to_the_identifier() { + let source = r#" +variables { + player: + 0: stamina +} +rule ("player reads") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + Set Player Variable(Event Player, stamina, Add(Event Player.stamina, 1)); + } +} +"#; + let service = workshop_service_from_text(source); + let symbol = id_for( + &service, + serde_json::json!({"op": "listSymbols"}), + "stamina", + ); + let references = query( + &service, + serde_json::json!({"op": "findReferences", "symbol": symbol}), + ); + let references = references.as_array().unwrap(); + // Declaration, the write target, and the nested `Event Player.stamina` + // read all slice to `stamina`. + assert_eq!(references.len(), 3, "{references:?}"); + for reference in references { + assert_eq!( + span_text(source, &reference["span"]), + "stamina", + "reference span slices to the stamina identifier: {reference:?}" + ); + } +} + #[test] fn workshop_references_without_provenance_are_unmapped() { // #433: a reference without recorded provenance reports an unmapped