Prevent memory growth from file scanning

Author: AJenbo

docs/CHANGELOG.md

@@ -28,6 +28,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - **Elseif chain narrowing.** In `if/elseif/else` chains, the else branch now strips types from all preceding conditions. For example, `if (is_string($x)) {} elseif (is_int($x)) {} else { $x-> }` correctly narrows `$x` by excluding both `string` and `int`.
 - **Variadic `@param` template bindings.** `@param class-string<T> ...$items` now correctly binds the template parameter. Previously the `...` prefix prevented the parameter name from being recognized, so generic return types were not substituted.
+- **Laravel relationship classification.** Relationship return types that are fully-qualified to a non-Eloquent namespace (e.g. a custom `App\Relations\HasMany`) are no longer misclassified as Eloquent relationships. Only types under `Illuminate\Database\Eloquent\Relations\` or unqualified short names are recognized.
+- **Memory usage after go-to-implementation and find references.** Files transiently loaded during implementation scanning and workspace indexing are now evicted from the AST cache when they are not open in the editor. Previously these entries accumulated and increased memory usage over time.
 - **`@phpstan-type` aliases in foreach.** Type aliases defined via `@phpstan-type` or `@psalm-type` now resolve correctly when the aliased type is iterated in a `foreach` loop, destructured with `list()`/`[]`, or used as a foreach key type.
 - **Mixed `->` then `::` accessor chains.** Expressions like `$obj->prop::$staticProp` and `$obj->method()::staticMethod()` now resolve through the full chain instead of losing the instance prefix.
 - **Inline `(new Foo)->method()` chaining.** Parenthesized `new` expressions used as the root of a method chain now resolve for completion. Previously only assigned `new` expressions (`$x = new Foo()`) worked.

docs/todo.md

@@ -188,12 +188,6 @@ eventually but don't move the needle.
 | 72 | Switch → match conversion | Medium | Code Actions | [actions.md §4](todo/actions.md#4-switch--match-conversion) |
 | 73 | Incremental text sync | Medium | LSP Features | [lsp-features.md §17](todo/lsp-features.md#17-incremental-text-sync) |
 
-### Bug fixes
-
-| # | Item | Effort | Domain | Doc Link |
-|---|---|---|---|---|
-| 74 | Short-name collisions in `find_implementors` | Low | Bug Fixes | [bugs.md §1](todo/bugs.md#1-short-name-collisions-in-find_implementors) |
-| 75 | Evict transiently-loaded files from ast_map after GTI and Find References | Low | LSP Features | [bugs.md §13](todo/bugs.md#13-evict-transiently-loaded-files-from-ast_map-after-gti-and-find-references) |
 
 ---
 
@@ -216,7 +210,6 @@ separately by their own impact÷effort scoring.
 | L10 | `SoftDeletes` trait methods on Builder | ★ | ★ | [§10](todo/laravel.md#10-softdeletes-trait-methods-on-builder) |
 | L11 | `View::withX()` / `RedirectResponse::withX()` dynamic methods | ★ | ★★ | [§11](todo/laravel.md#11-viewwithx-and-redirectresponsewithx-dynamic-methods) |
 | L12 | `$appends` array | ★ | ★ | [§12](todo/laravel.md#12-appends-array) |
-| L13 | Relationship classification matches short name only | ★ | ★★ | [§13](todo/laravel.md#13-relationship-classification-matches-short-name-only) |
 
 ---
 

docs/todo/bugs.md

@@ -26,60 +26,32 @@ fallback is only used when FQN information is absent. `seen_fqns` in
 ---
 
 ## 2. GTD fires on parameter variable names and class declaration names
-**Impact: Medium · Effort: Low**
-
-Go-to-definition fires on parameter variable names (`$supplier`, `$country`)
-and class declaration names (`class Foo`), navigating to the same location —
-the cursor is already at the definition. This is noisy and unexpected:
-clicking a parameter name or a class declaration name should either do
-nothing or offer a different action (e.g. find references).
-
-### Current behaviour
-
-- **Parameter names:** Ctrl+Click on `$supplier` in a method signature
-  jumps to… `$supplier` in the same method signature. The `VarDefSite`
-  with `kind: Parameter` is correctly recorded, and `find_var_definition`
-  returns it — so the "definition" is the cursor's own position.
-
-- **Class declarations:** Ctrl+Click on `Foo` in `class Foo {` jumps to
-  the same `Foo` token. The `SymbolMap` records a `ClassDeclaration`
-  span, and `resolve_definition` resolves it to the same file and offset.
-
-### Fix
-
-In the definition handler, after resolving the definition location, check
-whether the target location is the same as (or within a few bytes of) the
-cursor position. If so, return `None` — there is no useful jump to make.
+**Impact: Medium · Effort: Low (fixed)**
 
-Alternatively, suppress at the `SymbolKind` level:
-- For `Variable` spans where `var_def_kind_at` returns `Some(Parameter)`,
-  skip definition.
-- For `ClassDeclaration` spans, skip definition.
+**Status:** Fixed. Three layers suppress self-referential jumps:
 
-### Tests to update
+1. `resolve_from_symbol` returns `None` for `ClassDeclaration` and
+   `MemberDeclaration` symbol kinds (the cursor is at the definition).
+2. `lookup_var_def_kind_at` detects when the cursor is on a variable
+   at its definition site (parameter, assignment LHS, foreach binding,
+   catch binding) and returns `None` before `find_var_definition` runs.
+3. A self-reference guard in `resolve_definition` suppresses jumps
+   when the resolved location points back to the cursor position.
 
-Several existing definition tests assert that parameter names and class
-declarations produce a definition result pointing to themselves. These should
-expect `None` instead.
+Tested with `test_goto_definition_parameter_at_definition_returns_none`
+and related tests in `definition_variables.rs`.
 
 ---
 
 ## 3. Relationship classification matches short name only
-**Impact: Low · Effort: Low**
-
-`classify_relationship` in `virtual_members/laravel.rs` strips the
-return type down to its short name (via `short_name`) and matches
-against a hardcoded list (`HasMany`, `BelongsTo`, etc.). This means
-any class whose short name collides with a Laravel relationship class
-(e.g. a custom `App\Relations\HasMany` that does not extend
-Eloquent's) would be incorrectly classified as a relationship.
+**Impact: Low · Effort: Low (fixed)**
 
-The fix would be to resolve the return type to its FQN (using the
-class loader or use-map) and verify it lives under
-`Illuminate\Database\Eloquent\Relations\` (or extends a class that
-does) before classifying. The short-name-only path could remain as a
-fast-path fallback when the FQN is already in the
-`Illuminate\Database\Eloquent\Relations` namespace.
+**Status:** Fixed. `classify_relationship` now checks whether a
+namespace-qualified return type lives under
+`Illuminate\Database\Eloquent\Relations\` before classifying.
+Unqualified short names (the common case for body-inferred types and
+use-imported docblock annotations) still match by short name only.
+A custom `App\Relations\HasMany` is no longer misclassified.
 
 ---
 
@@ -139,23 +111,26 @@ error message.
 ---
 
 ## 13. Evict transiently-loaded files from ast_map after GTI and Find References
-**Impact: Low · Effort: Low**
+**Impact: Low · Effort: Low (fixed)**
 
-Go-to-implementation (Phases 3 and 5) and Find References
-(`ensure_workspace_indexed`) parse files from disk and cache them in
-`ast_map`. Most of these are false positives that passed the cheap
-string pre-filter but don't actually contain matching symbols. Even the
-true matches are rarely needed afterwards (the user will open the one
-they care about through the editor, which triggers a fresh `did_open`).
+**Status:** Fixed. `find_implementors` snapshots `ast_map` keys before
+scanning and calls `evict_transient_entries` afterwards, removing any
+`ast_map`, `symbol_maps`, `use_map`, and `namespace_map` entries that
+were added during the scan and are not in `open_files`.
+`ensure_workspace_indexed` does the same via
+`evict_transient_ast_entries`, which preserves `symbol_maps` (since
+those are the purpose of the indexing scan) but evicts the other maps.
 
-Keeping these files in the ast_map wastes memory and pollutes subsequent
-Phase 1 scans with classes from files that aren't part of the user's
-working set.
+---
+
+## 14. Signature help fires on function definition sites
+**Impact: Low · Effort: Low**
 
-**Fix:** After `find_implementors` returns, remove any `ast_map` entries
-whose URI was not already present before the scan started. Same for
-`ensure_workspace_indexed`. Collect the set of existing URIs before the
-scan, then evict the difference afterwards. Files that are in
-`open_files` must never be evicted.
+Signature help triggers when the cursor is inside the parameter list of
+a function *definition* (e.g. `function unionDemo(string|int $value, ?User $maybe)`).
+Method definitions already suppress this, but standalone function
+definitions do not receive the same treatment.
 
-**Discovered via:** fixture conversion (call_expression/static_factory_return_self).
+The fix should mirror the method-definition suppression: detect that the
+cursor is inside a function declaration's parameter list and return
+`None` before attempting signature help resolution.

docs/todo/laravel.md

@@ -438,23 +438,3 @@ accessors, so in most cases the accessor method itself already produces
 the virtual property. Parsing `$appends` would only help when the
 accessor is defined in an unloaded parent class.
 
-#### 13. Relationship classification matches short name only
-
-| | |
-|---|---|
-| **Impact** | ★ — Nearly all codebases use Laravel's built-in relationship classes, so false positives are rare in practice. |
-| **Effort** | ★★ — Need to resolve the return type's FQN before matching, which may require a class loader call. |
-
-`classify_relationship` in `virtual_members/laravel.rs` strips the
-return type down to its short name (via `short_name`) and matches
-against a hardcoded list (`HasMany`, `BelongsTo`, etc.). This means
-any class whose short name collides with a Laravel relationship class
-(e.g. a custom `App\Relations\HasMany` that does not extend
-Eloquent's) would be incorrectly classified as a relationship.
-
-The fix would be to resolve the return type to its FQN (using the
-class loader or use-map) and verify it lives under
-`Illuminate\Database\Eloquent\Relations\` (or extends a class that
-does) before classifying. The short-name-only path could remain as a
-fast-path fallback when the FQN is already in the
-`Illuminate\Database\Eloquent\Relations` namespace.

src/definition/implementation.rs

@@ -47,6 +47,34 @@ impl Backend {
         uri: &str,
         content: &str,
         position: Position,
+    ) -> Option<Vec<Location>> {
+        // Snapshot ast_map keys before the scan so we can evict
+        // transiently-loaded entries afterwards (see §13 in bugs.md).
+        let pre_scan_uris: HashSet<String> = self
+            .ast_map
+            .lock()
+            .ok()
+            .map(|m| m.keys().cloned().collect())
+            .unwrap_or_default();
+
+        let result = self.resolve_implementation_inner(uri, content, position);
+
+        // Evict ast_map entries that were added during scanning
+        // (Phases 3 and 5 of find_implementors) but are not open in
+        // the editor.  This must happen after locate_class_declaration
+        // has read the cached content.
+        self.evict_transient_entries(&pre_scan_uris);
+
+        result
+    }
+
+    /// Inner implementation of [`resolve_implementation`] that performs
+    /// the actual symbol resolution without eviction concerns.
+    fn resolve_implementation_inner(
+        &self,
+        uri: &str,
+        content: &str,
+        position: Position,
     ) -> Option<Vec<Location>> {
         // ── 1. Extract the word under the cursor ────────────────────────
         // Primary path: consult the precomputed symbol map.

src/references/mod.rs

@@ -775,6 +775,15 @@ impl Backend {
     /// `composer.json` `config.vendor-dir`, defaulting to `vendor`) is
     /// skipped during the filesystem walk.
     fn ensure_workspace_indexed(&self) {
+        // Snapshot ast_map keys before scanning so we can evict
+        // transiently-loaded entries afterwards (see §13 in bugs.md).
+        let pre_scan_uris: HashSet<String> = self
+            .ast_map
+            .lock()
+            .ok()
+            .map(|m| m.keys().cloned().collect())
+            .unwrap_or_default();
+
         // Collect URIs that already have symbol maps.
         let existing_uris: HashSet<String> = self
             .symbol_maps
@@ -860,6 +869,13 @@ impl Backend {
                 }
             }
         }
+
+        // ── Evict transient ast_map entries ─────────────────────────────
+        // The scan above may have added files to ast_map, use_map, and
+        // namespace_map that are not open in the editor.  Symbol maps are
+        // preserved (they are the whole point of this scan), but the other
+        // maps can be evicted to save memory.
+        self.evict_transient_ast_entries(&pre_scan_uris);
     }
 }
 

src/util.rs

@@ -761,6 +761,94 @@ impl Backend {
         }
     }
 
+    /// Evict `ast_map` (and associated map) entries that were added
+    /// during a transient scan (go-to-implementation, find references).
+    ///
+    /// `pre_scan_uris` is the set of URIs that were already in `ast_map`
+    /// before the scan started.  Any URI that is now in `ast_map` but was
+    /// not in `pre_scan_uris` — and is not currently open in the editor —
+    /// is removed from `ast_map`, `symbol_maps`, `use_map`, and
+    /// `namespace_map`.  This prevents memory bloat from files that were
+    /// parsed only to check whether they contain a matching symbol.
+    ///
+    /// Use [`evict_transient_ast_entries`](Self::evict_transient_ast_entries)
+    /// instead when `symbol_maps` should be preserved (e.g. after
+    /// `ensure_workspace_indexed`, where the symbol maps are the whole
+    /// point of the scan).
+    pub(crate) fn evict_transient_entries(
+        &self,
+        pre_scan_uris: &std::collections::HashSet<String>,
+    ) {
+        self.evict_transient_inner(pre_scan_uris, true);
+    }
+
+    /// Like [`evict_transient_entries`](Self::evict_transient_entries)
+    /// but preserves `symbol_maps`.
+    ///
+    /// `ensure_workspace_indexed` builds symbol maps so that find
+    /// references can scan them.  The `ast_map`, `use_map`, and
+    /// `namespace_map` entries for those files are no longer needed
+    /// after indexing and can be evicted to save memory.
+    pub(crate) fn evict_transient_ast_entries(
+        &self,
+        pre_scan_uris: &std::collections::HashSet<String>,
+    ) {
+        self.evict_transient_inner(pre_scan_uris, false);
+    }
+
+    /// Shared implementation for transient entry eviction.
+    ///
+    /// When `evict_symbol_maps` is true, `symbol_maps` entries are
+    /// removed alongside `ast_map`, `use_map`, and `namespace_map`.
+    fn evict_transient_inner(
+        &self,
+        pre_scan_uris: &std::collections::HashSet<String>,
+        evict_symbol_maps: bool,
+    ) {
+        // Collect URIs that were added during the scan.
+        let new_uris: Vec<String> = self
+            .ast_map
+            .lock()
+            .ok()
+            .map(|m| {
+                m.keys()
+                    .filter(|uri| !pre_scan_uris.contains(*uri))
+                    .cloned()
+                    .collect()
+            })
+            .unwrap_or_default();
+
+        if new_uris.is_empty() {
+            return;
+        }
+
+        // Never evict files that are currently open in the editor.
+        let open: std::collections::HashSet<String> = self
+            .open_files
+            .lock()
+            .ok()
+            .map(|f| f.keys().cloned().collect())
+            .unwrap_or_default();
+
+        for uri in &new_uris {
+            if open.contains(uri) {
+                continue;
+            }
+            if let Ok(mut map) = self.ast_map.lock() {
+                map.remove(uri);
+            }
+            if evict_symbol_maps && let Ok(mut map) = self.symbol_maps.lock() {
+                map.remove(uri);
+            }
+            if let Ok(mut map) = self.use_map.lock() {
+                map.remove(uri);
+            }
+            if let Ok(mut map) = self.namespace_map.lock() {
+                map.remove(uri);
+            }
+        }
+    }
+
     pub(crate) async fn log(&self, typ: MessageType, message: String) {
         if let Some(client) = &self.client {
             client.log_message(typ, message).await;