From cc38580106095b1acf3c307dd6079076fec812e3 Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Wed, 17 Nov 2021 09:23:25 +1100 Subject: [PATCH] fix(lsp): retain module dependencies when parse is invalid (#12782) Fixes #12753 --- cli/lsp/diagnostics.rs | 24 ++++---- cli/lsp/documents.rs | 89 +++++++++++++++++------------- cli/lsp/language_server.rs | 38 +++++++------ cli/lsp/tsc.rs | 32 +++++++---- cli/tests/integration/lsp_tests.rs | 6 +- 5 files changed, 109 insertions(+), 80 deletions(-) diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index f84d22b45c..ddc18f18fc 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -492,19 +492,17 @@ async fn generate_deps_diagnostics( .get_version(document.specifier(), &DiagnosticSource::Deno); if version != current_version { let mut diagnostics = Vec::new(); - if let Some(dependencies) = document.dependencies() { - for (_, dependency) in dependencies { - diagnose_dependency( - &mut diagnostics, - &documents, - &dependency.maybe_code, - ); - diagnose_dependency( - &mut diagnostics, - &documents, - &dependency.maybe_type, - ); - } + for (_, dependency) in document.dependencies() { + diagnose_dependency( + &mut diagnostics, + &documents, + &dependency.maybe_code, + ); + diagnose_dependency( + &mut diagnostics, + &documents, + &dependency.maybe_type, + ); } diagnostics_vec.push(( document.specifier().clone(), diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 9892eab1fe..51a63eb229 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -245,6 +245,8 @@ impl SyntheticModule { } #[derive(Debug, Clone)] struct DocumentInner { + /// contains the last-known-good set of dependencies from parsing the module + dependencies: Arc>, fs_version: String, line_index: Arc, maybe_language_id: Option, @@ -282,9 +284,15 @@ impl Document { maybe_resolver, Some(&parser), )); + let dependencies = if let Some(Ok(module)) = &maybe_module { + Arc::new(module.dependencies.clone()) + } else { + Arc::new(BTreeMap::new()) + }; let text_info = SourceTextInfo::new(content); let line_index = Arc::new(LineIndex::new(text_info.text_str())); Self(Arc::new(DocumentInner { + dependencies, fs_version, line_index, maybe_language_id: None, @@ -317,9 +325,15 @@ impl Document { } else { None }; + let dependencies = if let Some(Ok(module)) = &maybe_module { + Arc::new(module.dependencies.clone()) + } else { + Arc::new(BTreeMap::new()) + }; let source = SourceTextInfo::new(content); let line_index = Arc::new(LineIndex::new(source.text_str())); Self(Arc::new(DocumentInner { + dependencies, fs_version: "1".to_string(), line_index, maybe_language_id: Some(language_id), @@ -379,14 +393,20 @@ impl Document { } else { None }; - let source = SourceTextInfo::new(content); + let dependencies = if let Some(Ok(module)) = &maybe_module { + Arc::new(module.dependencies.clone()) + } else { + self.0.dependencies.clone() + }; + let text_info = SourceTextInfo::new(content); let line_index = if index_valid == IndexValid::All { line_index } else { - Arc::new(LineIndex::new(source.text_str())) + Arc::new(LineIndex::new(text_info.text_str())) }; Ok(Document(Arc::new(DocumentInner { - text_info: source, + dependencies, + text_info, line_index, maybe_module, maybe_lsp_version: Some(version), @@ -499,15 +519,13 @@ impl Document { self.0.maybe_warning.clone() } - pub fn dependencies(&self) -> Option> { - let module = self.maybe_module()?.as_ref().ok()?; - Some( - module - .dependencies - .iter() - .map(|(s, d)| (s.clone(), d.clone())) - .collect(), - ) + pub fn dependencies(&self) -> Vec<(String, deno_graph::Dependency)> { + self + .0 + .dependencies + .iter() + .map(|(s, d)| (s.clone(), d.clone())) + .collect() } /// If the supplied position is within a dependency range, return the resolved @@ -911,35 +929,32 @@ impl DocumentsInner { specifiers: Vec, referrer: &ModuleSpecifier, ) -> Option>> { - let doc = self.get(referrer)?; + let dependencies = self.get(referrer)?.0.dependencies.clone(); let mut results = Vec::new(); - if let Some(Ok(module)) = doc.maybe_module() { - let dependencies = module.dependencies.clone(); - for specifier in specifiers { - if specifier.starts_with("asset:") { - if let Ok(specifier) = ModuleSpecifier::parse(&specifier) { - let media_type = MediaType::from(&specifier); - results.push(Some((specifier, media_type))); - } else { - results.push(None); - } - } else if let Some(dep) = dependencies.get(&specifier) { - if let Some(Ok((specifier, _))) = &dep.maybe_type { - results.push(self.resolve_dependency(specifier)); - } else if let Some(Ok((specifier, _))) = &dep.maybe_code { - results.push(self.resolve_dependency(specifier)); - } else { - results.push(None); - } - } else if let Some(Some(Ok((specifier, _)))) = - self.resolve_imports_dependency(&specifier) - { - // clone here to avoid double borrow of self - let specifier = specifier.clone(); - results.push(self.resolve_dependency(&specifier)); + for specifier in specifiers { + if specifier.starts_with("asset:") { + if let Ok(specifier) = ModuleSpecifier::parse(&specifier) { + let media_type = MediaType::from(&specifier); + results.push(Some((specifier, media_type))); } else { results.push(None); } + } else if let Some(dep) = dependencies.get(&specifier) { + if let Some(Ok((specifier, _))) = &dep.maybe_type { + results.push(self.resolve_dependency(specifier)); + } else if let Some(Ok((specifier, _))) = &dep.maybe_code { + results.push(self.resolve_dependency(specifier)); + } else { + results.push(None); + } + } else if let Some(Some(Ok((specifier, _)))) = + self.resolve_imports_dependency(&specifier) + { + // clone here to avoid double borrow of self + let specifier = specifier.clone(); + results.push(self.resolve_dependency(&specifier)); + } else { + results.push(None); } } Some(results) diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index dbe00861eb..e82afd9d04 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -987,17 +987,16 @@ impl Inner { let asset_or_document = self.get_cached_asset_or_document(&specifier)?; let line_index = asset_or_document.line_index(); - let req = tsc::RequestMethod::GetNavigationTree(specifier); - let navigation_tree: tsc::NavigationTree = self - .ts_server - .request(self.snapshot()?, req) - .await - .map_err(|err| { - error!("Failed to request to tsserver {}", err); - LspError::invalid_request() + let navigation_tree = + self.get_navigation_tree(&specifier).await.map_err(|err| { + error!( + "Error getting document symbols for \"{}\": {}", + specifier, err + ); + LspError::internal_error() })?; - let response = if let Some(child_items) = navigation_tree.child_items { + let response = if let Some(child_items) = &navigation_tree.child_items { let mut document_symbols = Vec::::new(); for item in child_items { item @@ -1543,7 +1542,7 @@ impl Inner { let asset_or_doc = self.get_cached_asset_or_document(&specifier)?; let line_index = asset_or_doc.line_index(); let req = tsc::RequestMethod::GetReferences(( - specifier, + specifier.clone(), line_index.offset_tsc(params.text_document_position.position)?, )); let maybe_references: Option> = self @@ -1563,9 +1562,14 @@ impl Inner { } let reference_specifier = resolve_url(&reference.document_span.file_name).unwrap(); - let asset_or_doc = - self.get_asset_or_document(&reference_specifier).await?; - results.push(reference.to_location(asset_or_doc.line_index(), self)); + let reference_line_index = if reference_specifier == specifier { + line_index.clone() + } else { + let asset_or_doc = + self.get_asset_or_document(&reference_specifier).await?; + asset_or_doc.line_index() + }; + results.push(reference.to_location(reference_line_index, self)); } self.performance.measure(mark); @@ -2157,8 +2161,8 @@ impl Inner { LspError::invalid_request() })?; - let semantic_tokens: SemanticTokens = - semantic_classification.to_semantic_tokens(line_index); + let semantic_tokens = + semantic_classification.to_semantic_tokens(line_index)?; let response = if !semantic_tokens.data.is_empty() { Some(SemanticTokensResult::Tokens(semantic_tokens)) } else { @@ -2200,8 +2204,8 @@ impl Inner { LspError::invalid_request() })?; - let semantic_tokens: SemanticTokens = - semantic_classification.to_semantic_tokens(line_index); + let semantic_tokens = + semantic_classification.to_semantic_tokens(line_index)?; let response = if !semantic_tokens.data.is_empty() { Some(SemanticTokensRangeResult::Tokens(semantic_tokens)) } else { diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index b5ce48fab4..4a61d7403a 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -38,6 +38,8 @@ use deno_core::OpFn; use deno_core::RuntimeOptions; use deno_runtime::tokio_util::create_basic_runtime; use log::warn; +use lspower::jsonrpc::Error as LspError; +use lspower::jsonrpc::Result as LspResult; use lspower::lsp; use regex::Captures; use regex::Regex; @@ -1122,7 +1124,7 @@ impl Classifications { pub fn to_semantic_tokens( &self, line_index: Arc, - ) -> lsp::SemanticTokens { + ) -> LspResult { let token_count = self.spans.len() / 3; let mut builder = SemanticTokensBuilder::new(); for i in 0..token_count { @@ -1141,16 +1143,26 @@ impl Classifications { let start_pos = line_index.position_tsc(offset.into()); let end_pos = line_index.position_tsc(TextSize::from(offset + length)); - // start_pos.line == end_pos.line is always true as there are no multiline tokens - builder.push( - start_pos.line, - start_pos.character, - end_pos.character - start_pos.character, - token_type, - token_modifiers, - ); + if start_pos.line == end_pos.line + && start_pos.character <= end_pos.character + { + builder.push( + start_pos.line, + start_pos.character, + end_pos.character - start_pos.character, + token_type, + token_modifiers, + ); + } else { + log::error!( + "unexpected positions\nstart_pos: {:?}\nend_pos: {:?}", + start_pos, + end_pos + ); + return Err(LspError::internal_error()); + } } - builder.build(None) + Ok(builder.build(None)) } fn get_token_type_from_classification(ts_classification: u32) -> u32 { diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index 96689f8a39..bf92589254 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -1066,10 +1066,10 @@ fn lsp_hover_dependency() { ); } -// Regression test for #12753 +// This tests for a regression covered by denoland/deno#12753 where the lsp was +// unable to resolve dependencies when there was an invalid syntax in the module #[test] -#[ignore] -fn lsp_hover_keep_type_info_after_invalid_syntax_change() { +fn lsp_hover_deps_preserved_when_invalid_parse() { let mut client = init("initialize_params.json"); did_open( &mut client,