From c004e5075192fc973b13b8660a4ed817d3ccdd14 Mon Sep 17 00:00:00 2001 From: Pedro Pessoa <54516747+yzPeedro@users.noreply.github.com> Date: Fri, 6 Nov 2026 22:42:35 -0300 Subject: [PATCH] fix(editor): let plugin completions and definitions own the strings they answer (#1364) Inside PHP strings, plugin completions were buried under project file paths and go to definition mixed the plugin's answer with the server's guesses and with IDE helper stubs. Now path items are left out when a plugin answered at that position, and in PHP and Blade paths only show once the string reads like a path. Inside a string literal (checked on the document's syntax tree) plugin definitions replace the server's; elsewhere both are kept, the plugin's first, so e.g. MyBatis mapper navigation still offers jdtls' answer. Generated helpers (_ide_helper*.php, .phpstorm.meta.php, vendor/_laravel_ide*) give way to real declarations, an empty plugin answer is marked incomplete so typing asks again, and a callable accepted before a closing quote no longer gets (). Closes #1336 Co-authored-by: yzPeedro <54516747+yzPeedro@users.noreply.github.com> --- crates/ide-host/src/ops/lsp.rs | 134 ++++++++++++++++++++++++++++-- src/editor/lsp/completion.test.ts | 17 ++++ src/editor/lsp/completion.ts | 33 +++++++- 3 files changed, 176 insertions(+), 8 deletions(-) diff --git a/crates/ide-host/src/ops/lsp.rs b/crates/ide-host/src/ops/lsp.rs index 738efde2f..c2dff41be 100644 --- a/crates/ide-host/src/ops/lsp.rs +++ b/crates/ide-host/src/ops/lsp.rs @@ -47,7 +47,7 @@ pub async fn lsp_completion( let Some((plugins, d)) = plugin_providers(host, window, doc, ProviderKind::Completion) else { return result }; let items = plugin_call(move || plugins.provide_completion(&d, pos, trigger.as_deref())).await.unwrap_or_default(); if items.is_empty() { - return result; + return Ok(result.unwrap_or(CompletionList { is_incomplete: true, items: Vec::new() })); } let mut list = result.unwrap_or(CompletionList { is_incomplete: false, items: Vec::new() }); list.items.extend(items.into_iter().map(|(_, item)| plugin_completion(item))); @@ -222,14 +222,65 @@ pub async fn lsp_goto(host: &Host, window: &str, doc: DocId, pos: Pos, kind: Got } let Some((plugins, d)) = plugin_providers(host, window, doc, ProviderKind::Definition) else { return result }; let found = plugin_call(move || plugins.provide_definition(&d, pos)).await.unwrap_or_default(); - if found.is_empty() { - return result; + let found: Vec = + found.into_iter().map(|l| Location { path: l.path.into(), range: l.range, uri: None }).collect(); + let in_string = !found.is_empty() && in_string_literal(host, window, doc, pos).await; + definition_targets(result, found, in_string) +} + +async fn in_string_literal(host: &Host, window: &str, doc: DocId, pos: Pos) -> bool { + let Ok(project) = host.project(window) else { return false }; + blocking(move || { + let snap = project.documents.text_snapshot(doc)?; + if snap.large { + return Ok(false); + } + let s = ide_syntax::Snapshot::doc(doc, snap.version, &snap.language, &snap.text); + Ok(string_at(&project.structure, &s, pos)) + }) + .await + .unwrap_or(false) +} + +fn string_at(structure: &ide_syntax::StructureService, snap: &ide_syntax::Snapshot, pos: Pos) -> bool { + let cancel = ide_core::CancellationToken::new(); + let Ok(Some(at)) = structure.node_at(snap, Range::new(pos, pos), &cancel) else { return false }; + let stringy = |kind: &str| kind.contains("string") || kind.contains("heredoc") || kind.contains("nowdoc"); + let part = |kind: &str| kind.contains("content") || kind.contains("fragment") || kind.contains("escape"); + stringy(&at.node.kind) || (part(&at.node.kind) && at.ancestors.first().is_some_and(|parent| stringy(&parent.kind))) +} + +fn definition_targets( + server: IdeResult>, + plugin: Vec, + in_string: bool, +) -> IdeResult> { + if plugin.is_empty() { + return server.map(without_generated_helpers); } - let mut out = result.unwrap_or_default(); - out.extend(found.into_iter().map(|l| Location { path: l.path.into(), range: l.range, uri: None })); + if in_string { + return Ok(plugin); + } + let same_place = |l: &Location| plugin.iter().any(|p| p.path == l.path && p.range.start.line == l.range.start.line); + let server: Vec = + without_generated_helpers(server.unwrap_or_default()).into_iter().filter(|l| !same_place(l)).collect(); + let mut out = plugin; + out.extend(server); Ok(out) } +fn without_generated_helpers(locations: Vec) -> Vec { + let generated = |l: &Location| { + let path = l.path.to_string_lossy().replace('\\', "/"); + let file = path.rsplit('/').next().unwrap_or(&path); + file.starts_with("_ide_helper") || file == ".phpstorm.meta.php" || path.contains("/vendor/_laravel_ide") + }; + if locations.iter().all(generated) { + return locations; + } + locations.into_iter().filter(|l| !generated(l)).collect() +} + /// Gutter markers from plugins (`line-markers` providers) for `doc`. pub async fn plugin_line_markers( host: &Host, @@ -956,6 +1007,79 @@ mod tests { assert!(eager.edit.is_some() && eager.data.is_none()); } + #[test] + fn plugin_definitions_own_strings_and_join_the_server_elsewhere() { + let at = |path: &str, line: u32| Location { + path: path.into(), + range: Range::new(Pos::new(line, 0), Pos::new(line, 4)), + uri: None, + }; + let paths = |found: IdeResult>| { + found.unwrap().into_iter().map(|l| l.path.to_string_lossy().into_owned()).collect::>() + }; + let xml = at("/p/src/main/resources/UserMapper.xml", 3); + let java = at("/p/src/main/java/UserMapper.java", 7); + let helper = at("/p/_ide_helper.php", 1); + let route = at("/p/routes/web.php", 2); + let guess = at("/p/app/helpers.php", 9); + + assert_eq!( + paths(definition_targets(Ok(vec![java.clone(), helper.clone()]), vec![xml.clone()], false)), + ["/p/src/main/resources/UserMapper.xml", "/p/src/main/java/UserMapper.java"] + ); + assert_eq!( + paths(definition_targets(Ok(vec![xml.clone(), java.clone()]), vec![xml.clone()], false)), + ["/p/src/main/resources/UserMapper.xml", "/p/src/main/java/UserMapper.java"] + ); + assert_eq!( + paths(definition_targets(Ok(vec![guess.clone(), helper.clone()]), vec![route.clone()], true)), + ["/p/routes/web.php"] + ); + assert_eq!( + paths(definition_targets(Err(ide_core::IdeError::unavailable("no server")), vec![route.clone()], false)), + ["/p/routes/web.php"] + ); + assert_eq!(paths(definition_targets(Ok(vec![helper, guess]), Vec::new(), false)), ["/p/app/helpers.php"]); + } + + #[test] + fn positions_inside_string_literals_are_told_apart() { + let syntax = std::sync::Arc::new(ide_language::Syntax::new()); + let structure = ide_syntax::StructureService::new(std::sync::Arc::new(ide_language::SyntaxCache::new(syntax))); + let php = ide_core::LanguageId::new("php"); + let text = "| found.into_iter().map(|l| l.path.to_string_lossy().into_owned()).collect::>(); + let real = at("/app/app/Models/User.php"); + let helper = at("/app/_ide_helper_models.php"); + let meta = at("/app/.phpstorm.meta.php"); + let vendor = at("/app/vendor/_laravel_idea/_ide_helper.php"); + assert_eq!( + paths(without_generated_helpers(vec![helper.clone(), real.clone(), meta.clone(), vendor])), + ["/app/app/Models/User.php"] + ); + assert_eq!( + paths(without_generated_helpers(vec![helper.clone(), meta.clone()])), + ["/app/_ide_helper_models.php", "/app/.phpstorm.meta.php"] + ); + assert!(without_generated_helpers(Vec::new()).is_empty()); + } + #[test] fn every_completion_kind_the_plugin_api_documents_is_understood() { let wit = include_str!("../../../../wit/providers.wit"); diff --git a/src/editor/lsp/completion.test.ts b/src/editor/lsp/completion.test.ts index e75cf69b0..5416bbcbc 100644 --- a/src/editor/lsp/completion.test.ts +++ b/src/editor/lsp/completion.test.ts @@ -15,6 +15,7 @@ import { import { applyLspCompletion, callParens, + offersPaths, commitsLookup, completionExtension, explicitCompletion, @@ -373,11 +374,27 @@ describe("insert parentheses", () => { it("skips calls that already have parentheses and imports", () => { expect(callParens(item(), line("a.cr(x)"), 2, 4)).toBeNull(); + expect(callParens(item(), line("cr'"), 0, 2)).toBeNull(); + expect(callParens(item(), line('cr"'), 0, 2)).toBeNull(); expect(callParens(item(), line("import { cr"), 9, 11)).toBeNull(); expect(callParens(item(), line("use crate::cr"), 11, 13)).toBeNull(); }); }); +describe("file paths in strings", () => { + it("are offered in PHP and Blade only once the string reads like a path", () => { + expect(offersPaths("php", "config('app.")).toBe(false); + expect(offersPaths("php", "User::create(['na")).toBe(false); + expect(offersPaths("php", "require './")).toBe(true); + expect(offersPaths("blade", "@include('partials/")).toBe(true); + }); + + it("are offered as before in other languages", () => { + expect(offersPaths("typescript", "import x from 'mod")).toBe(true); + expect(offersPaths("rust", 'include_str!("da')).toBe(true); + }); +}); + describe("sort suggestions alphabetically", () => { const result = (labels: string[]) => ({ from: 0, options: labels.map((label) => ({ label })), validFor: /^\w*$/ }); diff --git a/src/editor/lsp/completion.ts b/src/editor/lsp/completion.ts index 4cbb7543c..f1e4cd8aa 100644 --- a/src/editor/lsp/completion.ts +++ b/src/editor/lsp/completion.ts @@ -51,7 +51,7 @@ import { selectableText } from "@/ui/selectableText"; import { iconNode } from "./iconMarkup"; import { parameterInfoOrLeave, triggerSignatureHelp } from "./signature"; import { featureBackoff } from "./availability"; -import { pathCompletionSource } from "./fileReferences"; +import { pathCompletionSource, pathContext, pathLike } from "./fileReferences"; import { editorPrefs } from "@/platform/editorSettings"; import { tx } from "@/platform/i18n"; import { showHint } from "./hint"; @@ -298,7 +298,7 @@ export function callParens( ): "inside" | "after" | null { if (item.isSnippet || !item.kind || !CALLABLE.has(item.kind) || /[(<[{]/.test(item.insertText)) return null; const next = line.text.charAt(end - line.from); - if (next && /[(\p{L}\p{N}_$]/u.test(next)) return null; + if (next && /[(\p{L}\p{N}_$'"`]/u.test(next)) return null; if (IMPORT_LINE.test(line.text.slice(0, start - line.from))) return null; const sig = item.labelDetail ?? item.detail ?? ""; return /^\s*\(\s*\)/.test(sig) || /^\s*fn\s*\(\s*\)|^\s*\(\)\s*(=>|->|:)/.test(sig) ? "after" : "inside"; @@ -764,12 +764,39 @@ const completionFooter = ViewPlugin.fromClass( }, ); +let pluginClaim: { pos: number; claimed: Promise } | null = null; + +const claimingLspSource: CompletionSource = (ctx) => { + const result = Promise.resolve(lspCompletionSource(ctx)); + pluginClaim = { + pos: ctx.pos, + claimed: result.then((r) => !!r?.options.some((o) => isLsp(o) && o.lsp.server === 0)).catch(() => false), + }; + return result; +}; + +const PATH_ONLY_WHEN_PATH_LIKE = new Set(["php", "blade"]); + +export function offersPaths(language: string, before: string): boolean { + if (!PATH_ONLY_WHEN_PATH_LIKE.has(language)) return true; + const found = pathContext(before, language); + return !!found && pathLike(found.prefix); +} + +const pathFallbackSource: CompletionSource = async (ctx) => { + const line = ctx.state.doc.lineAt(ctx.pos); + if (!offersPaths(ctx.state.facet(lspDoc)?.language ?? "", line.text.slice(0, ctx.pos - line.from))) return null; + const claim = pluginClaim; + if (claim && claim.pos === ctx.pos && (await claim.claimed)) return null; + return pathCompletionSource(ctx); +}; + export function completionExtension() { return [ lookupKeys, prefetchResolve, autocompletion({ - override: [gated(lspCompletionSource), gated(contributedSource), gated(pathCompletionSource)], + override: [gated(claimingLspSource), gated(contributedSource), gated(pathFallbackSource)], icons: false, addToOptions: [ { render: kindBadge, position: 20 },