From b7456fed703b148ed627b80afc3ec4c01e1eafef Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Wed, 29 Jan 2025 16:25:32 -0800 Subject: [PATCH] fix(lsp): ignore errors on ambient module imports (#27855) This makes it so imports of ambient modules (e.g. `$app/environment` in svelte, any virtual module in vite, or other module provided by a bundler) don't error in the LSP. The way this works is that when we request diagnostics from TSC, we also respond with the list of ambient modules. Then, in the diagnostics code, we save diagnostics (produced by deno) that may be invalidated as an ambient module and wait to publish the diagnostics until we've received the ambient modules from TSC. The actual ambient modules you get from TSC can contain globs, e.g. `*.css`. So when we get new ambient modules, we compile them all into a regex and check erroring imports against that regex. Ambient modules should change rarely, so in most cases we should be using a pre-compiled regex, which executes in linear time (wrt the specifier length). TODO: - Ideally we should only publish once, right now we publish with the filtered specifiers and then the TSC ones - deno check (#27633) --- cli/lsp/diagnostics.rs | 451 +++++++++++++++++++++++++++------ cli/lsp/tsc.rs | 44 +++- cli/tsc/98_lsp.js | 45 +++- tests/integration/lsp_tests.rs | 61 ++++- 4 files changed, 499 insertions(+), 102 deletions(-) diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 126e8ef01d..abd3574d98 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -11,6 +11,7 @@ use deno_ast::MediaType; use deno_config::deno_json::LintConfig; use deno_core::anyhow::anyhow; use deno_core::error::AnyError; +use deno_core::parking_lot::Mutex; use deno_core::parking_lot::RwLock; use deno_core::resolve_url; use deno_core::serde::Deserialize; @@ -38,7 +39,7 @@ use import_map::ImportMap; use import_map::ImportMapErrorKind; use log::error; use tokio::sync::mpsc; -use tokio::sync::Mutex; +use tokio::sync::Mutex as AsyncMutex; use tokio::time::Duration; use tokio_util::sync::CancellationToken; use tower_lsp::lsp_types as lsp; @@ -54,12 +55,14 @@ use super::language_server; use super::language_server::StateSnapshot; use super::performance::Performance; use super::tsc; +use super::tsc::ScopedAmbientModules; use super::tsc::TsServer; use super::urls::uri_parse_unencoded; use super::urls::url_to_uri; use super::urls::LspUrlMap; use crate::graph_util; use crate::graph_util::enhanced_resolution_error_message; +use crate::lsp::logging::lsp_warn; use crate::lsp::lsp_custom::DiagnosticBatchNotificationParams; use crate::resolver::CliSloppyImportsResolver; use crate::sys::CliSys; @@ -94,6 +97,7 @@ pub enum DiagnosticSource { Deno, Lint, Ts, + DeferredDeno, } impl DiagnosticSource { @@ -102,6 +106,7 @@ impl DiagnosticSource { Self::Deno => "deno", Self::Lint => "deno-lint", Self::Ts => "deno-ts", + Self::DeferredDeno => "deno", } } } @@ -113,7 +118,7 @@ struct DiagnosticsPublisher { client: Client, state: Arc, diagnostics_by_specifier: - Mutex>, + AsyncMutex>, } impl DiagnosticsPublisher { @@ -382,7 +387,117 @@ impl DiagnosticsState { } } -#[derive(Debug)] +#[derive(Debug, Default)] +struct AmbientModules { + regex: Option, + dirty: bool, +} + +#[derive(Debug, Default)] +struct DeferredDiagnostics { + diagnostics: Option>, + ambient_modules_by_scope: HashMap, AmbientModules>, +} + +impl DeferredDiagnostics { + fn invalidate(&mut self, specifiers: &[ModuleSpecifier]) { + if let Some(diagnostics) = &mut self.diagnostics { + diagnostics.retain(|d| !specifiers.contains(&d.document_specifier)); + } + for ambient in self.ambient_modules_by_scope.values_mut() { + ambient.dirty = true; + } + } + fn invalidate_all(&mut self) { + self.diagnostics = None; + for ambient in self.ambient_modules_by_scope.values_mut() { + ambient.dirty = true; + } + } + + fn take_filtered_diagnostics(&mut self) -> Option { + let diagnostics = self.diagnostics.take()?; + for diagnostic in &diagnostics { + let Some(ambient) = self.ambient_modules_by_scope.get(&diagnostic.scope) + else { + self.diagnostics = Some(diagnostics); + return None; + }; + if ambient.dirty { + self.diagnostics = Some(diagnostics); + return None; + } + } + + Some( + diagnostics + .into_iter() + .map(|diagnostic| { + let ambient = self + .ambient_modules_by_scope + .get(&diagnostic.scope) + .unwrap(); // checked above, but gross + let filtered = if let Some(regex) = &ambient.regex { + diagnostic + .diagnostics + .into_iter() + .filter_map(|(import_url, diag)| { + if regex.is_match(import_url.as_str()) { + None + } else { + Some(diag) + } + }) + .collect() + } else { + diagnostic.diagnostics.into_iter().map(|d| d.1).collect() + }; + DiagnosticRecord { + specifier: diagnostic.document_specifier, + versioned: VersionedDiagnostics { + version: diagnostic.version, + diagnostics: filtered, + }, + } + }) + .collect(), + ) + } + + fn update_ambient_modules(&mut self, new: ScopedAmbientModules) { + for (scope, value) in new { + let ambient = self.ambient_modules_by_scope.entry(scope).or_default(); + ambient.dirty = false; + if let Some(value) = value { + if value.is_empty() { + ambient.regex = None; + continue; + } + let mut regex_string = String::with_capacity(value.len() * 8); + regex_string.push('('); + let last = value.len() - 1; + for (idx, part) in value.into_iter().enumerate() { + let trimmed = part.trim_matches('"'); + let escaped = regex::escape(trimmed); + let regex = escaped.replace("\\*", ".*"); + regex_string.push_str(®ex); + if idx != last { + regex_string.push('|'); + } + } + regex_string.push_str(")$"); + if let Ok(regex) = regex::Regex::new(®ex_string).inspect_err(|e| { + lsp_warn!("failed to compile ambient modules pattern: {e} (pattern is {regex_string:?})"); + }) { + ambient.regex = Some(regex); + } else { + ambient.regex = None; + } + } + } + } +} + pub struct DiagnosticsServer { channel: Option>, ts_diagnostics: TsDiagnosticsStore, @@ -391,6 +506,22 @@ pub struct DiagnosticsServer { ts_server: Arc, batch_counter: DiagnosticBatchCounter, state: Arc, + deferred_diagnostics: Arc>, +} + +impl std::fmt::Debug for DiagnosticsServer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("DiagnosticsServer") + .field("channel", &self.channel) + .field("ts_diagnostics", &self.ts_diagnostics) + .field("client", &self.client) + .field("performance", &self.performance) + .field("ts_server", &self.ts_server) + .field("batch_counter", &self.batch_counter) + .field("state", &self.state) + .field("deferred_diagnostics", &*self.deferred_diagnostics.lock()) + .finish() + } } impl DiagnosticsServer { @@ -408,6 +539,9 @@ impl DiagnosticsServer { ts_server, batch_counter: Default::default(), state, + deferred_diagnostics: Arc::new( + Mutex::new(DeferredDiagnostics::default()), + ), } } @@ -421,10 +555,12 @@ impl DiagnosticsServer { pub fn invalidate(&self, specifiers: &[ModuleSpecifier]) { self.ts_diagnostics.invalidate(specifiers); + self.deferred_diagnostics.lock().invalidate(specifiers); } pub fn invalidate_all(&self) { self.ts_diagnostics.invalidate_all(); + self.deferred_diagnostics.lock().invalidate_all(); if let Some(tx) = &self.channel { let _ = tx.send(ChannelMessage::Clear); } @@ -439,6 +575,7 @@ impl DiagnosticsServer { let performance = self.performance.clone(); let ts_diagnostics_store = self.ts_diagnostics.clone(); let ts_server = self.ts_server.clone(); + let deferred_diagnostics_state = self.deferred_diagnostics.clone(); let _join_handle = thread::spawn(move || { let runtime = create_basic_runtime(); @@ -485,6 +622,8 @@ impl DiagnosticsServer { let snapshot = snapshot.clone(); let config = snapshot.config.clone(); let url_map = url_map.clone(); + let deferred_diagnostics_state = + deferred_diagnostics_state.clone(); async move { if let Some(previous_handle) = previous_ts_handle { // Wait on the previous run to complete in order to prevent @@ -504,28 +643,49 @@ impl DiagnosticsServer { }; let mark = performance.mark("lsp.update_diagnostics_ts"); - let diagnostics = generate_ts_diagnostics( - snapshot.clone(), - &config, - &ts_server, - token.clone(), - ) - .await - .map_err(|err| { - if !token.is_cancelled() { - error!( - "Error generating TypeScript diagnostics: {}", - err - ); - token.cancel(); - } - }) - .unwrap_or_default(); - + let (diagnostics, ambient_modules_by_scope) = + generate_ts_diagnostics( + snapshot.clone(), + &config, + &ts_server, + token.clone(), + ) + .await + .map_err(|err| { + if !token.is_cancelled() { + error!( + "Error generating TypeScript diagnostics: {}", + err + ); + token.cancel(); + } + }) + .unwrap_or_default(); + deferred_diagnostics_state + .lock() + .update_ambient_modules(ambient_modules_by_scope); let mut messages_len = 0; if !token.is_cancelled() { ts_diagnostics_store.update(&diagnostics); - messages_len = diagnostics_publisher + { + let value = { + let mut deferred_diagnostics_state = + deferred_diagnostics_state.lock(); + deferred_diagnostics_state.take_filtered_diagnostics() + }; + if let Some(deferred) = value { + messages_len += diagnostics_publisher + .publish( + DiagnosticSource::DeferredDeno, + deferred, + &url_map, + snapshot.documents.as_ref(), + &token, + ) + .await; + } + } + messages_len += diagnostics_publisher .publish( DiagnosticSource::Ts, diagnostics, @@ -561,12 +721,14 @@ impl DiagnosticsServer { let snapshot = snapshot.clone(); let config = snapshot.config.clone(); let url_map = url_map.clone(); + let deferred_diagnostics_state = + deferred_diagnostics_state.clone(); async move { if let Some(previous_handle) = previous_deps_handle { previous_handle.await; } let mark = performance.mark("lsp.update_diagnostics_deps"); - let diagnostics = spawn_blocking({ + let (diagnostics, deferred) = spawn_blocking({ let token = token.clone(); let snapshot = snapshot.clone(); move || generate_deno_diagnostics(&snapshot, &config, token) @@ -576,7 +738,27 @@ impl DiagnosticsServer { let mut messages_len = 0; if !token.is_cancelled() { - messages_len = diagnostics_publisher + { + let value = { + let mut deferred_diagnostics_state = + deferred_diagnostics_state.lock(); + deferred_diagnostics_state.diagnostics = Some(deferred); + deferred_diagnostics_state.take_filtered_diagnostics() + }; + if let Some(deferred) = value { + messages_len += diagnostics_publisher + .publish( + DiagnosticSource::DeferredDeno, + deferred, + &url_map, + snapshot.documents.as_ref(), + &token, + ) + .await; + } + } + + messages_len += diagnostics_publisher .publish( DiagnosticSource::Deno, diagnostics, @@ -891,7 +1073,7 @@ async fn generate_ts_diagnostics( config: &Config, ts_server: &tsc::TsServer, token: CancellationToken, -) -> Result { +) -> Result<(DiagnosticVec, ScopedAmbientModules), AnyError> { let mut diagnostics_vec = Vec::new(); let specifiers = snapshot .documents @@ -901,13 +1083,14 @@ async fn generate_ts_diagnostics( let (enabled_specifiers, disabled_specifiers) = specifiers .into_iter() .partition::, _>(|s| config.specifier_enabled(s)); - let ts_diagnostics_map = if !enabled_specifiers.is_empty() { - ts_server - .get_diagnostics(snapshot.clone(), enabled_specifiers, token) - .await? - } else { - Default::default() - }; + let (ts_diagnostics_map, ambient_modules_by_scope) = + if !enabled_specifiers.is_empty() { + ts_server + .get_diagnostics(snapshot.clone(), enabled_specifiers, token) + .await? + } else { + Default::default() + }; for (specifier_str, mut ts_json_diagnostics) in ts_diagnostics_map { let specifier = resolve_url(&specifier_str)?; let suggestion_actions_settings = snapshot @@ -958,7 +1141,7 @@ async fn generate_ts_diagnostics( }, }); } - Ok(diagnostics_vec) + Ok((diagnostics_vec, ambient_modules_by_scope)) } #[derive(Debug, Deserialize)] @@ -1357,7 +1540,7 @@ fn diagnose_resolution( maybe_assert_type: Option<&str>, referrer_doc: &Document, import_map: Option<&ImportMap>, -) -> Vec { +) -> (Vec, Vec) { fn check_redirect_diagnostic( specifier: &ModuleSpecifier, doc: &Document, @@ -1386,6 +1569,7 @@ fn diagnose_resolution( } let mut diagnostics = vec![]; + let mut deferred_diagnostics = vec![]; match resolution { Resolution::Ok(resolved) => { let file_referrer = referrer_doc.file_referrer(); @@ -1477,21 +1661,30 @@ fn diagnose_resolution( // When the document is not available, it means that it cannot be found // in the cache or locally on the disk, so we want to issue a diagnostic // about that. + // these may be invalid, however, if this is an ambient module with + // no real source (as in the case of a virtual module). let deno_diagnostic = match specifier.scheme() { "file" => DenoDiagnostic::NoLocal(specifier.clone()), _ => DenoDiagnostic::NoCache(specifier.clone()), }; - diagnostics.push(deno_diagnostic); + deferred_diagnostics.push(deno_diagnostic); } } // The specifier resolution resulted in an error, so we want to issue a // diagnostic for that. - Resolution::Err(err) => { - diagnostics.push(DenoDiagnostic::ResolutionError(*err.clone())) - } + Resolution::Err(err) => match &**err { + ResolutionError::InvalidSpecifier { + error: SpecifierError::ImportPrefixMissing { .. }, + .. + } => { + deferred_diagnostics + .push(DenoDiagnostic::ResolutionError(*err.clone())); + } + _ => diagnostics.push(DenoDiagnostic::ResolutionError(*err.clone())), + }, _ => (), } - diagnostics + (diagnostics, deferred_diagnostics) } /// Generate diagnostics related to a dependency. The dependency is analyzed to @@ -1499,6 +1692,7 @@ fn diagnose_resolution( /// any diagnostics related to the resolved code or type dependency. fn diagnose_dependency( diagnostics: &mut Vec, + deferred_diagnostics: &mut Vec<(String, lsp::Diagnostic)>, snapshot: &language_server::StateSnapshot, referrer_doc: &Document, dependency_key: &str, @@ -1551,33 +1745,55 @@ fn diagnose_dependency( .is_some() }); - diagnostics.extend( - diagnose_resolution( - snapshot, - dependency_key, - if dependency.maybe_code.is_none() + let (resolution_diagnostics, deferred) = diagnose_resolution( + snapshot, + dependency_key, + if dependency.maybe_code.is_none() // If not @ts-types, diagnose the types if the code errored because // it's likely resolving into the node_modules folder, which might be // erroring correctly due to resolution only being for bundlers. Let this // fail at runtime if necessary, but don't bother erroring in the editor || !is_types_deno_types && matches!(dependency.maybe_type, Resolution::Ok(_)) && matches!(dependency.maybe_code, Resolution::Err(_)) - { - &dependency.maybe_type - } else { - &dependency.maybe_code - }, - dependency.is_dynamic, - dependency.maybe_attribute_type.as_deref(), - referrer_doc, - import_map, - ) - .iter() - .flat_map(|diag| { - import_ranges - .iter() - .map(|range| diag.to_lsp_diagnostic(range)) - }), + { + &dependency.maybe_type + } else { + &dependency.maybe_code + }, + dependency.is_dynamic, + dependency.maybe_attribute_type.as_deref(), + referrer_doc, + import_map, + ); + diagnostics.extend(resolution_diagnostics.iter().flat_map(|diag| { + import_ranges + .iter() + .map(|range| diag.to_lsp_diagnostic(range)) + })); + deferred_diagnostics.extend( + deferred + .iter() + .filter_map(|diag| match diag { + DenoDiagnostic::NoCache(url) | DenoDiagnostic::NoLocal(url) => Some( + Box::new( + import_ranges + .iter() + .map(|range| (url.to_string(), diag.to_lsp_diagnostic(range))), + ) as Box>, + ), + DenoDiagnostic::ResolutionError( + ResolutionError::InvalidSpecifier { + error: SpecifierError::ImportPrefixMissing { specifier, .. }, + .. + }, + ) => { + Some(Box::new(import_ranges.iter().map(|range| { + (specifier.clone(), diag.to_lsp_diagnostic(range)) + }))) + } + _ => None, + }) + .flatten(), ); if is_types_deno_types { @@ -1586,22 +1802,45 @@ fn diagnose_dependency( Resolution::Err(error) => documents::to_lsp_range(error.range()), Resolution::None => unreachable!(), }; - diagnostics.extend( - diagnose_resolution( - snapshot, - dependency_key, - &dependency.maybe_type, - dependency.is_dynamic, - dependency.maybe_attribute_type.as_deref(), - referrer_doc, - import_map, - ) - .iter() - .map(|diag| diag.to_lsp_diagnostic(&range)), + let (resolution_diagnostics, deferred) = diagnose_resolution( + snapshot, + dependency_key, + &dependency.maybe_type, + dependency.is_dynamic, + dependency.maybe_attribute_type.as_deref(), + referrer_doc, + import_map, ); + diagnostics.extend( + resolution_diagnostics + .iter() + .map(|diag| diag.to_lsp_diagnostic(&range)), + ); + deferred_diagnostics.extend(Box::new(deferred.iter().filter_map(|diag| { + match diag { + DenoDiagnostic::NoCache(url) | DenoDiagnostic::NoLocal(url) => { + Some((url.to_string(), diag.to_lsp_diagnostic(&range))) + } + DenoDiagnostic::ResolutionError( + ResolutionError::InvalidSpecifier { + error: SpecifierError::ImportPrefixMissing { specifier, .. }, + .. + }, + ) => Some((specifier.clone(), diag.to_lsp_diagnostic(&range))), + _ => None, + } + }))); } } +#[derive(Debug)] +struct DeferredDiagnosticRecord { + document_specifier: Url, + version: Option, + scope: Option, + diagnostics: Vec<(String, lsp::Diagnostic)>, +} + /// Generate diagnostics that come from Deno module resolution logic (like /// dependencies) or other Deno specific diagnostics, like the ability to use /// an import map to shorten an URL. @@ -1609,9 +1848,9 @@ fn generate_deno_diagnostics( snapshot: &language_server::StateSnapshot, config: &Config, token: CancellationToken, -) -> DiagnosticVec { +) -> (DiagnosticVec, Vec) { let mut diagnostics_vec = Vec::new(); - + let mut deferred_diagnostics = Vec::new(); for document in snapshot .documents .documents(DocumentsFilter::OpenDiagnosable) @@ -1620,11 +1859,13 @@ fn generate_deno_diagnostics( break; } let mut diagnostics = Vec::new(); + let mut deferred = Vec::new(); let specifier = document.specifier(); if config.specifier_enabled(specifier) { for (dependency_key, dependency) in document.dependencies() { diagnose_dependency( &mut diagnostics, + &mut deferred, snapshot, &document, dependency_key, @@ -1639,9 +1880,22 @@ fn generate_deno_diagnostics( diagnostics, }, }); + deferred_diagnostics.push(DeferredDiagnosticRecord { + document_specifier: specifier.clone(), + scope: if snapshot.documents.is_valid_file_referrer(specifier) { + snapshot.config.tree.scope_for_specifier(specifier).cloned() + } else { + snapshot + .documents + .get(specifier) + .and_then(|d| d.scope().cloned()) + }, + version: document.maybe_lsp_version(), + diagnostics: deferred, + }); } - diagnostics_vec + (diagnostics_vec, deferred_diagnostics) } #[cfg(test)] @@ -1759,9 +2013,10 @@ let c: number = "a"; Default::default(), ) .await - .unwrap(); + .unwrap() + .0; assert_eq!(get_diagnostics_for_single(diagnostics).len(), 4); - let diagnostics = generate_deno_diagnostics( + let diagnostics = generate_all_deno_diagnostics( &snapshot, &enabled_config, Default::default(), @@ -1793,9 +2048,10 @@ let c: number = "a"; Default::default(), ) .await - .unwrap(); + .unwrap() + .0; assert_eq!(get_diagnostics_for_single(diagnostics).len(), 0); - let diagnostics = generate_deno_diagnostics( + let diagnostics = generate_all_deno_diagnostics( &snapshot, &disabled_config, Default::default(), @@ -1819,6 +2075,37 @@ let c: number = "a"; .diagnostics } + fn generate_all_deno_diagnostics( + snapshot: &StateSnapshot, + config: &Config, + token: CancellationToken, + ) -> DiagnosticVec { + let (diagnostics, deferred) = + generate_deno_diagnostics(snapshot, config, token); + + let mut all_diagnostics = diagnostics + .into_iter() + .map(|d| (d.specifier.clone(), d)) + .collect::>(); + for diag in deferred { + let existing = all_diagnostics + .entry(diag.document_specifier.clone()) + .or_insert_with(|| DiagnosticRecord { + specifier: diag.document_specifier.clone(), + versioned: VersionedDiagnostics { + diagnostics: vec![], + version: diag.version, + }, + }); + existing + .versioned + .diagnostics + .extend(diag.diagnostics.into_iter().map(|(_, d)| d)); + assert_eq!(existing.versioned.version, diag.version); + } + all_diagnostics.into_values().collect() + } + #[tokio::test] async fn test_deno_diagnostics_with_import_map() { let (temp_dir, snapshot) = setup( @@ -1848,7 +2135,7 @@ let c: number = "a"; .await; let config = mock_config(); let token = CancellationToken::new(); - let actual = generate_deno_diagnostics(&snapshot, &config, token); + let actual = generate_all_deno_diagnostics(&snapshot, &config, token); assert_eq!(actual.len(), 2); for record in actual { let relative_specifier = @@ -1975,7 +2262,7 @@ let c: number = "a"; .await; let config = mock_config(); let token = CancellationToken::new(); - let actual = generate_deno_diagnostics(&snapshot, &config, token); + let actual = generate_all_deno_diagnostics(&snapshot, &config, token); assert_eq!(actual.len(), 1); let record = actual.first().unwrap(); assert_eq!( @@ -2049,7 +2336,7 @@ let c: number = "a"; .await; let config = mock_config(); let token = CancellationToken::new(); - let actual = generate_deno_diagnostics(&snapshot, &config, token); + let actual = generate_all_deno_diagnostics(&snapshot, &config, token); assert_eq!(actual.len(), 1); let record = actual.first().unwrap(); assert_eq!( diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 0010e046f7..e523e0b31f 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -385,6 +385,11 @@ impl PendingChange { } } +pub type DiagnosticsMap = IndexMap>; +pub type ScopedAmbientModules = + HashMap, MaybeAmbientModules>; +pub type MaybeAmbientModules = Option>; + impl TsServer { pub fn new(performance: Arc) -> Self { let (tx, request_rx) = mpsc::unbounded_channel::(); @@ -466,7 +471,7 @@ impl TsServer { snapshot: Arc, specifiers: Vec, token: CancellationToken, - ) -> Result>, AnyError> { + ) -> Result<(DiagnosticsMap, ScopedAmbientModules), AnyError> { let mut diagnostics_map = IndexMap::with_capacity(specifiers.len()); let mut specifiers_by_scope = BTreeMap::new(); for specifier in specifiers { @@ -489,10 +494,20 @@ impl TsServer { for (scope, specifiers) in specifiers_by_scope { let req = TscRequest::GetDiagnostics((specifiers, snapshot.project_version)); - results.push_back(self.request_with_cancellation::>>(snapshot.clone(), req, scope, token.clone())); + results.push_back( + self + .request_with_cancellation::<(DiagnosticsMap, MaybeAmbientModules)>( + snapshot.clone(), + req, + scope.clone(), + token.clone(), + ) + .map(|res| (scope, res)), + ); } - while let Some(raw_diagnostics) = results.next().await { - let raw_diagnostics = raw_diagnostics + let mut ambient_modules_by_scope = HashMap::with_capacity(2); + while let Some((scope, raw_diagnostics)) = results.next().await { + let (raw_diagnostics, ambient_modules) = raw_diagnostics .inspect_err(|err| { if !token.is_cancelled() { lsp_warn!("Error generating TypeScript diagnostics: {err}"); @@ -506,8 +521,9 @@ impl TsServer { } diagnostics_map.insert(specifier, diagnostics); } + ambient_modules_by_scope.insert(scope, ambient_modules); } - Ok(diagnostics_map) + Ok((diagnostics_map, ambient_modules_by_scope)) } pub async fn cleanup_semantic_cache(&self, snapshot: Arc) { @@ -5703,7 +5719,7 @@ mod tests { ) .await; let specifier = temp_dir.url().join("a.ts").unwrap(); - let diagnostics = ts_server + let (diagnostics, _) = ts_server .get_diagnostics(snapshot, vec![specifier.clone()], Default::default()) .await .unwrap(); @@ -5749,7 +5765,7 @@ mod tests { ) .await; let specifier = temp_dir.url().join("a.ts").unwrap(); - let diagnostics = ts_server + let (diagnostics, _) = ts_server .get_diagnostics(snapshot, vec![specifier.clone()], Default::default()) .await .unwrap(); @@ -5779,7 +5795,7 @@ mod tests { ) .await; let specifier = temp_dir.url().join("a.ts").unwrap(); - let diagnostics = ts_server + let (diagnostics, _ambient) = ts_server .get_diagnostics(snapshot, vec![specifier.clone()], Default::default()) .await .unwrap(); @@ -5805,7 +5821,7 @@ mod tests { ) .await; let specifier = temp_dir.url().join("a.ts").unwrap(); - let diagnostics = ts_server + let (diagnostics, _ambient) = ts_server .get_diagnostics(snapshot, vec![specifier.clone()], Default::default()) .await .unwrap(); @@ -5855,7 +5871,7 @@ mod tests { ) .await; let specifier = temp_dir.url().join("a.ts").unwrap(); - let diagnostics = ts_server + let (diagnostics, _ambient) = ts_server .get_diagnostics(snapshot, vec![specifier.clone()], Default::default()) .await .unwrap(); @@ -5888,7 +5904,7 @@ mod tests { ) .await; let specifier = temp_dir.url().join("a.ts").unwrap(); - let diagnostics = ts_server + let (diagnostics, _ambient) = ts_server .get_diagnostics(snapshot, vec![specifier.clone()], Default::default()) .await .unwrap(); @@ -5946,7 +5962,7 @@ mod tests { ) .await; let specifier = temp_dir.url().join("a.ts").unwrap(); - let diagnostics = ts_server + let (diagnostics, _ambient) = ts_server .get_diagnostics(snapshot, vec![specifier.clone()], Default::default()) .await .unwrap(); @@ -6038,7 +6054,7 @@ mod tests { ) .unwrap(); let specifier = temp_dir.url().join("a.ts").unwrap(); - let diagnostics = ts_server + let (diagnostics, _) = ts_server .get_diagnostics( snapshot.clone(), vec![specifier.clone()], @@ -6088,7 +6104,7 @@ mod tests { None, ); let specifier = temp_dir.url().join("a.ts").unwrap(); - let diagnostics = ts_server + let (diagnostics, _) = ts_server .get_diagnostics( snapshot.clone(), vec![specifier.clone()], diff --git a/cli/tsc/98_lsp.js b/cli/tsc/98_lsp.js index e9be7cc123..09b4472124 100644 --- a/cli/tsc/98_lsp.js +++ b/cli/tsc/98_lsp.js @@ -27,6 +27,9 @@ import { const core = globalThis.Deno.core; const ops = core.ops; +/** @type {Map} */ +const ambientModulesCacheByScope = new Map(); + const ChangeKind = { Opened: 0, Modified: 1, @@ -351,6 +354,28 @@ function formatErrorWithArgs(error, args) { return errorString; } +/** + * @param {string[]} a + * @param {string[]} b + */ +function arraysEqual(a, b) { + if (a === b) { + return true; + } + if (a === null || b === null) { + return false; + } + if (a.length !== b.length) { + return false; + } + for (let i = 0; i < a.length; i++) { + if (a[i] !== b[i]) { + return false; + } + } + return true; +} + /** * @param {number} id * @param {string} method @@ -432,7 +457,7 @@ function serverRequest(id, method, args, scope, maybeChange) { // (it's about to be invalidated anyway). const cachedProjectVersion = PROJECT_VERSION_CACHE.get(); if (cachedProjectVersion && projectVersion !== cachedProjectVersion) { - return respond(id, {}); + return respond(id, [{}, null]); } try { /** @type {Record} */ @@ -444,18 +469,30 @@ function serverRequest(id, method, args, scope, maybeChange) { ...ls.getSyntacticDiagnostics(specifier), ].filter(filterMapDiagnostic)); } - return respond(id, diagnosticMap); + let ambient = + ls.getProgram()?.getTypeChecker().getAmbientModules().map((symbol) => + symbol.getName() + ) ?? []; + const previousAmbient = ambientModulesCacheByScope.get(scope); + if ( + ambient && previousAmbient && arraysEqual(ambient, previousAmbient) + ) { + ambient = null; // null => use previous value + } else { + ambientModulesCacheByScope.set(scope, ambient); + } + return respond(id, [diagnosticMap, ambient]); } catch (e) { if ( !isCancellationError(e) ) { return respond( id, - {}, + [{}, null], formatErrorWithArgs(e, [id, method, args, scope, maybeChange]), ); } - return respond(id, {}); + return respond(id, [{}, null]); } } default: diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index a07d71a1cb..46d2f16809 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -6264,8 +6264,12 @@ fn lsp_code_actions_deno_cache_all() { "#, } })); + let mut deno_diagnostics = diagnostics.messages_with_source("deno"); + deno_diagnostics + .diagnostics + .sort_by_key(|d| d.range.start.line); assert_eq!( - diagnostics.messages_with_source("deno"), + deno_diagnostics, serde_json::from_value(json!({ "uri": "file:///a/file.ts", "diagnostics": [ @@ -6296,7 +6300,7 @@ fn lsp_code_actions_deno_cache_all() { })).unwrap() ); - let res = + let mut res = client .write_request( "textDocument/codeAction", @@ -6326,6 +6330,11 @@ fn lsp_code_actions_deno_cache_all() { } }), ); + res.as_array_mut().unwrap().iter_mut().for_each(|fix| { + fix["diagnostics"].as_array_mut().unwrap().sort_by_key(|v| { + v["range"]["start"]["line"].as_number().unwrap().as_i64() + }) + }); assert_eq!( res, json!([ @@ -17413,3 +17422,51 @@ fn type_reference_import_meta() { client.did_close_file(&source); } } + +#[test] +fn ambient_module_errors_suppressed() { + let context = TestContextBuilder::new().use_temp_cwd().build(); + let mut client = context.new_lsp_command().build(); + let temp_dir = context.temp_dir().path(); + + client.initialize_default(); + + temp_dir.join("ambient.d.ts").write( + r#" + declare module "$virtual/module" { + export const foo: number; + } + declare module "*.fake" { + const fake: string; + export default fake; + } + + "#, + ); + let source = source_file( + temp_dir.join("index.ts"), + r#" + /// + import { foo as _foo } from "$virtual/module"; + import _fake from "./not-real.fake"; + import _bad from "./not-real.bad"; + "#, + ); + let diagnostics = client.did_open_file(&source); + assert_eq!(diagnostics.all().len(), 1); + assert_eq!( + json!(diagnostics.all()), + json!([ + { + "range": source.range_of("\"./not-real.bad\""), + "severity": 1, + "code": "no-local", + "source": "deno", + "message": format!( + "Unable to load a local module: {}\nPlease check the file path.", + temp_dir.join("./not-real.bad").url_file() + ) + } + ]) + ); +}