Fix incorrect method resolution and crash

Author: AJenbo

src/completion/handler.rs

@@ -10,6 +10,7 @@
 /// are also housed here because they are exclusively used by the
 /// completion handler.
 use std::collections::HashMap;
+use std::panic;
 
 use tower_lsp::jsonrpc::Result;
 use tower_lsp::lsp_types::*;
@@ -363,22 +364,35 @@ impl Backend {
                 let suppress =
                     target.subject == "static" && current_class.is_some_and(|cc| cc.is_final);
 
-                let candidates = if suppress {
-                    vec![]
-                } else {
-                    Self::resolve_target_classes(
-                        &target.subject,
-                        target.access_kind,
-                        current_class,
-                        &classes,
-                        &content,
-                        cursor_offset.unwrap_or(0),
-                        &class_loader,
-                        Some(&function_loader),
-                    )
-                };
+                // Wrap resolution + inheritance merging in catch_unwind so
+                // that a stack overflow (e.g. from deep trait/inheritance
+                // resolution when the subject is a call expression like
+                // `collect($x)->`) doesn't crash the LSP server process.
+                // The variable-resolution path already has its own
+                // catch_unwind, but the direct call-expression path
+                // (resolve_call_return_types → type_hint_to_classes →
+                // class_loader → find_or_load_class → parse_php →
+                // resolve_class_with_inheritance) does not.
+                let member_items_result = panic::catch_unwind(panic::AssertUnwindSafe(|| {
+                    let candidates = if suppress {
+                        vec![]
+                    } else {
+                        Self::resolve_target_classes(
+                            &target.subject,
+                            target.access_kind,
+                            current_class,
+                            &classes,
+                            &content,
+                            cursor_offset.unwrap_or(0),
+                            &class_loader,
+                            Some(&function_loader),
+                        )
+                    };
+
+                    if candidates.is_empty() {
+                        return vec![];
+                    }
 
-                if !candidates.is_empty() {
                     // `parent::`, `self::`, and `static::` are syntactically
                     // `::` but semantically different from external static
                     // access: they show both static and instance members
@@ -449,9 +463,20 @@ impl Backend {
                             }
                         }
                     }
-                    if !all_items.is_empty() {
+                    all_items
+                }));
+
+                match member_items_result {
+                    Ok(all_items) if !all_items.is_empty() => {
                         return Ok(Some(CompletionResponse::Array(all_items)));
                     }
+                    Err(_) => {
+                        log::error!(
+                            "PHPantomLSP: panic during member-access completion for '{}'",
+                            target.subject
+                        );
+                    }
+                    _ => {}
                 }
             }
 

src/definition/member.rs

@@ -177,6 +177,19 @@ impl Backend {
 
     // ─── Member Access Context Extraction ───────────────────────────────────
 
+    /// Check whether the cursor is on the right-hand side of a member
+    /// access operator (`->`, `?->`, or `::`).
+    ///
+    /// Returns `true` when the word under the cursor is preceded by one
+    /// of these operators — meaning the word is a member name, NOT a
+    /// standalone function / class / constant.  This is used by
+    /// [`resolve_definition`](super::resolve) to prevent falling through
+    /// to standalone symbol resolution when member resolution fails
+    /// (e.g. because the owning class couldn't be determined).
+    pub(super) fn is_member_access_context(content: &str, position: Position) -> bool {
+        Self::extract_member_access_context(content, position).is_some()
+    }
+
     /// Detect the access operator (`::`, `->`, `?->`) immediately before the
     /// word under the cursor and extract the subject to its left.
     ///

src/definition/resolve.rs

@@ -62,12 +62,28 @@ impl Backend {
             return None;
         }
 
-        // ── NEW: Try member access resolution (::, ->, ?->) ──
+        // ── Member access resolution (::, ->, ?->) ──
         // If the cursor is on a member name (right side of an operator),
         // resolve the owning class and jump to the member declaration.
+        //
+        // When a member-access operator IS detected but resolution fails
+        // (e.g. the owning class couldn't be determined because a helper
+        // function like `collect()` isn't indexed), we must return early
+        // so that the member name (e.g. `map`) is NOT misinterpreted as
+        // a standalone function / class / constant.  Without this guard,
+        // `collect($x)->map(` would fall through and resolve `map` to a
+        // global `map()` helper function — or even crash while trying.
+        let is_member_access = Self::is_member_access_context(content, position);
         if let Some(location) = self.resolve_member_definition(uri, content, position, &word) {
             return Some(location);
         }
+        if is_member_access {
+            // The cursor is on the RHS of `->`, `?->`, or `::` but we
+            // couldn't resolve the owning class.  Don't fall through to
+            // standalone symbol resolution — there is no standalone
+            // symbol named `map`, `getName`, etc.
+            return None;
+        }
 
         // ── Handle `self`, `static`, `parent` keywords ──
         // When the cursor is on one of these keywords (e.g. `new self()`,

src/server.rs

@@ -238,10 +238,30 @@ impl LanguageServer for Backend {
             None
         };
 
-        if let Some(content) = content
-            && let Some(location) = self.resolve_definition(&uri, &content, position)
-        {
-            return Ok(Some(GotoDefinitionResponse::Scalar(location)));
+        if let Some(content) = content {
+            // Wrap in catch_unwind so that a stack overflow (e.g. from
+            // deep trait/inheritance resolution when the subject is a
+            // call expression like `collect($x)->map(`) doesn't crash
+            // the LSP server process.
+            let uri_owned = uri.clone();
+            let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+                self.resolve_definition(&uri_owned, &content, position)
+            }));
+
+            match result {
+                Ok(Some(location)) => {
+                    return Ok(Some(GotoDefinitionResponse::Scalar(location)));
+                }
+                Ok(None) => {}
+                Err(_) => {
+                    log::error!(
+                        "PHPantomLSP: panic during goto_definition at {}:{}:{}",
+                        uri,
+                        position.line,
+                        position.character
+                    );
+                }
+            }
         }
 
         Ok(None)

tests/completion_methods.rs

@@ -4,6 +4,143 @@ use common::create_test_backend;
 use tower_lsp::LanguageServer;
 use tower_lsp::lsp_types::*;
 
+/// Regression test: completion on `collect($var)->` resolves Collection members.
+///
+/// The `collect()` helper returns `Collection` and the cursor is right
+/// after `->` — the subject is `collect($paymentOptions)` which is a
+/// standalone function call whose return type is a class.
+#[tokio::test]
+async fn test_completion_on_function_call_arrow() {
+    let backend = create_test_backend();
+
+    let uri = Url::parse("file:///collect_map.php").unwrap();
+    let text = r#"<?php
+
+class Collection {
+    /** @return static */
+    public function map(callable $callback): static {}
+
+    /** @return static */
+    public function values(): static {}
+
+    /** @return mixed */
+    public function first(): mixed {}
+}
+
+/**
+ * @return Collection
+ */
+function collect($value = []): Collection
+{
+    return new Collection($value);
+}
+
+class PaymentOptionLocale {
+    public bool $tokens_enabled;
+}
+
+class PaymentService {
+    public function getOptions(array $paymentOptions): void {
+        $formattedOptions = collect($paymentOptions)->map(function (PaymentOptionLocale $optionLocale) {
+            return [
+                'tokens_enabled' => $optionLocale->tokens_enabled,
+            ];
+        })->values();
+        $formattedOptions->
+    }
+}
+"#;
+
+    let open_params = DidOpenTextDocumentParams {
+        text_document: TextDocumentItem {
+            uri: uri.clone(),
+            language_id: "php".to_string(),
+            version: 1,
+            text: text.to_string(),
+        },
+    };
+    backend.did_open(open_params).await;
+
+    // ── Test 1: completion after `$formattedOptions->` ──
+    let target_line = text
+        .lines()
+        .enumerate()
+        .find(|(_, l)| l.trim() == "$formattedOptions->")
+        .map(|(i, _)| i)
+        .expect("should find $formattedOptions-> line");
+
+    let completion_params = CompletionParams {
+        text_document_position: TextDocumentPositionParams {
+            text_document: TextDocumentIdentifier { uri: uri.clone() },
+            position: Position {
+                line: target_line as u32,
+                character: 28,
+            },
+        },
+        work_done_progress_params: WorkDoneProgressParams::default(),
+        partial_result_params: PartialResultParams::default(),
+        context: None,
+    };
+
+    let result = backend.completion(completion_params).await.unwrap();
+    let items = match result {
+        Some(CompletionResponse::Array(items)) => items,
+        Some(CompletionResponse::List(list)) => list.items,
+        None => vec![],
+    };
+
+    let labels: Vec<&str> = items.iter().map(|i| i.label.as_str()).collect();
+    assert!(
+        labels.iter().any(|l| l.starts_with("map")),
+        "Expected 'map' in completions for Collection, got: {:?}",
+        labels
+    );
+    assert!(
+        labels.iter().any(|l| l.starts_with("values")),
+        "Expected 'values' in completions for Collection, got: {:?}",
+        labels
+    );
+
+    // ── Test 2: completion after `collect($paymentOptions)->` ──
+    // The cursor is right after `->` before `map` on the chained call line.
+    let chain_line = text
+        .lines()
+        .enumerate()
+        .find(|(_, l)| l.contains("collect($paymentOptions)->map("))
+        .map(|(i, _)| i)
+        .expect("should find collect()->map( line");
+
+    let chain_line_text = text.lines().nth(chain_line).unwrap();
+    let arrow_col = chain_line_text.find("->map(").unwrap() as u32 + 2; // after `->`
+
+    let completion_params2 = CompletionParams {
+        text_document_position: TextDocumentPositionParams {
+            text_document: TextDocumentIdentifier { uri },
+            position: Position {
+                line: chain_line as u32,
+                character: arrow_col + 3, // after `->map`
+            },
+        },
+        work_done_progress_params: WorkDoneProgressParams::default(),
+        partial_result_params: PartialResultParams::default(),
+        context: None,
+    };
+
+    // Must not crash and should offer Collection members
+    let result2 = backend.completion(completion_params2).await.unwrap();
+    let items2 = match result2 {
+        Some(CompletionResponse::Array(items)) => items,
+        Some(CompletionResponse::List(list)) => list.items,
+        None => vec![],
+    };
+    let labels2: Vec<&str> = items2.iter().map(|i| i.label.as_str()).collect();
+    assert!(
+        labels2.iter().any(|l| l.starts_with("map")),
+        "Expected 'map' in completions on chained collect()->, got: {:?}",
+        labels2
+    );
+}
+
 // ─── Method Insert Text with Parameters ─────────────────────────────────────
 
 #[tokio::test]