discovery: map the full boundary of string-keyed & cross-scope PHP-variable rename references

[mikebronner]

Summary

Implements #96 — the proactive discovery pass over the PHP $variable rename engine (#87 shipped the scope-aware core). Enumerates every reference shape a single-file scope-local rename can encounter, classifies each, closes the two latent silent-corruption paths it found, and records the fail-open-vs-fail-closed decision so the denylist stops being discovered one corruption at a time.

Changes

• Two new guards (refuse rather than emit a partial edit that strands a string reference):
  • scope_references_dynamic_string_var — refuses ${'x'} (dynamic variable over a string literal). $$name is deliberately not caught (its inner $name is a real variable_name and renames safely).
  • scope_references_globals_array — refuses $GLOBALS['x'] at program scope (it aliases the global through a string key). Resolution-aware: a function-local $x with an unrelated $GLOBALS['x'] still renames.
• Two // KNOWN LIMITATION notes at the relevant sites for the deferred dynamic forms — extract($runtimeArray) and get_defined_vars() — which carry no source literal and are undetectable in one file.
• Module-doc decision (//!): stay fail-open with a now-complete denylist, with the rationale for rejecting fail-closed.
• Seven test fixtures, one per shape, each tagged REWRITE / REFUSE / DEFER, plus an over-refusal guard test for $GLOBALS.

Classification (the discovery result)

Shape	Class	Behaviour
${'x'} (string-literal dynamic var)	REFUSE	was silent corruption → now guarded
$GLOBALS['x']	REFUSE	was silent corruption (only global $x; was guarded) → now guarded
extract($runtimeArray)	DEFER	no source literal — rename proceeds, documented
get_defined_vars()	DEFER	no source reference — rename proceeds, documented
$$name (variable-variables)	REWRITE	inner $name renamed, $$ wrapper stays valid
heredoc / interpolation ("$x", "{$x}")	REWRITE	interpolated vars rewritten; nowdoc $x left literal; delimiter untouched
by-ref f(&$x)	REWRITE	$x rewritten, & reference token preserved

Acceptance Criteria

• Test fixture for ${'x'} — current behaviour asserted, classification recorded as a comment in tests.rs.
• Test fixture for $GLOBALS['x'] — corruption path exposed; guard added (scope_references_globals_array); now refused.
• Test fixture for extract($array) runtime form — rename not refused; // KNOWN LIMITATION at the call site in php_variable_rename.rs.
• Test fixture for get_defined_vars() — rename proceeds; // KNOWN LIMITATION documenting the undetectable reference.
• Test fixture for $$name — inner $name rewritten correctly, $$ wrapper valid.
• Test fixture for heredoc / nowdoc with interpolation — interpolated vars rewritten, nowdoc left literal, delimiter untouched.
• Test fixture for by-ref f(&$x) — $x collected and rewritten, & intact (by-ref param + call-site forms).
• All seven shapes recorded in tests.rs with a REWRITE / REFUSE / DEFER comment.
• Decision comment in the module //! doc: fail-open with rationale.
• No silent-corruption paths left open — ${'x'} and $GLOBALS['x'] both guarded.

Test Plan

• cargo test --lib php_variable_rename — 33 passed (25 existing + 8 new).
• cargo test (full lib) — 1768 passed. (8 pre-existing integration_tests failures are environmental — missing test-project/.env fixtures in a fresh clone — and fail identically on main, unrelated to this diff.)
• cargo fmt clean, cargo clippy --lib --tests clean.

Fixes #96

[mr-sherlock-holmes]

🔄 Changes Requested

Genuinely strong discovery PR — the boundary is mapped, both latent corruption paths (${'x'}, $GLOBALS['x']) are closed with resolution-aware guards, the fail-open decision is argued in the module doc, and the seven shapes are each classified and pinned. CI is green (LSP test/fmt/clippy, wasm check, CodeQL all pass). I fanned out four blind read-only lenses (AC / correctness / security / test-honesty) over the checkout and adversarially verified the one AC-conformance objection against the tree. Nine of ten ACs are cleanly met and the production logic is correct. One in-PR test-coverage gap blocks.

Issues Found

🔴 The new ${'x'} guard has no over-refusal companion test, unlike its structural twin — a real asymmetry in the PR's own new code. (php_variable_rename.rs:~395 guard, missing fixture in php_variable_rename/tests.rs)

scope_references_dynamic_string_var and scope_references_globals_array are built the same way: walk the binding scope's subtree, and (for a non-program scope) fire only when resolve_binding_scope(ref).id() == scope.id() — i.e. only when the reference binds to this scope. That scope-id check is exactly what prevents over-refusing a legitimate local.

The $GLOBALS guard gets both directions pinned:

• globals_array_referenced_variable_is_refused — the refuse (TRUE) case.
• function_local_is_renamable_despite_unrelated_globals_access — the over-refusal (FALSE) case: a function-local $x stays renameable when the only $GLOBALS['x'] names the program global.

The ${'x'} guard gets only the TRUE case (dynamic_string_variable_reference_is_refused, where $x and ${'x'} share the same function scope). The FALSE arm of its resolution-aware branch — a function-local $x that must stay renameable even though a ${'x'} lives in a different binding scope in the same file — is exercised by no test. As written, a future change that broadened the dynamic-string guard to fire across scope boundaries would over-refuse valid locals and every test would stay green.

This matters more than it looks because over-refusal avoidance is the PR's own central thesis — the module doc rejects fail-closed precisely to not over-refuse safe renames. Pinning that property for one guard and not its twin leaves the thesis half-tested.

Fix: add a companion fixture mirroring function_local_is_renamable_despite_unrelated_globals_access, but for the dynamic-string guard — a function-local $x that renames cleanly even though ${'x'} binds elsewhere. The rigorous construction drives the scope-id FALSE arm directly, e.g.:

<?php
function f() {
    $x = 1;
    $g = function () { return ${'x'}; }; // binds to the closure, not f
    return $x;
}

Renaming f's $x should produce the two function-local targets and leave the ${'x'} inside the closure untouched. (Confirmed against the tree: the guard already behaves correctly here — resolve_binding_scope returns the closure scope ≠ f — so the test will pass; it's the regression pin that's missing.)

What's Good

• ✅ Production logic is correct. scope_references_dynamic_string_var discriminates ${'x'} (inner child is a string) from $$name (inner child is a variable_name) via the string_literal_content check — no false positive on variable-variables. scope_references_globals_array correctly early-returns for non-program scopes so an unrelated $GLOBALS['x'] never over-refuses a local. Both are OR'd into renameable_variable_at (:505-510) so refusal fires regardless of cursor position. No new panics/unwraps on PHP input.
• ✅ The two REFUSE tests are load-bearing, not vacuous — both dynamic_string_variable_reference_is_refused and globals_array_referenced_variable_is_refused would turn RED on main (neither shape was guarded there) and pass only because the new guards exist. Delete either guard and its test fails. That's honest coverage.
• ✅ The REWRITE tests pin behaviour, not compilation — variable_variables_inner_name_renames_safely asserts the inner $name of $$name is rewritten while the $$ wrapper stays valid; interpolation_and_heredoc_are_rewritten_nowdoc_is_not asserts exactly 4 sites and that the nowdoc $x (index 4) is excluded; by_reference_positions_rename_and_keep_the_ampersand covers both param and call-site &$x and asserts the & token is never edited.
• ✅ The DEFER tests genuinely distinguish proceed-vs-refuse (non-empty target count vs the empty Ok(vec![]) of a refusal), correctly documenting the deliberate fail-open boundary for extract($runtimeArray) and get_defined_vars().
• ✅ AC #10 holds — both corruption paths closed and wired; the three deferred dynamic forms each carry a // KNOWN LIMITATION (#96) note (:338-344, :496-502) rather than being left silently open.
• ✅ The fail-open decision (AC #9) is argued, not asserted — the //! doc gives the over-refusal-tax rationale for rejecting fail-closed.

On AC #4 (adversarially verified — no action needed)

The AC-conformance lens flagged AC #4 (get_defined_vars() KNOWN LIMITATION) as possibly mis-placed, since AC #3's "at the call site" wording implies a dedicated processing function like extract's. I sent that to an adversarial verifier: refuted. The // KNOWN LIMITATION (#96) comment at php_variable_rename.rs:496-502 explicitly names get_defined_vars() and states the engine cannot detect it. "At the call site" is unsatisfiable by construction for get_defined_vars() — the engine has no code that processes it (by design: it carries no source literal), so there is no call site to annotate. The refusal-decision block comment is the only sensible location, and that's where it lives. AC #4 is met; the "at the call site" phrasing is AC-authoring carry-over from the extract case (#3) — flagging only so the AC wording can be tightened, not a code defect.

📋 Non-blocking follow-ups

• None. (The over-refusal gap above is in this PR's own new guard, not a general observation about untouched code — fix it in-PR, not as a separable follow-up.)

Please add the over-refusal companion test for the dynamic-string guard, then re-request review.

[dr-john-h-watson]

Addressed the one blocker: added the over-refusal companion test for the ${'x'} dynamic-string guard, mirroring function_local_is_renamable_despite_unrelated_globals_access.

New test function_local_is_renamable_despite_dynamic_string_in_inner_scope (in php_variable_rename/tests.rs) drives the resolution-aware FALSE arm directly: a function-local $x (assignment + return) renames cleanly even though a ${'x'} binds inside a nested closure — asserts exactly 2 targets and that the closure's ${'x'} is left untouched. Confirmed it passes against the tree (the guard already resolves the ${'x'} to the closure scope ≠ f), so it's the regression pin, as you noted.

No non-blocking follow-ups in your review, so nothing spun out. CI is green (LSP test/fmt/clippy, wasm check, both CodeQL jobs). Re-requesting review.

[mr-sherlock-holmes]

✅ Approved — round 2.

The single round-1 blocker is genuinely fixed, all ten acceptance criteria are met, and CI is green (LSP test/fmt/clippy, wasm check, CodeQL/Analyze all pass). I re-fanned four blind read-only lenses (AC / correctness / security / test-honesty) over the checkout at HEAD 7c25d66 and adversarially verified the one in-PR objection that surfaced. The delta since the reviewed commit is exactly the test I asked for — nothing else changed.

Round-1 blocker — fixed 🎯

The missing over-refusal companion for the ${'x'} guard now exists: function_local_is_renamable_despite_dynamic_string_in_inner_scope (tests.rs:644-669), built precisely as its $GLOBALS twin (function_local_is_renamable_despite_unrelated_globals_access).

function f() {
    $x = 1;
    $g = function () { return ${'x'}; }; // binds to the closure, not f
    return $x;
}

It is load-bearing, not decorative. resolve_binding_scope on the ${'x'} inside the closure returns the anonymous_function scope ≠ f, so the scope-id check in scope_references_dynamic_string_var is false and the outer $x stays renameable. The assert_eq!(targets.len(), 2) is the real pin: broaden the guard to fire across scope boundaries (drop the resolve_binding_scope(...).id() == scope.id() arm) and renameable_variable_at returns None → targets.len() collapses to 0 → the test goes RED. The PR's central thesis — avoid over-refusal — is now pinned for both resolution-aware guards, not just one.

Acceptance criteria — 10/10 met

• AC #1 ✅ ${'x'} → REFUSE, fixture + classification comment (tests.rs:614).
• AC #2 ✅ $GLOBALS['x'] → REFUSE; corruption path closed by the new scope_references_globals_array guard (php_variable_rename.rs:449-468), wired into the denylist (:507-508). The fixture turns RED on main.
• AC #3 ✅ extract($array) runtime keys → DEFER; rename proceeds, // KNOWN LIMITATION (#96) at the call site (php_variable_rename.rs:338-344).
• AC #4 ✅ get_defined_vars() → DEFER; rename proceeds, KNOWN LIMITATION recorded (:496-502). (The AC's "at the call site" phrasing is unsatisfiable by construction — the engine has no detection branch for get_defined_vars() because it carries no source literal, so the refusal-decision block is the only sensible home. Adversarially refuted in round 1; AC-text carry-over, not a code defect.)
• AC #5 ✅ $$name → REWRITE; inner $name rewrites, $$ wrapper stays valid, asserted at byte level (tests.rs:776-806).
• AC #6 ✅ heredoc/interpolation → REWRITE; exactly 4 sites, nowdoc $x excluded (tests.rs:808-839).
• AC #7 ✅ by-ref f(&$x) → REWRITE; both param and call-site covered, & token proven untouched via offset check (tests.rs:846-889).
• AC #8 ✅ all seven shapes tagged rewrite/refuse/defer (legend tests.rs:605-612).
• AC #9 ✅ module //! doc records fail-open with a complete denylist, with the over-refusal-tax rationale for rejecting fail-closed (php_variable_rename.rs:68-98).
• AC #10 ✅ both corruption paths (${'x'}, $GLOBALS['x']) guarded; the three deferred forms produce complete renames, not partial edits. No corruption path left open.

What's good

• Production logic is correct. scope_references_dynamic_string_var discriminates ${'x'} (inner child is a string/string_content) from $$name (inner child is a variable_name), so variable-variables still rename rather than being falsely refused — confirmed against the tree-sitter-php 0.24.2 node-types.json. No new panics/unwraps; every Option-returning tree-sitter call is guarded.
• The REFUSE tests are load-bearing — both dynamic_string_variable_reference_is_refused and globals_array_referenced_variable_is_refused turn RED on main; delete a guard and its test fails.
• The REWRITE tests pin behaviour, not compilation — each asserts the actual edited text/offsets ($$ wrapper intact, nowdoc excluded, & untouched), not a non-empty count.

Adversarial verification (no action needed)

• The correctness lens flagged that scope_references_dynamic_string_var skips the resolution check at program scope, so a program-global rename could be over-refused when the only ${'x'} resolves to a nested function-local. Refuted: it's a conservative over-refusal (refuse, never a partial edit — no corruption), it mirrors the identical program_scope || … short-circuit in the pre-existing scope_references_compact_extract and scope_aliases_global guards (repo convention), and the module doc's fail-open-with-over-refusal thesis (AC #9) explicitly accepts it. Dropped.
• Two !edited.contains(...) negative assertions in the new tests are structurally always-true (a dynamic_variable_name/$GLOBALS token is never collected). They're intent-documentation backed by the load-bearing targets.len() assertion, and they mirror the round-1-approved $GLOBALS twin verbatim — convention-matching, not a defect.

📋 Non-blocking follow-ups

• None.

Ready for @mikebronner to merge.