agent: Improve LSP tool symbol resolution and definition lookup - #55803
agent: Improve LSP tool symbol resolution and definition lookup#55803AJenbo wants to merge 1 commit into
Conversation
c42646a to
ac9336c
Compare
| project.definitions(&resolved.buffer, resolved.position, cx) | ||
| // Try type definition first (e.g. navigates $order to the Order class), | ||
| // fall back to regular definition (e.g. the assignment site). | ||
| let type_def_task = project.update(cx, |project, cx| { |
There was a problem hiding this comment.
I'm not sure this is a good default behaviour. Getting type defintions can be useful, definitely, but so can finding the declaration of a variable (or struct/class/etc. field). I don't think one is necessarily more useful than the other.
I think there's scope for an extra tool, or perhaps merging the "goto X" tools into a single tool with a field that allows the agent to specify whether it wants to see the declaration, type definition, references, implementations, etc. For now though, we're just evaluating a limited subset.
There was a problem hiding this comment.
It now just calls project.definitions() directly instead of trying type_definitions first. Removed the doc comment paragraph about type definitions.
| /// of the symbol (e.g. PHP variables `$foo`). | ||
| fn find_word_bounded<'a>(haystack: &'a str, needle: &'a str) -> impl Iterator<Item = usize> + 'a { | ||
| find_word_bounded_impl(haystack, needle, false) | ||
| } |
There was a problem hiding this comment.
This function is quite complex, and encodes some language-specific details that I'm not sure we can rely on in all cases. For example, the string foo$ can be a full identifier (e.g. in Dart, where $ is a non-special symbol when used in identifiers), or it could be an identifier and operator (e.g. in Lean4, where $ is function application). Or a-b is an identifier in Nix, but in C-style langauges it's a minus b.
The ideal solution would be using the syntax tree, but that's a relatively complex change.
The point you make about void handlePayment(Payment payment) not being able to resolve the type is a fair point though, and we should handle this better. For this purpose, I actually think ignoring casing is actually not the right approach, since:
- the case can provide useful information (e.g.
const FOO: Foo = ...) - it doesn't actually work in languages that use
snake_caseorkebab-caseheavily
I think a simpler implementation could:
- split the haystack based on some conservative regex for non-identifier characters (whitespace, and symbols excluding
$,@,#,-, and maybe some others) - search the current line for an exact symbol match
- if that fails, try the line before and the line after
- repeat, expanding the context each time, failing after some maximum context size
There was a problem hiding this comment.
Replaced the complex find_word_bounded/find_word_bounded_impl/find_word_bounded_case_insensitive functions with a single find_symbol_on_line that uses is_identifier_char (alphanumeric, _, $, @, #, -) for token boundary checks. Removed case-insensitive fallback entirely.
| fn format_line_display(line_text: &str) -> String { | ||
| let display: String = line_text | ||
| .chars() | ||
| .skip_while(|c| c.is_whitespace()) | ||
| .take(MAX_LINE_DISPLAY_LEN) | ||
| .collect(); | ||
| display.trim_end().to_string() |
There was a problem hiding this comment.
I'm not sure if we need to strip whitespace - seems like a bit of a footgun if we end up concatenating multiple lines together, we lose indentation information.
But if we are going to do it, this method doesn't need to allocate
There was a problem hiding this comment.
Ok, it no longer strips leading whitespace. Just truncates to MAX_LINE_DISPLAY_LEN and trims trailing whitespace using &str slicing to reduce allocation.
Improve SymbolLocator::resolve() to be more robust when agents provide slightly inaccurate positions: - Word boundary matching: prevents matching 'Payment' inside 'PaymentProcessor', ensuring only standalone symbol occurrences match - Nearby line search (±4 lines): agents often land on blank lines before code due to selection ranges, so we search nearby when the exact line has no match - Case-insensitive fallback: handles agents using wrong casing (e.g. 'PaymentToken' matching 'paymentToken') - Special prefix support: correctly handles symbols starting with $, @, or # (e.g. PHP variables like $payment) Improve go_to_definition to try type_definitions first, falling back to regular definitions. This means navigating to a variable like $order takes you to the Order class rather than the assignment site, which is usually more useful when exploring code.
ac9336c to
62fb95d
Compare
This PR improves the
SymbolLocator::resolve()used by all LSP tools (go_to_definition,find_references,rename_symbol,get_code_actions) to be more robust when agents provide slightly inaccurate positions, and improvesgo_to_definitionto prefer type definitions.Symbol resolution improvements
The existing implementation does a plain substring match on the exact line specified. This fails in several common cases:
1. False substring matches — Searching for
Paymenton a line containingcaptureReservedPayment(Payment $payment)would match insidecaptureReservedPaymentfirst, positioning the cursor in the middle of the wrong symbol.Fixed with word boundary matching: Only standalone occurrences are matched. Word characters are alphanumeric and
_. Special prefixes ($,@,#) skip the left-boundary check since they already act as delimiters.2. Off-by-one line numbers — Agents frequently land on the blank line before code (e.g. the line before a function signature) because selection ranges and outline results sometimes point there.
Fixed with nearby line search (±4 lines): If the symbol isn't found on the exact line, nearby lines are searched before giving up.
3. Wrong casing — Agents sometimes use
PaymentTokenwhen the code haspaymentToken, especially when paraphrasing from memory.Fixed with case-insensitive fallback: Case-sensitive matching is tried first and preferred; case-insensitive is used only when no exact match exists.
Definition lookup improvement
go_to_definitionnow triestype_definitionsfirst, falling back to regulardefinitions. This means navigating to a variable like$ordertakes you to theOrderclass rather than the assignment site, which is usually more useful when exploring code.Testing
Unit tests cover word boundary matching, case-insensitive fallback, special prefix handling, and no-match cases.
Release Notes: