diff options
| -rw-r--r-- | cli/lsp/config.rs | 76 | ||||
| -rw-r--r-- | cli/lsp/language_server.rs | 79 | ||||
| -rw-r--r-- | cli/tests/integration/lsp_tests.rs | 66 | ||||
| -rw-r--r-- | test_util/src/builders.rs | 15 | ||||
| -rw-r--r-- | test_util/src/fs.rs | 4 | ||||
| -rw-r--r-- | test_util/src/lsp.rs | 10 |
6 files changed, 204 insertions, 46 deletions
diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index 03464d1f8..2438e8a85 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -425,12 +425,19 @@ pub struct Settings { pub workspace: WorkspaceSettings, } +#[derive(Debug)] +struct WithCanonicalizedSpecifier<T> { + /// Stored canonicalized specifier, which is used for file watcher events. + canonicalized_specifier: ModuleSpecifier, + file: T, +} + /// Contains the config file and dependent information. #[derive(Debug)] struct LspConfigFileInfo { - config_file: ConfigFile, + config_file: WithCanonicalizedSpecifier<ConfigFile>, /// An optional deno.lock file, which is resolved relative to the config file. - maybe_lockfile: Option<Arc<Mutex<Lockfile>>>, + maybe_lockfile: Option<WithCanonicalizedSpecifier<Arc<Mutex<Lockfile>>>>, /// The canonicalized node_modules directory, which is found relative to the config file. maybe_node_modules_dir: Option<PathBuf>, excluded_paths: Vec<Url>, @@ -469,14 +476,42 @@ impl Config { } pub fn maybe_config_file(&self) -> Option<&ConfigFile> { - self.maybe_config_file_info.as_ref().map(|c| &c.config_file) + self + .maybe_config_file_info + .as_ref() + .map(|c| &c.config_file.file) + } + + /// Canonicalized specifier of the config file, which should only be used for + /// file watcher events. Otherwise, prefer using the non-canonicalized path + /// as the rest of the CLI does for config files. + pub fn maybe_config_file_canonicalized_specifier( + &self, + ) -> Option<&ModuleSpecifier> { + self + .maybe_config_file_info + .as_ref() + .map(|c| &c.config_file.canonicalized_specifier) } pub fn maybe_lockfile(&self) -> Option<&Arc<Mutex<Lockfile>>> { self .maybe_config_file_info .as_ref() - .and_then(|c| c.maybe_lockfile.as_ref()) + .and_then(|c| c.maybe_lockfile.as_ref().map(|l| &l.file)) + } + + /// Canonicalized specifier of the lockfile, which should only be used for + /// file watcher events. Otherwise, prefer using the non-canonicalized path + /// as the rest of the CLI does for config files. + pub fn maybe_lockfile_canonicalized_specifier( + &self, + ) -> Option<&ModuleSpecifier> { + self.maybe_config_file_info.as_ref().and_then(|c| { + c.maybe_lockfile + .as_ref() + .map(|l| &l.canonicalized_specifier) + }) } pub fn clear_config_file(&mut self) { @@ -485,7 +520,17 @@ impl Config { pub fn set_config_file(&mut self, config_file: ConfigFile) { self.maybe_config_file_info = Some(LspConfigFileInfo { - maybe_lockfile: resolve_lockfile_from_config(&config_file), + maybe_lockfile: resolve_lockfile_from_config(&config_file).map( + |lockfile| { + let path = canonicalize_path_maybe_not_exists(&lockfile.filename) + .unwrap_or_else(|_| lockfile.filename.clone()); + WithCanonicalizedSpecifier { + canonicalized_specifier: ModuleSpecifier::from_file_path(path) + .unwrap(), + file: Arc::new(Mutex::new(lockfile)), + } + }, + ), maybe_node_modules_dir: resolve_node_modules_dir(&config_file), excluded_paths: config_file .to_files_config() @@ -496,7 +541,16 @@ impl Config { .into_iter() .filter_map(|path| ModuleSpecifier::from_file_path(path).ok()) .collect(), - config_file, + config_file: WithCanonicalizedSpecifier { + canonicalized_specifier: config_file + .specifier + .to_file_path() + .ok() + .and_then(|p| canonicalize_path_maybe_not_exists(&p).ok()) + .and_then(|p| ModuleSpecifier::from_file_path(p).ok()) + .unwrap_or_else(|| config_file.specifier.clone()), + file: config_file, + }, }); } @@ -739,9 +793,7 @@ fn specifier_enabled( .unwrap_or_else(|| settings.workspace.enable) } -fn resolve_lockfile_from_config( - config_file: &ConfigFile, -) -> Option<Arc<Mutex<Lockfile>>> { +fn resolve_lockfile_from_config(config_file: &ConfigFile) -> Option<Lockfile> { let lockfile_path = match config_file.resolve_lockfile_path() { Ok(Some(value)) => value, Ok(None) => return None, @@ -769,15 +821,13 @@ fn resolve_node_modules_dir(config_file: &ConfigFile) -> Option<PathBuf> { canonicalize_path_maybe_not_exists(&node_modules_dir).ok() } -fn resolve_lockfile_from_path( - lockfile_path: PathBuf, -) -> Option<Arc<Mutex<Lockfile>>> { +fn resolve_lockfile_from_path(lockfile_path: PathBuf) -> Option<Lockfile> { match Lockfile::new(lockfile_path, false) { Ok(value) => { if let Ok(specifier) = ModuleSpecifier::from_file_path(&value.filename) { lsp_log!(" Resolved lock file: \"{}\"", specifier); } - Some(Arc::new(Mutex::new(value))) + Some(value) } Err(err) => { lsp_warn!("Error loading lockfile: {:#}", err); diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 5a4c3aeae..3ac9610b3 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -1458,18 +1458,8 @@ impl Inner { &mut self, params: DidChangeWatchedFilesParams, ) { - fn has_lockfile_changed( - lockfile: &Lockfile, - changed_urls: &HashSet<Url>, - ) -> bool { - let lockfile_path = lockfile.filename.clone(); - let Ok(specifier) = ModuleSpecifier::from_file_path(&lockfile_path) else { - return false; - }; - if !changed_urls.contains(&specifier) { - return false; - } - match Lockfile::new(lockfile_path, false) { + fn has_lockfile_content_changed(lockfile: &Lockfile) -> bool { + match Lockfile::new(lockfile.filename.clone(), false) { Ok(new_lockfile) => { // only update if the lockfile has changed FastInsecureHasher::hash(lockfile) @@ -1482,6 +1472,53 @@ impl Inner { } } + fn has_config_changed(config: &Config, changes: &HashSet<Url>) -> bool { + // Check the canonicalized specifier here because file watcher + // changes will be for the canonicalized path in vscode, but also check the + // non-canonicalized specifier in order to please the tests and handle + // a client that might send that instead. + if config + .maybe_config_file_canonicalized_specifier() + .map(|s| changes.contains(s)) + .unwrap_or(false) + { + return true; + } + match config.maybe_config_file() { + Some(file) => { + if changes.contains(&file.specifier) { + return true; + } + } + None => { + // check for auto-discovery + if changes.iter().any(|url| { + url.path().ends_with("/deno.json") + || url.path().ends_with("/deno.jsonc") + }) { + return true; + } + } + } + + // if the lockfile has changed, reload the config as well + if let Some(lockfile) = config.maybe_lockfile() { + let lockfile_matches = config + .maybe_lockfile_canonicalized_specifier() + .map(|s| changes.contains(s)) + .or_else(|| { + ModuleSpecifier::from_file_path(&lockfile.lock().filename) + .ok() + .map(|s| changes.contains(&s)) + }) + .unwrap_or(false); + lockfile_matches && has_lockfile_content_changed(&lockfile.lock()) + } else { + // check for auto-discovery + changes.iter().any(|url| url.path().ends_with("/deno.lock")) + } + } + let mark = self .performance .mark("did_change_watched_files", Some(¶ms)); @@ -1493,23 +1530,7 @@ impl Inner { .collect(); // if the current deno.json has changed, we need to reload it - let has_config_changed = match self.config.maybe_config_file() { - Some(config_file) => changes.contains(&config_file.specifier), - None => { - // check for auto-discovery - changes.iter().any(|url| { - url.path().ends_with("/deno.json") - || url.path().ends_with("/deno.jsonc") - }) - } - } || match self.config.maybe_lockfile() { - Some(lockfile) => has_lockfile_changed(&lockfile.lock(), &changes), - None => { - // check for auto-discovery - changes.iter().any(|url| url.path().ends_with("/deno.lock")) - } - }; - if has_config_changed { + if has_config_changed(&self.config, &changes) { if let Err(err) = self.update_config_file().await { self.client.show_message(MessageType::WARNING, err); } diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index 261c98b52..dc68ab514 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -545,6 +545,72 @@ fn lsp_import_map_config_file_auto_discovered() { } #[test] +fn lsp_import_map_config_file_auto_discovered_symlink() { + let context = TestContextBuilder::new() + // DO NOT COPY THIS CODE. Very rare case where we want to force the temp + // directory on the CI to not be a symlinked directory because we are + // testing a scenario with a symlink to a non-symlink in the same directory + // tree. Generally you want to ensure your code works in symlinked directories + // so don't use this unless you have a similar scenario. + .temp_dir_path(std::env::temp_dir().canonicalize().unwrap()) + .use_temp_cwd() + .build(); + let temp_dir = context.temp_dir(); + temp_dir.create_dir_all("lib"); + temp_dir.write("lib/b.ts", r#"export const b = "b";"#); + + let mut client = context.new_lsp_command().capture_stderr().build(); + client.initialize_default(); + + // now create a symlink in the current directory to a subdir/deno.json + // and ensure the watched files notification still works + temp_dir.create_dir_all("subdir"); + temp_dir.write("subdir/deno.json", r#"{ }"#); + temp_dir.symlink_file( + temp_dir.path().join("subdir").join("deno.json"), + temp_dir.path().join("deno.json"), + ); + client.did_change_watched_files(json!({ + "changes": [{ + // the client will give a watched file changed event for the symlink's target + "uri": temp_dir.path().join("subdir/deno.json").canonicalize().uri(), + "type": 2 + }] + })); + + // this will discover the deno.json in the root + let search_line = format!( + "Auto-resolved configuration file: \"{}\"", + temp_dir.uri().join("deno.json").unwrap().as_str() + ); + client.wait_until_stderr_line(|line| line.contains(&search_line)); + + // now open a file which will cause a diagnostic because the import map is empty + let diagnostics = client.did_open(json!({ + "textDocument": { + "uri": temp_dir.uri().join("a.ts").unwrap(), + "languageId": "typescript", + "version": 1, + "text": "import { b } from \"/~/b.ts\";\n\nconsole.log(b);\n" + } + })); + assert_eq!(diagnostics.all().len(), 1); + + // update the import map to have the imports now + temp_dir.write("subdir/deno.json", r#"{ "imports": { "/~/": "./lib/" } }"#); + client.did_change_watched_files(json!({ + "changes": [{ + // now still say that the target path has changed + "uri": temp_dir.path().join("subdir/deno.json").canonicalize().uri(), + "type": 2 + }] + })); + assert_eq!(client.read_diagnostics().all().len(), 0); + + client.shutdown(); +} + +#[test] fn lsp_deno_task() { let context = TestContextBuilder::new().use_temp_cwd().build(); let temp_dir = context.temp_dir(); diff --git a/test_util/src/builders.rs b/test_util/src/builders.rs index 48c001a7e..bdc5efbe0 100644 --- a/test_util/src/builders.rs +++ b/test_util/src/builders.rs @@ -19,7 +19,6 @@ use crate::env_vars_for_npm_tests_no_sync_download; use crate::fs::PathRef; use crate::http_server; use crate::lsp::LspClientBuilder; -use crate::new_deno_dir; use crate::pty::Pty; use crate::strip_ansi_codes; use crate::testdata_path; @@ -36,6 +35,7 @@ pub struct TestContextBuilder { /// to the temp folder and runs the test from there. This is useful when /// the test creates files in the testdata directory (ex. a node_modules folder) copy_temp_dir: Option<String>, + temp_dir_path: Option<PathBuf>, cwd: Option<String>, envs: HashMap<String, String>, deno_exe: Option<PathRef>, @@ -50,6 +50,11 @@ impl TestContextBuilder { Self::new().use_http_server().add_npm_env_vars() } + pub fn temp_dir_path(mut self, path: impl AsRef<Path>) -> Self { + self.temp_dir_path = Some(path.as_ref().to_path_buf()); + self + } + pub fn use_http_server(mut self) -> Self { self.use_http_server = true; self @@ -117,9 +122,13 @@ impl TestContextBuilder { } pub fn build(&self) -> TestContext { - let deno_dir = new_deno_dir(); // keep this alive for the test + let temp_dir_path = self + .temp_dir_path + .clone() + .unwrap_or_else(std::env::temp_dir); + let deno_dir = TempDir::new_in(&temp_dir_path); let temp_dir = if self.use_separate_deno_dir { - TempDir::new() + TempDir::new_in(&temp_dir_path) } else { deno_dir.clone() }; diff --git a/test_util/src/fs.rs b/test_util/src/fs.rs index f9443c9c5..2a64fb9c4 100644 --- a/test_util/src/fs.rs +++ b/test_util/src/fs.rs @@ -324,6 +324,10 @@ impl TempDir { Self::new_inner(&std::env::temp_dir(), Some(prefix)) } + pub fn new_in(parent_dir: &Path) -> Self { + Self::new_inner(parent_dir, None) + } + pub fn new_with_path(path: &Path) -> Self { Self(Arc::new(TempDirInner::Path(PathRef(path.to_path_buf())))) } diff --git a/test_util/src/lsp.rs b/test_util/src/lsp.rs index 6fafc234c..59f67e3d9 100644 --- a/test_util/src/lsp.rs +++ b/test_util/src/lsp.rs @@ -641,16 +641,24 @@ impl LspClient { .stderr_lines_rx .as_ref() .expect("must setup with client_builder.capture_stderr()"); + let mut found_lines = Vec::new(); while Instant::now() < timeout_time { if let Ok(line) = lines_rx.try_recv() { if condition(&line) { return; } + found_lines.push(line); } std::thread::sleep(Duration::from_millis(20)); } - panic!("Timed out.") + eprintln!("==== STDERR OUTPUT ===="); + for line in found_lines { + eprintln!("{}", line) + } + eprintln!("== END STDERR OUTPUT =="); + + panic!("Timed out waiting on condition.") } pub fn initialize_default(&mut self) { |