Fix variable reassignment loses type when parameter name is reused

Author: AJenbo

docs/CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **Variable type after reassignment.** When a method parameter is reassigned mid-body (e.g. `$file = $result->getFile()`), subsequent member accesses now resolve against the new type instead of the original parameter type. Previously the diagnostic subject cache reused the first resolution for the entire method scope, producing false-positive "not found" warnings on members that exist only on the reassigned type.
 - **Null-safe method chain resolution.** Null-safe method calls (`$obj?->method()`) now resolve the return type correctly for variable type inference, including cross-file chains. Previously `?->` calls were ignored by the RHS resolution pipeline, losing the type for any variable assigned from a null-safe chain.
 - **Nullable and generic types in class lookup.** Variables typed as `?ClassName` or `Collection<Item>` now resolve correctly across all code paths. Previously the `?` prefix and generic parameters were not stripped before class lookup, causing the type engine to treat them as unknown types. This fixes completion, hover, go-to-definition, and false-positive diagnostics for any variable whose type uses the nullable shorthand or carries generic arguments.
 - **`@see` references with qualified class names.** Docblock `@see Fully\Qualified\ClassName` references no longer have the file's namespace prepended, which previously produced doubled names like `App\Models\App\Models\User`. Qualified names in `@see` tags are now treated as fully-qualified.

docs/todo.md

@@ -100,8 +100,6 @@ unlikely to move the needle for most users.
 
 | #   | Item                                                                                                                                                         | Impact      | Effort      |
 | --- | ------------------------------------------------------------------------------------------------------------------------------------------------------------ | ----------- | ----------- |
-|     | **[Bugs](todo/bugs.md)**                                                                                                                                     |             |             |
-| B4  | [Variable reassignment loses type when parameter name is reused](todo/bugs.md#b4-variable-reassignment-loses-type-when-parameter-name-is-reused)             | Medium      | Medium      |
 |     | **[Completion](todo/completion.md)**                                                                                                                         |             |             |
 | C1  | Array functions needing new code paths                                                                                                                       | Medium     | High        |
 | C10 | [Lazy documentation via `completionItem/resolve`](todo/completion.md#c10-lazy-documentation-via-completionitemresolve)                                        | Medium     | Medium      |

docs/todo/bugs.md

@@ -15,24 +15,4 @@ within the same impact tier.
 
 ---
 
-#### B4. Variable reassignment loses type when parameter name is reused
-
-| | |
-|---|---|
-| **Impact** | Medium |
-| **Effort** | Medium |
-
-When a method parameter is reassigned mid-body, PHPantom sometimes
-continues to use the parameter's original type instead of the new
-assignment's type.
-
-**Observed:** In `FileUploadService::uploadFile()`, the `$file`
-parameter is typed `UploadedFile`. Later, `$file = $result->getFile()`
-reassigns it to a different type. PHPantom still resolves `$file->id`
-and `$file->name` against `UploadedFile` instead of the model returned
-by `getFile()`. This produces 2 false-positive "not found" diagnostics.
-
-**Fix:** The variable resolution pipeline should prefer the most recent
-assignment when multiple definitions exist for the same variable name
-within the same scope at the cursor offset.
-
+No outstanding items.

src/analyse.rs

@@ -233,6 +233,7 @@ pub async fn run(options: AnalyseOptions) -> i32 {
                         if let Some(sm) = backend.symbol_maps.read().get(uri.as_str()) {
                             crate::completion::resolver::set_diagnostic_subject_cache_scopes(
                                 sm.scopes.clone(),
+                                sm.var_defs.clone(),
                             );
                         }
 

src/completion/resolver.rs

@@ -155,21 +155,32 @@ impl<'a> VarResolutionCtx<'a> {
 /// (unknown_member, argument_count) share results instead of
 /// re-resolving the same subjects independently.
 ///
-/// The cache key is `(subject_text, access_kind, scope_start)` where
-/// `scope_start` is the byte offset of the innermost enclosing
-/// function/method/closure body.  This ensures that two methods in
-/// the same class that both use `$order->` get independent cache
-/// entries when `$order` has a different type in each method.
+/// The cache key is `(subject_text, access_kind, scope_start,
+/// var_def_offset)` where `scope_start` is the byte offset of the
+/// innermost enclosing function/method/closure body and
+/// `var_def_offset` is the `effective_from` of the active variable
+/// definition (or `0` for non-variable subjects).  This ensures that
+/// two methods in the same class that both use `$order->` get
+/// independent cache entries, and that accesses before vs. after a
+/// variable reassignment within the same method also get independent
+/// entries.
 ///
-/// Scope boundaries are stored alongside the cache and set by
-/// [`set_diagnostic_subject_cache_scopes`].
-type DiagSubjectCache = HashMap<(String, AccessKind, u32), Vec<Arc<ClassInfo>>>;
+/// Scope boundaries and variable definitions are stored alongside the
+/// cache and set by [`set_diagnostic_subject_cache_scopes`].
+type DiagSubjectCache = HashMap<(String, AccessKind, u32, u32), Vec<Arc<ClassInfo>>>;
+
+/// File-level data stored alongside the diagnostic subject cache so
+/// that [`resolve_target_classes`] can compute the enclosing scope and
+/// active variable definition from the `cursor_offset` without needing
+/// a reference to the [`SymbolMap`].
+struct DiagSubjectCacheFileData {
+    /// Scope boundaries `(start_offset, end_offset)`.
+    scopes: Vec<(u32, u32)>,
+    /// Variable definition sites, cloned from the [`SymbolMap`].
+    var_defs: Vec<crate::symbol_map::VarDefSite>,
+}
 
-/// Scope boundaries `(start_offset, end_offset)` for the current file,
-/// stored alongside the diagnostic subject cache so that
-/// [`resolve_target_classes`] can compute the enclosing scope from the
-/// `cursor_offset` without needing a reference to the [`SymbolMap`].
-type DiagSubjectCacheState = (DiagSubjectCache, Vec<(u32, u32)>);
+type DiagSubjectCacheState = (DiagSubjectCache, DiagSubjectCacheFileData);
 
 thread_local! {
     static DIAG_SUBJECT_CACHE: RefCell<Option<DiagSubjectCacheState>> = const { RefCell::new(None) };
@@ -205,24 +216,40 @@ pub(crate) fn with_diagnostic_subject_cache() -> DiagSubjectCacheGuard {
         return DiagSubjectCacheGuard { owns_cache: false };
     }
     DIAG_SUBJECT_CACHE.with(|cell| {
-        *cell.borrow_mut() = Some((HashMap::new(), Vec::new()));
+        *cell.borrow_mut() = Some((
+            HashMap::new(),
+            DiagSubjectCacheFileData {
+                scopes: Vec::new(),
+                var_defs: Vec::new(),
+            },
+        ));
     });
     DiagSubjectCacheGuard { owns_cache: true }
 }
 
-/// Provide scope boundaries for the active diagnostic subject cache.
+/// Provide scope boundaries and variable definitions for the active
+/// diagnostic subject cache.
 ///
 /// Must be called while a [`DiagSubjectCacheGuard`] is alive.  The
 /// scopes are `(start_offset, end_offset)` pairs for every
 /// function, method, closure, and arrow function body in the file.
 /// They are used to compute the enclosing scope for each
 /// `cursor_offset`, ensuring that same-named variables in different
 /// methods resolve independently.
-pub(crate) fn set_diagnostic_subject_cache_scopes(scopes: Vec<(u32, u32)>) {
+///
+/// The `var_defs` are cloned from the [`SymbolMap`] and used to
+/// compute the active variable definition at each cursor offset,
+/// ensuring that accesses before and after a variable reassignment
+/// within the same method get independent cache entries.
+pub(crate) fn set_diagnostic_subject_cache_scopes(
+    scopes: Vec<(u32, u32)>,
+    var_defs: Vec<crate::symbol_map::VarDefSite>,
+) {
     DIAG_SUBJECT_CACHE.with(|cell| {
         let mut borrow = cell.borrow_mut();
-        if let Some((_map, stored_scopes)) = borrow.as_mut() {
-            *stored_scopes = scopes;
+        if let Some((_map, file_data)) = borrow.as_mut() {
+            file_data.scopes = scopes;
+            file_data.var_defs = var_defs;
         }
     });
 }
@@ -236,9 +263,9 @@ fn diag_cache_enclosing_scope(cursor_offset: u32) -> u32 {
     DIAG_SUBJECT_CACHE.with(|cell| {
         let borrow = cell.borrow();
         match borrow.as_ref() {
-            Some((_map, scopes)) => {
+            Some((_map, file_data)) => {
                 let mut best: u32 = 0;
-                for &(start, end) in scopes {
+                for &(start, end) in &file_data.scopes {
                     if start <= cursor_offset && cursor_offset <= end && start > best {
                         best = start;
                     }
@@ -250,6 +277,58 @@ fn diag_cache_enclosing_scope(cursor_offset: u32) -> u32 {
     })
 }
 
+/// Compute the `var_def_offset` discriminator for a subject at a given
+/// cursor offset.
+///
+/// For variable-based subjects (starting with `$`, excluding `$this`),
+/// returns the `effective_from` offset of the most recent variable
+/// definition visible at `cursor_offset`.  For non-variable subjects,
+/// returns `0`.
+///
+/// This ensures that the diagnostic subject cache distinguishes
+/// accesses to the same variable before and after a reassignment.
+fn diag_cache_var_def_offset(subject: &str, cursor_offset: u32) -> u32 {
+    if !subject.starts_with('$') || subject.starts_with("$this") {
+        return 0;
+    }
+    // Extract the bare variable name without '$' (e.g. "file" from
+    // "$file" or "$file->foo()").
+    let after_dollar = &subject[1..];
+    let var_name = after_dollar
+        .find("->")
+        .map(|i| &after_dollar[..i])
+        .unwrap_or(after_dollar);
+
+    DIAG_SUBJECT_CACHE.with(|cell| {
+        let borrow = cell.borrow();
+        match borrow.as_ref() {
+            Some((_map, file_data)) => {
+                let scope_start = {
+                    let mut best: u32 = 0;
+                    for &(start, end) in &file_data.scopes {
+                        if start <= cursor_offset && cursor_offset <= end && start > best {
+                            best = start;
+                        }
+                    }
+                    best
+                };
+                file_data
+                    .var_defs
+                    .iter()
+                    .rev()
+                    .find(|d| {
+                        d.name == var_name
+                            && d.scope_start == scope_start
+                            && d.effective_from <= cursor_offset
+                    })
+                    .map(|d| d.effective_from)
+                    .unwrap_or(0)
+            }
+            None => 0,
+        }
+    })
+}
+
 /// Resolve a completion subject to all candidate class types.
 ///
 /// When a variable is assigned different types in conditional branches
@@ -271,7 +350,13 @@ pub(crate) fn resolve_target_classes(
 ) -> Vec<Arc<ClassInfo>> {
     // ── Fast path: check the thread-local diagnostic cache ──────
     let scope_start = diag_cache_enclosing_scope(ctx.cursor_offset);
-    let cache_key = (subject.to_string(), access_kind, scope_start);
+    let var_def_offset = diag_cache_var_def_offset(subject, ctx.cursor_offset);
+    let cache_key = (
+        subject.to_string(),
+        access_kind,
+        scope_start,
+        var_def_offset,
+    );
     let cached = DIAG_SUBJECT_CACHE.with(|cell| {
         let borrow = cell.borrow();
         borrow

src/diagnostics/mod.rs

@@ -575,7 +575,10 @@ impl Backend {
             // can distinguish variables in different methods of the same
             // class (prevents cross-method cache pollution).
             if let Some(sm) = self.symbol_maps.read().get(uri_str) {
-                crate::completion::resolver::set_diagnostic_subject_cache_scopes(sm.scopes.clone());
+                crate::completion::resolver::set_diagnostic_subject_cache_scopes(
+                    sm.scopes.clone(),
+                    sm.var_defs.clone(),
+                );
             }
 
             self.collect_slow_diagnostics(uri_str, content, &mut slow_diagnostics);

src/diagnostics/unknown_members.rs

@@ -108,11 +108,21 @@ enum ScopeKey {
 }
 
 /// Cache key combining the subject text, access kind, and scope.
+///
+/// For variable-based subjects (starting with `$`, excluding `$this`),
+/// `var_def_offset` distinguishes accesses that fall under different
+/// definitions of the same variable.  Without this, the cache would
+/// return the parameter type for accesses after a reassignment.
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 struct SubjectCacheKey {
     subject_text: String,
     access_kind: AccessKind,
     scope: ScopeKey,
+    /// The `effective_from` offset of the active variable definition at
+    /// the point of access, or `0` for non-variable subjects.  This
+    /// ensures that accesses before and after a reassignment get
+    /// separate cache entries.
+    var_def_offset: u32,
 }
 
 /// The outcome of resolving a subject for diagnostic purposes.
@@ -292,7 +302,10 @@ impl Backend {
         // distinguish variables in different methods of the same class.
         // In production this is already set by the outer caller; for
         // standalone calls (tests, benchmarks) set it here.
-        crate::completion::resolver::set_diagnostic_subject_cache_scopes(symbol_map.scopes.clone());
+        crate::completion::resolver::set_diagnostic_subject_cache_scopes(
+            symbol_map.scopes.clone(),
+            symbol_map.var_defs.clone(),
+        );
 
         // ── Subject resolution cache for this diagnostic pass ───────────
         let mut subject_cache: SubjectCache = HashMap::new();
@@ -352,10 +365,30 @@ impl Backend {
             let fn_scope_start = symbol_map.find_enclosing_scope(span.start);
 
             // ── Look up or populate the subject cache ───────────────────
+            // For variable subjects (excluding $this), compute the
+            // active definition offset so that accesses before and
+            // after a reassignment get separate cache entries (B4).
+            let var_def_offset =
+                if subject_text.starts_with('$') && !subject_text.starts_with("$this") {
+                    // Extract the bare variable name (e.g. "$file" from
+                    // "$file" or from a chain like "$file->foo()").
+                    let var_name = subject_text
+                        .find("->")
+                        .map(|i| &subject_text[..i])
+                        .unwrap_or(subject_text);
+                    symbol_map.active_var_def_offset(
+                        &var_name[1..], // strip leading '$'
+                        span.start,
+                    )
+                } else {
+                    0
+                };
+
             let cache_key = SubjectCacheKey {
                 subject_text: subject_text.clone(),
                 access_kind,
                 scope: scope_key_for(current_class, fn_scope_start),
+                var_def_offset,
             };
 
             let outcome = subject_cache
@@ -3511,4 +3544,83 @@ class Service {
             "expected exactly one 'foo' diagnostic (in method b), got: {foo_diags:?}"
         );
     }
+
+    #[test]
+    fn no_false_positive_when_parameter_is_reassigned() {
+        // Regression test for B4: when a method parameter is reassigned
+        // mid-body, PHPantom should resolve subsequent accesses against
+        // the new type, not the original parameter type.
+        //
+        // Before the fix, the subject cache keyed by (subject_text,
+        // access_kind, scope) would cache the parameter type on the
+        // first `$file->` encounter and reuse it for accesses after
+        // the reassignment, producing false-positive "unknown member"
+        // diagnostics.
+        let php = r#"<?php
+class UploadedFile {
+    public string $originalName;
+}
+class FileModel {
+    public int $id;
+    public string $name;
+}
+class Result {
+    public function getFile(): FileModel { return new FileModel(); }
+}
+class FileUploadService {
+    public function uploadFile(UploadedFile $file): void {
+        $file->originalName;
+        $result = new Result();
+        $file = $result->getFile();
+        $file->id;
+        $file->name;
+    }
+}
+"#;
+        let backend = Backend::new_test();
+        let diags = collect(&backend, "file:///test.php", php);
+        assert!(
+            diags.is_empty(),
+            "expected no false positives when parameter is reassigned mid-body, got: {diags:?}"
+        );
+    }
+
+    #[test]
+    fn flags_unknown_member_after_reassignment() {
+        // The flip side of B4: after reassignment, members from the
+        // NEW type that don't exist should still be flagged.
+        let php = r#"<?php
+class TypeA {
+    public function onlyOnA(): void {}
+}
+class TypeB {
+    public function onlyOnB(): void {}
+}
+class Service {
+    public function process(TypeA $var): void {
+        $var->onlyOnA();
+        $var = new TypeB();
+        $var->onlyOnA();
+    }
+}
+"#;
+        let backend = Backend::new_test();
+        let diags = collect(&backend, "file:///test.php", php);
+        assert!(
+            diags
+                .iter()
+                .any(|d| d.message.contains("onlyOnA") && d.message.contains("TypeB")),
+            "expected diagnostic for onlyOnA() on TypeB after reassignment, got: {diags:?}"
+        );
+        // Exactly one diagnostic — the post-reassignment access.
+        let relevant: Vec<_> = diags
+            .iter()
+            .filter(|d| d.message.contains("onlyOnA"))
+            .collect();
+        assert_eq!(
+            relevant.len(),
+            1,
+            "expected exactly one 'onlyOnA' diagnostic (after reassignment), got: {relevant:?}"
+        );
+    }
 }