Fix regression from 500998f

Author: AJenbo

docs/CHANGELOG.md

@@ -55,6 +55,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Diagnostic code identifiers.** All diagnostic codes now use a consistent `snake_case` noun-phrase scheme: `unknown_variable`, `type_mismatch_argument`, `argument_count_mismatch`, `deprecated_usage`, `missing_implementation`. Users with editor filters matching on these codes will need to update them.
 - **Lower memory usage for lazily-loaded files.** Vendor and stub files no longer store per-file import tables and namespace maps after parsing, and go-to-implementation uses a dedicated reverse-inheritance index instead of scanning all parsed files.
 - **Lower memory usage for variable type tracking.**
+- **Faster variable name completion.** Variable name suggestions now use the precomputed symbol map instead of re-parsing the file. Foreach iteration variables correctly persist after the loop (matching PHP semantics), `@var` docblock variable names are included, and `unset()` removes variables from suggestions.
+- **Faster go-to-definition for variables.** Variable definition lookup no longer re-parses the file as a fallback; the precomputed symbol map handles all cases.
 - **Updated embedded phpstorm-stubs.**
 
 ### Fixed

docs/todo/refactor.md

@@ -214,34 +214,6 @@ Each item must include:
 
 # Outstanding items
 
-## Intentional overlap (reference, not actionable)
-
-These are parallel systems that exist for valid reasons. Documented here so
-they are factored into future design decisions.
-
-### `uri_classes_index` vs `fqn_class_index` vs `fqn_uri_index`
-
-Three maps storing overlapping class data:
-- `uri_classes_index`: URI → `Vec<Arc<ClassInfo>>` (needed for "all classes in this file")
-- `fqn_class_index`: FQN → `Arc<ClassInfo>` (O(1) FQN lookup)
-- `fqn_uri_index`: FQN → URI string (survives `didClose`, enables re-loading)
-
-Data is shared via `Arc`. Each serves a distinct query pattern.
-
-### PathBuf ↔ URI round-tripping
-
-The classmap scanner produces `PathBuf`, converts to URI strings for
-`fqn_uri_index`, then at lookup time parses back to `PathBuf` for file reading.
-A minor inefficiency, not a parallel system.
-
-### `find_class_in_ast_map` linear scan fallback
-
-`src/util.rs` L1436–1447: After the O(1) `fqn_class_index` lookup fails, falls
-back to linear scan of `uri_classes_index`. Covers race conditions during initial
-indexing. Legitimate safety net.
-
----
-
 ## Redundant backwards text walkers
 
 These functions in `src/completion/source/helpers.rs` scan backward with `rfind`
@@ -278,21 +250,6 @@ The SymbolMap is built once per file in `update_ast_inner` and stores symbol
 spans, variable definitions, scope boundaries, and call sites. Several features
 re-parse the AST to extract information the SymbolMap already has.
 
-### Variable definition lookup
-
-The SymbolMap has `var_defs` with `find_var_definition()`, but
-`definition/variable/mod.rs` ignores it and does its own full AST walk via
-`find_variable_definition_in_program()`. The SymbolMap's `var_defs` are used
-elsewhere (hover, forward walk) but the go-to-definition feature duplicates
-the work.
-
-### Variables-in-scope collection
-
-`collect_variables_in_scope` in `completion.rs` re-parses the AST to find all
-variables visible at the cursor. The SymbolMap's `var_defs` already has every
-variable definition site with scope boundaries; this could be a filtered query
-on `var_defs` instead of a full AST walk.
-
 ### Scope boundary detection
 
 The SymbolMap stores `scopes` (function/closure boundaries) and provides

src/completion/variable/completion.rs

@@ -10,6 +10,7 @@ use tower_lsp::lsp_types::*;
 
 use crate::Backend;
 use crate::symbol_map::SymbolMap;
+use crate::symbol_map::VarDefKind;
 use crate::util::position_to_byte_offset;
 
 impl Backend {
@@ -252,6 +253,26 @@ fn collect_variables_from_symbol_map(
         }
     }
 
+    // Remove variables whose most recent definition before cursor is an Unset.
+    let mut to_remove = Vec::new();
+    for var_name_with_dollar in &vars {
+        let name_without_dollar = &var_name_with_dollar[1..];
+        let last_def = map.var_defs.iter().rfind(|d| {
+            d.name == name_without_dollar
+                && visible_scopes.contains(&d.scope_start)
+                && d.effective_from <= cursor_offset
+        });
+        if let Some(d) = last_def
+            && d.kind == VarDefKind::Unset
+            && (d.block_end == u32::MAX || cursor_offset <= d.block_end)
+        {
+            to_remove.push(var_name_with_dollar.clone());
+        }
+    }
+    for name in to_remove {
+        vars.remove(&name);
+    }
+
     // Add `$this` when inside a non-static method.  In PHP, `$this`
     // is automatically available inside closures defined in instance
     // methods (since PHP 5.4), so we check ALL enclosing scopes.

src/symbol_map/docblock.rs

@@ -353,6 +353,62 @@ pub(super) fn extract_param_var_spans(docblock: &str, base_offset: u32) -> Vec<(
     results
 }
 
+/// Scan a docblock for `@var Type $varName` tokens and return
+/// `(name_without_dollar, file_byte_offset_of_dollar)` pairs.
+///
+/// These are used by the symbol-map extraction to emit
+/// [`VarDefSite`](super::VarDefSite) entries for inline `@var`
+/// docblocks so the forward walker treats them as variable definitions.
+pub(super) fn extract_var_docblock_var_spans(
+    docblock: &str,
+    base_offset: u32,
+) -> Vec<(String, u32)> {
+    let base_span = Span::new(
+        FileId::zero(),
+        Position::new(base_offset),
+        Position::new(base_offset + docblock.len() as u32),
+    );
+    let Some(info) = parse_docblock(docblock, base_span) else {
+        return Vec::new();
+    };
+
+    let mut results = Vec::new();
+
+    for tag in &info.tags {
+        let is_var = matches!(
+            tag.kind,
+            TagKind::Var | TagKind::PhpstanVar | TagKind::PsalmVar
+        );
+        if !is_var {
+            continue;
+        }
+
+        // The description is `TypeHint $varName desc` or just `$varName`.
+        // Find the `$` in the raw source covered by description_span so
+        // the file offset is accurate.
+        let desc_file_start = tag.description_span.start.offset;
+        let desc_in_doc_start = (desc_file_start - base_offset) as usize;
+        let desc_in_doc_end =
+            ((tag.description_span.end.offset - base_offset) as usize).min(docblock.len());
+        let raw_desc = &docblock[desc_in_doc_start..desc_in_doc_end];
+
+        if let Some(dollar_pos) = raw_desc.find('$') {
+            let rest = &raw_desc[dollar_pos..];
+            let name_end = rest[1..]
+                .find(|c: char| !c.is_alphanumeric() && c != '_')
+                .map(|i| i + 1)
+                .unwrap_or(rest.len());
+            if name_end > 1 {
+                let name = rest[1..name_end].to_string();
+                let file_offset = desc_file_start + dollar_pos as u32;
+                results.push((name, file_offset));
+            }
+        }
+    }
+
+    results
+}
+
 // ─── Type span emission ─────────────────────────────────────────────────────
 
 /// Emit `SymbolSpan` entries for a type token, splitting unions and

src/symbol_map/extraction.rs

@@ -11,7 +11,7 @@ use mago_syntax::ast::*;
 
 use super::docblock::{
     class_ref_span, class_ref_span_ctx, extract_docblock_symbols, extract_param_var_spans,
-    get_docblock_text_with_offset, is_navigable_type,
+    extract_var_docblock_var_spans, get_docblock_text_with_offset, is_navigable_type,
 };
 use super::{
     CallSite, ClassRefContext, SelfStaticParentKind, SymbolKind, SymbolMap, SymbolSpan,
@@ -298,7 +298,7 @@ fn extract_from_statement<'a>(
             extract_from_use_statement(use_stmt, &mut ctx.spans);
         }
         Statement::Expression(expr_stmt) => {
-            extract_inline_docblock(expr_stmt, ctx);
+            extract_inline_docblock(expr_stmt, ctx, scope_start);
             // Detect `assert($var instanceof ...)` and record its offset
             // as a sequential narrowing boundary for the diagnostic cache.
             if is_assert_instanceof(expr_stmt.expression) {
@@ -309,14 +309,14 @@ fn extract_from_statement<'a>(
         }
         Statement::Return(ret) => {
             emit_keyword(&ret.r#return, ctx);
-            extract_inline_docblock(ret, ctx);
+            extract_inline_docblock(ret, ctx, scope_start);
             if let Some(val) = ret.value {
                 extract_from_expression(val, ctx, scope_start);
             }
         }
         Statement::Echo(echo) => {
             emit_keyword(&echo.echo, ctx);
-            extract_inline_docblock(echo, ctx);
+            extract_inline_docblock(echo, ctx, scope_start);
             for expr in echo.values.iter() {
                 extract_from_expression(expr, ctx, scope_start);
             }
@@ -535,6 +535,18 @@ fn extract_from_statement<'a>(
             emit_keyword(&unset_stmt.unset, ctx);
             for val in unset_stmt.values.iter() {
                 extract_from_expression(val, ctx, scope_start);
+                if let Expression::Variable(Variable::Direct(dv)) = val {
+                    let name = dv.name.strip_prefix('$').unwrap_or(dv.name).to_string();
+                    ctx.var_defs.push(VarDefSite {
+                        offset: dv.span.start.offset,
+                        name,
+                        kind: VarDefKind::Unset,
+                        scope_start,
+                        effective_from: unset_stmt.span().end.offset,
+                        nesting_depth: ctx.cond_nesting_depth,
+                        block_end: ctx.cond_block_end_stack.last().copied().unwrap_or(u32::MAX),
+                    });
+                }
             }
         }
         Statement::Constant(constant) => {
@@ -1346,11 +1358,30 @@ fn extract_from_method<'a>(method: &'a Method<'a>, ctx: &mut ExtractionCtx<'a>)
 /// These comments are stored as trivia preceding the statement token.
 /// Unlike class/method docblocks, inline `@var` annotations don't define
 /// template parameters — we only care about the type spans they contain.
-fn extract_inline_docblock(node: &impl HasSpan, ctx: &mut ExtractionCtx<'_>) {
+fn extract_inline_docblock(node: &impl HasSpan, ctx: &mut ExtractionCtx<'_>, scope_start: u32) {
     if let Some((doc_text, doc_offset)) =
         get_docblock_text_with_offset(ctx.trivias, ctx.content, node)
     {
         let _tpl = extract_docblock_symbols(doc_text, doc_offset, &mut ctx.spans);
+
+        // Emit VarDefSite entries for `@var Type $varName` in inline docblocks.
+        for (name, file_offset) in extract_var_docblock_var_spans(doc_text, doc_offset) {
+            let name_len = name.len() as u32 + 1; // +1 for the `$` prefix
+            ctx.spans.push(SymbolSpan {
+                start: file_offset,
+                end: file_offset + name_len,
+                kind: SymbolKind::Variable { name: name.clone() },
+            });
+            ctx.var_defs.push(VarDefSite {
+                offset: file_offset,
+                name,
+                kind: VarDefKind::DocblockVar,
+                scope_start,
+                effective_from: file_offset,
+                nesting_depth: ctx.cond_nesting_depth,
+                block_end: ctx.cond_block_end_stack.last().copied().unwrap_or(u32::MAX),
+            });
+        }
     }
 }
 

src/symbol_map/mod.rs

@@ -405,6 +405,13 @@ pub(crate) enum VarDefKind {
     /// the pre-body offset to the correct function body scope for
     /// rename and find-references.
     DocblockParam,
+    /// A `@var Type $varName` mention in an inline docblock.  Recorded
+    /// so that the forward walker picks up the type annotation as a
+    /// variable definition site.
+    DocblockVar,
+    /// An `unset($var)` call.  Recorded so that variable completion can
+    /// suppress the variable after the unset point.
+    Unset,
 }
 
 /// Per-file symbol location index.

tests/integration/completion_unset.rs

@@ -569,13 +569,9 @@ async fn test_unset_removes_variable_from_name_suggestions() {
     let items = complete_at(&backend, &uri, text, 6, 9).await;
     let labels: Vec<&str> = items.iter().map(|i| i.label.as_str()).collect();
 
-    // Note: The SymbolMap-based variable collector does not track
-    // unset() removal, so $user still appears in suggestions.
-    // This is acceptable because unset() does not remove the variable
-    // from PHP's scope — it just sets it to undefined.
     assert!(
-        labels.contains(&"$user"),
-        "$user should still be suggested (SymbolMap does not track unset), got: {:?}",
+        !labels.contains(&"$user"),
+        "$user should NOT be suggested after unset, got: {:?}",
         labels
     );
     assert!(
@@ -607,15 +603,14 @@ async fn test_unset_removes_multiple_variables_from_name_suggestions() {
     let items = complete_at(&backend, &uri, text, 7, 9).await;
     let labels: Vec<&str> = items.iter().map(|i| i.label.as_str()).collect();
 
-    // SymbolMap does not track unset() removal.
     assert!(
-        labels.contains(&"$alpha"),
-        "$alpha should still be suggested (SymbolMap does not track unset), got: {:?}",
+        !labels.contains(&"$alpha"),
+        "$alpha should NOT be suggested after unset, got: {:?}",
         labels
     );
     assert!(
-        labels.contains(&"$beta"),
-        "$beta should still be suggested (SymbolMap does not track unset), got: {:?}",
+        !labels.contains(&"$beta"),
+        "$beta should NOT be suggested after unset, got: {:?}",
         labels
     );
     assert!(
@@ -670,10 +665,9 @@ async fn test_unset_removes_variable_top_level() {
     let items = complete_at(&backend, &uri, text, 4, 1).await;
     let labels: Vec<&str> = items.iter().map(|i| i.label.as_str()).collect();
 
-    // SymbolMap does not track unset() removal.
     assert!(
-        labels.contains(&"$user"),
-        "$user should still be suggested (SymbolMap does not track unset), got: {:?}",
+        !labels.contains(&"$user"),
+        "$user should NOT be suggested after unset, got: {:?}",
         labels
     );
     assert!(

tests/integration/completion_variable_names.rs

@@ -2109,12 +2109,9 @@ async fn test_var_docblock_variable_name_before_assignment() {
         .map(|i| i.label.as_str())
         .collect();
 
-    // Note: The SymbolMap-based variable collector does not currently
-    // emit VarDefSite entries for @var docblock variable names.
-    // The assignment `$x = ...` is the only VarDefSite here.
     assert!(
-        !var_labels.contains(&"$adminUser"),
-        "$adminUser not yet supported via SymbolMap @var extraction. Got: {:?}",
+        var_labels.contains(&"$adminUser"),
+        "$adminUser should be suggested from @var docblock. Got: {:?}",
         var_labels
     );
 }
@@ -2146,11 +2143,9 @@ async fn test_var_docblock_variable_name_in_method() {
         .map(|i| i.label.as_str())
         .collect();
 
-    // Note: The SymbolMap-based variable collector does not currently
-    // emit VarDefSite entries for @var docblock variable names.
     assert!(
-        !var_labels.contains(&"$myUser"),
-        "$myUser not yet supported via SymbolMap @var extraction. Got: {:?}",
+        var_labels.contains(&"$myUser"),
+        "$myUser should be suggested from @var docblock. Got: {:?}",
         var_labels
     );
 }
@@ -3539,10 +3534,9 @@ async fn test_completion_unset_removes_variable() {
         "$keep should still be visible after unset of another var. Got: {:?}",
         var_labels
     );
-    // SymbolMap does not track unset() removal.
     assert!(
-        var_labels.contains(&"$remove"),
-        "$remove should still be suggested (SymbolMap does not track unset). Got: {:?}",
+        !var_labels.contains(&"$remove"),
+        "$remove should NOT be suggested after unset. Got: {:?}",
         var_labels
     );
 }