From bac8e4f6f25367cf5b6c2095249cf144035a4fbd Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 1 Apr 2023 12:02:44 -0400 Subject: [PATCH] fix(repl): disable language server document preloading in the repl (#18543) This was an oversight because the repl uses the language server under the hood. Also, never preloads from a root directory. Part of #18538 --- cli/lsp/client.rs | 20 ++++++ cli/lsp/completions.rs | 2 +- cli/lsp/diagnostics.rs | 2 +- cli/lsp/documents.rs | 94 ++++++++++++++++++++++------- cli/lsp/language_server.rs | 2 +- cli/lsp/tsc.rs | 2 +- cli/tests/integration/repl_tests.rs | 14 +++++ 7 files changed, 110 insertions(+), 26 deletions(-) diff --git a/cli/lsp/client.rs b/cli/lsp/client.rs index d24d4c2a9e..e684dc09fc 100644 --- a/cli/lsp/client.rs +++ b/cli/lsp/client.rs @@ -26,6 +26,13 @@ pub enum TestingNotification { Progress(testing_lsp_custom::TestRunProgressParams), } +#[derive(Debug, Default, Copy, Clone, PartialEq, Eq)] +pub enum LspClientKind { + #[default] + CodeEditor, + Repl, +} + #[derive(Clone)] pub struct Client(Arc); @@ -44,6 +51,10 @@ impl Client { Self(Arc::new(ReplClient)) } + pub fn kind(&self) -> LspClientKind { + self.0.kind() + } + /// Gets additional methods that should only be called outside /// the LSP's lock to prevent deadlocking scenarios. pub fn when_outside_lsp_lock(&self) -> OutsideLockClient { @@ -149,6 +160,7 @@ impl OutsideLockClient { #[async_trait] trait ClientTrait: Send + Sync { + fn kind(&self) -> LspClientKind; async fn publish_diagnostics( &self, uri: lsp::Url, @@ -177,6 +189,10 @@ struct TowerClient(tower_lsp::Client); #[async_trait] impl ClientTrait for TowerClient { + fn kind(&self) -> LspClientKind { + LspClientKind::CodeEditor + } + async fn publish_diagnostics( &self, uri: lsp::Url, @@ -296,6 +312,10 @@ struct ReplClient; #[async_trait] impl ClientTrait for ReplClient { + fn kind(&self) -> LspClientKind { + LspClientKind::Repl + } + async fn publish_diagnostics( &self, _uri: lsp::Url, diff --git a/cli/lsp/completions.rs b/cli/lsp/completions.rs index cb8bd446da..c40d6f2386 100644 --- a/cli/lsp/completions.rs +++ b/cli/lsp/completions.rs @@ -519,7 +519,7 @@ mod tests { source_fixtures: &[(&str, &str)], location: &Path, ) -> Documents { - let mut documents = Documents::new(location); + let mut documents = Documents::new(location, Default::default()); for (specifier, source, version, language_id) in fixtures { let specifier = resolve_url(specifier).expect("failed to create specifier"); diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 8ba8ce074a..539868eca1 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -1079,7 +1079,7 @@ mod tests { location: &Path, maybe_import_map: Option<(&str, &str)>, ) -> StateSnapshot { - let mut documents = Documents::new(location); + let mut documents = Documents::new(location, Default::default()); for (specifier, source, version, language_id) in fixtures { let specifier = resolve_url(specifier).expect("failed to create specifier"); diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 0c27893a79..ca7bd2c820 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -1,6 +1,7 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. use super::cache::calculate_fs_version; +use super::client::LspClientKind; use super::text::LineIndex; use super::tsc; use super::tsc::AssetDocument; @@ -815,6 +816,8 @@ pub struct Documents { open_docs: HashMap, /// Documents stored on the file system. file_system_docs: Arc>, + /// Kind of the client that is using the documents. + lsp_client_kind: LspClientKind, /// Hash of the config used for resolution. When the hash changes we update /// dependencies. resolver_config_hash: u64, @@ -834,13 +837,14 @@ pub struct Documents { } impl Documents { - pub fn new(location: &Path) -> Self { + pub fn new(location: &Path, lsp_client_kind: LspClientKind) -> Self { Self { cache: HttpCache::new(location), dirty: true, dependents_map: Default::default(), open_docs: HashMap::default(), file_system_docs: Default::default(), + lsp_client_kind, resolver_config_hash: 0, imports: Default::default(), resolver: CliGraphResolver::default(), @@ -1248,33 +1252,50 @@ impl Documents { // update the file system documents let mut fs_docs = self.file_system_docs.lock(); - let mut not_found_docs = - fs_docs.docs.keys().cloned().collect::>(); - let open_docs = &mut self.open_docs; + match self.lsp_client_kind { + LspClientKind::CodeEditor => { + let mut not_found_docs = + fs_docs.docs.keys().cloned().collect::>(); + let open_docs = &mut self.open_docs; - for specifier in PreloadDocumentFinder::from_root_urls(&root_urls) { - // mark this document as having been found - not_found_docs.remove(&specifier); + log::debug!("Preloading documents from root urls..."); + for specifier in PreloadDocumentFinder::from_root_urls(&root_urls) { + // mark this document as having been found + not_found_docs.remove(&specifier); - if !open_docs.contains_key(&specifier) - && !fs_docs.docs.contains_key(&specifier) - { - fs_docs.refresh_document(&self.cache, resolver, &specifier); - } else { - // update the existing entry to have the new resolver - if let Some(doc) = fs_docs.docs.get_mut(&specifier) { + if !open_docs.contains_key(&specifier) + && !fs_docs.docs.contains_key(&specifier) + { + fs_docs.refresh_document(&self.cache, resolver, &specifier); + } else { + // update the existing entry to have the new resolver + if let Some(doc) = fs_docs.docs.get_mut(&specifier) { + if let Some(new_doc) = doc.maybe_with_new_resolver(resolver) { + *doc = new_doc; + } + } + } + } + + // clean up and remove any documents that weren't found + for uri in not_found_docs { + fs_docs.docs.remove(&uri); + } + } + LspClientKind::Repl => { + // This log statement is used in the tests to ensure preloading doesn't + // happen, which is not useful in the repl and could be very expensive + // if the repl is launched from a directory with a lot of descendants. + log::debug!("Skipping document preload for repl."); + + // for the repl, just update to use the new resolver + for doc in fs_docs.docs.values_mut() { if let Some(new_doc) = doc.maybe_with_new_resolver(resolver) { *doc = new_doc; } } } } - - // clean up and remove any documents that weren't found - for uri in not_found_docs { - fs_docs.docs.remove(&uri); - } - fs_docs.dirty = true; } @@ -1516,6 +1537,14 @@ struct PreloadDocumentFinder { } impl PreloadDocumentFinder { + fn is_allowed_root_dir(dir_path: &Path) -> bool { + if dir_path.parent().is_none() { + // never search the root directory of a drive + return false; + } + true + } + pub fn from_root_urls(root_urls: &Vec) -> Self { let mut finder = PreloadDocumentFinder { pending_dirs: Default::default(), @@ -1525,7 +1554,9 @@ impl PreloadDocumentFinder { for root_url in root_urls { if let Ok(path) = root_url.to_file_path() { if path.is_dir() { - finder.pending_dirs.push(path); + if Self::is_allowed_root_dir(&path) { + finder.pending_dirs.push(path); + } } else { finder.pending_files.push(path); } @@ -1655,7 +1686,7 @@ mod tests { fn setup(temp_dir: &TempDir) -> (Documents, PathBuf) { let location = temp_dir.path().join("deps"); - let documents = Documents::new(&location); + let documents = Documents::new(&location, Default::default()); (documents, location) } @@ -1905,4 +1936,23 @@ console.log(b, "hello deno"); ] ); } + + #[test] + pub fn test_pre_load_document_finder_disallowed_dirs() { + if cfg!(windows) { + let paths = + PreloadDocumentFinder::from_root_urls(&vec![ + Url::parse("file:///c:/").unwrap() + ]) + .collect::>(); + assert_eq!(paths, vec![]); + } else { + let paths = + PreloadDocumentFinder::from_root_urls(&vec![ + Url::parse("file:///").unwrap() + ]) + .collect::>(); + assert_eq!(paths, vec![]); + } + } } diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 754b5c95cd..15a7d907b8 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -463,7 +463,7 @@ impl Inner { ModuleRegistry::new(&module_registries_location, http_client.clone()) .unwrap(); let location = dir.deps_folder_path(); - let documents = Documents::new(&location); + let documents = Documents::new(&location, client.kind()); let deps_http_cache = HttpCache::new(&location); let cache_metadata = cache::CacheMetadata::new(deps_http_cache.clone()); let performance = Arc::new(Performance::default()); diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 2980e546b3..ef5d0e645c 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -3530,7 +3530,7 @@ mod tests { fixtures: &[(&str, &str, i32, LanguageId)], location: &Path, ) -> StateSnapshot { - let mut documents = Documents::new(location); + let mut documents = Documents::new(location, Default::default()); for (specifier, source, version, language_id) in fixtures { let specifier = resolve_url(specifier).expect("failed to create specifier"); diff --git a/cli/tests/integration/repl_tests.rs b/cli/tests/integration/repl_tests.rs index c3e7c42d65..a473dc2006 100644 --- a/cli/tests/integration/repl_tests.rs +++ b/cli/tests/integration/repl_tests.rs @@ -5,6 +5,7 @@ use test_util::assert_contains; use test_util::assert_ends_with; use test_util::assert_not_contains; use util::TempDir; +use util::TestContext; use util::TestContextBuilder; #[test] @@ -957,3 +958,16 @@ fn package_json_uncached_no_error() { console.expect("42") }); } + +#[test] +fn closed_file_pre_load_does_not_occur() { + TestContext::default() + .new_command() + .args_vec(["repl", "-A", "--log-level=debug"]) + .with_pty(|console| { + assert_contains!( + console.all_output(), + "Skipping document preload for repl.", + ); + }); +}