Eliminate recursion guards in class resolution

Author: AJenbo

docs/CHANGELOG.md

@@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- **Classes are pre-resolved for the whole workspace after startup.** Once the background index completes, every known class is resolved in dependency order even when workspace diagnostics are disabled, so the first completion, hover, or go-to-definition against any class reads a warm cache instead of resolving on demand. Edits still re-resolve only the affected classes. Contributed by @AJenbo.
 - **Continuous progress reporting.** The indexing progress bar now advances file by file with live counts (e.g. "Scanning vendor packages (3201/8544 files)") instead of jumping between a few fixed milestones. This covers single-project, monorepo, and non-Composer workspaces. Go to Implementation, Find References, and Type Hierarchy show the same live progress while they scan, including when one of them triggers the first full workspace index.
 - **Property hover now shows effective types as a `var` detail line.** Property hovers now mirror method hovers by displaying the resolved/effective property type above the PHP snippet as `**var**`, while the snippet itself shows only the native PHP property declaration. This keeps docblock-inferred, virtual, and schema-derived property types out of the generated signature block. Contributed by @calebdw.
 - **Updated the bundled mago toolchain to 1.43.0.** The parser, docblock parser, formatter, and supporting crates are refreshed to the latest upstream release. Contributed by @enwi in https://github.com/PHPantom-dev/phpantom_lsp/pull/234.
@@ -45,6 +46,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **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. Contributed by @AJenbo.
 - **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.
 - **Conditional return types recognize interpolated strings as strings.** A function with a conditional return type like `($key is string ? mixed : null)` now correctly resolves the `string` branch when called with an interpolated string argument (e.g. `config("{$prefix}.host")`). Previously the interpolated string was not recognized as a string literal, causing the return type to fall through to the else branch and resolve as `null`. Contributed by @calebdw.
 - **Renaming a constructor-promoted property parameter now cascades to `$this->prop` usages.** Renaming `private int $someField` in a constructor's parameter list previously only updated the parameter declaration itself, leaving every `$this->someField` reference elsewhere in the class stale. Rename, find references, document highlight, and linked editing now treat a promoted property parameter the same as an ordinary property declaration.

docs/todo/eager-resolution.md

@@ -254,74 +254,30 @@ inside the type resolution pipeline, not by resolution logic.
 
 ---
 
-#### Phase 4e: Eliminate recursion guards in class resolution (absorbs P21, P22)
-
-**Impact: Medium-High. Effort: Medium.**
-
-Once Phase 4d (or earlier) makes the resolved codebase metadata
-immutable after population, the re-entrant resolution that currently
-requires thread-local recursion guards cannot occur. This phase
-removes those guards and the depth caps they protect:
-
-1. **`RESOLVING` set and `MAX_RESOLVE_DEPTH` (30) in
-   `virtual_members/mod.rs`.** Thread-local set of class FQNs
-   currently being resolved. When a class is already in the set,
-   re-entrant calls return a partial result (base inheritance only,
-   no virtual members). This produces non-deterministic results:
-   whichever class in a mutual dependency (e.g. Model/Builder) is
-   resolved first gets full virtual members, the other gets degraded.
-   After eager population, all classes are resolved before any
-   consumer queries them, so re-entry cannot happen.
-
-2. **`RESOLVE_DEPTH` and `MAX_RESOLVE_TARGET_DEPTH` (60) in
-   `type_engine/resolver/`.** Thread-local depth counter for
-   `resolve_target_classes_expr_inner`. Guards against mutual
-   recursion between subject resolution, call-return resolution,
-   and variable resolution. The limit of 60 (vs typical chain
-   depth of 5-10) indicates the recursion is caused by accidental
-   re-entry into class resolution, not by the problem size. Once
-   class resolution is a cache lookup, this re-entry path vanishes.
-
-3. **LSP server eager population.** The `analyse.rs` CLI already
-   runs `populate_from_sorted` before diagnostics. The LSP server's
-   Tokio threads do not. Extend eager population to run on file
-   change in the LSP server (incrementally, not full re-population)
-   so that interactive requests also benefit from pre-resolved
-   metadata.
-
-**How the reference projects avoid this problem:**
-
-- **Mago:** topologically sorts classes (`codex/src/populator/
-  sorter.rs`) using a DFS with `visited` + `visiting` sets. Cycles
-  are broken silently when `visiting.contains(&class_like)`. Each
-  class is then populated exactly once by
-  `populate_class_like_metadata_iterative` (non-recursive, assumes
-  dependencies are done). No re-entrant resolution is possible.
-
-- **PHPStan:** member lookup on `ClassReflection` delegates to
-  `PhpClassReflectionExtension`, which calls PHP's native
-  `ReflectionClass` (already resolved by the runtime). Each class
-  has a single canonical instance via `MemoizingReflectionProvider`
-  (keyed by lowercase name), so re-entrant lookups hit the same
-  cached object. Explicit cycle guards exist only for specific
-  features: `$resolvingTypeAliasImports` for `@type-import` cycles,
-  `$inferClassConstructorPropertyTypesInProcess` for constructor
-  property inference.
-
-- **Phpactor:** three independent cycle-protection layers.
-  (1) `ClassHierarchyResolver::doResolve()` passes a `$resolved`
-  map by value; if a class name is already a key, recursion stops.
-  (2) Every reflection object carries a `$visited` array through its
-  constructor; `reflectClassLike()` throws `CycleDetected` on
-  re-entry. (3) Direct self-reference guards in `parent()` and
-  `ancestors()`.
-
-**Success criteria:**
-- `RESOLVING`, `RESOLVE_DEPTH`, `MAX_RESOLVE_DEPTH`, and
-  `MAX_RESOLVE_TARGET_DEPTH` are removed from the codebase.
-- `mark_resolving` / `unmark_resolving` functions are removed.
-- No test regressions.
-- LSP server runs eager population incrementally on file change.
+#### Phase 4e: Eliminate recursion guards in class resolution ✓
+
+Removed the thread-local `RESOLVING` set, both depth counters, and
+both depth caps (`MAX_RESOLVE_DEPTH` 30, `MAX_RESOLVE_TARGET_DEPTH`
+60). Genuine dependency cycles still exist (e.g. two Eloquent models
+whose relationship providers resolve each other, and classes loaded
+on demand mid-resolution), so the cycle-break itself cannot be
+deleted: it moved into `ResolvedCacheInner` as an in-flight set keyed
+by `(thread, FQN)` with an RAII release. Re-entry on the same class
+returns a base-inheritance partial exactly as before, but resolution
+is no longer cut off at an arbitrary nesting depth, other threads'
+concurrent resolutions are unaffected, and the state is per-`Backend`
+instead of a process-wide thread-local. Cache-less
+`resolve_class_fully` callers get a throwaway cache scoped to their
+call tree, so cycle-breaking applies uniformly (and nested provider
+work within the tree is deduplicated). The depth cap in
+`resolve_target_classes_expr_inner` was removed outright: its
+recursion follows the finite subject expression plus variable
+resolution's own keyed guards, and class resolution can no longer
+feed unbounded re-entry into it. The LSP server now runs eager
+population at the tail of the full background index even when
+workspace diagnostics are disabled; incremental re-population of
+evicted classes on file change was already in place in
+`parser/ast_update.rs`.
 
 #### Phase 4f: Remove inflated stack sizes (absorbs P25)
 

src/backend.rs

@@ -1,4 +1,5 @@
 //! `Backend` behaviour that doesn't belong in the LSP-dispatch layer
 //! (`server.rs`) or in a specific feature module.
 
+pub(crate) mod eager_population;
 pub(crate) mod file_access;

src/backend/eager_population.rs

@@ -0,0 +1,68 @@
+//! Eager resolved-class population for the LSP server.
+//!
+//! Mirrors the CLI analyse pipeline: after the workspace is fully
+//! indexed, every known class is resolved in dependency-first order so
+//! that interactive requests (completion, hover, diagnostics,
+//! go-to-definition) read pre-resolved metadata from the cache instead
+//! of recursing into class resolution.  Incremental re-population on
+//! file change is handled separately in `parser/ast_update.rs`, which
+//! re-resolves only the classes evicted by the edit.
+
+use std::sync::atomic::Ordering;
+use std::time::Duration;
+
+use crate::Backend;
+
+impl Backend {
+    /// Wait until `initialized` has finished, returning `false` when
+    /// the server shuts down first.
+    ///
+    /// Startup tasks that populate resolution caches must wait for
+    /// this: `initialized` clears those caches after the startup scan,
+    /// which would discard any population that ran earlier.
+    pub(crate) async fn wait_for_init_complete(&self) -> bool {
+        while !self.init_complete.load(Ordering::Acquire) {
+            if self.shutdown_flag.load(Ordering::Acquire) {
+                return false;
+            }
+            tokio::time::sleep(Duration::from_millis(200)).await;
+        }
+        true
+    }
+
+    /// Resolve every class in `uri_classes_index` in topological
+    /// (dependency-first) order, populating `resolved_class_cache`.
+    ///
+    /// Skips classes that are already cached, so calling this again
+    /// (e.g. from the workspace diagnostics pass after the full-index
+    /// task already populated) is cheap.
+    pub(crate) async fn eager_populate_resolved_classes(&self) {
+        let backend = self.clone_for_blocking();
+        crate::server::run_blocking_cancel_safe(move || {
+            let sorted_fqns = {
+                let uri_classes_index = backend.symbols.uri_classes_index.read();
+                crate::toposort::toposort_from_uri_classes_index(&uri_classes_index)
+            };
+            // Dedicated large-stack thread: class resolution walks
+            // parsed ASTs and can nest when the toposort misses
+            // dependencies (stubs, on-demand vendor loads).
+            std::thread::scope(|s| {
+                let backend = &backend;
+                let sorted_fqns = &sorted_fqns;
+                std::thread::Builder::new()
+                    .name("eager-populate".into())
+                    .stack_size(crate::PARSE_WORKER_STACK_SIZE)
+                    .spawn_scoped(s, move || {
+                        let class_loader = |name: &str| backend.find_or_load_class(name);
+                        crate::virtual_members::populate_from_sorted(
+                            sorted_fqns,
+                            &backend.resolved_class_cache,
+                            &class_loader,
+                        );
+                    })
+                    .expect("failed to spawn eager-populate thread");
+            });
+        })
+        .await;
+    }
+}

src/diagnostics/workspace.rs

@@ -195,11 +195,8 @@ impl Backend {
         // Wait for `initialized` to finish (it clears resolution caches
         // after the startup scan; starting before that would waste the
         // eager class population below).
-        while !self.init_complete.load(Ordering::Acquire) {
-            if self.shutdown_flag.load(Ordering::Acquire) {
-                return;
-            }
-            tokio::time::sleep(Duration::from_millis(200)).await;
+        if !self.wait_for_init_complete().await {
+            return;
         }
 
         let progress_token = self.progress_create("phpantom/workspace-diagnostics").await;
@@ -241,37 +238,12 @@ impl Backend {
         // ── Eager class population ──────────────────────────────────
         // Resolve every known class in dependency-first order so the
         // per-file collectors below hit a warm cache instead of
-        // recursing into class resolution.  Same approach as the CLI
-        // analyse pipeline.
+        // recursing into class resolution.  The full-index task already
+        // ran this; classes it populated are skipped, so this only
+        // resolves classes loaded since (or everything, if the pass was
+        // triggered another way).
         progress.set_percentage(1, "Resolving classes");
-        {
-            let backend = self.clone_for_blocking();
-            crate::server::run_blocking_cancel_safe(move || {
-                let sorted_fqns = {
-                    let uri_classes_index = backend.symbols.uri_classes_index.read();
-                    crate::toposort::toposort_from_uri_classes_index(&uri_classes_index)
-                };
-                // Dedicated large-stack thread: class resolution can
-                // nest deeply when the toposort misses dependencies.
-                std::thread::scope(|s| {
-                    let backend = &backend;
-                    let sorted_fqns = &sorted_fqns;
-                    std::thread::Builder::new()
-                        .name("ws-diag-populate".into())
-                        .stack_size(crate::PARSE_WORKER_STACK_SIZE)
-                        .spawn_scoped(s, move || {
-                            let class_loader = |name: &str| backend.find_or_load_class(name);
-                            crate::virtual_members::populate_from_sorted(
-                                sorted_fqns,
-                                &backend.resolved_class_cache,
-                                &class_loader,
-                            );
-                        })
-                        .expect("failed to spawn ws-diag-populate thread");
-                });
-            })
-            .await;
-        }
+        self.eager_populate_resolved_classes().await;
 
         // ── Per-file diagnostics, batched ───────────────────────────
         let mut uris = self.workspace_diagnostic_target_uris();

src/server.rs

@@ -1754,11 +1754,21 @@ impl Backend {
                 let _ = client.workspace_diagnostic_refresh().await;
             }
 
-            // With the whole workspace parsed, run the background
-            // workspace diagnostics pass (native collectors over every
-            // unopened user file, then project-wide external tools).
-            // Deliberately chained after the index so it never competes
-            // with startup for CPU.
+            // With the whole workspace parsed, eagerly resolve every
+            // class so interactive requests hit a warm cache.  This
+            // runs even when workspace diagnostics are disabled — it
+            // serves completion, hover, and go-to-definition too.
+            // Populating before `initialized` finishes would be wasted
+            // work: it clears the resolution caches after the startup
+            // scan.
+            if progress_backend.wait_for_init_complete().await {
+                progress_backend.eager_populate_resolved_classes().await;
+            }
+
+            // Then run the background workspace diagnostics pass
+            // (native collectors over every unopened user file, then
+            // project-wide external tools).  Deliberately chained after
+            // the index so it never competes with startup for CPU.
             progress_backend.run_workspace_diagnostics().await;
         });
     }

src/type_engine/resolver/mod.rs

@@ -162,37 +162,15 @@ pub(crate) fn resolve_target_classes_expr(
 
 /// Inner implementation of [`resolve_target_classes_expr`] without
 /// chain caching.  The outer function handles cache lookup/store.
+///
+/// Recursion here follows the finite structure of the subject
+/// expression plus variable resolution, which carries its own keyed
+/// re-entry guards; class resolution cannot re-enter it (cycles are
+/// broken inside `resolve_class_fully`), so no depth cap is needed.
 fn resolve_target_classes_expr_inner(
     expr: &SubjectExpr,
     access_kind: AccessKind,
     ctx: &ResolutionCtx<'_>,
-) -> Vec<ResolvedType> {
-    thread_local! {
-        static RESOLVE_DEPTH: std::cell::Cell<u32> = const { std::cell::Cell::new(0) };
-    }
-    let depth = RESOLVE_DEPTH.with(|d| {
-        let v = d.get() + 1;
-        d.set(v);
-        v
-    });
-    // Maximum nesting depth for `resolve_target_classes_expr_inner`.
-    // Breaks infinite recursion between subject resolution, call-return
-    // resolution, and variable resolution that can occur on files with
-    // deeply intertwined class hierarchies and virtual members.
-    const MAX_RESOLVE_TARGET_DEPTH: u32 = 60;
-    if depth > MAX_RESOLVE_TARGET_DEPTH {
-        RESOLVE_DEPTH.with(|d| d.set(depth - 1));
-        return vec![];
-    }
-    let result = resolve_target_classes_expr_inner_impl(expr, access_kind, ctx);
-    RESOLVE_DEPTH.with(|d| d.set(depth - 1));
-    result
-}
-
-fn resolve_target_classes_expr_inner_impl(
-    expr: &SubjectExpr,
-    access_kind: AccessKind,
-    ctx: &ResolutionCtx<'_>,
 ) -> Vec<ResolvedType> {
     let current_class = ctx.current_class;
     let all_classes = ctx.all_classes;

src/virtual_members/cache.rs

@@ -70,6 +70,17 @@ pub struct ResolvedCacheInner {
     /// per-edit cache contents.
     is_laravel: bool,
     schema_index: SchemaIndex,
+    /// Classes currently being resolved by `resolve_class_fully_inner`,
+    /// keyed per thread so one thread's in-progress resolution never
+    /// degrades another thread's independent resolution of the same
+    /// class.  When resolution of a class re-enters itself on the same
+    /// thread (a genuine dependency cycle, e.g. two Eloquent models
+    /// whose relationship providers resolve each other), the re-entrant
+    /// call detects its own marker here and returns a base-inheritance
+    /// partial result instead of recursing forever.  Entries live only
+    /// for the duration of one synchronous resolution call tree; they
+    /// are removed by an RAII guard, so a panic cannot leak them.
+    in_flight: HashSet<(std::thread::ThreadId, Atom)>,
 }
 
 impl Default for ResolvedCacheInner {
@@ -80,6 +91,7 @@ impl Default for ResolvedCacheInner {
             reverse_deps: HashMap::new(),
             is_laravel: true,
             schema_index: SchemaIndex::default(),
+            in_flight: HashSet::new(),
         }
     }
 }
@@ -110,6 +122,19 @@ impl ResolvedCacheInner {
         self.is_laravel = is_laravel;
     }
 
+    /// Mark `fqn` as being resolved on the current thread.  Returns
+    /// `true` when freshly marked (caller should proceed with full
+    /// resolution) and `false` on re-entry (caller should break the
+    /// cycle with a partial result).
+    pub fn mark_in_flight(&mut self, fqn: Atom) -> bool {
+        self.in_flight.insert((std::thread::current().id(), fqn))
+    }
+
+    /// Remove the current thread's in-flight marker for `fqn`.
+    pub fn unmark_in_flight(&mut self, fqn: Atom) {
+        self.in_flight.remove(&(std::thread::current().id(), fqn));
+    }
+
     pub fn schema_index(&self) -> &SchemaIndex {
         &self.schema_index
     }
@@ -153,6 +178,10 @@ impl ResolvedCacheInner {
     }
 
     /// Remove all entries and indices.
+    ///
+    /// `in_flight` is left untouched: its entries belong to resolution
+    /// call trees currently running on other threads, not to the cached
+    /// contents being invalidated.
     pub fn clear(&mut self) {
         self.map.clear();
         self.fqn_keys.clear();