From 8fe9d2fcc8ed602bc8ee2403ff5283a37559e98d Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 11 Jun 2024 18:14:36 +0100 Subject: [PATCH] refactor(lsp): collect npm reqs by scope (#24172) --- cli/lsp/config.rs | 16 ++++----- cli/lsp/documents.rs | 57 +++++++++++++++++++----------- cli/lsp/language_server.rs | 72 +++++++++++++++++++++++++++++++------- cli/lsp/resolver.rs | 15 ++++++-- 4 files changed, 113 insertions(+), 47 deletions(-) diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index e445d34f03..75e0cd6bc3 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -778,13 +778,9 @@ impl Settings { specifier: &ModuleSpecifier, ) -> (&WorkspaceSettings, Option<&ModuleSpecifier>) { let Ok(path) = specifier_to_file_path(specifier) else { - return (&self.unscoped, None); + return (&self.unscoped, self.first_folder.as_ref()); }; for (folder_uri, settings) in self.by_workspace_folder.iter().rev() { - let mut settings = settings.as_ref(); - if self.first_folder.as_ref() == Some(folder_uri) { - settings = settings.or(Some(&self.unscoped)); - } if let Some(settings) = settings { let Ok(folder_path) = specifier_to_file_path(folder_uri) else { continue; @@ -794,7 +790,7 @@ impl Settings { } } } - (&self.unscoped, None) + (&self.unscoped, self.first_folder.as_ref()) } pub fn enable_settings_hash(&self) -> u64 { @@ -1572,6 +1568,10 @@ pub struct ConfigTree { } impl ConfigTree { + pub fn root_scope(&self) -> Option<&ModuleSpecifier> { + self.first_folder.as_ref() + } + pub fn root_data(&self) -> Option<&ConfigData> { self.first_folder.as_ref().and_then(|s| self.scopes.get(s)) } @@ -1583,10 +1583,6 @@ impl ConfigTree { .unwrap_or_default() } - pub fn root_lockfile(&self) -> Option<&Arc>> { - self.root_data().and_then(|d| d.lockfile.as_ref()) - } - pub fn root_import_map(&self) -> Option<&Arc> { self.root_data().and_then(|d| d.import_map.as_ref()) } diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 5624ccb568..a0e99d9bef 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -34,6 +34,7 @@ use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReq; use indexmap::IndexMap; use std::borrow::Cow; +use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashMap; use std::collections::HashSet; @@ -903,7 +904,8 @@ pub struct Documents { /// settings. resolver: Arc, /// The npm package requirements found in npm specifiers. - npm_specifier_reqs: Arc>, + npm_reqs_by_scope: + Arc, BTreeSet>>, /// Gets if any document had a node: specifier such that a @types/node package /// should be injected. has_injected_types_node_package: bool, @@ -1093,10 +1095,11 @@ impl Documents { false } - /// Returns a collection of npm package requirements. - pub fn npm_package_reqs(&mut self) -> Arc> { + pub fn npm_reqs_by_scope( + &mut self, + ) -> Arc, BTreeSet>> { self.calculate_npm_reqs_if_dirty(); - self.npm_specifier_reqs.clone() + self.npm_reqs_by_scope.clone() } /// Returns if a @types/node package was injected into the npm @@ -1322,31 +1325,36 @@ impl Documents { /// document and the value is a set of specifiers that depend on that /// document. fn calculate_npm_reqs_if_dirty(&mut self) { - let mut npm_reqs = HashSet::new(); - let mut has_node_builtin_specifier = false; + let mut npm_reqs_by_scope: BTreeMap<_, BTreeSet<_>> = Default::default(); + let mut scopes_with_node_builtin_specifier = HashSet::new(); let is_fs_docs_dirty = self.file_system_docs.set_dirty(false); if !is_fs_docs_dirty && !self.dirty { return; } let mut visit_doc = |doc: &Arc| { + let scope = doc + .file_referrer() + .and_then(|r| self.config.tree.scope_for_specifier(r)) + .or(self.config.tree.root_scope()); + let reqs = npm_reqs_by_scope.entry(scope.cloned()).or_default(); for dependency in doc.dependencies().values() { if let Some(dep) = dependency.get_code() { if dep.scheme() == "node" { - has_node_builtin_specifier = true; + scopes_with_node_builtin_specifier.insert(scope.cloned()); } if let Ok(reference) = NpmPackageReqReference::from_specifier(dep) { - npm_reqs.insert(reference.into_inner().req); + reqs.insert(reference.into_inner().req); } } if let Some(dep) = dependency.get_type() { if let Ok(reference) = NpmPackageReqReference::from_specifier(dep) { - npm_reqs.insert(reference.into_inner().req); + reqs.insert(reference.into_inner().req); } } } if let Some(dep) = doc.maybe_types_dependency().maybe_specifier() { if let Ok(reference) = NpmPackageReqReference::from_specifier(dep) { - npm_reqs.insert(reference.into_inner().req); + reqs.insert(reference.into_inner().req); } } }; @@ -1358,12 +1366,21 @@ impl Documents { } // fill the reqs from the lockfile - if let Some(lockfile) = self.config.tree.root_lockfile() { + // TODO(nayeemrmn): Iterate every lockfile here for multi-deno.json. + if let Some(lockfile) = self + .config + .tree + .root_data() + .and_then(|d| d.lockfile.as_ref()) + { + let reqs = npm_reqs_by_scope + .entry(self.config.tree.root_scope().cloned()) + .or_default(); let lockfile = lockfile.lock(); for key in lockfile.content.packages.specifiers.keys() { if let Some(key) = key.strip_prefix("npm:") { if let Ok(req) = PackageReq::from_str(key) { - npm_reqs.insert(req); + reqs.insert(req); } } } @@ -1372,17 +1389,15 @@ impl Documents { // Ensure a @types/node package exists when any module uses a node: specifier. // Unlike on the command line, here we just add @types/node to the npm package // requirements since this won't end up in the lockfile. - self.has_injected_types_node_package = has_node_builtin_specifier - && !npm_reqs.iter().any(|r| r.name == "@types/node"); - if self.has_injected_types_node_package { - npm_reqs.insert(PackageReq::from_str("@types/node").unwrap()); + for scope in scopes_with_node_builtin_specifier { + let reqs = npm_reqs_by_scope.entry(scope).or_default(); + if !reqs.iter().any(|r| r.name == "@types/node") { + self.has_injected_types_node_package = true; + reqs.insert(PackageReq::from_str("@types/node").unwrap()); + } } - self.npm_specifier_reqs = Arc::new({ - let mut reqs = npm_reqs.into_iter().collect::>(); - reqs.sort(); - reqs - }); + self.npm_reqs_by_scope = Arc::new(npm_reqs_by_scope); self.dirty = false; } diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index f8a9652258..657913cfb0 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -806,7 +806,7 @@ impl Inner { log::debug!("Skipped workspace walk due to client incapability."); return (Default::default(), false); } - let mut workspace_files = Default::default(); + let mut workspace_files = BTreeSet::default(); let entry_limit = 1000; let mut pending = VecDeque::new(); let mut entry_count = 0; @@ -816,10 +816,30 @@ impl Inner { .filter_map(|p| specifier_to_file_path(&p.0).ok()) .collect::>(); roots.sort(); - for i in 0..roots.len() { - if i == 0 || !roots[i].starts_with(&roots[i - 1]) { - if let Ok(read_dir) = std::fs::read_dir(&roots[i]) { - pending.push_back((roots[i].clone(), read_dir)); + let roots = roots + .iter() + .enumerate() + .filter(|(i, root)| *i == 0 || !root.starts_with(&roots[i - 1])) + .map(|(_, r)| r.clone()) + .collect::>(); + let mut root_ancestors = BTreeSet::new(); + for root in roots { + for ancestor in root.ancestors().skip(1) { + if root_ancestors.insert(ancestor.to_path_buf()) { + break; + } + } + if let Ok(read_dir) = std::fs::read_dir(&root) { + pending.push_back((root, read_dir)); + } + } + for root_ancestor in root_ancestors { + for deno_json in ["deno.json", "deno.jsonc"] { + let path = root_ancestor.join(deno_json); + if path.exists() { + if let Ok(specifier) = ModuleSpecifier::from_file_path(path) { + workspace_files.insert(specifier); + } } } } @@ -836,17 +856,15 @@ impl Inner { let Ok(specifier) = ModuleSpecifier::from_file_path(&path) else { continue; }; - // TODO(nayeemrmn): Don't walk folders that are `None` here and aren't - // in a `deno.json` scope. - if config.settings.specifier_enabled(&specifier) == Some(false) { - continue; - } let Ok(file_type) = entry.file_type() else { continue; }; let Some(file_name) = path.file_name() else { continue; }; + if config.settings.specifier_enabled(&specifier) == Some(false) { + continue; + } if file_type.is_dir() { let dir_name = file_name.to_string_lossy().to_lowercase(); // We ignore these directories by default because there is a @@ -1107,11 +1125,11 @@ impl Inner { } async fn refresh_npm_specifiers(&mut self) { - let package_reqs = self.documents.npm_package_reqs(); + let package_reqs = self.documents.npm_reqs_by_scope(); let resolver = self.resolver.clone(); // spawn due to the lsp's `Send` requirement let handle = - spawn(async move { resolver.set_npm_package_reqs(&package_reqs).await }); + spawn(async move { resolver.set_npm_reqs(&package_reqs).await }); if let Err(err) = handle.await.unwrap() { lsp_warn!("Could not set npm package requirements. {:#}", err); } @@ -3428,7 +3446,10 @@ impl Inner { roots.extend( self .documents - .npm_package_reqs() + .npm_reqs_by_scope() + .values() + .flatten() + .collect::>() .iter() .map(|req| ModuleSpecifier::parse(&format!("npm:{}", req)).unwrap()), ); @@ -3714,6 +3735,7 @@ mod tests { temp_dir.create_dir_all("root2/folder"); temp_dir.create_dir_all("root2/sub_folder"); + temp_dir.create_dir_all("root2/root2.1"); temp_dir.write("root2/file1.ts", ""); // yes, enabled temp_dir.write("root2/file2.ts", ""); // no, not enabled temp_dir.write("root2/folder/main.ts", ""); // yes, enabled @@ -3721,14 +3743,21 @@ mod tests { temp_dir.write("root2/sub_folder/a.js", ""); // no, not enabled temp_dir.write("root2/sub_folder/b.ts", ""); // no, not enabled temp_dir.write("root2/sub_folder/c.js", ""); // no, not enabled + temp_dir.write("root2/root2.1/main.ts", ""); // yes, enabled as separate root temp_dir.create_dir_all("root3/"); temp_dir.write("root3/mod.ts", ""); // no, not enabled + temp_dir.create_dir_all("root4_parent/root4"); + temp_dir.write("root4_parent/deno.json", ""); // yes, enabled as deno.json above root + temp_dir.write("root4_parent/root4/main.ts", ""); // yes, enabled + let mut config = Config::new_with_roots(vec![ temp_dir.uri().join("root1/").unwrap(), temp_dir.uri().join("root2/").unwrap(), + temp_dir.uri().join("root2/root2.1/").unwrap(), temp_dir.uri().join("root3/").unwrap(), + temp_dir.uri().join("root4_parent/root4/").unwrap(), ]); config.set_client_capabilities(ClientCapabilities { workspace: Some(Default::default()), @@ -3756,6 +3785,13 @@ mod tests { ..Default::default() }, ), + ( + temp_dir.uri().join("root2/root2.1/").unwrap(), + WorkspaceSettings { + enable: Some(true), + ..Default::default() + }, + ), ( temp_dir.uri().join("root3/").unwrap(), WorkspaceSettings { @@ -3763,6 +3799,13 @@ mod tests { ..Default::default() }, ), + ( + temp_dir.uri().join("root4_parent/root4/").unwrap(), + WorkspaceSettings { + enable: Some(true), + ..Default::default() + }, + ), ], ); @@ -3784,6 +3827,9 @@ mod tests { temp_dir.uri().join("root1/sub_dir/mod.ts").unwrap(), temp_dir.uri().join("root2/file1.ts").unwrap(), temp_dir.uri().join("root2/folder/main.ts").unwrap(), + temp_dir.uri().join("root2/root2.1/main.ts").unwrap(), + temp_dir.uri().join("root4_parent/deno.json").unwrap(), + temp_dir.uri().join("root4_parent/root4/main.ts").unwrap(), ]) ); } diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index bd09f0ad19..c06bbfc8d9 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -43,6 +43,8 @@ use deno_semver::package::PackageReq; use indexmap::IndexMap; use package_json::PackageJsonDepsProvider; use std::borrow::Cow; +use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::collections::HashMap; use std::collections::HashSet; use std::rc::Rc; @@ -161,13 +163,20 @@ impl LspResolver { self.jsr_resolver.as_ref().inspect(|r| r.did_cache()); } - pub async fn set_npm_package_reqs( + pub async fn set_npm_reqs( &self, - reqs: &[PackageReq], + reqs: &BTreeMap, BTreeSet>, ) -> Result<(), AnyError> { + let reqs = reqs + .values() + .flatten() + .collect::>() + .into_iter() + .cloned() + .collect::>(); if let Some(npm_resolver) = self.npm_resolver.as_ref() { if let Some(npm_resolver) = npm_resolver.as_managed() { - return npm_resolver.set_package_reqs(reqs).await; + return npm_resolver.set_package_reqs(&reqs).await; } } Ok(())