diff options
| -rw-r--r-- | cli/lsp/documents.rs | 112 | ||||
| -rw-r--r-- | cli/lsp/language_server.rs | 1 | ||||
| -rw-r--r-- | cli/lsp/resolver.rs | 82 | ||||
| -rw-r--r-- | tests/integration/lsp_tests.rs | 243 | ||||
| -rw-r--r-- | tests/util/server/src/lsp.rs | 130 |
5 files changed, 282 insertions, 286 deletions
diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index d008dbb74..42c67c45d 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -13,9 +13,7 @@ use super::tsc::AssetDocument; use crate::cache::HttpCache; use crate::graph_util::CliJsrUrlProvider; use crate::lsp::logging::lsp_warn; -use crate::resolver::SloppyImportsFsEntry; -use crate::resolver::SloppyImportsResolution; -use crate::resolver::SloppyImportsResolver; +use deno_graph::source::Resolver; use deno_runtime::fs_util::specifier_to_file_path; use dashmap::DashMap; @@ -390,7 +388,7 @@ impl Document { d.with_new_resolver( s, &CliJsrUrlProvider, - Some(graph_resolver), + Some(&graph_resolver), Some(npm_resolver), ), ) @@ -400,7 +398,7 @@ impl Document { maybe_types_dependency = self.maybe_types_dependency.as_ref().map(|d| { Arc::new(d.with_new_resolver( &CliJsrUrlProvider, - Some(graph_resolver), + Some(&graph_resolver), Some(npm_resolver), )) }); @@ -854,8 +852,6 @@ pub struct Documents { /// Gets if any document had a node: specifier such that a @types/node package /// should be injected. has_injected_types_node_package: bool, - /// If --unstable-sloppy-imports is enabled. - unstable_sloppy_imports: bool, } impl Documents { @@ -869,7 +865,6 @@ impl Documents { resolver: Default::default(), npm_specifier_reqs: Default::default(), has_injected_types_node_package: false, - unstable_sloppy_imports: false, } } @@ -996,54 +991,17 @@ impl Documents { &self, specifier: &ModuleSpecifier, ) -> Option<ModuleSpecifier> { - if self.unstable_sloppy_imports && specifier.scheme() == "file" { - Some( - self - .resolve_unstable_sloppy_import(specifier) - .into_specifier() - .into_owned(), - ) + let specifier = if let Ok(jsr_req_ref) = + JsrPackageReqReference::from_specifier(specifier) + { + Cow::Owned(self.resolver.jsr_to_registry_url(&jsr_req_ref)?) } else { - let specifier = if let Ok(jsr_req_ref) = - JsrPackageReqReference::from_specifier(specifier) - { - Cow::Owned(self.resolver.jsr_to_registry_url(&jsr_req_ref)?) - } else { - Cow::Borrowed(specifier) - }; - if !DOCUMENT_SCHEMES.contains(&specifier.scheme()) { - return None; - } - self.resolver.resolve_redirects(&specifier) + Cow::Borrowed(specifier) + }; + if !DOCUMENT_SCHEMES.contains(&specifier.scheme()) { + return None; } - } - - fn resolve_unstable_sloppy_import<'a>( - &self, - specifier: &'a ModuleSpecifier, - ) -> SloppyImportsResolution<'a> { - SloppyImportsResolver::resolve_with_stat_sync( - specifier, - ResolutionMode::Types, - |path| { - if let Ok(specifier) = ModuleSpecifier::from_file_path(path) { - if self.open_docs.contains_key(&specifier) - || self.cache.contains(&specifier) - { - return Some(SloppyImportsFsEntry::File); - } - } - path.metadata().ok().and_then(|m| { - if m.is_file() { - Some(SloppyImportsFsEntry::File) - } else if m.is_dir() { - Some(SloppyImportsFsEntry::Dir) - } else { - None - } - }) - }, - ) + self.resolver.resolve_redirects(&specifier) } /// Return `true` if the specifier can be resolved to a document. @@ -1226,12 +1184,7 @@ impl Documents { ) { self.config = Arc::new(config.clone()); self.cache = cache; - let config_data = config.tree.root_data(); - let config_file = config_data.and_then(|d| d.config_file.as_deref()); self.resolver = resolver.clone(); - self.unstable_sloppy_imports = config_file - .map(|c| c.has_unstable("sloppy-imports")) - .unwrap_or(false); { let fs_docs = &self.file_system_docs; // Clean up non-existent documents. @@ -1404,7 +1357,6 @@ fn node_resolve_npm_req_ref( pub struct OpenDocumentsGraphLoader<'a> { pub inner_loader: &'a mut dyn deno_graph::source::Loader, pub open_docs: &'a HashMap<ModuleSpecifier, Arc<Document>>, - pub unstable_sloppy_imports: bool, } impl<'a> OpenDocumentsGraphLoader<'a> { @@ -1426,32 +1378,6 @@ impl<'a> OpenDocumentsGraphLoader<'a> { } None } - - fn resolve_unstable_sloppy_import<'b>( - &self, - specifier: &'b ModuleSpecifier, - ) -> SloppyImportsResolution<'b> { - SloppyImportsResolver::resolve_with_stat_sync( - specifier, - ResolutionMode::Types, - |path| { - if let Ok(specifier) = ModuleSpecifier::from_file_path(path) { - if self.open_docs.contains_key(&specifier) { - return Some(SloppyImportsFsEntry::File); - } - } - path.metadata().ok().and_then(|m| { - if m.is_file() { - Some(SloppyImportsFsEntry::File) - } else if m.is_dir() { - Some(SloppyImportsFsEntry::Dir) - } else { - None - } - }) - }, - ) - } } impl<'a> deno_graph::source::Loader for OpenDocumentsGraphLoader<'a> { @@ -1460,17 +1386,9 @@ impl<'a> deno_graph::source::Loader for OpenDocumentsGraphLoader<'a> { specifier: &ModuleSpecifier, options: deno_graph::source::LoadOptions, ) -> deno_graph::source::LoadFuture { - let specifier = if self.unstable_sloppy_imports { - self - .resolve_unstable_sloppy_import(specifier) - .into_specifier() - } else { - Cow::Borrowed(specifier) - }; - - match self.load_from_docs(&specifier) { + match self.load_from_docs(specifier) { Some(fut) => fut, - None => self.inner_loader.load(&specifier, options), + None => self.inner_loader.load(specifier, options), } } @@ -1531,7 +1449,7 @@ fn analyze_module( // dynamic imports like import(`./dir/${something}`) in the LSP file_system: &deno_graph::source::NullFileSystem, jsr_url_provider: &CliJsrUrlProvider, - maybe_resolver: Some(resolver.as_graph_resolver()), + maybe_resolver: Some(&resolver.as_graph_resolver()), maybe_npm_resolver: Some(resolver.as_graph_npm_resolver()), }, )), diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 67eaca74e..3d8ee7e8d 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -251,7 +251,6 @@ impl LanguageServer { let mut loader = crate::lsp::documents::OpenDocumentsGraphLoader { inner_loader: &mut inner_loader, open_docs: &open_docs, - unstable_sloppy_imports: cli_options.unstable_sloppy_imports(), }; let graph = module_graph_creator .create_graph_with_loader(GraphKind::All, roots.clone(), &mut loader) diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index ebe254b9d..f336c6b03 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -21,6 +21,8 @@ use crate::npm::ManagedCliNpmResolver; use crate::resolver::CliGraphResolver; use crate::resolver::CliGraphResolverOptions; use crate::resolver::CliNodeResolver; +use crate::resolver::SloppyImportsFsEntry; +use crate::resolver::SloppyImportsResolver; use crate::util::progress_bar::ProgressBar; use crate::util::progress_bar::ProgressBarStyle; use deno_cache_dir::HttpCache; @@ -60,6 +62,7 @@ pub struct LspResolver { redirect_resolver: Option<Arc<RedirectResolver>>, graph_imports: Arc<IndexMap<ModuleSpecifier, GraphImport>>, config: Arc<Config>, + unstable_sloppy_imports: bool, } impl Default for LspResolver { @@ -73,6 +76,7 @@ impl Default for LspResolver { redirect_resolver: None, graph_imports: Default::default(), config: Default::default(), + unstable_sloppy_imports: false, } } } @@ -140,6 +144,10 @@ impl LspResolver { npm_config_hash, redirect_resolver, graph_imports, + unstable_sloppy_imports: config_data + .and_then(|d| d.config_file.as_ref()) + .map(|cf| cf.has_unstable("sloppy-imports")) + .unwrap_or(false), config: Arc::new(config.clone()), }) } @@ -162,6 +170,7 @@ impl LspResolver { redirect_resolver: self.redirect_resolver.clone(), graph_imports: self.graph_imports.clone(), config: self.config.clone(), + unstable_sloppy_imports: self.unstable_sloppy_imports, }) } @@ -181,8 +190,11 @@ impl LspResolver { Ok(()) } - pub fn as_graph_resolver(&self) -> &dyn Resolver { - self.graph_resolver.as_ref() + pub fn as_graph_resolver(&self) -> LspGraphResolver { + LspGraphResolver { + inner: &self.graph_resolver, + unstable_sloppy_imports: self.unstable_sloppy_imports, + } } pub fn as_graph_npm_resolver(&self) -> &dyn NpmResolver { @@ -307,6 +319,68 @@ impl LspResolver { } } +#[derive(Debug)] +pub struct LspGraphResolver<'a> { + inner: &'a CliGraphResolver, + unstable_sloppy_imports: bool, +} + +impl<'a> Resolver for LspGraphResolver<'a> { + fn default_jsx_import_source(&self) -> Option<String> { + self.inner.default_jsx_import_source() + } + + fn default_jsx_import_source_types(&self) -> Option<String> { + self.inner.default_jsx_import_source_types() + } + + fn jsx_import_source_module(&self) -> &str { + self.inner.jsx_import_source_module() + } + + fn resolve( + &self, + specifier_text: &str, + referrer_range: &deno_graph::Range, + mode: deno_graph::source::ResolutionMode, + ) -> Result<deno_ast::ModuleSpecifier, deno_graph::source::ResolveError> { + let specifier = self.inner.resolve(specifier_text, referrer_range, mode)?; + if self.unstable_sloppy_imports && specifier.scheme() == "file" { + Ok( + SloppyImportsResolver::resolve_with_stat_sync( + &specifier, + mode, + |path| { + path.metadata().ok().and_then(|m| { + if m.is_file() { + Some(SloppyImportsFsEntry::File) + } else if m.is_dir() { + Some(SloppyImportsFsEntry::Dir) + } else { + None + } + }) + }, + ) + .into_specifier() + .into_owned(), + ) + } else { + Ok(specifier) + } + } + + fn resolve_types( + &self, + specifier: &deno_ast::ModuleSpecifier, + ) -> Result< + Option<(deno_ast::ModuleSpecifier, Option<deno_graph::Range>)>, + deno_graph::source::ResolveError, + > { + self.inner.resolve_types(specifier) + } +} + async fn create_npm_resolver( config_data: &ConfigData, global_cache_path: Option<&Path>, @@ -399,9 +473,7 @@ fn create_graph_resolver( bare_node_builtins_enabled: config_file .map(|cf| cf.has_unstable("bare-node-builtins")) .unwrap_or(false), - // Don't set this for the LSP because instead we'll use the OpenDocumentsLoader - // because it's much easier and we get diagnostics/quick fixes about a redirected - // specifier for free. + // not used in the LSP as the LspGraphResolver handles this sloppy_imports_resolver: None, })) } diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 9d82e0afd..997e89050 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -12,70 +12,13 @@ use test_util::assert_starts_with; use test_util::assertions::assert_json_subset; use test_util::deno_cmd_with_deno_dir; use test_util::env_vars_for_npm_tests; +use test_util::lsp::range_of; +use test_util::lsp::source_file; use test_util::lsp::LspClient; use test_util::testdata_path; use test_util::TestContextBuilder; use tower_lsp::lsp_types as lsp; -/// Helper to get the `lsp::Range` of the `n`th occurrence of -/// `text` in `src`. `n` is zero-based, like most indexes. -fn range_of_nth( - n: usize, - text: impl AsRef<str>, - src: impl AsRef<str>, -) -> lsp::Range { - let text = text.as_ref(); - - let src = src.as_ref(); - - let start = src - .match_indices(text) - .nth(n) - .map(|(i, _)| i) - .unwrap_or_else(|| panic!("couldn't find text {text} in source {src}")); - let end = start + text.len(); - let mut line = 0; - let mut col = 0; - let mut byte_idx = 0; - - let pos = |line, col| lsp::Position { - line, - character: col, - }; - - let mut start_pos = None; - let mut end_pos = None; - for c in src.chars() { - if byte_idx == start { - start_pos = Some(pos(line, col)); - } - if byte_idx == end { - end_pos = Some(pos(line, col)); - break; - } - if c == '\n' { - line += 1; - col = 0; - } else { - col += c.len_utf16() as u32; - } - byte_idx += c.len_utf8(); - } - if start_pos.is_some() && end_pos.is_none() { - // range extends to end of string - end_pos = Some(pos(line, col)); - } - - let (start, end) = (start_pos.unwrap(), end_pos.unwrap()); - lsp::Range { start, end } -} - -/// Helper to get the `lsp::Range` of the first occurrence of -/// `text` in `src`. Equivalent to `range_of_nth(0, text, src)`. -fn range_of(text: impl AsRef<str>, src: impl AsRef<str>) -> lsp::Range { - range_of_nth(0, text, src) -} - #[test] fn lsp_startup_shutdown() { let context = TestContextBuilder::new().use_temp_cwd().build(); @@ -12106,15 +12049,19 @@ fn lsp_deno_future_env_byonm() { } #[test] -fn lsp_sloppy_imports_warn() { +fn lsp_sloppy_imports() { let context = TestContextBuilder::new().use_temp_cwd().build(); let temp_dir = context.temp_dir(); let temp_dir = temp_dir.path(); temp_dir .join("deno.json") .write(r#"{ "unstable": ["sloppy-imports"] }"#); - // should work when exists on the fs and when doesn't + // for sloppy imports, the file must exist on the file system + // to be resolved correctly temp_dir.join("a.ts").write("export class A {}"); + temp_dir.join("b.ts").write("export class B {}"); + temp_dir.join("c.js").write("export class C {}"); + temp_dir.join("c.d.ts").write("export class C {}"); let mut client = context.new_lsp_command().build(); client.initialize(|builder| { builder.set_root_uri(temp_dir.uri_dir()); @@ -12161,137 +12108,67 @@ fn lsp_sloppy_imports_warn() { ), }, })); - assert_eq!( - diagnostics.messages_with_source("deno"), - lsp::PublishDiagnosticsParams { - uri: temp_dir.join("file.ts").uri_file(), - diagnostics: vec![ - lsp::Diagnostic { - range: lsp::Range { - start: lsp::Position { - line: 0, - character: 19 - }, - end: lsp::Position { - line: 0, - character: 24 - } - }, - severity: Some(lsp::DiagnosticSeverity::INFORMATION), - code: Some(lsp::NumberOrString::String("redirect".to_string())), - source: Some("deno".to_string()), - message: format!( - "The import of \"{}\" was redirected to \"{}\".", - temp_dir.join("a").uri_file(), - temp_dir.join("a.ts").uri_file() - ), - data: Some(json!({ - "specifier": temp_dir.join("a").uri_file(), - "redirect": temp_dir.join("a.ts").uri_file() - })), - ..Default::default() - }, - lsp::Diagnostic { - range: lsp::Range { - start: lsp::Position { - line: 1, - character: 19 - }, - end: lsp::Position { - line: 1, - character: 27 - } - }, - severity: Some(lsp::DiagnosticSeverity::INFORMATION), - code: Some(lsp::NumberOrString::String("redirect".to_string())), - source: Some("deno".to_string()), - message: format!( - "The import of \"{}\" was redirected to \"{}\".", - temp_dir.join("b.js").uri_file(), - temp_dir.join("b.ts").uri_file() - ), - data: Some(json!({ - "specifier": temp_dir.join("b.js").uri_file(), - "redirect": temp_dir.join("b.ts").uri_file() - })), - ..Default::default() - } - ], - version: Some(1), - } + + assert_eq!(json!(diagnostics.all()), json!([])); + + client.shutdown(); +} + +#[test] +fn lsp_sloppy_imports_prefers_dts() { + let context = TestContextBuilder::new().use_temp_cwd().build(); + let temp_dir = context.temp_dir(); + let temp_dir = temp_dir.path(); + + temp_dir + .join("deno.json") + .write(r#"{ "unstable": ["sloppy-imports"] }"#); + + let mut client: LspClient = context + .new_lsp_command() + .set_root_dir(temp_dir.clone()) + .build(); + client.initialize_default(); + + temp_dir.join("a.js").write("export const foo: number;"); + + let a_dts = source_file(temp_dir.join("a.d.ts"), "export const foo = 3;"); + let file = source_file( + temp_dir.join("file.ts"), + "import { foo } from './a.js';\nconsole.log(foo);", ); + let diagnostics = client.did_open_file(&file); + // no warnings because "a.js" exists + assert_eq!(diagnostics.all().len(), 0); - let res = client.write_request( - "textDocument/codeAction", + let diagnostics = client.did_open_file(&a_dts); + assert_eq!(diagnostics.all().len(), 0, "Got {:#?}", diagnostics.all()); + + let response = client.write_request( + "textDocument/references", json!({ - "textDocument": { - "uri": temp_dir.join("file.ts").uri_file() - }, - "range": { - "start": { "line": 0, "character": 19 }, - "end": { "line": 0, "character": 24 } - }, + "textDocument": a_dts.identifier(), + "position": a_dts.range_of("foo").start, "context": { - "diagnostics": [{ - "range": { - "start": { "line": 0, "character": 19 }, - "end": { "line": 0, "character": 24 } - }, - "severity": 3, - "code": "redirect", - "source": "deno", - "message": format!( - "The import of \"{}\" was redirected to \"{}\".", - temp_dir.join("a").uri_file(), - temp_dir.join("a.ts").uri_file() - ), - "data": { - "specifier": temp_dir.join("a").uri_file(), - "redirect": temp_dir.join("a.ts").uri_file(), - }, - }], - "only": ["quickfix"] + "includeDeclaration": false } }), ); - assert_eq!( - res, - json!([{ - "title": "Update specifier to its redirected specifier.", - "kind": "quickfix", - "diagnostics": [{ - "range": { - "start": { "line": 0, "character": 19 }, - "end": { "line": 0, "character": 24 } - }, - "severity": 3, - "code": "redirect", - "source": "deno", - "message": format!( - "The import of \"{}\" was redirected to \"{}\".", - temp_dir.join("a").uri_file(), - temp_dir.join("a.ts").uri_file() - ), - "data": { - "specifier": temp_dir.join("a").uri_file(), - "redirect": temp_dir.join("a.ts").uri_file() - }, - }], - "edit": { - "changes": { - temp_dir.join("file.ts").uri_file(): [{ - "range": { - "start": { "line": 0, "character": 19 }, - "end": { "line": 0, "character": 24 } - }, - "newText": "\"./a.ts\"" - }] - } + assert_json_subset( + response, + json!([ + { + "uri": file.uri(), + // the import + "range": file.range_of("foo"), + }, + { + "uri": file.uri(), + // the usage + "range": file.range_of_nth(1, "foo"), } - }]) + ]), ); - - client.shutdown(); } #[test] diff --git a/tests/util/server/src/lsp.rs b/tests/util/server/src/lsp.rs index c4219b942..f91ea5634 100644 --- a/tests/util/server/src/lsp.rs +++ b/tests/util/server/src/lsp.rs @@ -778,6 +778,12 @@ impl LspClient { } } + pub fn did_open_file(&mut self, file: &SourceFile) -> CollectedDiagnostics { + self.did_open(json!({ + "textDocument": file.text_document(), + })) + } + pub fn did_open(&mut self, params: Value) -> CollectedDiagnostics { self.did_open_raw(params); self.read_diagnostics() @@ -1138,6 +1144,130 @@ impl CollectedDiagnostics { } } +#[derive(Debug, Clone)] +pub struct SourceFile { + path: PathRef, + src: String, + lang: &'static str, + version: i32, +} + +impl SourceFile { + pub fn new(path: PathRef, src: String) -> Self { + path.write(&src); + Self::new_in_mem(path, src) + } + + pub fn new_in_mem(path: PathRef, src: String) -> Self { + let lang = match path.as_path().extension().unwrap().to_str().unwrap() { + "js" => "javascript", + "ts" | "d.ts" => "typescript", + "json" => "json", + other => panic!("unsupported file extension: {other}"), + }; + Self { + path, + src, + lang, + version: 1, + } + } + + pub fn range_of(&self, text: &str) -> lsp::Range { + range_of(text, &self.src) + } + + pub fn range_of_nth(&self, n: usize, text: &str) -> lsp::Range { + range_of_nth(n, text, &self.src) + } + + pub fn uri(&self) -> lsp::Url { + self.path.uri_file() + } + + pub fn text_document(&self) -> lsp::TextDocumentItem { + lsp::TextDocumentItem { + uri: self.uri(), + language_id: self.lang.to_string(), + version: self.version, + text: self.src.clone(), + } + } + + pub fn identifier(&self) -> lsp::TextDocumentIdentifier { + lsp::TextDocumentIdentifier { uri: self.uri() } + } +} + +/// Helper to create a `SourceFile` and write its contents to disk. +pub fn source_file(path: PathRef, src: impl AsRef<str>) -> SourceFile { + SourceFile::new(path, src.as_ref().to_string()) +} + +/// Helper to create a `SourceFile` in memory without writing to disk. +pub fn source_file_in_mem(path: PathRef, src: impl AsRef<str>) -> SourceFile { + SourceFile::new_in_mem(path, src.as_ref().to_string()) +} + +/// Helper to get the `lsp::Range` of the `n`th occurrence of +/// `text` in `src`. `n` is zero-based, like most indexes. +pub fn range_of_nth( + n: usize, + text: impl AsRef<str>, + src: impl AsRef<str>, +) -> lsp::Range { + let text = text.as_ref(); + + let src = src.as_ref(); + + let start = src + .match_indices(text) + .nth(n) + .map(|(i, _)| i) + .unwrap_or_else(|| panic!("couldn't find text {text} in source {src}")); + let end = start + text.len(); + let mut line = 0; + let mut col = 0; + let mut byte_idx = 0; + + let pos = |line, col| lsp::Position { + line, + character: col, + }; + + let mut start_pos = None; + let mut end_pos = None; + for c in src.chars() { + if byte_idx == start { + start_pos = Some(pos(line, col)); + } + if byte_idx == end { + end_pos = Some(pos(line, col)); + break; + } + if c == '\n' { + line += 1; + col = 0; + } else { + col += c.len_utf16() as u32; + } + byte_idx += c.len_utf8(); + } + if start_pos.is_some() && end_pos.is_none() { + // range extends to end of string + end_pos = Some(pos(line, col)); + } + + let (start, end) = (start_pos.unwrap(), end_pos.unwrap()); + lsp::Range { start, end } +} + +/// Helper to get the `lsp::Range` of the first occurrence of +/// `text` in `src`. Equivalent to `range_of_nth(0, text, src)`. +pub fn range_of(text: impl AsRef<str>, src: impl AsRef<str>) -> lsp::Range { + range_of_nth(0, text, src) +} + #[cfg(test)] mod tests { use super::*; |