From 73127aff69bf4d1d5a6c65a5b84d852db3725ebc Mon Sep 17 00:00:00 2001 From: Marco Roth Date: Wed, 19 Aug 2026 02:31:42 +0200 Subject: [PATCH] Analysis: Follow state into the locals it is assigned to A template reads state and then names it something else. `<% total = @items.size %>` puts `@items` in `total`, and `<%= total %>` was attributed to nothing, so changing `@items` looked like it touched one node when it decides two. This is the same shape as a block parameter, which carries state under another name for as long as its block is open, and it is bound the same way. A local carries whatever its right hand side reads, which makes chains work without anything extra: `a = @items.size` then `b = a * 2` puts both `a` and `b` in reach of `@items`. Only the right hand side counts, so `@items == other` binds nothing, since a comparison assigns nothing. Scope is the block that made the assignment. A local first assigned inside a block is gone when the block closes, the way Ruby has it, so an alias set is snapshotted on entry and restored on exit instead of subtracting what was added. A partial's declared locals are its state, and `dependency_index` enumerated only instance variables and constants, so asking what a partial's `query` reaches returned nothing at all. They are enumerated now. All three languages, with the same five templates asserted in each. Rust and TypeScript reach the assignment through the prism node they already carry, and slice the right hand side by the offsets it gives them. --- .../packages/analysis/src/dependency-index.ts | 44 +++++++++++- .../analysis/test/dependency-index.test.ts | 28 ++++++++ lib/herb/analysis/template_dependencies.rb | 2 +- .../node_dependency_collector.rb | 52 +++++++++++++- rust/herb-analysis/src/state_flow.rs | 71 +++++++++++++++++-- rust/herb-analysis/tests/state_flow_test.rs | 53 ++++++++++++++ .../node_dependency_collector.rbs | 10 +++ test/analysis/template_dependencies_test.rb | 46 ++++++++++++ 8 files changed, 293 insertions(+), 13 deletions(-) diff --git a/javascript/packages/analysis/src/dependency-index.ts b/javascript/packages/analysis/src/dependency-index.ts index 1da5beff5..0370574b6 100644 --- a/javascript/packages/analysis/src/dependency-index.ts +++ b/javascript/packages/analysis/src/dependency-index.ts @@ -38,6 +38,39 @@ function referencesAny(code: string | undefined, aliases: string[]): boolean { return aliases.some(alias => referencesState(code, alias)) } +const ASSIGNMENT_NODES = new Set([ + "LocalVariableWriteNode", + "LocalVariableOrWriteNode", + "LocalVariableAndWriteNode", + "LocalVariableOperatorWriteNode", +]) + +function assignedNames(node: Node, source: string, aliases: string[]): string[] { + const prism = (node as { prismNode?: unknown }).prismNode as any + + if (!prism) return [] + + const names: string[] = [] + + const walk = (candidate: any) => { + if (!candidate) return + + if (ASSIGNMENT_NODES.has(candidate.constructor?.name) && candidate.value?.location) { + const { startOffset, length } = candidate.value.location + const right = source.slice(startOffset, startOffset + length) + const name = String(candidate.name) + + if (referencesAny(right, aliases) && !names.includes(name)) names.push(name) + } + + for (const child of candidate.compactChildNodes?.() ?? []) walk(child) + } + + walk(prism) + + return names +} + function blockBindings(node: Node, aliases: string[]): string[] { if (!isERBBlockNode(node)) return [] if (!referencesAny(expressionOf(node), aliases)) return [] @@ -164,6 +197,13 @@ export function affectedNodes(backend: HerbBackend, source: string, state: strin record(node, "expression", expressionOf(node)) } + if (isERBContentNode(node)) { + for (const name of assignedNames(node, source, aliases)) { + if (!aliases.includes(name)) aliases.push(name) + } + } + + const outer = [...aliases] const bound = blockBindings(node, aliases) aliases.push(...bound) @@ -174,7 +214,7 @@ export function affectedNodes(backend: HerbBackend, source: string, state: strin path.pop() } - for (const name of bound) aliases.splice(aliases.indexOf(name), 1) + if (bound.length > 0) aliases.splice(0, aliases.length, ...outer) } for (const [index, child] of childrenOf(document).entries()) { @@ -190,7 +230,7 @@ export function dependencyIndex(backend: HerbBackend, file: string, source: stri const dependencies = collectTemplateDependencies(backend, file, source, options) const index: Record = {} - for (const state of [...dependencies.instanceVariables, ...dependencies.constants]) { + for (const state of [...dependencies.instanceVariables, ...dependencies.constants, ...dependencies.localsDeclared]) { const nodes = affectedNodes(backend, source, state) if (nodes.length > 0) index[state] = nodes diff --git a/javascript/packages/analysis/test/dependency-index.test.ts b/javascript/packages/analysis/test/dependency-index.test.ts index fb34b711d..2f30aa87c 100644 --- a/javascript/packages/analysis/test/dependency-index.test.ts +++ b/javascript/packages/analysis/test/dependency-index.test.ts @@ -15,6 +15,34 @@ describe("dependencyIndex", () => { return dependencyIndex(Herb, FILE, source) } + test("follows state into a local assigned from it", () => { + const nodes = affectedNodes(Herb, `<% total = @items.size %>

<%= total %>

`, "@items") + + expect(nodes.map(node => node.expression)).toEqual(["total = @items.size", "total"]) + }) + + test("follows state through a chain of assignments", () => { + const nodes = affectedNodes(Herb, `<% a = @items.size %><% b = a * 2 %>

<%= b %>

`, "@items") + + expect(nodes.map(node => node.expression)).toEqual(["a = @items.size", "b = a * 2", "b"]) + }) + + test("leaves a local assigned from something else", () => { + expect(affectedNodes(Herb, `<% other = 5 %>

<%= other %>

`, "@items")).toEqual([]) + }) + + test("does not take a comparison for an assignment", () => { + const nodes = affectedNodes(Herb, `<% if @items == other %>

<%= other %>

<% end %>`, "@items") + + expect(nodes.map(node => node.expression)).toEqual(["if @items == other"]) + }) + + test("stops a local assigned inside a block at the end of it", () => { + const nodes = affectedNodes(Herb, `<% @rows.each do |r| %><% inner = r.x %><%= inner %><% end %><%= inner %>`, "@rows") + + expect(nodes.map(node => node.expression)).toEqual(["@rows.each do |r|", "inner = r.x", "inner"]) + }) + test("follows state through a block parameter", () => { const nodes = affectedNodes(Herb, ``, "@items") diff --git a/lib/herb/analysis/template_dependencies.rb b/lib/herb/analysis/template_dependencies.rb index 7b41e0b05..ecb2f64fc 100644 --- a/lib/herb/analysis/template_dependencies.rb +++ b/lib/herb/analysis/template_dependencies.rb @@ -95,7 +95,7 @@ def dependency_index(file_path) index = {} #: Hash[String, Array[Hash[Symbol, untyped]]] - (result.instance_variables + result.constants).each do |state| + (result.instance_variables + result.constants + result.locals_declared).each do |state| nodes = affected_nodes(file_path, state) index[state] = nodes if nodes.any? end diff --git a/lib/herb/analysis/template_dependencies/node_dependency_collector.rb b/lib/herb/analysis/template_dependencies/node_dependency_collector.rb index 9a19bf334..969f5e595 100644 --- a/lib/herb/analysis/template_dependencies/node_dependency_collector.rb +++ b/lib/herb/analysis/template_dependencies/node_dependency_collector.rb @@ -8,6 +8,12 @@ class TemplateDependencies class NodeDependencyCollector < ::Herb::Visitor BRANCH_BODY_PROPERTIES = [:statements, :body, :children, :conditions].freeze #: Array[Symbol] BRANCH_CONTINUATION_PROPERTIES = [:subsequent, :else_clause, :rescue_clause, :ensure_clause].freeze #: Array[Symbol] + ASSIGNMENT_NODES = [ + Prism::LocalVariableWriteNode, + Prism::LocalVariableOrWriteNode, + Prism::LocalVariableAndWriteNode, + Prism::LocalVariableOperatorWriteNode, + ].freeze #: Array[untyped] attr_reader :affected @@ -42,6 +48,8 @@ def visit_html_open_tag_node(node) def visit_erb_content_node(node) check_erb_expression(node, :text_content) + + bind_assignments(node) end def visit_erb_if_node(node) @@ -125,8 +133,9 @@ def check_block_for_state(node, type) end def visit_branching_node(node) - bound = bindings_for(node) - @aliases.merge(bound) + outer = @aliases.dup + + @aliases.merge(bindings_for(node)) BRANCH_BODY_PROPERTIES.each do |property| next unless node.respond_to?(property) @@ -143,7 +152,44 @@ def visit_branching_node(node) visit(child) end ensure - @aliases.subtract(bound) if bound + @aliases.replace(outer) if outer + end + + def bind_assignments(node) + code = node.content&.value&.strip + + return unless code + + assigned_names(code).each { |name| @aliases.add(name) } + end + + def assigned_names(code) + result = Prism.parse(code) + + return [] if result.errors.any? + + names = [] #: Array[String] + collect_assignments(result.value, names) + + names + end + + def collect_assignments(node, names) + if ASSIGNMENT_NODES.any? { |type| node.is_a?(type) } + right = right_hand_side(node) + + names << node.name.to_s if right && references_state?(right) + end + + node.child_nodes.compact.each { |child| collect_assignments(child, names) } + end + + def right_hand_side(node) + return nil unless node.respond_to?(:value) + + value = node.value #: untyped + + value&.slice end def bindings_for(node) diff --git a/rust/herb-analysis/src/state_flow.rs b/rust/herb-analysis/src/state_flow.rs index 2940ed015..193a518ad 100644 --- a/rust/herb-analysis/src/state_flow.rs +++ b/rust/herb-analysis/src/state_flow.rs @@ -104,7 +104,7 @@ impl StateFlow { for (index, child) in result.value.children.iter().enumerate() { path.push(index); - collect_affected(child, &mut aliases, &mut path, &mut affected); + collect_affected(child, &source, &mut aliases, &mut path, &mut affected); path.pop(); } @@ -115,7 +115,12 @@ impl StateFlow { let result = self.analyze(file); let mut index = BTreeMap::new(); - for state in result.instance_variables.iter().chain(result.constants.iter()) { + for state in result + .instance_variables + .iter() + .chain(result.constants.iter()) + .chain(result.locals_declared.iter()) + { let nodes = self.affected_nodes(file, state); if !nodes.is_empty() { @@ -295,7 +300,7 @@ fn is_word_byte(byte: u8) -> bool { byte.is_ascii_alphanumeric() || byte == b'_' } -fn collect_affected(node: &AnyNode, aliases: &mut Vec, path: &mut Vec, affected: &mut Vec) { +fn collect_affected(node: &AnyNode, source: &str, aliases: &mut Vec, path: &mut Vec, affected: &mut Vec) { let kind = match node { AnyNode::ERBContentNode(_) => Some("text_content"), AnyNode::ERBIfNode(_) => Some("conditional"), @@ -326,17 +331,25 @@ fn collect_affected(node: &AnyNode, aliases: &mut Vec, path: &mut Vec bool { @@ -392,6 +405,50 @@ fn references_any(code: &str, aliases: &[String]) -> bool { aliases.iter().any(|name| expression_references(code, name)) } +const ASSIGNMENT_NODES: [&str; 4] = [ + "LocalVariableWriteNode", + "LocalVariableOrWriteNode", + "LocalVariableAndWriteNode", + "LocalVariableOperatorWriteNode", +]; + +fn assigned_names(node: &AnyNode, source: &str, aliases: &[String]) -> Vec { + let AnyNode::ERBContentNode(inner) = node else { + return Vec::new(); + }; + + let Some(prism) = inner.prism() else { + return Vec::new(); + }; + + let mut names = Vec::new(); + + collect_assignments(prism, source, aliases, &mut names); + + names +} + +fn collect_assignments(node: &herb::prism::PrismNode, source: &str, aliases: &[String], names: &mut Vec) { + if ASSIGNMENT_NODES.contains(&node.node_type.as_str()) { + if let Some(name) = node.name.as_ref() { + let assigns = node.children.iter().any(|child| { + source + .get(child.start_offset..child.end_offset) + .map(|right| references_any(right, aliases)) + .unwrap_or(false) + }); + + if assigns && !names.contains(name) { + names.push(name.clone()); + } + } + } + + for child in &node.children { + collect_assignments(child, source, aliases, names); + } +} + fn collect_attributes(element: &herb::nodes::HTMLElementNode, aliases: &[String], path: &[usize], affected: &mut Vec) { let Some(open_tag) = element.open_tag.as_ref() else { return; diff --git a/rust/herb-analysis/tests/state_flow_test.rs b/rust/herb-analysis/tests/state_flow_test.rs index 8fc925be9..a1ed8bf45 100644 --- a/rust/herb-analysis/tests/state_flow_test.rs +++ b/rust/herb-analysis/tests/state_flow_test.rs @@ -348,3 +348,56 @@ fn reports_what_the_ruby_collector_reports() { reported ); } + +fn expressions_for(name: &str, template: &str, state: &str) -> Vec { + let project = Project::new(name); + let entry = project.write("app/views/posts/index.html.erb", template); + + project + .flow() + .affected_nodes(&entry, state) + .into_iter() + .filter_map(|node| node.expression) + .collect() +} + +#[test] +fn follows_state_into_a_local_assigned_from_it() { + assert_eq!( + vec!["total = @items.size".to_string(), "total".to_string()], + expressions_for("assign_simple", "<% total = @items.size %>

<%= total %>

", "@items") + ); +} + +#[test] +fn follows_state_through_a_chain_of_assignments() { + assert_eq!( + vec!["a = @items.size".to_string(), "b = a * 2".to_string(), "b".to_string()], + expressions_for("assign_chain", "<% a = @items.size %><% b = a * 2 %>

<%= b %>

", "@items") + ); +} + +#[test] +fn leaves_a_local_assigned_from_something_else() { + assert!(expressions_for("assign_unrelated", "<% other = 5 %>

<%= other %>

", "@items").is_empty()); +} + +#[test] +fn does_not_take_a_comparison_for_an_assignment() { + assert_eq!( + vec!["if @items == other".to_string()], + expressions_for("assign_comparison", "<% if @items == other %>

<%= other %>

<% end %>", "@items") + ); +} + +#[test] +fn stops_a_local_assigned_inside_a_block_at_the_end_of_it() { + assert_eq!( + vec!["@rows.each do |r|".to_string(), "inner = r.x".to_string(), "inner".to_string()], + expressions_for( + "assign_scope", + "<% @rows.each do |r| %><% inner = r.x %><%= inner %><% end %><%= inner %>", + "@rows" + ) + ); +} diff --git a/sig/herb/analysis/template_dependencies/node_dependency_collector.rbs b/sig/herb/analysis/template_dependencies/node_dependency_collector.rbs index a5ac2ea4b..41c3a1397 100644 --- a/sig/herb/analysis/template_dependencies/node_dependency_collector.rbs +++ b/sig/herb/analysis/template_dependencies/node_dependency_collector.rbs @@ -8,6 +8,8 @@ module Herb BRANCH_CONTINUATION_PROPERTIES: Array[Symbol] + ASSIGNMENT_NODES: Array[untyped] + attr_reader affected: untyped def initialize: (untyped state, untyped helper_registry, untyped custom_helpers, ?conditions_only: untyped) -> untyped @@ -48,6 +50,14 @@ module Herb def visit_branching_node: (untyped node) -> untyped + def bind_assignments: (untyped node) -> untyped + + def assigned_names: (untyped code) -> untyped + + def collect_assignments: (untyped node, untyped names) -> untyped + + def right_hand_side: (untyped node) -> untyped + def bindings_for: (untyped node) -> untyped def block_parameters: (untyped code) -> untyped diff --git a/test/analysis/template_dependencies_test.rb b/test/analysis/template_dependencies_test.rb index 1f8a7bd7e..ed7602da3 100644 --- a/test/analysis/template_dependencies_test.rb +++ b/test/analysis/template_dependencies_test.rb @@ -461,6 +461,52 @@ def markdown(text) refute_includes affected, File.join(@view_root, "posts/_field.html.erb") end + test "affected_nodes follows state into a local assigned from it" do + path = write_template("posts/index.html.erb", "<% total = @items.size %>

<%= total %>

") + + expressions = analyzer.affected_nodes(path, "@items").map { |node| node[:expression] } + + assert_equal ["total = @items.size", "total"], expressions + end + + test "affected_nodes follows state through a chain of assignments" do + path = write_template("posts/index.html.erb", "<% a = @items.size %><% b = a * 2 %>

<%= b %>

") + + expressions = analyzer.affected_nodes(path, "@items").map { |node| node[:expression] } + + assert_equal ["a = @items.size", "b = a * 2", "b"], expressions + end + + test "affected_nodes leaves a local assigned from something else" do + path = write_template("posts/index.html.erb", "<% other = 5 %>

<%= other %>

") + + assert_empty analyzer.affected_nodes(path, "@items") + end + + test "affected_nodes does not take a comparison for an assignment" do + path = write_template("posts/index.html.erb", "<% if @items == other %>

<%= other %>

<% end %>") + + expressions = analyzer.affected_nodes(path, "@items").map { |node| node[:expression] } + + assert_equal ["if @items == other"], expressions + end + + test "affected_nodes stops a local assigned inside a block at the end of it" do + path = write_template("posts/index.html.erb", "<% @rows.each do |r| %><% inner = r.x %><%= inner %><% end %><%= inner %>") + + expressions = analyzer.affected_nodes(path, "@rows").map { |node| node[:expression] } + + assert_equal ["@rows.each do |r|", "inner = r.x", "inner"], expressions + end + + test "dependency_index reports what a partial's declared locals reach" do + path = write_template("posts/_card.html.erb", "<%# locals: (query:, page: 1) %>\n

<%= query %>

<%= page %>") + + index = analyzer.dependency_index(path) + + assert_equal ["page", "query"], index.keys.sort + end + test "affected_nodes follows state through a block parameter" do path = write_template("posts/index.html.erb", "
    <% @items.each do |item| %>
  • <%= item.name %>
    • <% end %>
")