Fix property vs method resolution

Author: AJenbo

src/completion/resolver.rs

@@ -1243,6 +1243,7 @@ impl Backend {
 ///     a null default).
 ///   - `is SomeType`: not statically resolvable from AST; falls through
 ///     to the else branch.
+///
 /// Split a call-expression subject into the call body and any textual
 /// arguments.  Handles both `"app()"` → `("app", "")` and
 /// `"app(A::class)"` → `("app", "A::class")`.

src/definition/resolve.rs

@@ -32,6 +32,19 @@ enum MemberKind {
     Constant,
 }
 
+/// Hint about whether the member access looks like a method call or a property
+/// access.  Used to disambiguate when a class has both a method and a property
+/// with the same name (e.g. `id()` method vs `$id` property).
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum MemberAccessHint {
+    /// Followed by `(` — looks like a method call.
+    MethodCall,
+    /// No `(` after the name — looks like a property / constant access.
+    PropertyAccess,
+    /// Cannot determine (fallback to original order).
+    Unknown,
+}
+
 impl Backend {
     /// Handle a "go to definition" request.
     ///
@@ -337,6 +350,9 @@ impl Backend {
             return None;
         }
 
+        // Determine whether this looks like a method call or property access.
+        let access_hint = Self::detect_member_access_hint(content, position, member_name);
+
         // 4. Try each candidate class and pick the first one where the
         //    member actually exists (directly or via inheritance).
         for target_class in &candidates {
@@ -345,10 +361,11 @@ impl Backend {
                     .unwrap_or_else(|| target_class.clone());
 
             // Check that the member is actually present on the declaring class.
-            let member_kind = match Self::classify_member(&declaring_class, member_name) {
-                Some(k) => k,
-                None => continue, // member not on this candidate, try next
-            };
+            let member_kind =
+                match Self::classify_member(&declaring_class, member_name, access_hint) {
+                    Some(k) => k,
+                    None => continue, // member not on this candidate, try next
+                };
 
             // Locate the file that contains the declaring class.
             if let Some((class_uri, class_content)) =
@@ -374,7 +391,7 @@ impl Backend {
         let declaring_class = Self::find_declaring_class(target_class, member_name, &class_loader)
             .unwrap_or_else(|| target_class.clone());
 
-        let member_kind = Self::classify_member(&declaring_class, member_name)?;
+        let member_kind = Self::classify_member(&declaring_class, member_name, access_hint)?;
 
         let (class_uri, class_content) =
             self.find_class_file_content(&declaring_class.name, uri, content)?;
@@ -653,17 +670,103 @@ impl Backend {
     /// checking the class's parsed information.
     ///
     /// Returns `None` if the member is not found in the class.
-    fn classify_member(class: &ClassInfo, member_name: &str) -> Option<MemberKind> {
-        if class.methods.iter().any(|m| m.name == member_name) {
-            return Some(MemberKind::Method);
+    fn classify_member(
+        class: &ClassInfo,
+        member_name: &str,
+        hint: MemberAccessHint,
+    ) -> Option<MemberKind> {
+        let has_method = class.methods.iter().any(|m| m.name == member_name);
+        let has_property = class.properties.iter().any(|p| p.name == member_name);
+        let has_constant = class.constants.iter().any(|c| c.name == member_name);
+
+        match hint {
+            MemberAccessHint::PropertyAccess => {
+                // Prefer property/constant over method when there's no `()`.
+                if has_property {
+                    return Some(MemberKind::Property);
+                }
+                if has_constant {
+                    return Some(MemberKind::Constant);
+                }
+                if has_method {
+                    return Some(MemberKind::Method);
+                }
+            }
+            MemberAccessHint::MethodCall => {
+                // Prefer method when followed by `()`.
+                if has_method {
+                    return Some(MemberKind::Method);
+                }
+                if has_property {
+                    return Some(MemberKind::Property);
+                }
+                if has_constant {
+                    return Some(MemberKind::Constant);
+                }
+            }
+            MemberAccessHint::Unknown => {
+                // Default order: method, property, constant.
+                if has_method {
+                    return Some(MemberKind::Method);
+                }
+                if has_property {
+                    return Some(MemberKind::Property);
+                }
+                if has_constant {
+                    return Some(MemberKind::Constant);
+                }
+            }
+        }
+        None
+    }
+
+    /// Determine whether the member name at the given position is followed by
+    /// `(` (indicating a method call) or not (indicating property / constant
+    /// access).
+    fn detect_member_access_hint(
+        content: &str,
+        position: Position,
+        member_name: &str,
+    ) -> MemberAccessHint {
+        let lines: Vec<&str> = content.lines().collect();
+        let line = match lines.get(position.line as usize) {
+            Some(l) => *l,
+            None => return MemberAccessHint::Unknown,
+        };
+        let chars: Vec<char> = line.chars().collect();
+        let col = (position.character as usize).min(chars.len());
+
+        // Find the end of the member name by walking right from the cursor.
+        let is_word_char = |c: char| c.is_alphanumeric() || c == '_';
+
+        let mut end = col;
+        // If cursor is on a word char, walk right to end of word.
+        if end < chars.len() && is_word_char(chars[end]) {
+            while end < chars.len() && is_word_char(chars[end]) {
+                end += 1;
+            }
+        } else if end > 0 && is_word_char(chars[end - 1]) {
+            // Cursor is just past the word; `end` is already correct.
+        } else {
+            // Try to find the member name by searching forward from col.
+            if let Some(idx) = line[col..].find(member_name) {
+                end = col + idx + member_name.len();
+            } else {
+                return MemberAccessHint::Unknown;
+            }
         }
-        if class.properties.iter().any(|p| p.name == member_name) {
-            return Some(MemberKind::Property);
+
+        // Skip whitespace after the word.
+        let mut i = end;
+        while i < chars.len() && chars[i].is_whitespace() {
+            i += 1;
         }
-        if class.constants.iter().any(|c| c.name == member_name) {
-            return Some(MemberKind::Constant);
+
+        if i < chars.len() && chars[i] == '(' {
+            MemberAccessHint::MethodCall
+        } else {
+            MemberAccessHint::PropertyAccess
         }
-        None
     }
 
     /// Walk up the inheritance chain to find the class that actually declares
@@ -679,7 +782,7 @@ impl Backend {
         const MAX_DEPTH: usize = 20;
 
         // Check if this class directly declares the member.
-        if Self::classify_member(class, member_name).is_some() {
+        if Self::classify_member(class, member_name, MemberAccessHint::Unknown).is_some() {
             return Some(class.clone());
         }
 
@@ -695,7 +798,7 @@ impl Backend {
         for _ in 0..MAX_DEPTH {
             let parent_name = current.parent_class.as_ref()?;
             let parent = class_loader(parent_name)?;
-            if Self::classify_member(&parent, member_name).is_some() {
+            if Self::classify_member(&parent, member_name, MemberAccessHint::Unknown).is_some() {
                 return Some(parent);
             }
             // Check traits used by the parent class.
@@ -731,7 +834,8 @@ impl Backend {
             } else {
                 continue;
             };
-            if Self::classify_member(&trait_info, member_name).is_some() {
+            if Self::classify_member(&trait_info, member_name, MemberAccessHint::Unknown).is_some()
+            {
                 return Some(trait_info);
             }
             // Recurse into traits used by this trait.

tests/definition.rs

@@ -4195,3 +4195,184 @@ async fn test_goto_definition_union_return_type_cross_file() {
         other => panic!("Expected Scalar location, got: {:?}", other),
     }
 }
+
+// ─── Property vs Method Disambiguation ──────────────────────────────────────
+
+/// When a class has both a property `$id` and a method `id()`, goto-definition
+/// on `$user->id` (no parentheses) should navigate to the *property*, not the
+/// method.
+#[tokio::test]
+async fn test_goto_definition_property_preferred_over_method_without_parens() {
+    let backend = create_test_backend();
+
+    let uri = Url::parse("file:///test.php").unwrap();
+    //                                           line
+    let text = concat!(
+        "<?php\n",                              // 0
+        "class User {\n",                       // 1
+        "    public int $id;\n",                // 2  ← property declaration
+        "    public string $name;\n",           // 3
+        "\n",                                   // 4
+        "    public function id(): int {\n",    // 5  ← method declaration
+        "        return $this->id;\n",          // 6
+        "    }\n",                              // 7
+        "\n",                                   // 8
+        "    public function test(): void {\n", // 9
+        "        $user = new User();\n",        // 10
+        "        $val = $user->id;\n",          // 11 ← property access (no parens)
+        "        $val2 = $user->id();\n",       // 12 ← method call (with parens)
+        "    }\n",                              // 13
+        "}\n",                                  // 14
+    );
+
+    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;
+
+    // ── Case 1: `$user->id` (no parens) → should go to the $id property on line 2
+    let params = GotoDefinitionParams {
+        text_document_position_params: TextDocumentPositionParams {
+            text_document: TextDocumentIdentifier { uri: uri.clone() },
+            position: Position {
+                line: 11,
+                character: 24, // on "id" in `$user->id`
+            },
+        },
+        work_done_progress_params: WorkDoneProgressParams::default(),
+        partial_result_params: PartialResultParams::default(),
+    };
+
+    let result = backend.goto_definition(params).await.unwrap();
+    assert!(
+        result.is_some(),
+        "Should resolve $user->id to the property declaration"
+    );
+
+    match result.unwrap() {
+        GotoDefinitionResponse::Scalar(location) => {
+            assert_eq!(location.uri, uri);
+            assert_eq!(
+                location.range.start.line, 2,
+                "$user->id (no parens) should go to the $id property on line 2, not the id() method on line 5"
+            );
+        }
+        other => panic!("Expected Scalar location, got: {:?}", other),
+    }
+
+    // ── Case 2: `$user->id()` (with parens) → should go to the id() method on line 5
+    let params = GotoDefinitionParams {
+        text_document_position_params: TextDocumentPositionParams {
+            text_document: TextDocumentIdentifier { uri: uri.clone() },
+            position: Position {
+                line: 12,
+                character: 25, // on "id" in `$user->id()`
+            },
+        },
+        work_done_progress_params: WorkDoneProgressParams::default(),
+        partial_result_params: PartialResultParams::default(),
+    };
+
+    let result = backend.goto_definition(params).await.unwrap();
+    assert!(
+        result.is_some(),
+        "Should resolve $user->id() to the method declaration"
+    );
+
+    match result.unwrap() {
+        GotoDefinitionResponse::Scalar(location) => {
+            assert_eq!(location.uri, uri);
+            assert_eq!(
+                location.range.start.line, 5,
+                "$user->id() (with parens) should go to the id() method on line 5, not the $id property on line 2"
+            );
+        }
+        other => panic!("Expected Scalar location, got: {:?}", other),
+    }
+}
+
+/// Same disambiguation but via `$this->` inside the class itself.
+#[tokio::test]
+async fn test_goto_definition_this_property_vs_method_disambiguation() {
+    let backend = create_test_backend();
+
+    let uri = Url::parse("file:///test.php").unwrap();
+    let text = concat!(
+        "<?php\n",                                 // 0
+        "class Order {\n",                         // 1
+        "    public float $total;\n",              // 2  ← property
+        "\n",                                      // 3
+        "    public function total(): float {\n",  // 4  ← method
+        "        return $this->total;\n",          // 5  ← property access
+        "    }\n",                                 // 6
+        "\n",                                      // 7
+        "    public function display(): void {\n", // 8
+        "        echo $this->total;\n",            // 9  ← property access
+        "        echo $this->total();\n",          // 10 ← method call
+        "    }\n",                                 // 11
+        "}\n",                                     // 12
+    );
+
+    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;
+
+    // `$this->total` on line 9 (no parens) → property on line 2
+    let params = GotoDefinitionParams {
+        text_document_position_params: TextDocumentPositionParams {
+            text_document: TextDocumentIdentifier { uri: uri.clone() },
+            position: Position {
+                line: 9,
+                character: 23, // on "total" in `$this->total`
+            },
+        },
+        work_done_progress_params: WorkDoneProgressParams::default(),
+        partial_result_params: PartialResultParams::default(),
+    };
+
+    let result = backend.goto_definition(params).await.unwrap();
+    match result.unwrap() {
+        GotoDefinitionResponse::Scalar(location) => {
+            assert_eq!(
+                location.range.start.line, 2,
+                "$this->total (no parens) should go to the $total property on line 2"
+            );
+        }
+        other => panic!("Expected Scalar location, got: {:?}", other),
+    }
+
+    // `$this->total()` on line 10 (with parens) → method on line 4
+    let params = GotoDefinitionParams {
+        text_document_position_params: TextDocumentPositionParams {
+            text_document: TextDocumentIdentifier { uri: uri.clone() },
+            position: Position {
+                line: 10,
+                character: 23, // on "total" in `$this->total()`
+            },
+        },
+        work_done_progress_params: WorkDoneProgressParams::default(),
+        partial_result_params: PartialResultParams::default(),
+    };
+
+    let result = backend.goto_definition(params).await.unwrap();
+    match result.unwrap() {
+        GotoDefinitionResponse::Scalar(location) => {
+            assert_eq!(
+                location.range.start.line, 4,
+                "$this->total() (with parens) should go to the total() method on line 4"
+            );
+        }
+        other => panic!("Expected Scalar location, got: {:?}", other),
+    }
+}