From 5088b25f2315fa45e912377356a89ba2a44dbcda Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 6 Nov 2024 06:26:46 +0000 Subject: [PATCH] feat(lsp): auto-import completions from byonm dependencies (#26680) --- cli/lsp/analysis.rs | 76 +++++++++----- cli/lsp/documents.rs | 28 ----- cli/lsp/language_server.rs | 13 ++- cli/lsp/resolver.rs | 45 ++++++++ cli/lsp/tsc.rs | 102 ++++++++++++------ tests/integration/lsp_tests.rs | 185 +++++++++++++++++++++++++++++---- 6 files changed, 344 insertions(+), 105 deletions(-) diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 0769c90477..98215855c9 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -12,7 +12,9 @@ use super::urls::url_to_uri; use crate::args::jsr_url; use crate::lsp::search::PackageSearchApi; use crate::tools::lint::CliLinter; +use crate::util::path::relative_specifier; use deno_config::workspace::MappedResolution; +use deno_graph::source::ResolutionMode; use deno_lint::diagnostic::LintDiagnosticRange; use deno_ast::SourceRange; @@ -228,6 +230,7 @@ pub struct TsResponseImportMapper<'a> { documents: &'a Documents, maybe_import_map: Option<&'a ImportMap>, resolver: &'a LspResolver, + tsc_specifier_map: &'a tsc::TscSpecifierMap, file_referrer: ModuleSpecifier, } @@ -236,12 +239,14 @@ impl<'a> TsResponseImportMapper<'a> { documents: &'a Documents, maybe_import_map: Option<&'a ImportMap>, resolver: &'a LspResolver, + tsc_specifier_map: &'a tsc::TscSpecifierMap, file_referrer: &ModuleSpecifier, ) -> Self { Self { documents, maybe_import_map, resolver, + tsc_specifier_map, file_referrer: file_referrer.clone(), } } @@ -387,6 +392,11 @@ impl<'a> TsResponseImportMapper<'a> { } } } + } else if let Some(dep_name) = self + .resolver + .file_url_to_package_json_dep(specifier, Some(&self.file_referrer)) + { + return Some(dep_name); } // check if the import map has this specifier @@ -457,19 +467,36 @@ impl<'a> TsResponseImportMapper<'a> { specifier: &str, referrer: &ModuleSpecifier, ) -> Option { - if let Ok(specifier) = referrer.join(specifier) { - if let Some(specifier) = self.check_specifier(&specifier, referrer) { - return Some(specifier); - } - } - let specifier = specifier.strip_suffix(".js").unwrap_or(specifier); - for ext in SUPPORTED_EXTENSIONS { - let specifier_with_ext = format!("{specifier}{ext}"); - if self - .documents - .contains_import(&specifier_with_ext, referrer) + let specifier_stem = specifier.strip_suffix(".js").unwrap_or(specifier); + let specifiers = std::iter::once(Cow::Borrowed(specifier)).chain( + SUPPORTED_EXTENSIONS + .iter() + .map(|ext| Cow::Owned(format!("{specifier_stem}{ext}"))), + ); + for specifier in specifiers { + if let Some(specifier) = self + .resolver + .as_graph_resolver(Some(&self.file_referrer)) + .resolve( + &specifier, + &deno_graph::Range { + specifier: referrer.clone(), + start: deno_graph::Position::zeroed(), + end: deno_graph::Position::zeroed(), + }, + ResolutionMode::Types, + ) + .ok() + .and_then(|s| self.tsc_specifier_map.normalize(s.as_str()).ok()) + .filter(|s| self.documents.exists(s, Some(&self.file_referrer))) { - return Some(specifier_with_ext); + if let Some(specifier) = self + .check_specifier(&specifier, referrer) + .or_else(|| relative_specifier(referrer, &specifier)) + .filter(|s| !s.contains("/node_modules/")) + { + return Some(specifier); + } } } None @@ -559,8 +586,9 @@ fn try_reverse_map_package_json_exports( pub fn fix_ts_import_changes( referrer: &ModuleSpecifier, changes: &[tsc::FileTextChanges], - import_mapper: &TsResponseImportMapper, + language_server: &language_server::Inner, ) -> Result, AnyError> { + let import_mapper = language_server.get_ts_response_import_mapper(referrer); let mut r = Vec::new(); for change in changes { let mut text_changes = Vec::new(); @@ -605,7 +633,7 @@ pub fn fix_ts_import_changes( fn fix_ts_import_action<'a>( referrer: &ModuleSpecifier, action: &'a tsc::CodeFixAction, - import_mapper: &TsResponseImportMapper, + language_server: &language_server::Inner, ) -> Option> { if !matches!( action.fix_name.as_str(), @@ -621,6 +649,7 @@ fn fix_ts_import_action<'a>( let Some(specifier) = specifier else { return Some(Cow::Borrowed(action)); }; + let import_mapper = language_server.get_ts_response_import_mapper(referrer); if let Some(new_specifier) = import_mapper.check_unresolved_specifier(specifier, referrer) { @@ -728,7 +757,7 @@ pub fn ts_changes_to_edit( })) } -#[derive(Debug, Deserialize)] +#[derive(Debug, Deserialize, Serialize)] #[serde(rename_all = "camelCase")] pub struct CodeActionData { pub specifier: ModuleSpecifier, @@ -998,11 +1027,8 @@ impl CodeActionCollection { "The action returned from TypeScript is unsupported.", )); } - let Some(action) = fix_ts_import_action( - specifier, - action, - &language_server.get_ts_response_import_mapper(specifier), - ) else { + let Some(action) = fix_ts_import_action(specifier, action, language_server) + else { return Ok(()); }; let edit = ts_changes_to_edit(&action.changes, language_server)?; @@ -1051,10 +1077,12 @@ impl CodeActionCollection { specifier: &ModuleSpecifier, diagnostic: &lsp::Diagnostic, ) { - let data = Some(json!({ - "specifier": specifier, - "fixId": action.fix_id, - })); + let data = action.fix_id.as_ref().map(|fix_id| { + json!(CodeActionData { + specifier: specifier.clone(), + fix_id: fix_id.clone(), + }) + }); let title = if let Some(description) = &action.fix_all_description { description.clone() } else { diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 8609aed05e..ce13c32157 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -1059,34 +1059,6 @@ impl Documents { self.cache.is_valid_file_referrer(specifier) } - /// Return `true` if the provided specifier can be resolved to a document, - /// otherwise `false`. - pub fn contains_import( - &self, - specifier: &str, - referrer: &ModuleSpecifier, - ) -> bool { - let file_referrer = self.get_file_referrer(referrer); - let maybe_specifier = self - .resolver - .as_graph_resolver(file_referrer.as_deref()) - .resolve( - specifier, - &deno_graph::Range { - specifier: referrer.clone(), - start: deno_graph::Position::zeroed(), - end: deno_graph::Position::zeroed(), - }, - ResolutionMode::Types, - ) - .ok(); - if let Some(import_specifier) = maybe_specifier { - self.exists(&import_specifier, file_referrer.as_deref()) - } else { - false - } - } - pub fn resolve_document_specifier( &self, specifier: &ModuleSpecifier, diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 61a02f036e..2554fa34b1 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -1837,7 +1837,7 @@ impl Inner { fix_ts_import_changes( &code_action_data.specifier, &combined_code_actions.changes, - &self.get_ts_response_import_mapper(&code_action_data.specifier), + self, ) .map_err(|err| { error!("Unable to remap changes: {:#}", err); @@ -1890,7 +1890,7 @@ impl Inner { refactor_edit_info.edits = fix_ts_import_changes( &action_data.specifier, &refactor_edit_info.edits, - &self.get_ts_response_import_mapper(&action_data.specifier), + self, ) .map_err(|err| { error!("Unable to remap changes: {:#}", err); @@ -1921,7 +1921,8 @@ impl Inner { // todo(dsherret): this should probably just take the resolver itself // as the import map is an implementation detail .and_then(|d| d.resolver.maybe_import_map()), - self.resolver.as_ref(), + &self.resolver, + &self.ts_server.specifier_map, file_referrer, ) } @@ -2284,7 +2285,11 @@ impl Inner { .into(), scope.cloned(), ) - .await; + .await + .unwrap_or_else(|err| { + error!("Unable to get completion info from TypeScript: {:#}", err); + None + }); if let Some(completions) = maybe_completion_info { response = Some( diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index 9ce76005e5..f5df24d575 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -74,6 +74,7 @@ struct LspScopeResolver { pkg_json_resolver: Option>, redirect_resolver: Option>, graph_imports: Arc>, + package_json_deps_by_resolution: Arc>, config_data: Option>, } @@ -88,6 +89,7 @@ impl Default for LspScopeResolver { pkg_json_resolver: None, redirect_resolver: None, graph_imports: Default::default(), + package_json_deps_by_resolution: Default::default(), config_data: None, } } @@ -165,6 +167,33 @@ impl LspScopeResolver { ) }) .unwrap_or_default(); + let package_json_deps_by_resolution = (|| { + let node_resolver = node_resolver.as_ref()?; + let package_json = config_data?.maybe_pkg_json()?; + let referrer = package_json.specifier(); + let dependencies = package_json.dependencies.as_ref()?; + let result = dependencies + .iter() + .flat_map(|(name, _)| { + let req_ref = + NpmPackageReqReference::from_str(&format!("npm:{name}")).ok()?; + let specifier = into_specifier_and_media_type(Some( + node_resolver + .resolve_req_reference( + &req_ref, + &referrer, + NodeResolutionMode::Types, + ) + .ok()?, + )) + .0; + Some((specifier, name.clone())) + }) + .collect(); + Some(result) + })(); + let package_json_deps_by_resolution = + Arc::new(package_json_deps_by_resolution.unwrap_or_default()); Self { cjs_tracker: lsp_cjs_tracker, graph_resolver, @@ -174,6 +203,7 @@ impl LspScopeResolver { pkg_json_resolver: Some(pkg_json_resolver), redirect_resolver, graph_imports, + package_json_deps_by_resolution, config_data: config_data.cloned(), } } @@ -216,6 +246,9 @@ impl LspScopeResolver { redirect_resolver: self.redirect_resolver.clone(), pkg_json_resolver: Some(pkg_json_resolver), graph_imports: self.graph_imports.clone(), + package_json_deps_by_resolution: self + .package_json_deps_by_resolution + .clone(), config_data: self.config_data.clone(), }) } @@ -407,6 +440,18 @@ impl LspResolver { ))) } + pub fn file_url_to_package_json_dep( + &self, + specifier: &ModuleSpecifier, + file_referrer: Option<&ModuleSpecifier>, + ) -> Option { + let resolver = self.get_scope_resolver(file_referrer); + resolver + .package_json_deps_by_resolution + .get(specifier) + .cloned() + } + pub fn in_node_modules(&self, specifier: &ModuleSpecifier) -> bool { fn has_node_modules_dir(specifier: &ModuleSpecifier) -> bool { // consider any /node_modules/ directory as being in the node_modules diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 5fcdb3575a..6f63ced5be 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -236,7 +236,7 @@ pub struct TsServer { performance: Arc, sender: mpsc::UnboundedSender, receiver: Mutex>>, - specifier_map: Arc, + pub specifier_map: Arc, inspector_server: Mutex>>, pending_change: Mutex>, } @@ -882,20 +882,22 @@ impl TsServer { options: GetCompletionsAtPositionOptions, format_code_settings: FormatCodeSettings, scope: Option, - ) -> Option { + ) -> Result, AnyError> { let req = TscRequest::GetCompletionsAtPosition(Box::new(( self.specifier_map.denormalize(&specifier), position, options, format_code_settings, ))); - match self.request(snapshot, req, scope).await { - Ok(maybe_info) => maybe_info, - Err(err) => { - log::error!("Unable to get completion info from TypeScript: {:#}", err); - None - } - } + self + .request::>(snapshot, req, scope) + .await + .map(|mut info| { + if let Some(info) = &mut info { + info.normalize(&self.specifier_map); + } + info + }) } pub async fn get_completion_details( @@ -3642,6 +3644,12 @@ pub struct CompletionInfo { } impl CompletionInfo { + fn normalize(&mut self, specifier_map: &TscSpecifierMap) { + for entry in &mut self.entries { + entry.normalize(specifier_map); + } + } + pub fn as_completion_response( &self, line_index: Arc, @@ -3703,11 +3711,17 @@ pub struct CompletionItemData { #[derive(Debug, Deserialize, Serialize)] #[serde(rename_all = "camelCase")] -struct CompletionEntryDataImport { +struct CompletionEntryDataAutoImport { module_specifier: String, file_name: String, } +#[derive(Debug)] +pub struct CompletionNormalizedAutoImportData { + raw: CompletionEntryDataAutoImport, + normalized: ModuleSpecifier, +} + #[derive(Debug, Default, Deserialize, Serialize)] #[serde(rename_all = "camelCase")] pub struct CompletionEntry { @@ -3740,9 +3754,28 @@ pub struct CompletionEntry { is_import_statement_completion: Option, #[serde(skip_serializing_if = "Option::is_none")] data: Option, + /// This is not from tsc, we add it for convenience during normalization. + /// Represents `self.data.file_name`, but normalized. + #[serde(skip)] + auto_import_data: Option, } impl CompletionEntry { + fn normalize(&mut self, specifier_map: &TscSpecifierMap) { + let Some(data) = &self.data else { + return; + }; + let Ok(raw) = + serde_json::from_value::(data.clone()) + else { + return; + }; + if let Ok(normalized) = specifier_map.normalize(&raw.file_name) { + self.auto_import_data = + Some(CompletionNormalizedAutoImportData { raw, normalized }); + } + } + fn get_commit_characters( &self, info: &CompletionInfo, @@ -3891,25 +3924,24 @@ impl CompletionEntry { if let Some(source) = &self.source { let mut display_source = source.clone(); - if let Some(data) = &self.data { - if let Ok(import_data) = - serde_json::from_value::(data.clone()) + if let Some(import_data) = &self.auto_import_data { + if let Some(new_module_specifier) = language_server + .get_ts_response_import_mapper(specifier) + .check_specifier(&import_data.normalized, specifier) + .or_else(|| relative_specifier(specifier, &import_data.normalized)) { - if let Ok(import_specifier) = resolve_url(&import_data.file_name) { - if let Some(new_module_specifier) = language_server - .get_ts_response_import_mapper(specifier) - .check_specifier(&import_specifier, specifier) - .or_else(|| relative_specifier(specifier, &import_specifier)) - { - display_source.clone_from(&new_module_specifier); - if new_module_specifier != import_data.module_specifier { - specifier_rewrite = - Some((import_data.module_specifier, new_module_specifier)); - } - } else if source.starts_with(jsr_url().as_str()) { - return None; - } + if new_module_specifier.contains("/node_modules/") { + return None; } + display_source.clone_from(&new_module_specifier); + if new_module_specifier != import_data.raw.module_specifier { + specifier_rewrite = Some(( + import_data.raw.module_specifier.clone(), + new_module_specifier, + )); + } + } else if source.starts_with(jsr_url().as_str()) { + return None; } } // We want relative or bare (import-mapped or otherwise) specifiers to @@ -4212,6 +4244,13 @@ impl TscSpecifierMap { return specifier.to_string(); } let mut specifier = original.to_string(); + if specifier.contains("/node_modules/.deno/") + && !specifier.contains("/node_modules/@types/node/") + { + // The ts server doesn't give completions from files in + // `node_modules/.deno/`. We work around it like this. + specifier = specifier.replace("/node_modules/", "/$node_modules/"); + } let media_type = MediaType::from_specifier(original); // If the URL-inferred media type doesn't correspond to tsc's path-inferred // media type, force it to be the same by appending an extension. @@ -4329,7 +4368,7 @@ fn op_is_cancelled(state: &mut OpState) -> bool { fn op_is_node_file(state: &mut OpState, #[string] path: String) -> bool { let state = state.borrow::(); let mark = state.performance.mark("tsc.op.op_is_node_file"); - let r = match ModuleSpecifier::parse(&path) { + let r = match state.specifier_map.normalize(path) { Ok(specifier) => state.state_snapshot.resolver.in_node_modules(&specifier), Err(_) => false, }; @@ -4609,7 +4648,10 @@ fn op_script_names(state: &mut OpState) -> ScriptNames { for doc in &docs { let specifier = doc.specifier(); let is_open = doc.is_open(); - if is_open || specifier.scheme() == "file" { + if is_open + || (specifier.scheme() == "file" + && !state.state_snapshot.resolver.in_node_modules(specifier)) + { let script_names = doc .scope() .and_then(|s| result.by_scope.get_mut(s)) @@ -6035,6 +6077,7 @@ mod tests { Some(temp_dir.url()), ) .await + .unwrap() .unwrap(); assert_eq!(info.entries.len(), 22); let details = ts_server @@ -6194,6 +6237,7 @@ mod tests { Some(temp_dir.url()), ) .await + .unwrap() .unwrap(); let entry = info .entries diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 56221a0269..af5f9de23e 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -6628,6 +6628,23 @@ export class DuckConfig { }] }] } + }, { + "title": "Add all missing imports", + "kind": "quickfix", + "diagnostics": [{ + "range": { + "start": { "line": 0, "character": 50 }, + "end": { "line": 0, "character": 67 } + }, + "severity": 1, + "code": 2304, + "source": "deno-ts", + "message": "Cannot find name 'DuckConfigOptions'." + }], + "data": { + "specifier": "file:///a/file00.ts", + "fixId": "fixMissingImport" + } }, { "title": "Add import from \"./file01.ts\"", "kind": "quickfix", @@ -6656,23 +6673,6 @@ export class DuckConfig { }] }] } - }, { - "title": "Add all missing imports", - "kind": "quickfix", - "diagnostics": [{ - "range": { - "start": { "line": 0, "character": 50 }, - "end": { "line": 0, "character": 67 } - }, - "severity": 1, - "code": 2304, - "source": "deno-ts", - "message": "Cannot find name 'DuckConfigOptions'." - }], - "data": { - "specifier": "file:///a/file00.ts", - "fixId": "fixMissingImport" - } }]) ); let res = client.write_request( @@ -8125,6 +8125,151 @@ fn lsp_npm_completions_auto_import_and_quick_fix_no_import_map() { client.shutdown(); } +#[test] +fn lsp_npm_auto_import_and_quick_fix_byonm() { + let context = TestContextBuilder::new() + .use_http_server() + .use_temp_cwd() + .add_npm_env_vars() + .build(); + let temp_dir = context.temp_dir(); + temp_dir.write("deno.json", json!({}).to_string()); + temp_dir.write( + "package.json", + json!({ + "dependencies": { + "cowsay": "*", + }, + }) + .to_string(), + ); + context + .new_command() + .args("install") + .run() + .skip_output_check(); + temp_dir.write("other.ts", "import \"cowsay\";\n"); + let mut client = context.new_lsp_command().build(); + client.initialize_default(); + let diagnostics = client.did_open(json!({ + "textDocument": { + "uri": temp_dir.url().join("file.ts").unwrap(), + "languageId": "typescript", + "version": 1, + "text": "think({ text: \"foo\" });\n", + }, + })); + let list = client.get_completion_list( + temp_dir.url().join("file.ts").unwrap(), + (0, 5), + json!({ "triggerKind": 1 }), + ); + assert!(!list.is_incomplete); + let item = list + .items + .iter() + .find(|item| item.label == "think") + .unwrap(); + let res = client.write_request("completionItem/resolve", item); + assert_eq!( + res, + json!({ + "label": "think", + "labelDetails": { + "description": "cowsay", + }, + "kind": 3, + "detail": "function think(options: IOptions): string", + "documentation": { + "kind": "markdown", + "value": "\n\n*@param* \noptions ## Face :\nEither choose a mode (set the value as true) **_or_**\nset your own defined eyes and tongue to `e` and `T`.\n- ### `e` : eyes\n- ### `T` : tongue\n\n## Cow :\nEither specify a cow name (e.g. \"fox\") **_or_**\nset the value of `r` to true which selects a random cow.\n- ### `r` : random selection\n- ### `f` : cow name - from `cows` folder\n\n## Modes :\nModes are just ready-to-use faces, here's their list:\n- #### `b` : borg\n- #### `d` : dead \n- #### `g` : greedy\n- #### `p` : paranoia\n- #### `s` : stoned\n- #### `t` : tired\n- #### `w` : youthful\n- #### `y` : wired \n\n*@example* \n```\n// custom cow and face\ncowsay.think({\n text: 'Hello world!',\n e: '^^', // eyes\n T: 'U ', // tongue\n f: 'USA' // name of the cow from `cows` folder\n})\n\n// using a random cow\ncowsay.think({\n text: 'Hello world!',\n e: 'xx', // eyes\n r: true, // random mode - use a random cow.\n})\n\n// using a mode\ncowsay.think({\n text: 'Hello world!',\n y: true, // using y mode - youthful mode\n})\n```", + }, + "sortText": "￿16_0", + "additionalTextEdits": [ + { + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 }, + }, + "newText": "import { think } from \"cowsay\";\n\n", + }, + ], + }), + ); + let diagnostics = diagnostics + .messages_with_file_and_source( + temp_dir.url().join("file.ts").unwrap().as_str(), + "deno-ts", + ) + .diagnostics; + let res = client.write_request( + "textDocument/codeAction", + json!(json!({ + "textDocument": { + "uri": temp_dir.url().join("file.ts").unwrap(), + }, + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 5 }, + }, + "context": { + "diagnostics": &diagnostics, + "only": ["quickfix"], + }, + })), + ); + assert_eq!( + res, + json!([ + { + "title": "Add import from \"cowsay\"", + "kind": "quickfix", + "diagnostics": &diagnostics, + "edit": { + "documentChanges": [{ + "textDocument": { + "uri": temp_dir.url().join("file.ts").unwrap(), + "version": 1, + }, + "edits": [{ + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 }, + }, + "newText": "import { think } from \"cowsay\";\n\n", + }], + }], + }, + }, + { + "title": "Add missing function declaration 'think'", + "kind": "quickfix", + "diagnostics": &diagnostics, + "edit": { + "documentChanges": [ + { + "textDocument": { + "uri": temp_dir.url().join("file.ts").unwrap(), + "version": 1, + }, + "edits": [ + { + "range": { + "start": { "line": 1, "character": 0 }, + "end": { "line": 1, "character": 0 }, + }, + "newText": "\nfunction think(arg0: { text: string; }) {\n throw new Error(\"Function not implemented.\");\n}\n", + }, + ], + }, + ], + }, + }, + ]), + ); + client.shutdown(); +} + #[test] fn lsp_completions_node_specifier() { let context = TestContextBuilder::new().use_temp_cwd().build(); @@ -8237,8 +8382,8 @@ fn lsp_infer_return_type() { let context = TestContextBuilder::new().use_temp_cwd().build(); let temp_dir = context.temp_dir(); temp_dir.write("deno.json", json!({}).to_string()); - let types_file = source_file( - temp_dir.path().join("types.d.ts"), + temp_dir.write( + "types.d.ts", r#" export interface SomeInterface { someField: number; @@ -8319,7 +8464,7 @@ fn lsp_infer_return_type() { "start": { "line": 1, "character": 20 }, "end": { "line": 1, "character": 20 }, }, - "newText": format!(": import(\"{}\").SomeInterface", types_file.url()), + "newText": ": import(\"./types.d.ts\").SomeInterface", }, ], },