Skip to content

Commit f70231e

Browse files
committed
Fix incorrectly finding definitions in comments
1 parent b4d13cc commit f70231e

4 files changed

Lines changed: 1732 additions & 4 deletions

File tree

example.php

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,38 @@ function handleIntersection(User&Loggable $entity): void {
360360
echo $typed; // Ctrl+Click on $typed → jumps to assignment
361361

362362

363+
// ── Type Hint Go-to-Definition ──────────────────────────────────────────────
364+
// Ctrl+Click on any class/interface name used as a type hint — in parameters,
365+
// return types, property types, catch blocks, and even inside docblock
366+
// annotations — to jump to its definition.
367+
368+
// Parameter type hints (simple, nullable, union, intersection):
369+
function typeHintGtdParam(User $u): void {} // Ctrl+Click User
370+
function typeHintGtdNullable(?User $u): void {} // Ctrl+Click User after ?
371+
function typeHintGtdUnion(User|AdminUser $u): void {} // Ctrl+Click either type
372+
function typeHintGtdIntersection(Renderable&Loggable $x): void {} // Ctrl+Click either type
373+
374+
// Return type hints:
375+
function typeHintGtdReturn(): Response { return new Response(200); } // Ctrl+Click Response
376+
377+
// Catch block exception types — Ctrl+Click NotFoundException or ValidationException:
378+
try { typeHintGtdParam(new User('', '')); } catch (NotFoundException|ValidationException $e) {}
379+
380+
// Extends / implements — Ctrl+Click User, Renderable, or Loggable in scaffolding below.
381+
382+
// Docblock type references — Ctrl+Click class names inside these annotations:
383+
/**
384+
* @param TypedCollection<int, User> $items Ctrl+Click User or TypedCollection
385+
* @return Response Ctrl+Click Response
386+
* @throws NotFoundException Ctrl+Click NotFoundException
387+
*/
388+
function typeHintGtdDocblock($items) { return new Response(200); }
389+
390+
/** @var User $docblockVar */
391+
$docblockVar = getUnknownValue();
392+
$docblockVar->getEmail(); // Ctrl+Click User in the @var above
393+
394+
363395
// ── Callable Snippet Insertion ──────────────────────────────────────────────
364396
// Completion inserts snippets with tab-stops for required params:
365397

src/definition/resolve.rs

Lines changed: 67 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -699,24 +699,87 @@ impl Backend {
699699
pub fn find_definition_position(content: &str, class_name: &str) -> Option<Position> {
700700
let keywords = ["class", "interface", "trait", "enum"];
701701

702+
// Track whether we are inside a `/* … */` block comment.
703+
let mut in_block_comment = false;
704+
702705
for (line_idx, line) in content.lines().enumerate() {
706+
// ── Block-comment tracking ──────────────────────────────────
707+
// Walk through the line handling `/*` and `*/` toggles so we
708+
// know whether the keyword match is inside a comment.
709+
let mut effective_line = String::new();
710+
let line_bytes = line.as_bytes();
711+
let mut i = 0;
712+
while i < line_bytes.len() {
713+
if in_block_comment {
714+
// Look for closing `*/`.
715+
if i + 1 < line_bytes.len()
716+
&& line_bytes[i] == b'*'
717+
&& line_bytes[i + 1] == b'/'
718+
{
719+
in_block_comment = false;
720+
// Replace the `*/` with spaces to preserve column offsets.
721+
effective_line.push(' ');
722+
effective_line.push(' ');
723+
i += 2;
724+
} else {
725+
effective_line.push(' ');
726+
i += 1;
727+
}
728+
} else if i + 1 < line_bytes.len()
729+
&& line_bytes[i] == b'/'
730+
&& line_bytes[i + 1] == b'*'
731+
{
732+
// Opening `/*` — rest of line (until `*/`) is a comment.
733+
in_block_comment = true;
734+
effective_line.push(' ');
735+
effective_line.push(' ');
736+
i += 2;
737+
} else if i + 1 < line_bytes.len()
738+
&& line_bytes[i] == b'/'
739+
&& line_bytes[i + 1] == b'/'
740+
{
741+
// Line comment `//` — blank out the rest of the line.
742+
while i < line_bytes.len() {
743+
effective_line.push(' ');
744+
i += 1;
745+
}
746+
} else if line_bytes[i] == b'#' {
747+
// Line comment `#` — blank out the rest of the line.
748+
while i < line_bytes.len() {
749+
effective_line.push(' ');
750+
i += 1;
751+
}
752+
} else {
753+
effective_line.push(line_bytes[i] as char);
754+
i += 1;
755+
}
756+
}
757+
703758
for keyword in &keywords {
704759
// Search for `keyword ClassName` making sure ClassName is
705760
// followed by a word boundary (whitespace, `{`, `:`, end of
706761
// line) so we don't match partial names.
707762
let pattern = format!("{} {}", keyword, class_name);
708-
if let Some(col) = line.find(&pattern) {
763+
if let Some(col) = effective_line.find(&pattern) {
709764
// Verify word boundary before the keyword: either start
710765
// of line or preceded by whitespace / non-alphanumeric.
711766
let before_ok = col == 0 || {
712-
let prev = line.as_bytes().get(col - 1).copied().unwrap_or(b' ');
767+
let prev = effective_line
768+
.as_bytes()
769+
.get(col - 1)
770+
.copied()
771+
.unwrap_or(b' ');
713772
!(prev as char).is_alphanumeric() && prev != b'_'
714773
};
715774

716775
// Verify word boundary after the class name.
717776
let after_pos = col + pattern.len();
718-
let after_ok = after_pos >= line.len() || {
719-
let next = line.as_bytes().get(after_pos).copied().unwrap_or(b' ');
777+
let after_ok = after_pos >= effective_line.len() || {
778+
let next = effective_line
779+
.as_bytes()
780+
.get(after_pos)
781+
.copied()
782+
.unwrap_or(b' ');
720783
!(next as char).is_alphanumeric() && next != b'_'
721784
};
722785

tests/definition_helpers.rs

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,116 @@ fn test_find_definition_position_no_partial_match() {
256256
assert!(pos.is_none());
257257
}
258258

259+
#[test]
260+
fn test_find_definition_position_skips_line_comment() {
261+
let content = concat!(
262+
"<?php\n",
263+
"// class AdminUser extends Model\n",
264+
"\n",
265+
"class AdminUser {\n",
266+
"}\n",
267+
);
268+
let pos = Backend::find_definition_position(content, "AdminUser");
269+
assert!(pos.is_some());
270+
let pos = pos.unwrap();
271+
assert_eq!(pos.line, 3, "Should skip the commented-out class on line 1");
272+
assert_eq!(pos.character, 0);
273+
}
274+
275+
#[test]
276+
fn test_find_definition_position_skips_hash_comment() {
277+
let content = concat!(
278+
"<?php\n",
279+
"# class Foo extends Bar\n",
280+
"\n",
281+
"class Foo {\n",
282+
"}\n",
283+
);
284+
let pos = Backend::find_definition_position(content, "Foo");
285+
assert!(pos.is_some());
286+
assert_eq!(
287+
pos.unwrap().line,
288+
3,
289+
"Should skip the #-commented class on line 1"
290+
);
291+
}
292+
293+
#[test]
294+
fn test_find_definition_position_skips_block_comment() {
295+
let content = concat!(
296+
"<?php\n",
297+
"/* class Widget {} */\n",
298+
"\n",
299+
"class Widget {\n",
300+
"}\n",
301+
);
302+
let pos = Backend::find_definition_position(content, "Widget");
303+
assert!(pos.is_some());
304+
assert_eq!(
305+
pos.unwrap().line,
306+
3,
307+
"Should skip the block-commented class on line 1"
308+
);
309+
}
310+
311+
#[test]
312+
fn test_find_definition_position_skips_multiline_block_comment() {
313+
let content = concat!(
314+
"<?php\n",
315+
"/*\n",
316+
" * class Order extends Model\n",
317+
" */\n",
318+
"\n",
319+
"class Order {\n",
320+
"}\n",
321+
);
322+
let pos = Backend::find_definition_position(content, "Order");
323+
assert!(pos.is_some());
324+
assert_eq!(pos.unwrap().line, 5, "Should skip multi-line block comment");
325+
}
326+
327+
#[test]
328+
fn test_find_definition_position_skips_docblock() {
329+
let content = concat!(
330+
"<?php\n",
331+
"/**\n",
332+
" * class Response extends BaseResponse\n",
333+
" */\n",
334+
"\n",
335+
"class Response {\n",
336+
"}\n",
337+
);
338+
let pos = Backend::find_definition_position(content, "Response");
339+
assert!(pos.is_some());
340+
assert_eq!(pos.unwrap().line, 5, "Should skip docblock comment");
341+
}
342+
343+
#[test]
344+
fn test_find_definition_position_inline_comment_after_code() {
345+
// The class declaration is real code; the comment is after it.
346+
let content = concat!("<?php\n", "class Config { // class Settings\n", "}\n",);
347+
let pos = Backend::find_definition_position(content, "Config");
348+
assert!(pos.is_some());
349+
assert_eq!(pos.unwrap().line, 1);
350+
351+
// "Settings" appears only inside the comment — should not match.
352+
let pos2 = Backend::find_definition_position(content, "Settings");
353+
assert!(
354+
pos2.is_none(),
355+
"Should not match class name inside trailing comment"
356+
);
357+
}
358+
359+
#[test]
360+
fn test_find_definition_position_only_in_comment_returns_none() {
361+
let content = concat!("<?php\n", "// class Ghost extends Model\n",);
362+
let pos = Backend::find_definition_position(content, "Ghost");
363+
assert!(
364+
pos.is_none(),
365+
"Should return None when class only exists in a comment"
366+
);
367+
}
368+
259369
#[test]
260370
fn test_find_definition_position_with_namespace() {
261371
let content = concat!(

0 commit comments

Comments
 (0)