From d92d2fe9b0bd5e6e29cb3b6f924472aec972b636 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Fri, 25 Oct 2024 21:52:50 +0100 Subject: fix(lsp): make missing import action fix infallible (#26539) --- cli/lsp/analysis.rs | 105 ++++++++++++++++++++++++---------------------------- 1 file changed, 49 insertions(+), 56 deletions(-) (limited to 'cli/lsp/analysis.rs') diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 8c2e8bb1d..39f1ae648 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -18,7 +18,6 @@ use deno_lint::diagnostic::LintDiagnosticRange; use deno_ast::SourceRange; use deno_ast::SourceRangedForSpanned; use deno_ast::SourceTextInfo; -use deno_core::anyhow::anyhow; use deno_core::error::custom_error; use deno_core::error::AnyError; use deno_core::serde::Deserialize; @@ -40,6 +39,7 @@ use import_map::ImportMap; use node_resolver::NpmResolver; use once_cell::sync::Lazy; use regex::Regex; +use std::borrow::Cow; use std::cmp::Ordering; use std::collections::HashMap; use std::collections::HashSet; @@ -598,68 +598,62 @@ pub fn fix_ts_import_changes( /// Fix tsc import code actions so that the module specifier is correct for /// resolution by Deno (includes the extension). -fn fix_ts_import_action( +fn fix_ts_import_action<'a>( referrer: &ModuleSpecifier, - action: &tsc::CodeFixAction, + action: &'a tsc::CodeFixAction, import_mapper: &TsResponseImportMapper, -) -> Result, AnyError> { - if matches!( +) -> Option> { + if !matches!( action.fix_name.as_str(), "import" | "fixMissingFunctionDeclaration" ) { - let change = action + return Some(Cow::Borrowed(action)); + } + let specifier = (|| { + let text_change = action.changes.first()?.text_changes.first()?; + let captures = IMPORT_SPECIFIER_RE.captures(&text_change.new_text)?; + Some(captures.get(1)?.as_str()) + })(); + let Some(specifier) = specifier else { + return Some(Cow::Borrowed(action)); + }; + if let Some(new_specifier) = + import_mapper.check_unresolved_specifier(specifier, referrer) + { + let description = action.description.replace(specifier, &new_specifier); + let changes = action .changes - .first() - .ok_or_else(|| anyhow!("Unexpected action changes."))?; - let text_change = change - .text_changes - .first() - .ok_or_else(|| anyhow!("Missing text change."))?; - if let Some(captures) = IMPORT_SPECIFIER_RE.captures(&text_change.new_text) - { - let specifier = captures - .get(1) - .ok_or_else(|| anyhow!("Missing capture."))? - .as_str(); - if let Some(new_specifier) = - import_mapper.check_unresolved_specifier(specifier, referrer) - { - let description = action.description.replace(specifier, &new_specifier); - let changes = action - .changes + .iter() + .map(|c| { + let text_changes = c + .text_changes .iter() - .map(|c| { - let text_changes = c - .text_changes - .iter() - .map(|tc| tsc::TextChange { - span: tc.span.clone(), - new_text: tc.new_text.replace(specifier, &new_specifier), - }) - .collect(); - tsc::FileTextChanges { - file_name: c.file_name.clone(), - text_changes, - is_new_file: c.is_new_file, - } + .map(|tc| tsc::TextChange { + span: tc.span.clone(), + new_text: tc.new_text.replace(specifier, &new_specifier), }) .collect(); - - return Ok(Some(tsc::CodeFixAction { - description, - changes, - commands: None, - fix_name: action.fix_name.clone(), - fix_id: None, - fix_all_description: None, - })); - } else if !import_mapper.is_valid_import(specifier, referrer) { - return Ok(None); - } - } + tsc::FileTextChanges { + file_name: c.file_name.clone(), + text_changes, + is_new_file: c.is_new_file, + } + }) + .collect(); + + Some(Cow::Owned(tsc::CodeFixAction { + description, + changes, + commands: None, + fix_name: action.fix_name.clone(), + fix_id: None, + fix_all_description: None, + })) + } else if !import_mapper.is_valid_import(specifier, referrer) { + None + } else { + Some(Cow::Borrowed(action)) } - - Ok(Some(action.clone())) } /// Determines if two TypeScript diagnostic codes are effectively equivalent. @@ -1004,8 +998,7 @@ impl CodeActionCollection { specifier, action, &language_server.get_ts_response_import_mapper(specifier), - )? - else { + ) else { return Ok(()); }; let edit = ts_changes_to_edit(&action.changes, language_server)?; @@ -1027,7 +1020,7 @@ impl CodeActionCollection { }); self .actions - .push(CodeActionKind::Tsc(code_action, action.clone())); + .push(CodeActionKind::Tsc(code_action, action.as_ref().clone())); if let Some(fix_id) = &action.fix_id { if let Some(CodeActionKind::Tsc(existing_fix_all, existing_action)) = -- cgit v1.2.3 From 826e42a5b5880c974ae019a7a21aade6a718062c Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 1 Nov 2024 12:27:00 -0400 Subject: fix: improved support for cjs and cts modules (#26558) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * cts support * better cjs/cts type checking * deno compile cjs/cts support * More efficient detect cjs (going towards stabilization) * Determination of whether .js, .ts, .jsx, or .tsx is cjs or esm is only done after loading * Support `import x = require(...);` Co-authored-by: Bartek Iwańczuk --- cli/lsp/analysis.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) (limited to 'cli/lsp/analysis.rs') diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 39f1ae648..0769c9047 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -36,7 +36,6 @@ use deno_semver::package::PackageReq; use deno_semver::package::PackageReqReference; use deno_semver::Version; use import_map::ImportMap; -use node_resolver::NpmResolver; use once_cell::sync::Lazy; use regex::Regex; use std::borrow::Cow; @@ -336,7 +335,12 @@ impl<'a> TsResponseImportMapper<'a> { .resolver .maybe_managed_npm_resolver(Some(&self.file_referrer)) { - if npm_resolver.in_npm_package(specifier) { + let in_npm_pkg = self + .resolver + .maybe_node_resolver(Some(&self.file_referrer)) + .map(|n| n.in_npm_package(specifier)) + .unwrap_or(false); + if in_npm_pkg { if let Ok(Some(pkg_id)) = npm_resolver.resolve_pkg_id_from_specifier(specifier) { @@ -1199,14 +1203,11 @@ impl CodeActionCollection { }), ); - match parsed_source.program_ref() { - deno_ast::swc::ast::Program::Module(module) => module - .body - .iter() - .find(|i| i.range().contains(&specifier_range)) - .map(|i| text_info.line_and_column_index(i.range().start)), - deno_ast::swc::ast::Program::Script(_) => None, - } + parsed_source + .program_ref() + .body() + .find(|i| i.range().contains(&specifier_range)) + .map(|i| text_info.line_and_column_index(i.range().start)) } async fn deno_types_for_npm_action( -- cgit v1.2.3 From 5088b25f2315fa45e912377356a89ba2a44dbcda Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 6 Nov 2024 06:26:46 +0000 Subject: feat(lsp): auto-import completions from byonm dependencies (#26680) --- cli/lsp/analysis.rs | 76 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 24 deletions(-) (limited to 'cli/lsp/analysis.rs') diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 0769c9047..98215855c 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 { -- cgit v1.2.3 From 7d326c269cfb8463d530ee5dd4bcabb7499e1ba7 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 12 Nov 2024 13:15:32 +0000 Subject: fix(lsp): skip code action edits that can't be converted (#26831) --- cli/lsp/analysis.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'cli/lsp/analysis.rs') diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 98215855c..683a59c21 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -10,6 +10,7 @@ use super::tsc; use super::urls::url_to_uri; use crate::args::jsr_url; +use crate::lsp::logging::lsp_warn; use crate::lsp::search::PackageSearchApi; use crate::tools::lint::CliLinter; use crate::util::path::relative_specifier; @@ -747,8 +748,14 @@ pub fn ts_changes_to_edit( ) -> Result, AnyError> { let mut text_document_edits = Vec::new(); for change in changes { - let text_document_edit = change.to_text_document_edit(language_server)?; - text_document_edits.push(text_document_edit); + let edit = match change.to_text_document_edit(language_server) { + Ok(e) => e, + Err(err) => { + lsp_warn!("Couldn't covert text document edit: {:#}", err); + continue; + } + }; + text_document_edits.push(edit); } Ok(Some(lsp::WorkspaceEdit { changes: None, -- cgit v1.2.3 From f091d1ad69b4e5217ae3272b641171781a372c4f Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 13 Nov 2024 10:10:09 -0500 Subject: feat(node): stabilize detecting if CJS via `"type": "commonjs"` in a package.json (#26439) This will respect `"type": "commonjs"` in a package.json to determine if `.js`/`.jsx`/`.ts`/.tsx` files are CJS or ESM. If the file is found to be ESM it will be loaded as ESM though. --- cli/lsp/analysis.rs | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) (limited to 'cli/lsp/analysis.rs') diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 683a59c21..9f26de70c 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -39,6 +39,7 @@ use deno_semver::package::PackageReq; use deno_semver::package::PackageReqReference; use deno_semver::Version; use import_map::ImportMap; +use node_resolver::NodeModuleKind; use once_cell::sync::Lazy; use regex::Regex; use std::borrow::Cow; @@ -467,6 +468,7 @@ impl<'a> TsResponseImportMapper<'a> { &self, specifier: &str, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, ) -> Option { let specifier_stem = specifier.strip_suffix(".js").unwrap_or(specifier); let specifiers = std::iter::once(Cow::Borrowed(specifier)).chain( @@ -477,7 +479,7 @@ impl<'a> TsResponseImportMapper<'a> { for specifier in specifiers { if let Some(specifier) = self .resolver - .as_graph_resolver(Some(&self.file_referrer)) + .as_cli_resolver(Some(&self.file_referrer)) .resolve( &specifier, &deno_graph::Range { @@ -485,6 +487,7 @@ impl<'a> TsResponseImportMapper<'a> { start: deno_graph::Position::zeroed(), end: deno_graph::Position::zeroed(), }, + referrer_kind, ResolutionMode::Types, ) .ok() @@ -507,10 +510,11 @@ impl<'a> TsResponseImportMapper<'a> { &self, specifier_text: &str, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, ) -> bool { self .resolver - .as_graph_resolver(Some(&self.file_referrer)) + .as_cli_resolver(Some(&self.file_referrer)) .resolve( specifier_text, &deno_graph::Range { @@ -518,6 +522,7 @@ impl<'a> TsResponseImportMapper<'a> { start: deno_graph::Position::zeroed(), end: deno_graph::Position::zeroed(), }, + referrer_kind, deno_graph::source::ResolutionMode::Types, ) .is_ok() @@ -586,6 +591,7 @@ fn try_reverse_map_package_json_exports( /// like an import and rewrite the import specifier to include the extension pub fn fix_ts_import_changes( referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, changes: &[tsc::FileTextChanges], language_server: &language_server::Inner, ) -> Result, AnyError> { @@ -602,8 +608,8 @@ pub fn fix_ts_import_changes( if let Some(captures) = IMPORT_SPECIFIER_RE.captures(line) { let specifier = captures.iter().skip(1).find_map(|s| s).unwrap().as_str(); - if let Some(new_specifier) = - import_mapper.check_unresolved_specifier(specifier, referrer) + if let Some(new_specifier) = import_mapper + .check_unresolved_specifier(specifier, referrer, referrer_kind) { line.replace(specifier, &new_specifier) } else { @@ -633,6 +639,7 @@ pub fn fix_ts_import_changes( /// resolution by Deno (includes the extension). fn fix_ts_import_action<'a>( referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, action: &'a tsc::CodeFixAction, language_server: &language_server::Inner, ) -> Option> { @@ -652,7 +659,7 @@ fn fix_ts_import_action<'a>( }; let import_mapper = language_server.get_ts_response_import_mapper(referrer); if let Some(new_specifier) = - import_mapper.check_unresolved_specifier(specifier, referrer) + import_mapper.check_unresolved_specifier(specifier, referrer, referrer_kind) { let description = action.description.replace(specifier, &new_specifier); let changes = action @@ -683,7 +690,7 @@ fn fix_ts_import_action<'a>( fix_id: None, fix_all_description: None, })) - } else if !import_mapper.is_valid_import(specifier, referrer) { + } else if !import_mapper.is_valid_import(specifier, referrer, referrer_kind) { None } else { Some(Cow::Borrowed(action)) @@ -1017,6 +1024,7 @@ impl CodeActionCollection { pub fn add_ts_fix_action( &mut self, specifier: &ModuleSpecifier, + specifier_kind: NodeModuleKind, action: &tsc::CodeFixAction, diagnostic: &lsp::Diagnostic, language_server: &language_server::Inner, @@ -1034,7 +1042,8 @@ impl CodeActionCollection { "The action returned from TypeScript is unsupported.", )); } - let Some(action) = fix_ts_import_action(specifier, action, language_server) + let Some(action) = + fix_ts_import_action(specifier, specifier_kind, action, language_server) else { return Ok(()); }; @@ -1276,6 +1285,9 @@ impl CodeActionCollection { import_start_from_specifier(document, i) })?; let referrer = document.specifier(); + let referrer_kind = language_server + .is_cjs_resolver + .get_doc_module_kind(document); let file_referrer = document.file_referrer(); let config_data = language_server .config @@ -1298,10 +1310,11 @@ impl CodeActionCollection { if !config_data.byonm { return None; } - if !language_server - .resolver - .is_bare_package_json_dep(&dep_key, referrer) - { + if !language_server.resolver.is_bare_package_json_dep( + &dep_key, + referrer, + referrer_kind, + ) { return None; } NpmPackageReqReference::from_str(&format!("npm:{}", &dep_key)).ok()? @@ -1320,7 +1333,7 @@ impl CodeActionCollection { } if language_server .resolver - .npm_to_file_url(&npm_ref, document.specifier(), file_referrer) + .npm_to_file_url(&npm_ref, referrer, referrer_kind, file_referrer) .is_some() { // The package import has types. -- cgit v1.2.3 From 617350e79c58b6e01984e3d7c7436d243d0e5cff Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 14 Nov 2024 15:24:25 -0500 Subject: refactor(resolver): move more resolution code into deno_resolver (#26873) Follow-up to cjs refactor. This moves most of the resolution code into the deno_resolver crate. Still pending is the npm resolution code. --- cli/lsp/analysis.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'cli/lsp/analysis.rs') diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 9f26de70c..044b1573b 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -344,9 +344,8 @@ impl<'a> TsResponseImportMapper<'a> { { let in_npm_pkg = self .resolver - .maybe_node_resolver(Some(&self.file_referrer)) - .map(|n| n.in_npm_package(specifier)) - .unwrap_or(false); + .in_npm_pkg_checker(Some(&self.file_referrer)) + .in_npm_package(specifier); if in_npm_pkg { if let Ok(Some(pkg_id)) = npm_resolver.resolve_pkg_id_from_specifier(specifier) -- cgit v1.2.3