refactor: one shared AST walker for the hand-rolled visitors

[sims1253]

Cleanup sprint, PR 6 of 7 (stacks on #160). Closes #152. Net −18 lines overall, but that's a new 357-line reusable walker module (+tests) against a −605 line reduction across the eleven files that had hand-rolled walkers. Every consumer is now a thin policy + callback.

The walker (ry_core::walk)

• walk_stmts/walk_stmt/walk_expr: pre-order, ControlFlow<B> early-termination with payload, fn_depth counting.
• Walk policy struct, five boolean knobs: assign_targets, assign_operands, dollar_args ($-synthesized idents), fn_bodies, control_tests — constructed via ..Walk::ALL (no Default, so partial-literal misuse is impossible).
• The refactor(checker): collapse the six collect.rs AST walkers into one #148-era stmt_any/expr_any in collect.rs now delegate to it: exactly one walker exists in the codebase, where before this PR there were two designs.

Conversion table

Family	Outcome
collect_condition_assignment_names (binop.rs)	converted
assigned_names_in_body (index.rs)	converted
collect_executed_identifiers pair (misc.rs)	converted + merged
collect_forwarded_calls pair (misc.rs)	converted + merged
RY098 force family (misc.rs, 5 fns)	left hand-rolled — its rules are per-child laziness (strict-builtin call args, literal-cond branch choice, first-in-eval-order stop), not subtree skips; documented in place, as #152 sanctioned
vector_intent_parameters (mod.rs)	converted
r6_rebound_members (call.rs)	converted
stmt_any/expr_any (collect.rs)	converted onto the shared core
collect_local_bindings pair (dump.rs)	converted + merged
index_scope_bodies pair (dump.rs)	converted + merged
attachment + dynamic-bindings walkers (ry-workspace)	converted

Nice confirmation of the knob axes: dump.rs's two walkers hold opposite fn-body policies (skip vs. walk); one fn_bodies flag expresses both.

Caught in review, fixed before push (b825060)

The family-by-family equivalence audit found two real traversal regressions where a wildcard Descend::Skip arm was one variant too broad: assigned_names_in_body stopped walking Expr::Block/Expr::If in expression position, and collect_condition_assignment_names pruned Stmt::Expr and non-Ident-target assigns inside condition blocks. Under-collection shapes that byte-identical snapshots cannot catch — exactly why the audit exists. Both restored to base semantics, with 9 new exact-set regression tests (4 proven to fail against the broken version). A third flagged case turned out to be a false positive (assign_operands was already true via Walk::ALL) — now explicit and pinned.

Gates

cargo test --workspace (43 suites, +22 new tests), clippy -D warnings, fmt, perf suite (8/8 --release -- --ignored), oracle suite with R; snapshots byte-identical throughout, including after the fixes.

Summary by CodeRabbit

• Bug Fixes
  • Improved code analysis across nested expressions, conditional blocks, assignments, and function bodies.
  • More accurately detects variable bindings and dynamic assignments while respecting function scope.
  • Improved recognition of statically specified packages loaded through library() and require(), including nested calls.
  • Refined handling of forwarded calls, executed identifiers, vector operations, and rebound members for more consistent inference.
• Tests
  • Added coverage for traversal, scoping, package detection, and dynamic-binding behavior.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: 34655732-3c88-4822-9cb9-b512a44cf8e5

📥 Commits

Reviewing files that changed from the base of the PR and between 942d7ae and c29c0ed.

📒 Files selected for processing (11)

• crates/ry-checker/src/collect.rs
• crates/ry-checker/src/infer/binop.rs
• crates/ry-checker/src/infer/call.rs
• crates/ry-checker/src/infer/index.rs
• crates/ry-checker/src/infer/misc.rs
• crates/ry-checker/src/infer/mod.rs
• crates/ry-cli/src/dump.rs
• crates/ry-core/src/lib.rs
• crates/ry-core/src/walk.rs
• crates/ry-workspace/src/lib.rs
• crates/ry-workspace/src/packages.rs

---

📝 Walkthrough

Walkthrough

Changes

The PR adds a configurable AST walker to ry-core. Checker, CLI, and workspace collectors now use shared traversal with explicit policies for assignment edges, control tests, function bodies, and function depth. Tests cover walker behavior and collector-specific traversal rules.

Shared AST walker

Layer / File(s)	Summary
Walker API and traversal semantics crates/ry-core/src/lib.rs, crates/ry-core/src/walk.rs	Adds public walker types, entry points, traversal policies, callback pruning, early termination, function-depth tracking, and tests.
Checker binding and assignment analyses crates/ry-checker/src/collect.rs, crates/ry-checker/src/infer/binop.rs, crates/ry-checker/src/infer/call.rs, crates/ry-checker/src/infer/index.rs	Replaces recursive visitors with configured walker callbacks for expression matching, assignment collection, rebound members, and assigned names.
Checker expression and intent analyses crates/ry-checker/src/infer/misc.rs, crates/ry-checker/src/infer/mod.rs	Migrates forwarded-call, executed-identifier, and vector-intent traversal while retaining the force/identifier analysis that requires evaluation-order rules.
CLI and workspace collector migration crates/ry-cli/src/dump.rs, crates/ry-workspace/src/lib.rs, crates/ry-workspace/src/packages.rs	Uses shared traversal for scope indexing, dynamic bindings, and static package attachments. Tests preserve function-depth and nested-call behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AST
  participant walk_stmts
  participant collect_dynamic_bindings_stmts
  participant BindingSets
  AST->>walk_stmts: provide statements and subexpressions
  walk_stmts->>collect_dynamic_bindings_stmts: visit nodes with function depth
  collect_dynamic_bindings_stmts->>BindingSets: record native symbols and dynamic bindings
  walk_stmts-->>collect_dynamic_bindings_stmts: continue through configured children

Loading

Poem

A rabbit hops through AST leaves,
Shared paths replace tangled weaves.
Names and calls are gathered bright,
Function depths stay measured right.
Tests guard each branch from flight.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch cleanup/06-walkers

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[pullfrog]

Important

The conversion itself is equivalence-clean — I audited all eleven families against their removed hand-rolled recursion and found no traversal deviations — but two of the converted ry-workspace walkers ship without the pinning test #152's acceptance criteria require. Details below.

Reviewed changes

Full diff (11 files, 2 commits) with a family-by-family equivalence audit of every conversion against the removed recursion, plus local gates: cargo test --workspace (43 suites, 0 failures), clippy --workspace --all-targets -- -D warnings, and cargo fmt --check — all green.

• New ry_core::walk module: pre-order walk_stmts/walk_stmt/walk_expr skeleton with ControlFlow<B> early termination, Descend::Skip subtree pruning, fn_depth counting, and a five-knob Walk policy constructed via ..Walk::ALL.
• Eleven hand-rolled visitors converted to thin policy + callback: six infer/ families from #152's list, the #148-era stmt_any/expr_any (now delegating to the same core), the two dump.rs pairs (merged), and the two ry-workspace walkers.
• RY098 force family left hand-rolled with an in-place rationale (per-child laziness / evaluation-order rules), exactly as #152 sanctioned.
• 13 new tests: exact-set regression pins for the two families that regressed during development (collect_condition_assignment_names, assigned_names_in_body), plus walker-level knob, fn_depth, and Break/Skip tests.

Equivalence notes from the audit, for the record: the knob axes check out at every seam — assign_operands gating only <-/<<- (never %<>%, matching every base walker), stmt_any's every-node pred being Call-only (verified against all five leaf predicates), index_scope_bodies's index-then-Skip matching index_function_body's own recursion, and vector_intent_parameters's FunctionDef-skip vs function-literal-walk split.

⚠️ Two converted walkers ship without the pinning test #152 requires

#152's acceptance criteria say "each converted walker keeps a test that pins its difference." The two ry-workspace conversions have none. attached_packages is only exercised by top-level library(...) calls in CLI e2e fixtures, so the "counts wherever the call appears" semantic this PR's new doc sentence claims is unpinned. And nothing in the worktree tests collect_dynamic_bindings_stmts at all — its most delicate semantic, the fn_depth == 0 gate that keeps a bare two-argument assign("x", v) namespace-scoped only at file top level, now rides on the new walker with zero coverage. This PR's own pre-push history (two traversal regressions, both in families that lacked pins at the time) is the argument for why these two need pins before merge.

Technical details

# Pin the ry-workspace walker conversions

## Affected sites
- crates/ry-workspace/src/packages.rs:130 — `attached_packages`: walks `Walk::ALL`
  including function bodies; only coverage is top-level `library()` in
  `crates/ry-cli/tests/config_e2e.rs` fixtures.
- crates/ry-workspace/src/lib.rs:397 — `collect_dynamic_bindings_stmts`: the
  `name == "assign" && fn_depth == 0` arm, the positional/named `envir` arms,
  and the `.Call(ffi_enquo)` native-symbol witness have no test references
  anywhere (`grep -rn "delayedAssign\|makeActiveBinding" crates` hits only the
  implementation and an unrelated NSE list).

## Required outcome
- A test pinning `library()`/`require()` inside a nested function body still
  attaching the package.
- A test pinning bare two-argument `assign("x", v)` recording at `fn_depth == 0`
  but NOT inside a function body, while named/positional `envir` variants still
  record at depth >= 1.

## Suggested approach
Parse small sources with `RParser` and call the two functions directly,
mirroring the `assert_exact` HashSet style of the new test module in
`crates/ry-checker/src/infer/binop.rs`.

ℹ️ Nitpicks

• The PR body's "+10 new tests" undercounts: the diff adds 13 #[test] functions (5 in binop.rs, 4 in index.rs, 4 in ry-core/src/walk.rs).
• "exactly one walker exists in the codebase" shouldn't be read literally: collect_parameter_uses_in_stmt/_in_expr and first_parameter_use_in_stmt/_in_expr (crates/ry-checker/src/collect.rs:759, :885) also hand-roll Stmt/Expr recursion. They're outside #152's listed scope and fit the same per-child-rules exclusion documented for the RY098 family, but a follow-up issue would keep them from being lost.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using openai-compatible/glm-5.3 ｜ 𝕏}

[sims1253]

Addressed the pinning-test finding in e938dbb: 9 direct tests on the two ry-workspace walkers — nested-body library()/require() attachment (and the negative controls), the bare-assign fn_depth == 0 gate, named/positional envir variants at depth ≥ 1, non-literal targets, and the .Call/.External2 native-symbol witness. Notable from writing them: the envir arms are presence-based (any named envir/env/assign.env or the positional slot), never value-based — the test module documents this explicitly. PR body test count corrected (13 → 22). #152's acceptance criteria are now met.

[pullfrog]

✅ No new issues found.

Reviewed changes

Re-reviewed the delta since the prior review (b825060): a single commit, e938dbb, adding exactly the two ry-workspace pinning test modules the prior review asked for. Full-diff coverage re-confirmed across all 11 files; cargo test -p ry-workspace (19/19), clippy --all-targets -- -D warnings, and cargo fmt --check all green locally.

• Pinned collect_dynamic_bindings_stmts with 7 exact-set tests in dynamic_binding_tests: the fn_depth == 0 bare-assign gate (braced blocks don't count toward depth, function bodies do), named envir/env/assign.env recording at any depth, positional environment slots (third argument of assign/makeActiveBinding, fourth of delayedAssign), a named non-environment argument (inherits =) staying depth-gated, non-literal target names recording nothing, and the .Call(ffi_sym, ...) native-symbol witness with its string-entry-point and named-argument negative controls.
• Pinned attached_packages with 2 tests: library()/require() attaching from inside function bodies and a nested if, plus negative controls (requireNamespace(), loadNamespace(), a runtime-computed package name, and my_library).
• Verified both pins actually bite via mutation: deleting the fn_depth == 0 gate makes bare_assign_records_only_at_top_level fail ({"top", "nested"}), and walking with fn_bodies: false makes the attachment test fail ({"dplyr"} only). Every expectation was also cross-checked line-by-line against both the converted walker and the removed hand-rolled recursion.
• Corrected the PR body test count: "+22 new tests" now matches the actual 22 #[test] additions (13 from the earlier commits + 9 here).

  ^{｜ View workflow run ｜ Using openai-compatible/glm-5.3 ｜ 𝕏}