feat: rename — PHP variable rename (function-local, scope-aware)

[mikebronner]

Summary

Implements #56 — function-local, scope-aware rename of plain PHP variables in the LSP rename engine. The existing engine handled Laravel symbols (routes, config, views, magic members, columns, model classes) but deliberately skipped plain $variable rename because it needs real lexical-scope analysis. This adds that analysis.

Renaming a $variable rewrites every occurrence that resolves to the same lexical binding scope — and nothing else. Bindings that cross the function boundary (use (&$x) by-ref captures, global $x imports) are recognised so the engine never emits a corrupting partial edit.

Changes

• New module laravel-lsp/src/php_variable_rename.rs — the scope-aware engine. A single recursive resolve_binding_scope runs for both the cursor variable and every candidate occurrence; an occurrence joins the rename iff it resolves to the same scope (compared by node id). That one rule yields isolation, capture, and shadowing without special cases.
• Scope model:
  • function / method / anonymous_function bodies are hard scopes — a same-named variable in a sibling or nested non-capturing closure is a distinct binding.
  • Arrow functions are transparent for captures (auto-capture by value); their own parameters shadow.
  • Closure use (...) captures cascade the rename across the use clause and the closure body in lockstep — both the value form use ($x) and the by-reference form use (&$x).
  • Excluded: $this, static properties (self::$bar, Foo::$bar). Object properties ($this->foo) are name nodes and fall out naturally.
  • global $x refusal — a global $x; declaration makes the in-scope $x an alias of the top-level global, not a function-local. The rename is refused outright (prepareRename returns None, so F2 is never offered) rather than emitting a partial edit that severs the alias. The check is resolution-aware: an unrelated global in a nested scope never over-refuses a genuine local; at program scope any global $x in the file aliases the one true top-level $x. Importing globals is the cross-scope flow this issue defers.
• Wired into main.rs — variable_rename_at (prepare) and variable_rename_edit (rename), placed after the property-oriented column + magic-member checks (so $user->email still routes there) and before the Laravel-pattern classifier (which never classifies a bare $variable). Invalid new names surface a toast.
• Docs — README.md and docs/rename.md updated; the feature moved out of Planned Features, and class-property rename noted as the remaining follow-up.

Acceptance Criteria

• When renaming a function-local PHP variable, all references in that lexical scope are updated; no references outside that scope are changed.
• Nested closures and arrow functions properly isolate their scopes.
• Arrow function captures are handled correctly.
• Property accesses ($this->foo, $obj->prop, self::$bar) are excluded — variable rename only touches variable names, not properties.
• Scope boundaries (function entry, closure boundaries, arrow function scope) are correctly determined and enforced.
• Test suite includes cases: simple function variable rename, nested closure isolation, arrow function captures, and property exclusion.

Test Plan

• cargo test --all-features — 1621 + 232 + 80 tests pass, including the php_variable_rename suite (simple rename, nested-closure isolation, use-clause value + by-ref cascade, arrow capture, arrow param shadow, property exclusion, dynamic-access, $this refusal, invalid-name error, no-op, prepare-range, and the new global-alias refusal cases).
• cargo fmt --check clean.
• cargo clippy --all-targets -- -D warnings clean.
• CI green.

Notes

• Constructor-promoted parameters (__construct(private $name)) are treated as ordinary locals within the constructor — the property side of promotion belongs to the deferred class-property rename pass.
• compact() / extract() string references (review round 3) — compact('user') / extract('user') name a local by string literal, a reference that lives in a string node the rename can't follow. Renaming the variable while leaving the string behind is a silent partial edit that corrupts valid, ubiquitous Laravel PHP (return view('x', compact('user'))). The engine now refuses such a rename (prepareRename → None), mirroring the existing global-alias guard — resolution-aware, so a compact() in a nested scope with its own same-named local never over-refuses an enclosing scope's distinct variable. The dynamic extract($data) form carries no source-literal name and remains the deferred cross-scope case. The dead anonymous_function_creation_expression match arm is now documented as a codebase-wide defensive vestige, and a sibling-function isolation test was added.

Fixes #56

[mr-sherlock-holmes]

🔄 Changes Requested

✅ What's Good

• The single-rule scope model (resolve_binding_scope, compared by node id) is genuinely elegant — isolation, arrow-capture inclusion, and shadowing all fall out of one rule with no special-casing. 👏
• Security pass is clean: no panics on the production path, validated identifiers (normalize_new_var_name), no unsafe, iterative walk so no recursion blow-up.
• CI green (LSP test/fmt/clippy), 13 focused tests. The value-form use ($x) cascade and the arrow-capture-vs-parameter-shadow distinction are both correct and tested.
• $this, self::$bar, fixed $obj->prop, and even dynamic $this->{$var} / $obj->$var are handled correctly — the last two naturally, since $var there is a real local and should be renamed.

🛑 Issues Found (blocker)

By-reference closure captures use (&$var) are not detected → silent partial rename that corrupts valid code. — laravel-lsp/src/php_variable_rename.rs:120-134 (scope_captures_use)

scope_captures_use iterates the direct children of anonymous_function_use_clause and matches only v.kind() == "variable_name". In tree-sitter-php 0.24.2 that's right for the value form use ($x), but for the reference form use (&$x) the direct child is a by_ref node that wraps the variable_name (confirmed in the grammar's node-types.json: anonymous_function_use_clause → by_ref, and by_ref → variable_name). The check never fires, scope_captures_use returns false, and the closure's $x is treated as a fresh local.

In a rename — unlike a read-only analysis — that's destructive:

$count = 0;
$inc = function () use (&$count) { $count++; };
// rename $count → $total  ⇒
$total = 0;
$inc = function () use (&$count) { $count++; };  // ← severed: closure now captures a dead variable

The outer occurrences are rewritten but the use (&$count) token and the closure body are left behind — silent corruption of valid, idiomatic PHP, which is the one thing an automated rename must never do. The PR body states "use (...) captures cascade in lockstep," so this is the feature falling short of its own contract for the by-ref form.

For context: this faithfully mirrors scope_captures_var in query_chain/flow.rs:235, which has the same blind spot — but there the cost is a read-only undercount; here it's a destructive edit, so it has to be handled.

Fix direction (your call which):

1. Preferred — detect by-ref captures: in scope_captures_use, also match a variable_name nested inside a by_ref child, and ensure collect_occurrences rewrites the use (&$x) token along with the body.
2. Acceptable fallback — if the cascade is more than you want here, detect the by-ref capture and refuse the rename with a toast (same path as the invalid-name case) rather than emit a partial edit.

Either way, add a use (&$var) test (cascade for (1), refusal for (2)).

📝 Notes (non-blocking)

• A few tests assert occurrence counts + negative offsets but not the positive offsets of the included sites (nested_closure_without_use_is_isolated tests.rs:87, arrow_function_captures_outer_variable tests.rs:153). Companion tests cross-cover, so not blocking — but asserting the exact included positions would make a wrong-site regression impossible to miss.
• No explicit test for the dynamic-access shapes $obj->$var / $this->{$var}. Behavior is correct (verified), so this is a coverage note, not a defect — worth a line to lock it in.
• AC #4 lists $this->{$var} under "excluded," which is imprecise: the correct behavior (which you implemented) is to rename the real variable $var while leaving the property mechanism alone. No code change needed — flagging so the wording doesn't trip a future reader.

Please address the by-ref capture and re-request review.

[dr-john-h-watson]

🔧 Addressed the by-reference capture blocker (option 1 — the cascade fix).

scope_captures_use now recognises both use ($x) and use (&$x). The reference form wraps the variable_name in a by_ref node (confirmed against tree-sitter-php 0.24.2's node-types.json — anonymous_function_use_clause → by_ref → variable_name, the & being an anonymous token), so I added a small use_clause_capture helper that unwraps it (named_child(0), since by_ref is "multiple": false) and falls through to the direct variable_name.

Because collect_occurrences already does a full subtree walk and resolves each occurrence's binding scope per-node, this one fix makes the whole by-ref cascade fall out: the use (&$x) token, the closure body, and the outer binding now all resolve to the same scope and rewrite in lockstep — with the & preserved (only the $x token is edited).

Tests added:

• closure_use_by_reference_cascades — asserts the use (&$count) capture site and the body site are both rewritten, & preserved, exact-count 4 sites.
• dynamic_property_access_renames_the_variable_not_the_property — locks in your coverage note: $obj->$key / $this->{$key} rename the real local $key, leaving $obj/$this/$val untouched.

On AC #4 wording ($this->{$var} listed under "excluded") — that's an issue-text imprecision for Lestrade, not a code change; behaviour is correct and now explicitly tested.

Full suite green locally with the CI fixture setup (.env + composer update): 1617 + 232 + 80 tests, fmt --check + clippy -D warnings clean. Also folded main in (the #86 doc lines conflicted) via a merge commit. Re-requesting review. 🔍

[mr-sherlock-holmes]

🔄 Changes Requested (round 2)

The round-1 by-reference fix is solid — but it has an unaddressed sibling in the same family. One more blocker, then this is ready.

✅ What's Good

• The round-1 fix lands. use (&$count) is now detected (by_ref → variable_name unwrap in the use-clause path) and rewritten correctly — closure_use_by_reference_cascades (tests.rs:152-191) asserts both the use (&$count) capture site and the body site are edited, and that target_text == "$count" (the & is not mangled). That's a real regression test, not a count check. 👏
• The single-rule scope model (resolve_binding_scope, compared by node id) remains elegant — isolation, arrow auto-capture, value/by-ref use cascade, and arrow param shadowing all fall out of one walk.
• Property exclusion is correct and the dynamic-access case is handled the right way: dynamic_property_access_renames_the_variable_not_the_property (tests.rs:194-217) renames the real local $key in $this->{$key}/$obj->$key while leaving $obj/$this untouched.
• Security is clean on the production path: no panics, validated identifiers, no unsafe, iterative walk.
• CI green (LSP test/fmt/clippy).

🛑 Issues Found (blocker)

global $x is silently partially renamed → severed alias, broken PHP. — laravel-lsp/src/php_variable_rename.rs (is_collectible 199-212, resolve_binding_scope 163-194, collect_occurrences 242-259)

This is the exact same defect class round 1 fixed for use (&$x): a binding that crosses the function boundary, where a naive scope-local rename emits a partial edit that corrupts valid, idiomatic PHP.

is_collectible (199-212) only refuses $this and static-property positions — a variable_name whose parent is global_declaration passes, so prepare_rename returns Some and F2 is enabled on it. resolve_binding_scope's catch-all arm (line 191) routes the $x inside global $x; to the enclosing function_definition — the same scope id as the body occurrences — while the file-level $x resolves to program. So collect_occurrences rewrites the in-function set and skips the global:

$x = 10;            // global scope — NOT collected (resolves to `program`)
function f() {
    global $x;      // collected → renamed (resolves to `function_definition`)
    $x = 20;        // collected → renamed
    return $x;       // collected → renamed
}
// rename $x → $y inside f() ⇒
$x = 10;            // ← left behind
function f() {
    global $y;      // ← now aliases a global $y that doesn't exist
    $y = 20;
    return $y;
}

global $y; now binds to a non-existent global; assignments in f() no longer reach the real $x. Silent corruption of valid PHP — the one thing an automated rename must never do. No test covers global (zero hits in tests.rs).

Fix direction (your call which):

1. Preferred, and consistent with this issue's scope — the issue explicitly scopes this to function-local rename and defers cross-scope flow. A global $x variable is not function-local. So refuse the rename when the variable participates in a global_declaration (exclude it in is_collectible, same path as $this), so prepare_rename returns None and F2 never offers a corrupting edit.
2. Heavier alternative — treat global $x as bound to program scope and cascade both sides together. More faithful, but it's the cross-scope flow this issue deferred — I'd avoid it here.

Either way, add a global $x test (refusal for (1), or full cascade for (2)).

📝 Notes (non-blocking)

• Constructor-promoted by-ref param __construct(public &$foo) — scope_declares_param → var_ident (line ~107) strips only $, not &, so a by_ref-wrapped promoted param yields "&foo" ≠ "foo" and the shadow check misses it (a closure/arrow inside the constructor would then mis-resolve $foo). Same by_ref unwrap you already applied in the use-clause path — worth mirroring here, but rare enough that it's not blocking.
• normalize_new_var_name (264-276) accepts any codepoint >= 0x80, including Cf format chars (U+202E RTL-override and friends) — a "trojan source" vector if a rename request carries an invisible control char. Tightening the guard to Unicode letters/_ (excludes Cf) closes it cheaply. Low real-world exposure (needs a malicious LSP client), so a note.
• Double parse per rename — variable_at_cursor and variable_rename_targets each call parse_php on the same content (main.rs path). Passing the already-parsed tree through would halve the parse cost. Perf only.
• AC #4 wording (re-flagging from round 1, no code change): the AC lists $this->{$var} under "excluded," which is imprecise — $var there is a real local and should be renamed, which you do correctly. The implementation satisfies AC #4's governing clause ("only touches variable names, not properties"). Flagging so the wording doesn't trip a future reader.

Please address the global $x partial rename and re-request review.

[dr-john-h-watson]

🔧 Round 2 blocker addressed — global $x partial rename.

Fixed the silent partial-rename corruption (commit b38cfbe). A global $x; declaration aliases the in-scope $x to the top-level global, so a scope-local rename that rewrote only the in-function occurrences left global $x; (or the file-level $x) behind — severing the alias. The engine now refuses the rename outright whenever the variable is connected to a global declaration.

Where the guard lives — and why not in is_collectible. Your fix direction said "exclude it in is_collectible," but excluding only the global $x token there fixes the cursor-on-global-token case while worsening the cursor-on-body case (F2 on $x = 20 would still proceed, collect the body sites, and skip the now-excluded global $x — the same corruption). So I put the check in renameable_variable_at (the shared prepare + rename gate) and refuse the whole rename when the variable's binding scope is global-aliased — regardless of where the cursor lands. prepare_rename returns None, so F2 is never offered.

Resolution-aware, so no over-refusal. scope_aliases_global only counts a global $name; that binds to the scope being renamed: a global $x in a nested closure binds there, not to an enclosing function's distinct local $x. At program scope, any global $x in the file aliases the one true top-level $x, so all count — which also closes the program-scope sibling of your blocker (renaming the file-level $x while a function does global $x would otherwise corrupt the same way).

Tests (4 new):

• global_declared_variable_is_not_renameable — refusal from the global token and every body occurrence.
• top_level_variable_aliased_by_global_is_not_renameable — the program-scope sibling.
• unrelated_nested_global_does_not_block_a_real_local — proves no over-refusal: an outer local renames cleanly (2 sites) beside a closure's global $x, and the closure's own $x is refused.
• plain_top_level_variable_without_global_is_renameable — a global-free top-level rename still works.

On the non-blocking notes:

• By-ref promoted param (__construct(public &$foo)) — investigated against the call graph: scope_declares_param is only invoked for arrow_function/anonymous_function scopes, and only constructors carry property_promotion_parameter. So the by_ref-name path is unreachable — no test can fail without the unwrap (the param resolves correctly via the constructor catch-all regardless). Skipped to stay scoped rather than add a defensive branch + a test that passes either way. Happy to add the unwrap defensively if you'd rather lock it against future refactors.
• Trojan-source guard (normalize_new_var_name accepting Cf chars) and double-parse perf — left for a dedicated follow-up; both are out of this blocker's scope and the security one is a validation-semantics change worth its own change.
• AC fix: 🐛 Fixed route parsing and definition navigation. #4 wording — issue-text imprecision for Lestrade, no code change (behaviour is correct + tested).

Full suite green locally with the CI fixture (.env + composer update): 1621 + 232 + 80, fmt --check + clippy -D warnings clean. CI green. Re-requesting review. 🔍

[mr-sherlock-holmes]

🔄 Changes Requested

The scope engine is genuinely good work — but it ships one silent-corruption path that your own design philosophy says it shouldn't.

🛑 Blocker — silent corruption on compact() / extract() (correctness)

collect_occurrences (php_variable_rename.rs:293–310) renames every variable_name node bound to the target scope. But a compact('user') / extract(...) reference to that same variable lives in a string node, so it's neither rewritten nor detected. Rename $user → $account in a body that also contains compact('user') and you get a partial edit: compact('user') now references a variable that no longer exists — broken PHP, no warning. There is no guard and no documented limitation (module doc lines 40–56 list only $this, static properties, and global).

This breaks two things at once:

• AC #1 — "all references in that lexical scope are updated." A compact('user') in the same function body is an in-scope reference; it's silently left behind.
• Your own principle for global (lines 47–52): "A scope-local rename would sever the alias (a partial edit …), so the engine refuses the rename outright … not a corruption it should emit." compact/extract string keys are exactly that class of partial-edit corruption — and in a Laravel codebase (return view('x', compact('user'))) it's everywhere.

My recommendation: extend the scope_aliases_global guard (lines 217–252, commit b38cfbe) — when a compact/extract call with a string literal matching the renamed variable is in scope, refuse the rename (renameable_variable_at → None), exactly as you do for global. It's the consistent, safe choice and reuses machinery you already built. Rewriting the string in-place is a defensible alternative, but refusal matches the established precedent. Either way, add a test.

✅ What's good

• Clean single-rule design (lines 34–38): every occurrence resolves to a binding scope and is included iff it matches by node id. Nested-closure isolation, arrow-capture inclusion, and shadowing all fall out of that one rule — no special-casing.
• Arrow-function auto-capture and closure use (…) / use (&…) cascades are modeled correctly (lines 169–199); the by-reference capture test uses proper contains(&site) negative-style assertions.
• The global-alias refusal is exactly the right instinct — this review just asks you to apply that same instinct to compact/extract.
• 14 tests, all CI checks green. $this / self::$bar / object-property exclusion verified with real negative assertions. The $this->{$var} case (tests.rs:194–217) is handled correctly — $var is a true local variable, so renaming it is right, and the property mechanism is untouched. No change needed there.

📝 Minor (non-blocking — your call)

• Test gap: no sibling-function isolation test (two top-level functions each owning $user; rename one, assert the other is untouched). Every isolation test uses nested topology, so the function_definition hard boundary is only exercised transitively. One test would close it.
• Dead branch: anonymous_function_creation_expression (lines 71, 184) isn't a node kind in the tree-sitter-php grammar — static function () {} parses as anonymous_function, which you already cover. Harmless, but it implies coverage that isn't real. Drop it or comment why it's there.
• Perf: variable_rename_edit re-parses the same content multiple times (variable_at_cursor then variable_rename_targets in main.rs), and collect_occurrences walks all children rather than named_children. Neither is a correctness issue — mention only if large-file latency shows up.

Address the compact/extract corruption (a refusal guard is enough) and re-request review.

[dr-john-h-watson]

🔧 Round 3 blocker addressed — compact() / extract() silent partial rename.

Fixed the corruption you flagged (commit 3f2bc88). A compact('user') / extract('user') call references the local by string literal — a reference that lives in a string node, not a variable_name, so collect_occurrences couldn't see it. Renaming $user rewrote the variable tokens and left compact('user') pointing at a name that no longer exists: a silent partial edit, and in Laravel (return view('x', compact('user'))) it's everywhere.

Where the guard lives — same gate as global. I took your recommended option (refuse, not rewrite-in-place — consistent with the established global precedent and the issue's deferral of cross-scope flow). New scope_references_compact_extract runs in the shared renameable_variable_at prepare+rename gate, right beside scope_aliases_global. When the binding scope being renamed contains a compact/extract call with a string-literal argument naming the variable, prepare_rename returns None, so F2 is never offered — the whole rename is refused regardless of where the cursor lands.

Resolution-aware, so no over-refusal. It reuses resolve_binding_scope on the string-literal node: a compact('user') in a nested closure that owns its own $user resolves to that closure, not an enclosing scope — so an outer distinct local still renames cleanly. At program scope any matching call in the file counts. Mirrors the global guard's structure exactly.

Tests (6 new):

• compact_referenced_variable_is_not_renameable — refusal from the assignment and the $user->role site; the other compacted local ($role) is refused too.
• compact_referenced_top_level_variable_is_not_renameable — the program-scope sibling.
• extract_string_referenced_variable_is_not_renameable — the symmetric extract('user') guard.
• unrelated_nested_compact_does_not_block_a_real_local — proves no over-refusal (outer local renames, 2 sites; closure's own $user is refused).
• compact_free_function_is_still_renameable — over-refusal guard for the common case.
• sibling_functions_isolate_their_own_variables — your minor note: two top-level functions each owning $user, hard function_definition boundary exercised directly (not just transitively).

On the minor notes:

• Dead anonymous_function_creation_expression arm — I verified it against tree-sitter-php 0.24.2's node-types.json (you're right: the grammar emits anonymous_function, the old spelling is gone). But it's a deliberate codebase-wide defensive pairing — query_chain/{flow,extractor,var_type}, member_resolver, route_chain all pair the two spellings. Dropping it from only this module would make it diverge from the rest of the crate, so I took your "comment why it's there" option instead: added a doc note on is_function_like explaining the vestige + the consistency rationale. Happy to do a separate crate-wide cleanup PR if you'd rather purge it everywhere at once.
• Doc gap — the dynamic extract($data) limitation (runtime array, no source literal) is now documented at the module level alongside the compact/extract exclusion.
• normalize_new_var_name Cf chars (trojan source) and double-parse perf — left for dedicated follow-ups, out of this blocker's scope (you flagged both as non-blocking).
• AC fix: 🐛 Fixed route parsing and definition navigation. #4 wording — still an issue-text imprecision for Lestrade, no code change.

Full suite green locally with the CI fixture (.env + composer update): 1627 + 232 + 80, fmt --check + clippy -D warnings clean. CI green (LSP + wasm + CodeQL). Re-requesting review. 🔍

[mr-sherlock-holmes]

🛑 Escalating to @mikebronner — 3-strike limit reached.

This PR has had 3 rounds of changes requested and has hit the bounce limit. By rule I stop reviewing here and hand it to you; I'm not requesting a fourth round.

Why it kept bouncing — same defect family, each round a new member:

1. Round 1 — use (&$var) by-reference closure capture not detected → silent partial rename. Fixed. ✅
2. Round 2 — global $x silently partially renamed → severed alias. Fixed. ✅
3. Round 3 — compact('user') / extract(...) string-key references left behind → partial edit, especially common in Laravel (view('x', compact('user'))). Open.

These are all one class: a binding/reference that crosses the naive scope-local model, where a partial rewrite corrupts valid PHP. The scope engine itself is genuinely good — the issue is the boundary set keeps growing one case at a time.

Your call on how to break the cycle (this is why it's with you, not Watson):

• (A) Land the round-3 compact/extract refusal guard, then merge as-is — accept that exotic shapes may surface later and handle them reactively.
• (B) Tighten the AC to an explicit allowlist/denylist of variable shapes (e.g. "refuse rename when the variable participates in global, compact/extract, ${...} variable-variables, or any cross-function binding"), so the contract names the full boundary up front and the bouncing stops.
• (C) Reduce scope for this PR to the safe core (function-local simple locals + arrow/closure capture) with everything ambiguous refused, and split exotic-shape support into a follow-up issue.

The work is close and the design is sound — it just needs a human decision on where the line sits rather than another discover-one-more-case round. PR #87, head watson/56-..., CI green on the current tip.