Fix diagnostic cache poisoning by depth-limited variable resolution

Author: AJenbo

docs/CHANGELOG.md

@@ -61,6 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Closure parameter inference from function-level `@template` bindings.** Functions like `array_any`, `array_all`, and `array_find` that declare `@template` parameters bound through an array parameter and used in a `callable` parameter now infer concrete types for untyped closure arguments. For example, `array_any($this->items, fn($item) => $item->name !== '')` where `$this->items` is `array<int, Product>` now resolves `$item` as `Product`. Previously `$item` was unresolved, producing false-positive `unresolved_member_access` diagnostics. This also fixes a related issue where `@param` tags in phpstorm-stubs that omit the parameter name (e.g. `@param callable(TValue, TKey): bool` without `$callback`) failed to enrich the parameter's type hint, preventing callable parameter type extraction.
 - **Property chain arguments in template substitution.** Expressions like `$this->items` or `$obj->prop` passed as arguments to templated functions now resolve their type for template binding. Previously only bare variables (e.g. `$items`) were resolved, so `array_any($this->items, fn($x) => ...)` could not infer the element type even when the property had a fully substituted generic type.
 - **Variable reassignment inside `try` blocks now tracked by the analyzer.** When a variable was reassigned to a different type inside a `try` block, subsequent accesses within the same block still resolved against the original type, producing false-positive `unknown_member` diagnostics. The same applied to `catch` and `finally` blocks. Assignments preceding the cursor within the same block are now treated as sequential.
+- **Diagnostic subject cache no longer poisoned by depth-limited variable resolution.** When a variable was reassigned inside a nested `foreach` via a self-referential expression (e.g. `$total = $total->add(…)`), the recursive RHS resolution could reach the `MAX_VAR_RESOLUTION_DEPTH` safety limit and return empty. This empty result was cached in `DIAG_SUBJECT_CACHE`, causing later top-level lookups (at depth 0) to hit the poisoned cache entry and report false "type could not be resolved" diagnostics. The cache now skips storing empty results when the depth guard has fired, using a sticky thread-local flag that stays set until the outermost `resolve_variable_types` call returns.
 - **Types with `covariant` or `contravariant` variance annotations in generic args now parse correctly.** PHPStan/Larastan annotations like `BelongsTo<Category, covariant $this>` previously failed to parse, causing the entire type to become unresolvable. This broke Eloquent relationship property synthesis (virtual properties were not generated when the `@return` annotation used variance modifiers) and member lookup on relationship method return types (e.g. `$model->category()->associate()` reported `associate()` as not found). The variance keywords are now stripped from generic parameter positions before parsing.
 - **Diagnostics now work for vendor files open in the editor.** Projects using `--prefer-source` or monorepo setups where libraries are edited from the `vendor/` directory no longer have diagnostics suppressed. Previously all vendor files were unconditionally skipped, hiding syntax errors, deprecation warnings, and all other diagnostics. The `analyze` command also accepts vendor paths as explicit overrides (e.g. `phpantom_lsp analyze vendor/some-package/src/`).
 - **Full-line diagnostic suppression no longer requires a code mapping.** Full-line diagnostics (from tools like PHPStan that only report line numbers) are now suppressed whenever any precise diagnostic exists on the same line, regardless of error codes. Previously suppression relied on a hand-maintained mapping between error codes from different tools, which was incomplete and allowed redundant full-line underlines to obscure the precise location of the issue.

src/completion/resolver.rs

@@ -517,12 +517,23 @@ pub(crate) fn resolve_target_classes(
     let result = resolve_target_classes_expr(&expr, access_kind, ctx);
 
     // ── Populate the cache if active ────────────────────────────
-    DIAG_SUBJECT_CACHE.with(|cell| {
-        let mut borrow = cell.borrow_mut();
-        if let Some((map, _)) = borrow.as_mut() {
-            map.insert(cache_key, result.clone());
-        }
-    });
+    // Skip caching empty results when the variable resolution depth
+    // guard has fired.  A depth-limited empty result does not mean
+    // the variable is genuinely unresolvable — it only means the
+    // recursion was too deep *this time*.  Caching such an empty
+    // vec would poison the cache: a later top-level lookup (at
+    // depth 0) would hit the cached empty entry and produce a
+    // false "type could not be resolved" diagnostic.
+    let skip_cache = result.is_empty()
+        && super::variable::resolution::is_var_resolution_depth_limited();
+    if !skip_cache {
+        DIAG_SUBJECT_CACHE.with(|cell| {
+            let mut borrow = cell.borrow_mut();
+            if let Some((map, _)) = borrow.as_mut() {
+                map.insert(cache_key, result.clone());
+            }
+        });
+    }
 
     result
 }

src/completion/variable/resolution.rs

@@ -58,6 +58,13 @@ use crate::completion::resolver::{Loaders, VarResolutionCtx};
 // directly (diagnostics, hover, foreach resolution).
 thread_local! {
     static VAR_RESOLUTION_DEPTH: Cell<u8> = const { Cell::new(0) };
+    /// Sticky flag set when `resolve_variable_types` returns empty
+    /// because the depth guard fired.  The flag stays set until the
+    /// outermost `resolve_variable_types` call returns, so callers
+    /// up the stack (e.g. `resolve_target_classes`) can detect that
+    /// an empty result was caused by the depth limit — not because
+    /// the variable is genuinely unresolvable — and skip caching it.
+    static VAR_RESOLUTION_DEPTH_LIMITED: Cell<bool> = const { Cell::new(false) };
 }
 
 /// Maximum nesting depth for `resolve_variable_types` calls.
@@ -74,6 +81,18 @@ thread_local! {
 /// safety net for any remaining recursive patterns.
 const MAX_VAR_RESOLUTION_DEPTH: u8 = 3;
 
+/// Returns `true` when any `resolve_variable_types` call on the
+/// current stack has returned empty because the depth guard fired.
+///
+/// The flag is sticky: once set, it stays `true` until the outermost
+/// `resolve_variable_types` call returns and clears it.  This lets
+/// callers further up the stack (e.g. `resolve_target_classes`) detect
+/// that an empty result was caused by the depth limit — not because
+/// the variable is genuinely unresolvable — and skip caching it.
+pub(crate) fn is_var_resolution_depth_limited() -> bool {
+    VAR_RESOLUTION_DEPTH_LIMITED.with(|f| f.get())
+}
+
 /// Build a [`VarClassStringResolver`] closure from a [`VarResolutionCtx`].
 ///
 /// The returned closure resolves a variable name (e.g. `"$requestType"`)
@@ -174,6 +193,7 @@ pub(crate) fn resolve_variable_types(
     });
     if depth >= MAX_VAR_RESOLUTION_DEPTH {
         VAR_RESOLUTION_DEPTH.with(|d| d.set(d.get().saturating_sub(1)));
+        VAR_RESOLUTION_DEPTH_LIMITED.with(|f| f.set(true));
         return vec![];
     }
 
@@ -194,7 +214,17 @@ pub(crate) fn resolve_variable_types(
         resolve_variable_in_statements(program.statements.iter(), &ctx)
     });
 
-    VAR_RESOLUTION_DEPTH.with(|d| d.set(d.get().saturating_sub(1)));
+    let new_depth = VAR_RESOLUTION_DEPTH.with(|d| {
+        let v = d.get().saturating_sub(1);
+        d.set(v);
+        v
+    });
+    // Clear the sticky depth-limited flag when the outermost call
+    // returns.  Inner calls leave it set so that every caller in
+    // the stack can observe it.
+    if new_depth == 0 {
+        VAR_RESOLUTION_DEPTH_LIMITED.with(|f| f.set(false));
+    }
     result
 }
 
@@ -222,6 +252,7 @@ pub(crate) fn resolve_variable_types_branch_aware(
     });
     if depth >= MAX_VAR_RESOLUTION_DEPTH {
         VAR_RESOLUTION_DEPTH.with(|d| d.set(d.get().saturating_sub(1)));
+        VAR_RESOLUTION_DEPTH_LIMITED.with(|f| f.set(true));
         return vec![];
     }
 
@@ -246,7 +277,14 @@ pub(crate) fn resolve_variable_types_branch_aware(
         },
     );
 
-    VAR_RESOLUTION_DEPTH.with(|d| d.set(d.get().saturating_sub(1)));
+    let new_depth = VAR_RESOLUTION_DEPTH.with(|d| {
+        let v = d.get().saturating_sub(1);
+        d.set(v);
+        v
+    });
+    if new_depth == 0 {
+        VAR_RESOLUTION_DEPTH_LIMITED.with(|f| f.set(false));
+    }
     result
 }
 

src/diagnostics/unknown_members.rs

@@ -5098,4 +5098,161 @@ final class PurchaseFileProductCollection extends Collection {
             "expected no diagnostics for $this->items on generic Collection subclass, got: {diags:?}"
         );
     }
+
+    #[test]
+    fn no_false_positive_when_variable_reassigned_inside_try_inside_foreach() {
+        // B3 follow-up: when a variable is assigned before a foreach,
+        // then reassigned inside a try block nested inside the foreach
+        // body, the type should still resolve for accesses after the
+        // reassignment (still inside the try).
+        //
+        // Real-world pattern from OrderService:137:
+        //   $remaining = $order->amount;          // Decimal via @property
+        //   foreach ($payments as $payment) {
+        //       try {
+        //           $remaining = $remaining->sub($toCapture);  // ← should resolve
+        //       } catch (...) {}
+        //   }
+        let php = r#"<?php
+class Decimal {
+    public function sub(string $v): self { return new self(); }
+    public function isZero(): bool { return true; }
+    public function isNegative(): bool { return true; }
+    public function isPositive(): bool { return true; }
+    public function toFixed(int $places): string { return ''; }
+}
+
+/**
+ * @property Decimal $amount
+ * @property string $state
+ */
+class Payment {
+}
+
+/**
+ * @property Decimal $amount
+ */
+class Order {
+}
+
+class CaptureException extends \Exception {}
+class InvalidStateException extends \Exception {}
+class CaptureService {
+    public function captureReservedPayment(Payment $p, Decimal $amount): void {}
+}
+
+class OrderService {
+    /** @param list<Payment> $payments */
+    public function capture(Order $order, array $payments): void {
+        $remaining = $order->amount;
+        foreach ($payments as $payment) {
+            if ($payment->state === 'paid') {
+                $remaining = $remaining->sub('1');
+            }
+        }
+
+        $svc = new CaptureService();
+        foreach ($payments as $payment) {
+            if ($payment->state !== 'reserved') {
+                continue;
+            }
+
+            $toCapture = $remaining->isPositive() ? $payment->amount : $remaining;
+            if ($toCapture->isZero() || $toCapture->isNegative()) {
+                break;
+            }
+
+            try {
+                $svc->captureReservedPayment($payment, $toCapture);
+                $remaining = $remaining->sub('1');
+            } catch (CaptureException|InvalidStateException $e) {
+            }
+        }
+
+        if ($remaining->isPositive() && !$remaining->isZero()) {
+            throw new \RuntimeException('remaining: ' . $remaining->toFixed(2));
+        }
+    }
+}
+"#;
+        let backend = Backend::new_test();
+        backend.config.lock().diagnostics.unresolved_member_access = Some(true);
+        let diags = collect(&backend, "file:///test.php", php);
+        assert!(
+            diags.is_empty(),
+            "expected no diagnostics for variable reassigned inside try-inside-foreach, got: {diags:?}"
+        );
+    }
+
+    #[test]
+    fn no_false_positive_when_variable_reassigned_inside_nested_foreach() {
+        // Regression test for cache poisoning by depth-limited variable
+        // resolution.  When `$orderCostPrice` is reassigned inside a
+        // nested foreach via `$orderCostPrice = $orderCostPrice->add(…)`,
+        // the self-referential RHS triggers recursive calls to
+        // resolve_variable_types.  With two levels of foreach nesting
+        // the recursion reaches MAX_VAR_RESOLUTION_DEPTH, producing an
+        // empty result.  If that empty result is cached in
+        // DIAG_SUBJECT_CACHE, the later top-level resolution (at
+        // depth 0) for the *outer* foreach access hits the poisoned
+        // cache entry and reports "type could not be resolved".
+        //
+        // Real-world pattern from OrderService:618:
+        //   $zero = new Decimal('0');
+        //   $orderCostPrice = $zero;
+        //   foreach ($order->getOrderProducts() as $line) {
+        //       if ($product->isBundle()) {
+        //           foreach ($bundleProducts as $bp) {
+        //               $productCostPrice = $bp->supplier_price_dkk ?? $zero;
+        //               $orderCostPrice = $orderCostPrice->add($productCostPrice->mul($qty));
+        //           }
+        //           continue;
+        //       }
+        //       $productCostPrice = $product->supplier_price_dkk ?? $zero;
+        //       $orderCostPrice = $orderCostPrice->add($productCostPrice->mul($qty));
+        //   }
+        //   return $orderCostPrice->mul($rate);
+        let php = r#"<?php
+class Decimal {
+    public function add(string $v): self { return new self(); }
+    public function mul(string $v): self { return new self(); }
+}
+
+class Item {
+    public Decimal $cost;
+    public function isBundle(): bool { return false; }
+    /** @return list<Item> */
+    public function getChildren(): array { return []; }
+}
+
+class OrderService {
+    /** @param list<Item> $items */
+    public function calculateCost(array $items): Decimal {
+        $zero = new Decimal();
+        $result = $zero;
+        foreach ($items as $item) {
+            if ($item->isBundle()) {
+                $children = $item->getChildren();
+                foreach ($children as $child) {
+                    $result = $result->add($child->cost->mul('1'));
+                }
+
+                continue;
+            }
+
+            $result = $result->add($item->cost->mul('1'));
+        }
+
+        return $result->mul('1');
+    }
+}
+"#;
+        let backend = Backend::new_test();
+        backend.config.lock().diagnostics.unresolved_member_access = Some(true);
+        let diags = collect(&backend, "file:///test.php", php);
+        assert!(
+            diags.is_empty(),
+            "expected no diagnostics for variable reassigned inside nested foreach loops, got: {diags:?}"
+        );
+    }
 }