Rename no longer rewrites unrelated code

Author: AJenbo

docs/CHANGELOG.md

@@ -54,6 +54,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - **Parameter name inlay hints no longer shift to the wrong parameter when only part of a multi-line call is visible.** Editors request inlay hints only for the currently visible viewport, and when a call's arguments were split across the viewport boundary, every hint after the first excluded argument was labelled with the previous parameter's name instead of its own. This was most noticeable on multi-line constructor calls with several arguments, such as those using constructor property promotion.
 - **Linked editing no longer types into unrelated parts of the file.** Adding a line above a variable (for example inserting a `/** */` docblock) and then typing could insert the typed characters at two arbitrary places further down, such as in the middle of a method name and in front of a trailing comment. PHPantom is re-reading the file in the background while you type, and linked editing was handing the editor positions measured against the previous version of the file, which the editor then edited on trust. Linked editing now verifies that the positions it reports still point at the variable, and offers nothing at all if they do not, so a keystroke can never land somewhere unexpected. It also becomes available in Blade templates, where the reported positions are checked against the template itself rather than the generated PHP.
+- **Rename no longer rewrites unrelated code when a file has just been edited.** PHPantom re-reads a file in the background after every keystroke, and rename could hand the editor positions measured against the previous version of that file, so confirming the rename edited whatever now sat at those positions. Rename and the rename preview now check that every position they report still points at the symbol, in each file the rename touches, and offer nothing at all when one does not. Retrying a moment later, once the background re-read has caught up, renames normally.
 - **Diagnostic and request workers no longer copy the embedded stub indexes.** Every diagnostic pass and every hover, completion, or go-to-definition request cloned the full embedded stub class, function, and constant indexes (thousands of entries each) instead of sharing them. During workspace analysis this happened twice per file, and the resulting allocation churn serialised the parallel diagnostic workers in the memory allocator. The indexes are now shared, roughly halving `analyze` wall time on large Laravel projects and removing the same overhead from every editor request.
 - **Member resolution no longer degrades on deeply nested class dependencies.** Class resolution previously cut off at internal nesting limits, silently returning incomplete member sets (missing completions, spurious unknown-member diagnostics) on projects with deeply intertwined hierarchies and virtual members. Those limits are gone: only genuine dependency cycles, such as two Eloquent models whose relationships reference each other, fall back to a partial view, and they now do so deterministically instead of depending on which class happened to resolve first on a given thread.
 - **Completion works after closing a multi-line closure argument on the same line as the next chain operator.** Typing `->` immediately after a call like `->map(function (...) { ... })->` now resolves the full receiver instead of returning no member suggestions until the operator is moved to a new line. Contributed by @calebdw.

docs/todo.md

@@ -113,7 +113,7 @@ unlikely to move the needle for most users.
 | D5  | [External tool diagnostic suppression actions](todo/diagnostics.md#d5-external-tool-diagnostic-suppression-actions)                                                         | Low         | Low         |
 | D15 | [Unused parameter diagnostic](todo/diagnostics.md#d15-unused-parameter-diagnostic)                                                                                          | Low         | Low         |
 |     | **[Bug Fixes](todo/bugs.md)**                                                                                                                                               |             |             |
-| B1  | [Rename can build edits from a symbol map that predates the buffer](todo/bugs.md#b1-rename-can-build-edits-from-a-symbol-map-that-predates-the-buffer)                       | Medium-High | Low-Medium  |
+| B2  | [Namespace rename writes an FQN into every name of a group `use`](todo/bugs.md#b2-namespace-rename-writes-an-fqn-into-every-name-of-a-group-use)                             | Medium      | Low         |
 |     | **[Code Actions](todo/actions.md)**                                                                                                                                         |             |             |
 | A40 | [Generate method from call](todo/actions.md#a40-generate-method-from-call)                                                                                                  | Medium-High | Medium      |
 | A41 | [Create class from non-existing name](todo/actions.md#a41-create-class-from-non-existing-name)                                                                              | Medium      | Medium      |

docs/todo/bugs.md

@@ -7,37 +7,41 @@ pipeline so it produces correct data. Downstream consumers
 (diagnostics, hover, completion, definition) should never need
 to second-guess upstream output.
 
-## B1 Rename can build edits from a symbol map that predates the buffer
-
-`symbol_maps` is refreshed on a background task after each `didChange`,
-and is left untouched entirely when a parse panics. Until that lands, a
-request reads a map whose byte offsets index the *previous* version of
-the file. Converting those offsets against the newer buffer yields
-ranges over unrelated code.
-
-Linked editing hit this in practice (inserting a line above a variable
-and typing mirrored the keystrokes into the middle of a method name
-further down the file). It was fixed by checking
-`SymbolMap::matches_source` and verifying that every range it reports
-still spells the variable, dropping the whole response otherwise.
-
-`rename` and `prepareRename` read the same map and turn the same offsets
-into `TextEdit`s, so the same corruption is reachable there:
-
-- `rename/prepare.rs` builds the prepare-rename range straight from
-  `span.start`/`span.end` via `offset_to_position` against `content`.
-- `rename/namespace.rs` and `rename/class.rs` read `symbol_maps` per
-  file and emit edits from the span offsets they find.
-
-It is much harder to trigger than linked editing was, because rename is
-an explicit user action taken after a pause rather than something the
-editor fires on every cursor move, and because the rename box shows the
-range so a wrong one is visible before the user confirms. It is the same
-defect class, though, and the fix is the same: gate the edit-producing
-paths on `matches_source` and verify each emitted range against the
-buffer text before returning it.
-
-Note that the cross-file paths need slightly more than linked editing
-did: they read maps for files other than the request URI, so each one
-must be checked against *that* file's content, not the buffer the
-request arrived on.
+## B2 Namespace rename writes an FQN into every name of a group `use`
+
+Renaming namespace `App\Old` to `App\New` turns
+
+```php
+use App\Old\{Foo, Bar};
+```
+
+into
+
+```php
+use App\New\{App\New\Foo, App\New\Bar};
+```
+
+which is a parse error. This is covered by
+`rename_namespace_updates_group_use` in `src/rename/tests.rs`, but the
+assertion only checks that `App\New` appears somewhere in the result, so
+it passes on the broken output.
+
+Two paths in `rename/namespace.rs` edit the same line:
+
+- `collect_use_statement_edits` scans the source and rewrites the group
+  prefix (`App\Old` → `App\New`), which is the correct and complete edit.
+- `collect_fqn_reference_edits` walks the symbol map, where each name
+  inside the braces is a `ClassReference` span whose recorded `name` is
+  the *composed* FQN (`App\Old\Foo`) while the span itself covers only
+  the trailing `Foo`. It therefore emits a second edit replacing `Foo`
+  with the full `App\New\Foo`.
+
+The two edits do not overlap, so the `dedup_by(ranges_overlap)` pass that
+resolves the ordinary `use App\Old\Foo;` case does not catch them.
+
+The fix belongs in `collect_fqn_reference_edits`: a span whose source
+text is only a segment-aligned tail of its recorded name is a group-use
+member, and the group prefix edit already covers it, so it should be
+skipped. `source_spells_reference` in the same file already
+distinguishes the two shapes. Extend the test to assert the full
+expected line rather than a substring.

src/rename/class.rs

@@ -437,11 +437,24 @@ impl Backend {
                 && namespace_changed
                 && let Some(sm) = self.symbol_maps.read().get(file_uri_str).cloned()
             {
-                if let Some(ns_span) = sm
-                    .spans
-                    .iter()
-                    .find(|s| matches!(&s.kind, SymbolKind::NamespaceDeclaration { .. }))
-                {
+                // This is the one edit in this function built straight from
+                // symbol-map offsets rather than from a verified reference
+                // location, so it needs the same guard: the map must
+                // describe the file, and the span must still spell the
+                // namespace it claims to.
+                if !sm.matches_source(&file_content) {
+                    return None;
+                }
+
+                if let Some((ns_span, ns_name)) = sm.spans.iter().find_map(|s| match &s.kind {
+                    SymbolKind::NamespaceDeclaration { name } => Some((s, name)),
+                    _ => None,
+                }) {
+                    if file_content.get(ns_span.start as usize..ns_span.end as usize)
+                        != Some(ns_name.as_str())
+                    {
+                        return None;
+                    }
                     let start = offset_to_position(&file_content, ns_span.start as usize);
                     let end = offset_to_position(&file_content, ns_span.end as usize);
                     file_edits.push(TextEdit {

src/rename/mod.rs

@@ -21,6 +21,9 @@
 //!   `use` import FQN is updated (last segment only), aliases are
 //!   preserved, and collisions with existing imports are resolved by
 //!   introducing an alias.
+//! - Staleness guards: every range that becomes a `TextEdit` is checked
+//!   against the text of the file it belongs to before the response goes
+//!   out.  See [`validate`] for why.
 //! - Namespace rename: when renaming a namespace segment, all
 //!   `namespace` declarations, `use` statements, and fully-qualified
 //!   references across the workspace are updated.  When a PSR-4
@@ -30,5 +33,6 @@
 mod class;
 mod namespace;
 mod prepare;
+mod validate;
 
 mod tests;

src/rename/namespace.rs

@@ -139,13 +139,19 @@ impl Backend {
             self.collect_use_statement_edits(&content, old_prefix, new_prefix, &mut file_edits);
 
             // 3. Update inline FQN references from the symbol map.
-            self.collect_fqn_reference_edits(
+            //    Steps 1 and 2 scan `content` directly, so only this one
+            //    can be poisoned by a map that predates the file; when it
+            //    is, the whole rename is dropped.  Emitting the other two
+            //    files' edits would leave the workspace half-renamed.
+            if !self.collect_fqn_reference_edits(
                 file_uri,
                 &content,
                 old_prefix,
                 new_prefix,
                 &mut file_edits,
-            );
+            ) {
+                return None;
+            }
 
             if !file_edits.is_empty() {
                 // Sort edits by start position descending so they don't
@@ -371,19 +377,29 @@ impl Backend {
 
     /// Collect text edits for inline FQN references (e.g. `\App\Old\Foo`
     /// in type hints or docblocks) that contain the old prefix.
+    ///
+    /// Returns `false` when the file's symbol map cannot be trusted
+    /// against its current text, in which case no edits are collected and
+    /// the caller must abandon the rename.
     fn collect_fqn_reference_edits(
         &self,
         file_uri: &str,
         content: &str,
         old_prefix: &str,
         new_prefix: &str,
         edits: &mut Vec<TextEdit>,
-    ) {
+    ) -> bool {
         let symbol_map = match self.symbol_maps.read().get(file_uri) {
             Some(sm) => sm.clone(),
-            None => return,
+            // No map means no offsets to mistrust: this file's namespace
+            // and use-statement edits come from scanning `content`.
+            None => return true,
         };
 
+        if !symbol_map.matches_source(content) {
+            return false;
+        }
+
         let old_prefix_lower = old_prefix.to_lowercase();
 
         for span in &symbol_map.spans {
@@ -411,6 +427,13 @@ impl Backend {
                 .get(span.start as usize..span.end as usize)
                 .unwrap_or("");
 
+            // A same-length edit keeps `matches_source` happy while still
+            // moving the token, so re-read the span and require it to
+            // spell the name the map recorded.
+            if !source_spells_reference(source, name) {
+                return false;
+            }
+
             // Skip use-statement references (they don't have `\` in span
             // unless they are inline FQN like `\App\Foo` in code).
             // Actually, use-statement spans DO contain the full FQN.
@@ -444,6 +467,8 @@ impl Backend {
                 new_text: new_name,
             });
         }
+
+        true
     }
 
     /// Determine PSR-4 directory rename operations for a namespace rename.
@@ -517,6 +542,26 @@ impl Backend {
     }
 }
 
+/// Whether `source` is text that can spell a reference recorded as `name`.
+///
+/// Normally the span covers the whole name, but a group `use`
+/// (`use App\Old\{Foo, Bar};`) records the composed FQN `App\Old\Foo` on a
+/// span that covers only the trailing `Foo`, so a segment-aligned tail
+/// counts too.  Anything else means the offsets no longer describe the
+/// buffer.
+fn source_spells_reference(source: &str, name: &str) -> bool {
+    let source = strip_fqn_prefix(source);
+    let name = strip_fqn_prefix(name);
+    source.eq_ignore_ascii_case(name)
+        || name
+            .len()
+            .checked_sub(source.len())
+            .and_then(|split| name.split_at_checked(split))
+            .is_some_and(|(prefix, tail)| {
+                prefix.ends_with('\\') && tail.eq_ignore_ascii_case(source)
+            })
+}
+
 // ─── Namespace segment helpers ──────────────────────────────────────────────
 
 /// Given a namespace name (e.g. `"App\\Bar\\Service"`) and its starting

src/rename/prepare.rs

@@ -16,6 +16,7 @@ use crate::text_position::offset_to_position;
 use crate::util::build_fqn;
 
 use super::namespace::find_namespace_segment_at_offset;
+use super::validate::span_spells_its_name;
 
 impl Backend {
     /// Handle `textDocument/prepareRename`.
@@ -32,6 +33,13 @@ impl Backend {
     ) -> Option<PrepareRenameResponse> {
         let span = self.lookup_symbol_at_position(uri, content, position)?;
 
+        // The range below is built from this span's byte offsets, and the
+        // editor shows it as the text about to be replaced.  A map that
+        // predates the buffer would put it over unrelated code.
+        if !self.rename_map_matches(uri, content) || !span_spells_its_name(content, &span) {
+            return None;
+        }
+
         // Reject non-renameable symbols.
         if let SymbolKind::SelfStaticParent(_) = &span.kind {
             // self, static, parent, and $this are never renameable.
@@ -86,6 +94,14 @@ impl Backend {
     ) -> Option<WorkspaceEdit> {
         let span = self.lookup_symbol_at_position(uri, content, position)?;
 
+        // Every edit below is derived, directly or through find-references,
+        // from this span.  If the map it came from predates the buffer the
+        // whole response is nonsense, so drop it rather than rename the
+        // wrong symbol.
+        if !self.rename_map_matches(uri, content) || !span_spells_its_name(content, &span) {
+            return None;
+        }
+
         // Reject non-renameable symbols (same logic as prepare_rename).
         if let SymbolKind::SelfStaticParent(_) = &span.kind {
             // self, static, parent, and $this are never renameable.
@@ -119,6 +135,13 @@ impl Backend {
             return None;
         }
 
+        // The reference finders read one symbol map per file, so each
+        // location has to be checked against *that* file's text, not the
+        // buffer this request arrived on.
+        if !self.rename_locations_verified(&span.kind, &locations) {
+            return None;
+        }
+
         // Determine whether this is a property rename.  Properties are
         // special because the `$` prefix is part of the declaration but
         // usage sites via `->` or `?->` don't include it.