fix(lsp): retain module dependencies when parse is invalid (#12782)

Fixes #12753

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(),

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<BTreeMap<String, deno_graph::Dependency>>,
   fs_version: String,
   line_index: Arc<LineIndex>,
   maybe_language_id: Option<LanguageId>,
@@ -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<Vec<(String, deno_graph::Dependency)>> {
-    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<String>,
     referrer: &ModuleSpecifier,
   ) -> Option<Vec<Option<(ModuleSpecifier, MediaType)>>> {
-    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)

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::<DocumentSymbol>::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<Vec<tsc::ReferenceEntry>> = 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 {

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<LineIndex>,
-  ) -> lsp::SemanticTokens {
+  ) -> LspResult<lsp::SemanticTokens> {
     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 {

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,