diff --git a/README.md b/README.md index 8eb4712e..80ad5b59 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ Each feature has a focused reference under [`docs/`](docs/) β€” click through to | [πŸ”— Go-to-Definition](docs/go-to-definition.md) | Jump to views, components, routes, config, translations, env, assets, middleware, bindings, Artisan commands β€” plus query-chain columns / relations / tables and Eloquent magic members | | [ℹ️ Hover](docs/hover.md) | Intelephense-style summary cards for every recognised pattern, including semantic cards for Eloquent magic (scopes, accessors, relationships, cast-aware column types) | | [πŸ” Find References](docs/find-references.md) | Every call site across the project, vendor packages included β€” including the magic-member usages Intelephense can't see | -| [✏️ Rename](docs/rename.md) | Atomic rename of routes, configs, translations, env vars, views, components, Livewire, middleware, bindings, PHP classes (models, controllers, jobs, services, form requests), magic members β€” and database columns, migration included | +| [✏️ Rename](docs/rename.md) | Atomic rename of routes, configs, translations, env vars, views, components, Livewire, middleware, bindings, PHP classes (models, controllers, jobs, services, form requests), magic members, function-local PHP variables β€” and database columns, migration included | | [πŸ”’ Code Lens](docs/code-lens.md) | Opt-in reference counts above magic members, routes, config / translation / env keys, and Blade templates β€” plus an unused-symbol warning | | [πŸ’‘ Autocomplete](docs/autocomplete.md) | Cast types, model properties, query chains, builder methods, Blade / loop / slot variables, Pennant flags | | [❌ Diagnostics](docs/diagnostics.md) | Missing views / components / features, invalid rules, query-chain typos against your real schema | @@ -228,10 +228,10 @@ After saving, restart Intelephense (`Cmd+Shift+P β†’ lsp: restart`). For the lic ## 🚧 Planned Features -**Rename β€” remaining work** (the class-backed kinds, the FQCN class-rename engine across all common PHP class kinds, magic members, and database columns shipped; variables didn't): +**Rename β€” remaining work** (the class-backed kinds, the FQCN class-rename engine across all common PHP class kinds, magic members, database columns, and function-local PHP variables shipped): - πŸ“ **Blade variable rename** β€” scope-aware within a template (`@foreach`, `@php`, etc.), plus cross-file via the `view('x', ['key' => …])` / `compact('key')` linkage from controller into view. -- πŸ”§ **PHP variable rename** β€” scope-aware function-local. Class properties (`$this->foo`) are out of scope for this round and folded into a future class-property rename. +- πŸ”§ **Class-property rename** β€” `$this->foo`, `self::$bar`, and dynamic property access have different reference shapes than local variables, so they get their own scope-aware pass. (Function-local PHP variable rename shipped.) **Framework integrations:** diff --git a/docs/rename.md b/docs/rename.md index ad4945f7..77a932ef 100644 --- a/docs/rename.md +++ b/docs/rename.md @@ -2,7 +2,7 @@ [← Back to README](../README.md) -Press `F2` (or right-click β†’ **"Rename Symbol"**) on a route name, config key, translation key, environment variable, view, Blade component, Livewire component, middleware alias, container binding, PHP class (Eloquent model, controller, job, service, form request, or any other project class), magic member (relationship / scope / accessor), or database column. The extension rewrites every call site AND the declaration site (or moves the backing file, or generates the migration) in one atomic operation. +Press `F2` (or right-click β†’ **"Rename Symbol"**) on a route name, config key, translation key, environment variable, view, Blade component, Livewire component, middleware alias, container binding, PHP class (Eloquent model, controller, job, service, form request, or any other project class), magic member (relationship / scope / accessor), database column, or a function-local PHP variable. The extension rewrites every call site AND the declaration site (or moves the backing file, or generates the migration) in one atomic operation. You can also right-click a `.blade.php` file in Zed's file explorer β†’ **Rename** β†’ call sites update atomically with the file move. @@ -110,4 +110,24 @@ The transforms run both ways: `active` ↔ `scopeActive`, `full_name` ↔ `getFu **Vendor-located files refuse to rename** β€” never moves a Composer-installed view, component, or Livewire class, and never rewrites a middleware alias or binding registered inside `vendor/`. You'll see a toast explaining why instead of a silent no-op. -**Not yet renameable** (out of scope for this round, planned follow-up): Blade variables (`@foreach`, `@php` locals + the `view('x', ['key' => …])` / `compact('key')` linkage), PHP function-local variables. `prepare_rename` returns nothing for these so F2 silently does nothing. +**Function-local PHP variables** rename scope-aware, within a single file. Press `F2` on a `$variable` and every occurrence that resolves to the *same lexical binding* is rewritten β€” and nothing else: + +```php +function greet($user) { // F2 on any $user here… + $user = trim($user); + return "Hello " . $user; // …renames all four occurrences to $account +} +``` + +Scope boundaries are respected exactly: + +- **Nested closures isolate.** A `function () { … }` does not auto-capture, so an identically-named `$user` inside it is a separate binding β€” renaming the outer one leaves it alone (and vice versa). A `use ($user)` capture *does* tie the two together, so the rename cascades across the `use (…)` clause and the closure body in lockstep. +- **Arrow functions auto-capture.** `fn () => $base` shares `$base` with the enclosing scope, so renaming `$base` reaches inside the arrow body. The arrow's *own* parameters still shadow: `fn ($user) => $user` is its own binding. +- **Properties are never touched.** `$this->user` and `$obj->prop` are property accesses, not variables; `self::$bar` / `Foo::$bar` are class properties even though they're spelled with a `$`. `$this` itself is never renameable. Renaming a class property is a separate, scope-different operation tracked as a follow-up. +- **Ambiguous references are refused, never half-renamed.** Some shapes name a local by something other than a plain `$variable` token β€” a string key, or a cross-scope alias β€” that a single-file, scope-local rewrite can't follow. Rather than emit a corrupting partial edit, the engine refuses outright (`F2` does nothing): + - **`compact('user')` / `extract('user')`** name the local *by string literal* β€” ubiquitous in Laravel (`return view('x', compact('user'))`). Renaming `$user` would orphan the `'user'` string. + - **`global $user`** aliases the in-scope variable to the top-level global; a scope-local rewrite would sever the alias. + + This safety boundary is deliberately conservative and not yet exhaustive β€” more exotic dynamic shapes (`${'user'}`, `$GLOBALS['user']`, `extract($runtimeArray)`, `get_defined_vars()`) reference a local without a plain `$variable` token and aren't all detected yet. Mapping the complete set, and deciding refuse-vs-rewrite for each, is tracked in [#96](https://github.com/mike-bronner/zed-laravel/issues/96). + +**Not yet renameable** (out of scope for this round, planned follow-up): Blade variables (`@foreach`, `@php` locals + the `view('x', ['key' => …])` / `compact('key')` linkage), and class properties (`$this->foo`, `self::$bar`). `prepare_rename` returns nothing for these so F2 silently does nothing. diff --git a/laravel-lsp/src/lib.rs b/laravel-lsp/src/lib.rs index 0665a87e..5bdbffa7 100644 --- a/laravel-lsp/src/lib.rs +++ b/laravel-lsp/src/lib.rs @@ -56,6 +56,7 @@ pub mod pattern_disk_cache; pub mod pattern_indexer; pub mod php_class; // php_outline was consolidated into `laravel_introspector::walker`. +pub mod php_variable_rename; pub mod queries; pub mod query_chain; pub mod references; diff --git a/laravel-lsp/src/main.rs b/laravel-lsp/src/main.rs index 41ac45da..3e4f1c78 100644 --- a/laravel-lsp/src/main.rs +++ b/laravel-lsp/src/main.rs @@ -14835,6 +14835,69 @@ return [ })) } + /// prepare-rename for a plain PHP local variable (`$user`): return the + /// range of the `$name` token under the cursor, or `None` if the cursor + /// isn't on a renameable local variable. Pure-PHP, single-file β€” + /// scope-aware analysis lives in [`laravel_lsp::php_variable_rename`]. + async fn variable_rename_at(&self, uri: &Url, position: Position) -> Option { + use laravel_lsp::query_chain::position_to_byte_offset; + let content = self + .documents + .read() + .await + .get(uri) + .map(|(c, _)| c.clone())?; + let byte = position_to_byte_offset(&content, position.line, position.character)?; + let (line, start, end) = + laravel_lsp::php_variable_rename::variable_at_cursor(&content, byte)?; + Some(Range { + start: Position { + line, + character: start, + }, + end: Position { + line, + character: end, + }, + }) + } + + /// rename for a plain PHP local variable: rewrite every occurrence that + /// resolves to the same binding scope (function-local, scope-aware). + /// + /// Returns `Ok(None)` when the cursor isn't on a local variable, so the + /// caller can fall through to the Laravel-pattern / class rename pipeline. + /// Returns `Err` only when the cursor *is* on a variable but the new name + /// isn't a legal PHP identifier β€” surfaced to the user as a toast. + async fn variable_rename_edit( + &self, + uri: &Url, + position: Position, + new_name: &str, + ) -> jsonrpc::Result> { + use laravel_lsp::query_chain::position_to_byte_offset; + let Some(content) = self.documents.read().await.get(uri).map(|(c, _)| c.clone()) else { + return Ok(None); + }; + let Ok(file_path) = uri.to_file_path() else { + return Ok(None); + }; + let Some(byte) = position_to_byte_offset(&content, position.line, position.character) + else { + return Ok(None); + }; + // Not on a local variable β†’ let the caller continue the pipeline. + if laravel_lsp::php_variable_rename::variable_at_cursor(&content, byte).is_none() { + return Ok(None); + } + match laravel_lsp::php_variable_rename::variable_rename_targets( + &content, &file_path, byte, new_name, + ) { + Ok(targets) => Ok(laravel_lsp::rename::build_rename_edit(&targets)), + Err(msg) => Err(laravel_lsp::rename::rename_error(msg)), + } + } + /// Create a goto location for a url('path') call /// Navigates to the file in public directory if it exists async fn create_url_location_from_salsa( @@ -20071,6 +20134,15 @@ impl LanguageServer for LaravelLanguageServer { } } + // Plain PHP local variable (`$user`): pure-PHP, single-file, + // scope-aware rename. Checked after the property-oriented magic / + // column member renames above (so `$user->email` still routes there) + // and before the Laravel-pattern classifier, which never classifies a + // bare `$variable`. + if let Some(range) = self.variable_rename_at(uri, position).await { + return Ok(Some(PrepareRenameResponse::Range(range))); + } + let root_path = self.root_path.read().await.clone(); let symbol = match classify_with_decl_fallback( self, @@ -20180,6 +20252,18 @@ impl LanguageServer for LaravelLanguageServer { return Ok(Some(edit)); } + // Plain PHP local variable (`$user`): function-local, scope-aware + // rewrite of every in-scope occurrence. Checked after the + // property-oriented column / magic-member renames (so `$user->email` + // still routes there) and before the Laravel-pattern classifier, which + // never classifies a bare `$variable`. `Ok(None)` here means "not a + // local variable" β€” fall through to the rest of the pipeline. + match self.variable_rename_edit(uri, position, &new_name).await { + Ok(Some(edit)) => return Ok(Some(edit)), + Err(e) => return Err(e), + Ok(None) => {} + } + let root_path = self.root_path.read().await.clone(); let symbol = match classify_with_decl_fallback( self, diff --git a/laravel-lsp/src/php_variable_rename.rs b/laravel-lsp/src/php_variable_rename.rs new file mode 100644 index 00000000..3e0a0b50 --- /dev/null +++ b/laravel-lsp/src/php_variable_rename.rs @@ -0,0 +1,492 @@ +//! Function-local, scope-aware rename for plain PHP variables (`$user`). +//! +//! The Laravel rename engine in [`crate::rename`] handles string-keyed and +//! class-backed Laravel symbols (routes, config keys, views, components, …). +//! It deliberately never touches plain PHP local variables β€” that needs real +//! lexical-scope analysis, not pattern classification. This module is that +//! analysis. +//! +//! ## What it does +//! +//! Renaming `$user` rewrites *every* occurrence of `$user` that resolves to the +//! same binding β€” and nothing else. The unit of a rename is one **binding +//! scope**: the nearest enclosing function-like node that owns the variable. +//! +//! ## Scope model +//! +//! Every variable occurrence resolves to a binding scope by walking up through +//! its function-like ancestors: +//! +//! - **`function_definition` / `method_declaration`** β€” a hard boundary. Params +//! and locals live here; an identically-named variable in a sibling function +//! is a different binding. +//! - **`anonymous_function` (`function () { … }`)** β€” also a hard boundary. A +//! closure does *not* auto-capture: a `$user` used inside is a fresh local +//! unless the closure declares `use ($user)`. When it captures via `use`, the +//! captured variable is bound to the *outer* `$user`, so a rename cascades +//! across the `use` clause and the closure body in lockstep. +//! - **`arrow_function` (`fn () => …`)** β€” transparent for captures. Arrow +//! functions auto-capture outer variables by value, so `fn () => $user` +//! shares `$user`'s binding with the enclosing scope. Its *own parameters* +//! still shadow: `fn ($user) => $user` is a separate binding. +//! - **`program`** β€” the top-level (global) script scope. +//! +//! The same resolver runs for the cursor variable and for every candidate +//! occurrence; an occurrence is part of the rename iff it resolves to the +//! *same* binding scope (compared by node id). That single rule yields nested +//! closure isolation, arrow-capture inclusion, and correct shadowing without +//! any special cases. +//! +//! ## Explicitly excluded +//! +//! - **`$this`** β€” never renameable. +//! - **Static properties** (`self::$bar`, `Foo::$bar`) β€” the `$bar` token is a +//! class property, not a local variable, even though tree-sitter spells it as +//! a `variable_name`. Object properties (`$this->foo`, `$obj->prop`) are +//! `name` nodes, not `variable_name`, so they fall out naturally. +//! - **`global`-imported variables** (`global $x;`) β€” a `global $x` declaration +//! makes the in-scope `$x` an alias of the top-level global, so it is no +//! longer function-local. A scope-local rename would sever the alias (a +//! partial edit leaving `global $x;` or the file-level `$x` behind), so the +//! engine refuses the rename outright. Importing globals is the cross-scope +//! flow this feature defers, not a corruption it should emit. +//! - **`compact()` / `extract()` string-named variables** β€” `compact('user')` +//! references the local `$user` *by string*, and `extract()` can name one the +//! same way. That reference lives in a `string` node, not a `variable_name`, +//! so a scope-local rename can't see it: it would rewrite `$user` and leave +//! `compact('user')` pointing at a variable that no longer exists (broken, +//! idiomatic Laravel β€” `return view('x', compact('user'))`). Same instinct as +//! the `global` guard: refuse rather than emit the partial edit. The dynamic +//! `extract($data)` form, whose names come from a runtime array with no +//! literal in the source, is the cross-scope flow this feature defers and is +//! not detectable here. +//! +//! 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, not here. + +use std::path::Path; +use tree_sitter::Node; + +use crate::rename::EditTarget; + +/// Function-like node kinds that bound a lexical scope. `program` is the +/// top-level script scope and always terminates the upward walk. +/// +/// `anonymous_function_creation_expression` is a vestigial spelling from older +/// tree-sitter-php grammars (0.24.2 emits `anonymous_function`); it is kept here +/// to mirror the defensive pairing used across the rest of the crate +/// (`query_chain`, `member_resolver`, `route_chain`), so this module stays +/// consistent rather than silently diverging. +fn is_function_like(node: Node) -> bool { + matches!( + node.kind(), + "function_definition" + | "method_declaration" + | "anonymous_function" + | "anonymous_function_creation_expression" + | "arrow_function" + | "program" + ) +} + +/// The text of a `variable_name` node with the leading `$` stripped, i.e. the +/// bare identifier (`$user` β†’ `user`). `None` if the bytes aren't valid UTF-8. +fn var_ident<'a>(node: Node<'_>, bytes: &'a [u8]) -> Option<&'a str> { + let text = std::str::from_utf8(bytes.get(node.start_byte()..node.end_byte())?).ok()?; + Some(text.trim_start_matches('$')) +} + +/// The nearest function-like ancestor *above* `node` (never `node` itself). +/// Always returns `Some` for a node in a parsed tree β€” `program` is the +/// backstop at the root. +fn enclosing_function_like(node: Node) -> Option { + let mut cur = node.parent(); + while let Some(n) = cur { + if is_function_like(n) { + return Some(n); + } + cur = n.parent(); + } + None +} + +/// Whether `scope` declares a parameter named `name` (without `$`). Scans the +/// scope's direct `formal_parameters` child β€” covers normal, variadic, and +/// constructor-promoted parameters. +fn scope_declares_param(scope: Node, name: &str, bytes: &[u8]) -> bool { + let mut c = scope.walk(); + for child in scope.children(&mut c) { + if child.kind() != "formal_parameters" { + continue; + } + let mut pc = child.walk(); + for param in child.children(&mut pc) { + if matches!( + param.kind(), + "simple_parameter" | "variadic_parameter" | "property_promotion_parameter" + ) { + if let Some(name_node) = param.child_by_field_name("name") { + if var_ident(name_node, bytes) == Some(name) { + return true; + } + } + } + } + } + false +} + +/// The `variable_name` captured by one child of an +/// `anonymous_function_use_clause`, unwrapping the `by_ref` wrapper that the +/// reference form `use (&$x)` introduces. Returns `None` for children that +/// aren't a simple variable capture (e.g. punctuation, or a `by_ref` over a +/// non-variable expression). In tree-sitter-php the value form `use ($x)` +/// places the `variable_name` as a direct child, while the reference form +/// `use (&$x)` wraps it in a `by_ref` node β€” both must be recognised so that a +/// by-reference capture isn't mistaken for a fresh closure-local. +fn use_clause_capture<'t>(child: Node<'t>) -> Option> { + match child.kind() { + "variable_name" => Some(child), + // `by_ref` wraps exactly one expression; for `use (&$x)` that's the + // `variable_name` (the `&` is an anonymous token). Other by-ref targets + // aren't simple variable captures, so filter to `variable_name`. + "by_ref" => child.named_child(0).filter(|n| n.kind() == "variable_name"), + _ => None, + } +} + +/// Whether `scope` (an anonymous function) captures `name` via a `use (…)` +/// clause β€” both the value form `use ($x)` and the reference form `use (&$x)`. +fn scope_captures_use(scope: Node, name: &str, bytes: &[u8]) -> bool { + let mut c = scope.walk(); + for child in scope.children(&mut c) { + if child.kind() == "anonymous_function_use_clause" { + let mut inner = child.walk(); + for v in child.children(&mut inner) { + if let Some(var) = use_clause_capture(v) { + if var_ident(var, bytes) == Some(name) { + return true; + } + } + } + return false; + } + } + false +} + +/// Resolve the binding scope that owns the variable `name` at `var_node`. +/// +/// Walks up through function-like ancestors applying the scope model +/// documented at the module level: hard boundaries (function/method/closure) +/// stop the walk; arrow functions and `use`-captured closures are transparent +/// (the binding lives in the enclosing scope). +fn resolve_binding_scope<'t>(var_node: Node<'t>, name: &str, bytes: &[u8]) -> Node<'t> { + let mut probe = var_node; + loop { + let Some(scope) = enclosing_function_like(probe) else { + // No function-like ancestor at all β€” the node is at the root. + return probe; + }; + match scope.kind() { + "arrow_function" => { + if scope_declares_param(scope, name, bytes) { + return scope; + } + // Auto-capture by value: look in the enclosing scope. + probe = scope; + } + "anonymous_function" | "anonymous_function_creation_expression" => { + if scope_declares_param(scope, name, bytes) { + return scope; + } + if scope_captures_use(scope, name, bytes) { + // Bound to the outer variable via `use (…)`. + probe = scope; + } else { + // Fresh local inside the closure (closures don't auto-capture). + return scope; + } + } + // `function_definition`, `method_declaration`, `program`: hard scope. + _ => return scope, + } + } +} + +/// Whether the variable `name`, as bound in `scope`, is connected to a +/// `global $name;` declaration β€” making it an alias of the top-level global +/// rather than a pure function-local. A scope-local rename of such a variable +/// would emit a partial edit (severing the alias by leaving `global $x;` or the +/// file-level `$x` behind), so the engine refuses it: importing globals is the +/// cross-scope flow this feature defers. +/// +/// The distinction is resolution-aware, so an unrelated `global` never +/// over-refuses a genuine local: +/// - In a function/method/closure scope, `name` is global-aliased iff a +/// `global $name;` declaration **bound to that same scope** (by node id) +/// appears within it β€” a `global $name;` in a *nested* scope binds there, not +/// here, so it doesn't taint this scope's distinct local. +/// - In the top-level `program` scope, *any* `global $name;` anywhere in the +/// file aliases the one true global `$name`, so all of them count. +fn scope_aliases_global(scope: Node, name: &str, bytes: &[u8]) -> bool { + let program_scope = scope.kind() == "program"; + let mut stack = vec![scope]; + while let Some(n) = stack.pop() { + if n.kind() == "variable_name" + && n.parent().map(|p| p.kind()) == Some("global_declaration") + && var_ident(n, bytes) == Some(name) + && (program_scope || resolve_binding_scope(n, name, bytes).id() == scope.id()) + { + return true; + } + let mut c = n.walk(); + for child in n.children(&mut c) { + stack.push(child); + } + } + false +} + +/// The bare function name of a `function_call_expression` (`compact`, +/// `extract`), or `None` for a dynamic or namespaced call. +fn call_function_name<'a>(call: Node, bytes: &'a [u8]) -> Option<&'a str> { + let f = call.child_by_field_name("function")?; + if f.kind() == "name" { + f.utf8_text(bytes).ok() + } else { + None + } +} + +/// The decoded content of a single/double-quoted string literal, read from its +/// `string_content` child so quotes are excluded. `None` for a non-string node +/// or an empty literal (which can't name a variable anyway). +fn string_literal_content<'a>(node: Node, bytes: &'a [u8]) -> Option<&'a str> { + if !matches!(node.kind(), "string" | "encapsed_string") { + return None; + } + let mut c = node.walk(); + for child in node.children(&mut c) { + if child.kind() == "string_content" { + return child.utf8_text(bytes).ok(); + } + } + None +} + +/// Whether the variable `name`, as bound in `scope`, is named by a string +/// literal argument of a `compact(...)` or `extract(...)` call within that same +/// scope. Such a call references the local *by string* (`compact('user')`) β€” a +/// reference that lives in a `string` node, not a `variable_name`, so the rename +/// can't rewrite it. Renaming the variable while leaving the string behind is a +/// silent partial edit that corrupts valid (and, in Laravel, ubiquitous) PHP, so +/// the engine refuses β€” the same instinct as [`scope_aliases_global`]. +/// +/// Resolution-aware, so a `compact('user')` in a nested scope that owns its own +/// `$user` never over-refuses an enclosing scope's distinct local: +/// - In a function/method/closure scope, the call counts iff the `$name` it +/// references resolves (from the call site) to that same scope by node id. +/// - In the top-level `program` scope, any matching call in the file counts. +/// +/// Only positional **string-literal** arguments are matched. The dynamic +/// `extract($data)` form carries no literal name in the source and is the +/// cross-scope flow this feature defers. +fn scope_references_compact_extract(scope: Node, name: &str, bytes: &[u8]) -> bool { + let program_scope = scope.kind() == "program"; + let mut stack = vec![scope]; + while let Some(n) = stack.pop() { + if n.kind() == "function_call_expression" + && matches!(call_function_name(n, bytes), Some("compact" | "extract")) + { + if let Some(args) = n.child_by_field_name("arguments") { + let mut ac = args.walk(); + for arg in args.named_children(&mut ac) { + // tree-sitter wraps each call argument in an `argument` node; + // the string literal is its last named child. + let expr = if arg.kind() == "argument" { + let mut ic = arg.walk(); + arg.named_children(&mut ic).last() + } else { + Some(arg) + }; + let Some(expr) = expr else { continue }; + if string_literal_content(expr, bytes) == Some(name) + && (program_scope + || resolve_binding_scope(expr, name, bytes).id() == scope.id()) + { + return true; + } + } + } + } + let mut c = n.walk(); + for child in n.children(&mut c) { + stack.push(child); + } + } + false +} + +/// Whether a matching `variable_name` node should participate in the rename. +/// Excludes `$this` and static-property positions (`self::$bar`), which are +/// spelled as `variable_name` but are not local variables. +fn is_collectible(node: Node, bytes: &[u8]) -> bool { + if var_ident(node, bytes) == Some("this") { + return false; + } + if let Some(parent) = node.parent() { + if parent.kind() == "scoped_property_access_expression" + && parent.child_by_field_name("name").map(|n| n.id()) == Some(node.id()) + { + // `self::$bar` / `Foo::$bar` β€” a class property, not a local. + return false; + } + } + true +} + +/// Locate the renameable local-variable `variable_name` node at `cursor_byte`. +/// Handles the cursor landing on the inner identifier or the `$` token (both +/// children of `variable_name`). Returns `None` when the cursor isn't on a +/// renameable local variable (e.g. `$this`, a static property, or a non-variable). +fn renameable_variable_at<'t>( + root: Node<'t>, + bytes: &[u8], + cursor_byte: usize, +) -> Option> { + let hit = root.descendant_for_byte_range(cursor_byte, cursor_byte)?; + let var_node = if hit.kind() == "variable_name" { + hit + } else if hit.parent().map(|p| p.kind()) == Some("variable_name") { + hit.parent()? + } else { + return None; + }; + if !is_collectible(var_node, bytes) { + return None; + } + // Refuse a variable whose binding scope references it in a way the rename + // can't follow, where a scope-local edit would leave a dangling reference: + // - `global $x;` aliases it to the top-level global, and + // - `compact('x')` / `extract('x')` name it by string. + // Both are checked here (not in `is_collectible`) so the *whole* rename is + // refused regardless of where the cursor lands β€” collecting only the + // in-scope `variable_name` occurrences while leaving the alias or the string + // behind is the exact partial-rename corruption this guards against. + let name = var_ident(var_node, bytes)?; + let scope = resolve_binding_scope(var_node, name, bytes); + if scope_aliases_global(scope, name, bytes) + || scope_references_compact_extract(scope, name, bytes) + { + return None; + } + Some(var_node) +} + +/// Collect every `variable_name` node in `scope`'s subtree that names `name` +/// and resolves to `scope` as its binding scope. Descends into nested scopes +/// on purpose β€” arrow-function captures and `use`-clause variables live there, +/// and the per-node binding resolution filters out anything that belongs to a +/// different scope. +fn collect_occurrences<'t>(scope: Node<'t>, name: &str, bytes: &[u8]) -> Vec> { + let mut out = Vec::new(); + let mut stack = vec![scope]; + while let Some(n) = stack.pop() { + if n.kind() == "variable_name" + && var_ident(n, bytes) == Some(name) + && is_collectible(n, bytes) + && resolve_binding_scope(n, name, bytes).id() == scope.id() + { + out.push(n); + } + let mut c = n.walk(); + for child in n.children(&mut c) { + stack.push(child); + } + } + out +} + +/// Validate and normalize a user-supplied new variable name to the bare PHP +/// identifier (leading `$` optional and stripped). `None` if it isn't a legal +/// PHP variable name. +fn normalize_new_var_name(new_name: &str) -> Option { + let ident = new_name.trim().trim_start_matches('$'); + let mut chars = ident.chars(); + let first = chars.next()?; + // PHP identifiers: first char a letter, `_`, or any byte >= 0x80. + if !(first.is_ascii_alphabetic() || first == '_' || (first as u32) >= 0x80) { + return None; + } + if !chars.all(|c| c.is_ascii_alphanumeric() || c == '_' || (c as u32) >= 0x80) { + return None; + } + Some(ident.to_string()) +} + +/// The 0-based `(line, start_column, end_column)` of the renameable local +/// variable under `cursor_byte`, or `None` if the cursor isn't on one. Backs +/// `textDocument/prepareRename` β€” columns span the whole `$name` token. +pub fn variable_at_cursor(source: &str, cursor_byte: usize) -> Option<(u32, u32, u32)> { + let tree = crate::parser::parse_php(source).ok()?; + let bytes = source.as_bytes(); + let var_node = renameable_variable_at(tree.root_node(), bytes, cursor_byte)?; + let start = var_node.start_position(); + let end = var_node.end_position(); + Some((start.row as u32, start.column as u32, end.column as u32)) +} + +/// Build the [`EditTarget`]s renaming the local variable under `cursor_byte` +/// to `new_name`, across its whole binding scope. +/// +/// Returns `Err(message)` when the cursor *is* on a renameable variable but +/// `new_name` isn't a legal PHP identifier β€” the message surfaces to the user. +/// Returns `Ok(vec![])` (a no-op) when the cursor isn't on a local variable or +/// the new name equals the old; callers treat an empty result as "no edit". +pub fn variable_rename_targets( + source: &str, + file_path: &Path, + cursor_byte: usize, + new_name: &str, +) -> Result, String> { + let tree = crate::parser::parse_php(source).map_err(|e| format!("could not parse PHP: {e}"))?; + let bytes = source.as_bytes(); + + let Some(var_node) = renameable_variable_at(tree.root_node(), bytes, cursor_byte) else { + return Ok(Vec::new()); + }; + let Some(old_ident) = var_ident(var_node, bytes) else { + return Ok(Vec::new()); + }; + + let new_ident = normalize_new_var_name(new_name).ok_or_else(|| { + "Rename a variable to a valid PHP name (letters, digits or '_', not starting with a digit)." + .to_string() + })?; + if new_ident == old_ident { + return Ok(Vec::new()); + } + + let scope = resolve_binding_scope(var_node, old_ident, bytes); + let new_text = format!("${new_ident}"); + let targets = collect_occurrences(scope, old_ident, bytes) + .into_iter() + .map(|n| { + let start = n.start_position(); + let end = n.end_position(); + EditTarget { + file_path: file_path.to_path_buf(), + line: start.row as u32, + start_column: start.column as u32, + end_column: end.column as u32, + new_text: new_text.clone(), + } + }) + .collect(); + Ok(targets) +} + +#[cfg(test)] +mod tests; diff --git a/laravel-lsp/src/php_variable_rename/tests.rs b/laravel-lsp/src/php_variable_rename/tests.rs new file mode 100644 index 00000000..4b3509b8 --- /dev/null +++ b/laravel-lsp/src/php_variable_rename/tests.rs @@ -0,0 +1,603 @@ +use super::*; +use std::collections::HashSet; +use std::path::PathBuf; + +/// Byte offset of the `nth` (0-based) occurrence of `needle`, nudged one byte +/// in so the cursor lands *inside* the `$name` token (on the identifier). +fn cursor_byte(source: &str, needle: &str, nth: usize) -> usize { + let pos = source + .match_indices(needle) + .nth(nth) + .unwrap_or_else(|| panic!("occurrence {nth} of {needle:?} not found")) + .0; + pos + 1 +} + +/// Absolute byte offset of a 0-based `(line, column)` position. Uses +/// `split_inclusive` so newlines are counted exactly. +fn abs_byte(source: &str, line: u32, col: u32) -> usize { + let mut offset = 0usize; + for (i, l) in source.split_inclusive('\n').enumerate() { + if i as u32 == line { + return offset + col as usize; + } + offset += l.len(); + } + offset + col as usize +} + +/// The source text an edit target rewrites β€” used to assert every edit lands +/// on a `$name` token, never a stray slice. +fn target_text(source: &str, t: &EditTarget) -> String { + let line = source.split_inclusive('\n').nth(t.line as usize).unwrap(); + line[t.start_column as usize..t.end_column as usize].to_string() +} + +/// The set of absolute byte offsets the targets rewrite. +fn edited_offsets(source: &str, targets: &[EditTarget]) -> HashSet { + targets + .iter() + .map(|t| abs_byte(source, t.line, t.start_column)) + .collect() +} + +fn rename(source: &str, needle: &str, nth: usize, new_name: &str) -> Vec { + variable_rename_targets( + source, + &PathBuf::from("test.php"), + cursor_byte(source, needle, nth), + new_name, + ) + .expect("rename should not error") +} + +// ── Simple function-local rename ────────────────────────────────────────── + +#[test] +fn renames_every_in_scope_occurrence() { + let src = "\ +$key; + return $this->{$key} . $val; +} +"; + // `$obj->$key` and `$this->{$key}` use `$key` as a *real local variable* + // (the dynamic member name), so renaming $key must rewrite all three of its + // occurrences while leaving the property mechanism (`$obj`, `$this`) and the + // unrelated `$val` untouched. + let targets = rename(src, "$key", 0, "$prop"); + assert_eq!(targets.len(), 3, "param + $obj->$key + $this->{{$key}}"); + for t in &targets { + assert_eq!(target_text(src, t), "$key"); + assert_eq!(t.new_text, "$prop"); + } + let edited = edited_offsets(src, &targets); + // The objects and the unrelated local stay put. + assert!(!edited.contains(&abs_byte_of_match(src, "$obj", 0))); + assert!(!edited.contains(&abs_byte_of_match(src, "$this", 0))); + assert!(!edited.contains(&abs_byte_of_match(src, "$val", 0))); +} + +// ── Arrow-function captures + shadowing ─────────────────────────────────── + +#[test] +fn arrow_function_captures_outer_variable() { + let src = "\ + $x + $base; + return $add(5) + $base; +} +"; + // $base is auto-captured by the arrow function β€” renaming it reaches + // inside the arrow body. + let targets = rename(src, "$base", 0, "$origin"); + assert_eq!(targets.len(), 3, "assignment + arrow body + return"); + assert!(targets.iter().all(|t| t.new_text == "$origin")); +} + +#[test] +fn arrow_function_parameter_shadows_outer() { + let src = "\ + $x * 2; + return $g($x); +} +"; + // Renaming the OUTER $x leaves the arrow's parameter + body untouched. + let outer = rename(src, "$x", 0, "$seed"); + assert_eq!(outer.len(), 2, "outer scope only (assignment + call arg)"); + let arrow_param = abs_byte_of_match(src, "$x", 1); // `fn ($x)` + let arrow_body = abs_byte_of_match(src, "$x", 2); // `=> $x * 2` + let edited = edited_offsets(src, &outer); + assert!(!edited.contains(&arrow_param)); + assert!(!edited.contains(&arrow_body)); + + // Renaming the arrow's own $x touches only the arrow's two occurrences. + let inner = rename(src, "$x", 1, "$n"); + assert_eq!(inner.len(), 2, "arrow scope only"); + assert!(inner.iter().all(|t| t.line == 3)); +} + +// ── Property exclusion ──────────────────────────────────────────────────── + +#[test] +fn properties_are_not_caught_by_variable_rename() { + let src = "\ +user = $user; + return self::$user . $user; + } +} +"; + // Renaming the local $user (the method parameter): param + RHS of the + // assignment + the concatenation = 3 sites. The static property + // `self::$user` and the object property `$this->user` stay put. + let targets = rename(src, "$user", 1, "$person"); + assert_eq!(targets.len(), 3, "only the three local-variable sites"); + for t in &targets { + assert_eq!(target_text(src, t), "$user"); + } + + let edited = edited_offsets(src, &targets); + // `self::$user` β€” the `$user` token starts right after `self::`. + let static_prop = src.match_indices("self::$user").next().unwrap().0 + "self::".len(); + assert!( + !edited.contains(&static_prop), + "static property self::$user must be excluded" + ); + // The static property declaration on line 2 must also be untouched. + let decl_prop = abs_byte_of_match(src, "$user", 0); + assert!(!edited.contains(&decl_prop)); +} + +#[test] +fn this_is_not_renameable() { + let src = "x; }\n}\n"; + let byte = cursor_byte(src, "$this", 0); + assert!(variable_at_cursor(src, byte).is_none()); + assert!( + variable_rename_targets(src, &PathBuf::from("t.php"), byte, "$that") + .unwrap() + .is_empty() + ); +} + +// ── Global declarations (alias refusal) ─────────────────────────────────── + +#[test] +fn global_declared_variable_is_not_renameable() { + let src = "\ +role; + return view('profile', compact('user', 'role')); +} +"; + // `compact('user', 'role')` references the locals `$user` and `$role` *by + // string*. A scope-local rename would rewrite the `$user` tokens and leave + // `compact('user')` pointing at a variable that no longer exists β€” broken + // PHP, silently. Refuse from the assignment AND the `$user->role` site (the + // cursor can land on either). + for nth in [0usize, 1] { + let byte = cursor_byte(src, "$user", nth); + assert!( + variable_at_cursor(src, byte).is_none(), + "prepare must refuse at occurrence {nth}" + ); + assert!( + variable_rename_targets(src, &PathBuf::from("t.php"), byte, "$account") + .unwrap() + .is_empty(), + "rename must be a no-op at occurrence {nth}" + ); + } + // The other compacted local, `$role`, is equally unsafe and equally refused. + let role_byte = cursor_byte(src, "$role", 0); + assert!(variable_at_cursor(src, role_byte).is_none()); +} + +#[test] +fn compact_referenced_top_level_variable_is_not_renameable() { + let src = "\ +name; +} +"; + // Guard against over-refusal: a function with no `compact`/`extract` naming + // the variable renames normally. + let targets = rename(src, "$user", 0, "$account"); + assert_eq!(targets.len(), 2, "assignment + $user->name"); + assert!(targets.iter().all(|t| t.new_text == "$account")); +} + +// ── Sibling-function isolation ──────────────────────────────────────────── + +#[test] +fn sibling_functions_isolate_their_own_variables() { + let src = "\ + usize { + source + .match_indices(needle) + .nth(nth) + .unwrap_or_else(|| panic!("occurrence {nth} of {needle:?} not found")) + .0 +}