From a4d557126e49108db4c0dc42561ae032d2418b04 Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Thu, 24 Dec 2020 21:53:03 +1100 Subject: [PATCH] fix(lsp): provide diagnostics for unresolved modules (#8872) --- cli/lsp/analysis.rs | 62 +++++++++++++++++++++++------- cli/lsp/diagnostics.rs | 76 +++++++++++++++++++++++++++++++++++++ cli/lsp/language_server.rs | 33 +++++++++++++++- cli/lsp/sources.rs | 6 +-- cli/lsp/tsc.rs | 7 ++-- cli/tsc/99_main_compiler.js | 3 ++ 6 files changed, 166 insertions(+), 21 deletions(-) diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 7cf6aca371..26f38ef380 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -86,7 +86,6 @@ pub fn references_to_diagnostics( severity: Some(lsp_types::DiagnosticSeverity::Warning), code: Some(lsp_types::NumberOrString::String(code)), code_description: None, - // TODO(@kitsonk) this won't make sense for every diagnostic source: Some("deno-lint".to_string()), message, related_information: None, @@ -100,12 +99,13 @@ pub fn references_to_diagnostics( #[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct Dependency { pub is_dynamic: bool, - pub maybe_code: Option, - pub maybe_type: Option, + pub maybe_code: Option, + pub maybe_code_specifier_range: Option, + pub maybe_type: Option, } #[derive(Debug, Clone, PartialEq, Eq)] -pub enum ResolvedImport { +pub enum ResolvedDependency { Resolved(ModuleSpecifier), Err(String), } @@ -114,7 +114,7 @@ pub fn resolve_import( specifier: &str, referrer: &ModuleSpecifier, maybe_import_map: &Option, -) -> ResolvedImport { +) -> ResolvedDependency { let maybe_mapped = if let Some(import_map) = maybe_import_map { if let Ok(maybe_specifier) = import_map.resolve(specifier, referrer.as_str()) @@ -132,13 +132,13 @@ pub fn resolve_import( } else { match ModuleSpecifier::resolve_import(specifier, referrer.as_str()) { Ok(resolved) => resolved, - Err(err) => return ResolvedImport::Err(err.to_string()), + Err(err) => return ResolvedDependency::Err(err.to_string()), } }; let referrer_scheme = referrer.as_url().scheme(); let specifier_scheme = specifier.as_url().scheme(); if referrer_scheme == "https" && specifier_scheme == "http" { - return ResolvedImport::Err( + return ResolvedDependency::Err( "Modules imported via https are not allowed to import http modules." .to_string(), ); @@ -147,10 +147,10 @@ pub fn resolve_import( && !(specifier_scheme == "https" || specifier_scheme == "http") && !remapped { - return ResolvedImport::Err("Remote modules are not allowed to import local modules. Consider using a dynamic import instead.".to_string()); + return ResolvedDependency::Err("Remote modules are not allowed to import local modules. Consider using a dynamic import instead.".to_string()); } - ResolvedImport::Resolved(specifier) + ResolvedDependency::Resolved(specifier) } // TODO(@kitsonk) a lot of this logic is duplicated in module_graph.rs in @@ -160,7 +160,7 @@ pub fn analyze_dependencies( source: &str, media_type: &MediaType, maybe_import_map: &Option, -) -> Option<(HashMap, Option)> { +) -> Option<(HashMap, Option)> { let specifier_str = specifier.to_string(); let source_map = Rc::new(swc_common::SourceMap::default()); let mut maybe_type = None; @@ -222,7 +222,21 @@ pub fn analyze_dependencies( | swc_ecmascript::dep_graph::DependencyKind::ImportType => { dep.maybe_type = Some(resolved_import) } - _ => dep.maybe_code = Some(resolved_import), + _ => { + dep.maybe_code_specifier_range = Some(Range { + start: Position { + line: (desc.specifier_line - 1) as u32, + character: desc.specifier_col as u32, + }, + end: Position { + line: (desc.specifier_line - 1) as u32, + character: (desc.specifier_col + + desc.specifier.chars().count() + + 2) as u32, + }, + }); + dep.maybe_code = Some(resolved_import); + } } if maybe_resolved_type_import.is_some() && dep.maybe_type.is_none() { dep.maybe_type = maybe_resolved_type_import; @@ -293,27 +307,47 @@ mod tests { actual.get("https://cdn.skypack.dev/react").cloned(), Some(Dependency { is_dynamic: false, - maybe_code: Some(ResolvedImport::Resolved( + maybe_code: Some(ResolvedDependency::Resolved( ModuleSpecifier::resolve_url("https://cdn.skypack.dev/react") .unwrap() )), - maybe_type: Some(ResolvedImport::Resolved( + maybe_type: Some(ResolvedDependency::Resolved( ModuleSpecifier::resolve_url( "https://deno.land/x/types/react/index.d.ts" ) .unwrap() )), + maybe_code_specifier_range: Some(Range { + start: Position { + line: 8, + character: 27, + }, + end: Position { + line: 8, + character: 58, + } + }), }) ); assert_eq!( actual.get("https://deno.land/x/oak@v6.3.2/mod.ts").cloned(), Some(Dependency { is_dynamic: false, - maybe_code: Some(ResolvedImport::Resolved( + maybe_code: Some(ResolvedDependency::Resolved( ModuleSpecifier::resolve_url("https://deno.land/x/oak@v6.3.2/mod.ts") .unwrap() )), maybe_type: None, + maybe_code_specifier_range: Some(Range { + start: Position { + line: 5, + character: 11, + }, + end: Position { + line: 5, + character: 50, + } + }), }) ); } diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 1d0a1fac99..c468fb0fac 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -2,6 +2,7 @@ use super::analysis::get_lint_references; use super::analysis::references_to_diagnostics; +use super::analysis::ResolvedDependency; use super::language_server::StateSnapshot; use super::memory_cache::FileId; use super::tsc; @@ -9,6 +10,7 @@ use super::tsc; use crate::diagnostics; use crate::media_type::MediaType; +use deno_core::error::custom_error; use deno_core::error::AnyError; use deno_core::serde_json; use deno_core::serde_json::Value; @@ -19,6 +21,7 @@ use std::mem; #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub enum DiagnosticSource { + Deno, Lint, TypeScript, } @@ -261,3 +264,76 @@ pub async fn generate_ts_diagnostics( Ok(diagnostics) } + +pub async fn generate_dependency_diagnostics( + state_snapshot: StateSnapshot, + diagnostic_collection: DiagnosticCollection, +) -> Result { + tokio::task::spawn_blocking(move || { + let mut diagnostics = Vec::new(); + + let file_cache = state_snapshot.file_cache.read().unwrap(); + let mut sources = if let Ok(sources) = state_snapshot.sources.write() { + sources + } else { + return Err(custom_error("Deadlock", "deadlock locking sources")); + }; + for (specifier, doc_data) in state_snapshot.doc_data.iter() { + let file_id = file_cache.lookup(specifier).unwrap(); + let version = doc_data.version; + let current_version = diagnostic_collection.get_version(&file_id); + if version != current_version { + let mut diagnostic_list = Vec::new(); + if let Some(dependencies) = &doc_data.dependencies { + for (_, dependency) in dependencies.iter() { + if let (Some(code), Some(range)) = ( + &dependency.maybe_code, + &dependency.maybe_code_specifier_range, + ) { + match code.clone() { + ResolvedDependency::Err(message) => { + diagnostic_list.push(lsp_types::Diagnostic { + range: *range, + severity: Some(lsp_types::DiagnosticSeverity::Error), + code: None, + code_description: None, + source: Some("deno".to_string()), + message, + related_information: None, + tags: None, + data: None, + }) + } + ResolvedDependency::Resolved(specifier) => { + if !(state_snapshot.doc_data.contains_key(&specifier) || sources.contains(&specifier)) { + let is_local = specifier.as_url().scheme() == "file"; + diagnostic_list.push(lsp_types::Diagnostic { + range: *range, + severity: Some(lsp_types::DiagnosticSeverity::Error), + code: None, + code_description: None, + source: Some("deno".to_string()), + message: if is_local { + format!("Unable to load a local module: \"{}\".\n Please check the file path.", specifier) + } else { + format!("Unable to load the module: \"{}\".\n If the module exists, running `deno cache {}` should resolve this error.", specifier, specifier) + }, + related_information: None, + tags: None, + data: None, + }) + } + }, + } + } + } + } + diagnostics.push((file_id, version, diagnostic_list)) + } + } + + Ok(diagnostics) + }) + .await + .unwrap() +} diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 7834cab7fd..72d1b1ad38 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -178,9 +178,35 @@ impl LanguageServer { Ok::<(), AnyError>(()) }; - let (lint_res, ts_res) = tokio::join!(lint, ts); + let deps = async { + if enabled { + let diagnostics_collection = self.diagnostics.read().unwrap().clone(); + let diagnostics = diagnostics::generate_dependency_diagnostics( + self.snapshot(), + diagnostics_collection, + ) + .await?; + { + let mut diagnostics_collection = self.diagnostics.write().unwrap(); + for (file_id, version, diagnostics) in diagnostics { + diagnostics_collection.set( + file_id, + DiagnosticSource::Deno, + version, + diagnostics, + ); + } + } + self.publish_diagnostics().await? + }; + + Ok::<(), AnyError>(()) + }; + + let (lint_res, ts_res, deps_res) = tokio::join!(lint, ts, deps); lint_res?; ts_res?; + deps_res?; Ok(()) } @@ -213,6 +239,11 @@ impl LanguageServer { .diagnostics_for(file_id, DiagnosticSource::TypeScript) .cloned(), ); + diagnostics.extend( + diagnostics_collection + .diagnostics_for(file_id, DiagnosticSource::Deno) + .cloned(), + ); } let specifier = { let file_cache = self.file_cache.read().unwrap(); diff --git a/cli/lsp/sources.rs b/cli/lsp/sources.rs index 63b4ebd994..c6ab87f218 100644 --- a/cli/lsp/sources.rs +++ b/cli/lsp/sources.rs @@ -23,7 +23,7 @@ use std::time::SystemTime; #[derive(Debug, Clone, Default)] struct Metadata { dependencies: Option>, - maybe_types: Option, + maybe_types: Option, media_type: MediaType, source: String, version: String, @@ -255,7 +255,7 @@ impl Sources { let dependencies = &metadata.dependencies?; let dependency = dependencies.get(specifier)?; if let Some(type_dependency) = &dependency.maybe_type { - if let analysis::ResolvedImport::Resolved(resolved_specifier) = + if let analysis::ResolvedDependency::Resolved(resolved_specifier) = type_dependency { self.resolution_result(resolved_specifier) @@ -264,7 +264,7 @@ impl Sources { } } else { let code_dependency = &dependency.maybe_code.clone()?; - if let analysis::ResolvedImport::Resolved(resolved_specifier) = + if let analysis::ResolvedDependency::Resolved(resolved_specifier) = code_dependency { self.resolution_result(resolved_specifier) diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 4cd13f70d2..2a0f7d76cf 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -1,6 +1,6 @@ // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license. -use super::analysis::ResolvedImport; +use super::analysis::ResolvedDependency; use super::language_server::StateSnapshot; use super::text; use super::utils; @@ -839,9 +839,10 @@ fn resolve(state: &mut State, args: Value) -> Result { } else if let Some(resolved_import) = &dependency.maybe_code { resolved_import.clone() } else { - ResolvedImport::Err("missing dependency".to_string()) + ResolvedDependency::Err("missing dependency".to_string()) }; - if let ResolvedImport::Resolved(resolved_specifier) = resolved_import + if let ResolvedDependency::Resolved(resolved_specifier) = + resolved_import { if state .state_snapshot diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js index 9b08dee93c..de9e74d2ef 100644 --- a/cli/tsc/99_main_compiler.js +++ b/cli/tsc/99_main_compiler.js @@ -128,6 +128,9 @@ delete Object.prototype.__proto__; // TS2691: An import path cannot end with a '.ts' extension. Consider // importing 'bad-module' instead. 2691, + // TS2792: Cannot find module. Did you mean to set the 'moduleResolution' + // option to 'node', or to add aliases to the 'paths' option? + 2792, // TS5009: Cannot find the common subdirectory path for the input files. 5009, // TS5055: Cannot write file