From 01445940449cedc571dcbd69caa7da58de007f2b Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Sun, 31 Mar 2024 21:39:23 +0100 Subject: [PATCH] fix(lsp): don't apply preload limit to workspace walk (#23123) --- cli/lsp/config.rs | 28 ++++++++------- cli/lsp/documents.rs | 5 +++ cli/lsp/language_server.rs | 62 +++++--------------------------- tests/integration/lsp_tests.rs | 64 ++++++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 65 deletions(-) diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index 304a53b8b8..e808514290 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -777,11 +777,12 @@ impl ConfigSnapshot { pub struct Settings { pub unscoped: WorkspaceSettings, pub by_workspace_folder: BTreeMap>, + pub first_folder: Option, } impl Settings { pub fn first_root_uri(&self) -> Option<&ModuleSpecifier> { - self.by_workspace_folder.first_key_value().map(|e| e.0) + self.first_folder.as_ref() } /// Returns `None` if the value should be deferred to the presence of a @@ -836,13 +837,11 @@ impl Settings { let Ok(path) = specifier_to_file_path(specifier) else { return (&self.unscoped, None); }; - let mut is_first_folder = true; for (folder_uri, settings) in self.by_workspace_folder.iter().rev() { let mut settings = settings.as_ref(); - if is_first_folder { + if self.first_folder.as_ref() == Some(folder_uri) { settings = settings.or(Some(&self.unscoped)); } - is_first_folder = false; if let Some(settings) = settings { let Ok(folder_path) = specifier_to_file_path(folder_uri) else { continue; @@ -870,6 +869,7 @@ impl Settings { .map(|s| (&s.enable, &s.enable_paths, &s.disable_paths)), ); } + hasher.write_hashable(&self.first_folder); hasher.finish() } } @@ -908,6 +908,7 @@ impl Config { ) { self.settings.by_workspace_folder = folders.iter().map(|(s, _)| (s.clone(), None)).collect(); + self.settings.first_folder = folders.first().map(|(s, _)| s.clone()); self.workspace_folders = folders; } @@ -1125,6 +1126,7 @@ pub struct ConfigData { pub lockfile: Option>>, pub package_json: Option>, pub import_map: Option>, + pub import_map_from_settings: bool, watched_files: HashMap, } @@ -1317,6 +1319,7 @@ impl ConfigData { let mut import_map = None; let mut import_map_value = None; let mut import_map_specifier = None; + let mut import_map_from_settings = false; if let Some(config_file) = &config_file { if config_file.is_an_import_map() { import_map_value = Some(config_file.to_import_map_value_from_imports()); @@ -1325,15 +1328,15 @@ impl ConfigData { { import_map_specifier = Some(specifier); } - } else if let Some(import_map_str) = &settings.import_map { - if let Ok(specifier) = Url::parse(import_map_str) { - import_map_specifier = Some(specifier); - } else if let Some(folder_uri) = workspace_folder { - if let Ok(specifier) = folder_uri.join(import_map_str) { - import_map_specifier = Some(specifier); - } - } } + import_map_specifier = import_map_specifier.or_else(|| { + let import_map_str = settings.import_map.as_ref()?; + let specifier = Url::parse(import_map_str) + .ok() + .or_else(|| workspace_folder?.join(import_map_str).ok())?; + import_map_from_settings = true; + Some(specifier) + }); if let Some(specifier) = &import_map_specifier { if let Ok(path) = specifier_to_file_path(specifier) { watched_files @@ -1416,6 +1419,7 @@ impl ConfigData { lockfile: lockfile.map(Mutex::new).map(Arc::new), package_json: package_json.map(Arc::new), import_map: import_map.map(Arc::new), + import_map_from_settings, watched_files, } } diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 8479768a0f..d207330ad1 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -1393,10 +1393,15 @@ impl Documents { } } self.open_docs = open_docs; + let mut preload_count = 0; for specifier in workspace_files { if !config.specifier_enabled(specifier) { continue; } + if preload_count >= config.settings.unscoped.document_preload_limit { + break; + } + preload_count += 1; if !self.open_docs.contains_key(specifier) && !fs_docs.docs.contains_key(specifier) { diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 8f52f6d2df..a5923a84ad 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -832,50 +832,6 @@ impl Inner { file_fetcher } - fn resolve_import_map_specifier( - &self, - referrer: &ModuleSpecifier, - ) -> Result, AnyError> { - let Some(import_map_str) = self - .config - .settings - .get_for_specifier(referrer) - .0 - .import_map - .clone() - .and_then(|s| if s.is_empty() { None } else { Some(s) }) - else { - return Ok(None); - }; - lsp_log!( - "Using import map from workspace settings: \"{}\"", - import_map_str - ); - if let Some(config_file) = - self.config.tree.config_file_for_specifier(referrer) - { - if let Some(import_map_path) = &config_file.json.import_map { - lsp_log!("Warning: Import map \"{}\" configured in \"{}\" being ignored due to an import map being explicitly configured in workspace settings.", import_map_path, config_file.specifier); - } - } - if let Ok(url) = Url::parse(&import_map_str) { - Ok(Some(url)) - } else if let Some(root_uri) = self.config.root_uri() { - let root_path = specifier_to_file_path(root_uri)?; - let import_map_path = root_path.join(&import_map_str); - let import_map_url = - Url::from_file_path(import_map_path).map_err(|_| { - anyhow!("Bad file path for import map: {}", import_map_str) - })?; - Ok(Some(import_map_url)) - } else { - Err(anyhow!( - "The path to the import map (\"{}\") is not resolvable.", - import_map_str - )) - } - } - pub fn update_debug_flag(&self) { let internal_debug = self.config.workspace_settings().internal_debug; super::logging::set_lsp_debug_flag(internal_debug) @@ -1069,8 +1025,7 @@ impl Inner { fn walk_workspace(config: &Config) -> (BTreeSet, bool) { let mut workspace_files = Default::default(); - let document_preload_limit = - config.workspace_settings().document_preload_limit; + let entry_limit = 1000; let mut pending = VecDeque::new(); let mut entry_count = 0; let mut roots = config @@ -1091,7 +1046,7 @@ impl Inner { let Ok(entry) = entry else { continue; }; - if entry_count >= document_preload_limit { + if entry_count >= entry_limit { return (workspace_files, true); } entry_count += 1; @@ -3545,7 +3500,7 @@ impl Inner { vec![referrer.clone()] }; let workspace_settings = self.config.workspace_settings(); - let mut cli_options = CliOptions::new( + let cli_options = CliOptions::new( Flags { cache_path: self.maybe_global_cache_path.clone(), ca_stores: workspace_settings.certificate_stores.clone(), @@ -3553,6 +3508,12 @@ impl Inner { unsafely_ignore_certificate_errors: workspace_settings .unsafely_ignore_certificate_errors .clone(), + import_map_path: config_data.as_ref().and_then(|d| { + if d.import_map_from_settings { + return Some(d.import_map.as_ref()?.base_url().to_string()); + } + None + }), node_modules_dir: Some( config_data .as_ref() @@ -3573,11 +3534,6 @@ impl Inner { .and_then(|d| d.package_json.as_deref().cloned()), force_global_cache, )?; - // don't use the specifier in self.maybe_import_map because it's not - // necessarily an import map specifier (could be a deno.json) - if let Some(import_map) = self.resolve_import_map_specifier(&referrer)? { - cli_options.set_import_map_specifier(Some(import_map)); - } let open_docs = self.documents.documents(DocumentsFilter::OpenDiagnosable); Ok(Some(PrepareCacheResult { diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 5a81244fd4..91608d53c1 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -2568,6 +2568,70 @@ fn lsp_rename_synbol_file_scheme_edits_only() { client.shutdown(); } +// Regression test for https://github.com/denoland/deno/issues/23121. +#[test] +fn lsp_document_preload_limit_zero_deno_json_detection() { + let context = TestContextBuilder::new() + .use_http_server() + .use_temp_cwd() + .build(); + let temp_dir = context.temp_dir(); + temp_dir.write("deno.json", json!({}).to_string()); + let mut client = context.new_lsp_command().build(); + client.initialize(|builder| { + builder.set_preload_limit(0); + }); + let res = client + .read_notification_with_method::("deno/didChangeDenoConfiguration"); + assert_eq!( + res, + Some(json!({ + "changes": [{ + "scopeUri": temp_dir.uri(), + "fileUri": temp_dir.uri().join("deno.json").unwrap(), + "type": "added", + "configurationType": "denoJson", + }], + })) + ); + client.shutdown(); +} + +// Regression test for https://github.com/denoland/deno/issues/23141. +#[test] +fn lsp_import_map_setting_with_deno_json() { + let context = TestContextBuilder::new() + .use_http_server() + .use_temp_cwd() + .build(); + let temp_dir = context.temp_dir(); + temp_dir.write("deno.json", json!({}).to_string()); + temp_dir.write( + "import_map.json", + json!({ + "imports": { + "file2": "./file2.ts", + }, + }) + .to_string(), + ); + temp_dir.write("file2.ts", ""); + let mut client = context.new_lsp_command().build(); + client.initialize(|builder| { + builder.set_import_map("import_map.json"); + }); + let diagnostics = client.did_open(json!({ + "textDocument": { + "uri": "file:///a/file.ts", + "languageId": "typescript", + "version": 1, + "text": "import \"file2\";\n", + }, + })); + assert_eq!(json!(diagnostics.all()), json!([])); + client.shutdown(); +} + #[test] fn lsp_hover_typescript_types() { let context = TestContextBuilder::new()