From 7e06d33b34394fda96d27082a7a22a4ff91fa86f Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Thu, 14 Jul 2022 11:12:18 +1000 Subject: [PATCH] feat(lsp): provide import map remapping diags and fixes (#15165) --- Cargo.lock | 17 ++- cli/Cargo.toml | 2 +- cli/lsp/diagnostics.rs | 273 +++++++++++++++++++++++++++++++++---- cli/lsp/language_server.rs | 2 + 4 files changed, 263 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 52740f2745..f6c756680e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -812,7 +812,7 @@ dependencies = [ "fwdansi", "google-storage1", "http", - "import_map", + "import_map 0.12.1", "indexmap", "jsonc-parser", "libc", @@ -977,7 +977,7 @@ dependencies = [ "deno_ast", "deno_graph", "futures", - "import_map", + "import_map 0.11.0", "lazy_static", "regex", "serde", @@ -2186,6 +2186,19 @@ dependencies = [ "url 2.2.2", ] +[[package]] +name = "import_map" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b827962ca5aa6d5bbe313c14e73d7cc517487fa3bad380bb6bdbd8421e591a29" +dependencies = [ + "indexmap", + "log 0.4.17", + "serde", + "serde_json", + "url 2.2.2", +] + [[package]] name = "indexmap" version = "1.9.1" diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 41feb09e4c..12edb9413f 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -71,7 +71,7 @@ env_logger = "=0.9.0" eszip = "=0.22.0" fancy-regex = "=0.9.0" http = "=0.2.6" -import_map = "=0.11.0" +import_map = "=0.12.1" indexmap = "1.8.1" jsonc-parser = { version = "=0.19.0", features = ["serde"] } libc = "=0.2.126" diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 52ba1ba969..7c5ab936a6 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -6,7 +6,6 @@ use super::client::Client; use super::config::ConfigSnapshot; use super::documents; use super::documents::Document; -use super::documents::Documents; use super::language_server; use super::language_server::StateSnapshot; use super::performance::Performance; @@ -270,7 +269,7 @@ impl DiagnosticsServer { } let mark = performance.mark("update_diagnostics_deps", None::<()>); - let diagnostics = generate_deps_diagnostics( + let diagnostics = generate_deno_diagnostics( &snapshot, &config, token.clone(), @@ -572,11 +571,21 @@ struct DiagnosticDataRedirect { pub redirect: ModuleSpecifier, } +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct DiagnosticDataImportMapRemap { + pub from: String, + pub to: String, +} + /// An enum which represents diagnostic errors which originate from Deno itself. pub enum DenoDiagnostic { /// A `x-deno-warning` is associated with the specifier and should be displayed /// as a warning to the user. DenoWarn(String), + /// An informational diagnostic that indicates an existing specifier can be + /// remapped to an import map import specifier. + ImportMapRemap { from: String, to: String }, /// The import assertion type is incorrect. InvalidAssertType(String), /// A module requires an assertion type to be a valid import. @@ -606,6 +615,7 @@ impl DenoDiagnostic { match self { Self::DenoWarn(_) => "deno-warn", + Self::ImportMapRemap { .. } => "import-map-remap", Self::InvalidAssertType(_) => "invalid-assert-type", Self::NoAssertType => "no-assert-type", Self::NoCache(_) => "no-cache", @@ -633,6 +643,33 @@ impl DenoDiagnostic { ) -> Result { if let Some(lsp::NumberOrString::String(code)) = &diagnostic.code { let code_action = match code.as_str() { + "import-map-remap" => { + let data = diagnostic + .data + .clone() + .ok_or_else(|| anyhow!("Diagnostic is missing data"))?; + let DiagnosticDataImportMapRemap { from, to } = + serde_json::from_value(data)?; + lsp::CodeAction { + title: format!( + "Update \"{}\" to \"{}\" to use import map.", + from, to + ), + kind: Some(lsp::CodeActionKind::QUICKFIX), + diagnostics: Some(vec![diagnostic.clone()]), + edit: Some(lsp::WorkspaceEdit { + changes: Some(HashMap::from([( + specifier.clone(), + vec![lsp::TextEdit { + new_text: format!("\"{}\"", to), + range: diagnostic.range, + }], + )])), + ..Default::default() + }), + ..Default::default() + } + } "no-assert-type" => lsp::CodeAction { title: "Insert import assertion.".to_string(), kind: Some(lsp::CodeActionKind::QUICKFIX), @@ -717,7 +754,11 @@ impl DenoDiagnostic { if let Some(lsp::NumberOrString::String(code)) = code { matches!( code.as_str(), - "no-cache" | "no-cache-data" | "no-assert-type" | "redirect" + "import-map-remap" + | "no-cache" + | "no-cache-data" + | "no-assert-type" + | "redirect" ) } else { false @@ -729,6 +770,7 @@ impl DenoDiagnostic { pub fn to_lsp_diagnostic(&self, range: &lsp::Range) -> lsp::Diagnostic { let (severity, message, data) = match self { Self::DenoWarn(message) => (lsp::DiagnosticSeverity::WARNING, message.to_string(), None), + Self::ImportMapRemap { from, to } => (lsp::DiagnosticSeverity::HINT, format!("The import specifier can be remapped to \"{}\" which will resolve it via the active import map.", to), Some(json!({ "from": from, "to": to }))), Self::InvalidAssertType(assert_type) => (lsp::DiagnosticSeverity::ERROR, format!("The module is a JSON module and expected an assertion type of \"json\". Instead got \"{}\".", assert_type), None), Self::NoAssertType => (lsp::DiagnosticSeverity::ERROR, "The module is a JSON module and not being imported with an import assertion. Consider adding `assert { type: \"json\" }` to the import statement.".to_string(), None), Self::NoCache(specifier) => (lsp::DiagnosticSeverity::ERROR, format!("Uncached or missing remote URL: \"{}\".", specifier), Some(json!({ "specifier": specifier }))), @@ -750,10 +792,9 @@ impl DenoDiagnostic { } } -fn diagnose_dependency( +fn diagnose_resolved( diagnostics: &mut Vec, - documents: &Documents, - cache_metadata: &cache::CacheMetadata, + snapshot: &language_server::StateSnapshot, resolved: &deno_graph::Resolved, is_dynamic: bool, maybe_assert_type: Option<&str>, @@ -765,7 +806,7 @@ fn diagnose_dependency( let range = documents::to_lsp_range(range); // If the module is a remote module and has a `X-Deno-Warning` header, we // want a warning diagnostic with that message. - if let Some(metadata) = cache_metadata.get(specifier) { + if let Some(metadata) = snapshot.cache_metadata.get(specifier) { if let Some(message) = metadata.get(&cache::MetadataKey::Warning).cloned() { @@ -773,7 +814,7 @@ fn diagnose_dependency( .push(DenoDiagnostic::DenoWarn(message).to_lsp_diagnostic(&range)); } } - if let Some(doc) = documents.get(specifier) { + if let Some(doc) = snapshot.documents.get(specifier) { let doc_specifier = doc.specifier(); // If the module was redirected, we want to issue an informational // diagnostic that indicates this. This then allows us to issue a code @@ -828,9 +869,54 @@ fn diagnose_dependency( } } -/// Generate diagnostics for dependencies of a module, attempting to resolve -/// dependencies on the local file system or in the DENO_DIR cache. -async fn generate_deps_diagnostics( +/// Generate diagnostics related to a dependency. The dependency is analyzed to +/// determine if it can be remapped to the active import map as well as surface +/// any diagnostics related to the resolved code or type dependency. +fn diagnose_dependency( + diagnostics: &mut Vec, + snapshot: &language_server::StateSnapshot, + referrer: &ModuleSpecifier, + dependency_key: &str, + dependency: &deno_graph::Dependency, +) { + if let Some(import_map) = &snapshot.maybe_import_map { + if let Resolved::Ok { + specifier, range, .. + } = &dependency.maybe_code + { + if let Some(to) = import_map.lookup(specifier, referrer) { + if dependency_key != to { + diagnostics.push( + DenoDiagnostic::ImportMapRemap { + from: dependency_key.to_string(), + to, + } + .to_lsp_diagnostic(&documents::to_lsp_range(range)), + ); + } + } + } + } + diagnose_resolved( + diagnostics, + snapshot, + &dependency.maybe_code, + dependency.is_dynamic, + dependency.maybe_assert_type.as_deref(), + ); + diagnose_resolved( + diagnostics, + snapshot, + &dependency.maybe_type, + dependency.is_dynamic, + dependency.maybe_assert_type.as_deref(), + ); +} + +/// 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. +async fn generate_deno_diagnostics( snapshot: &language_server::StateSnapshot, config: &ConfigSnapshot, token: CancellationToken, @@ -844,22 +930,13 @@ async fn generate_deps_diagnostics( let mut diagnostics = Vec::new(); let specifier = document.specifier(); if config.specifier_enabled(specifier) { - for (_, dependency) in document.dependencies() { + for (dependency_key, dependency) in document.dependencies() { diagnose_dependency( &mut diagnostics, - &snapshot.documents, - &snapshot.cache_metadata, - &dependency.maybe_code, - dependency.is_dynamic, - dependency.maybe_assert_type.as_deref(), - ); - diagnose_dependency( - &mut diagnostics, - &snapshot.documents, - &snapshot.cache_metadata, - &dependency.maybe_type, - dependency.is_dynamic, - dependency.maybe_assert_type.as_deref(), + snapshot, + specifier, + &dependency_key, + &dependency, ); } } @@ -880,15 +957,18 @@ mod tests { use crate::lsp::config::Settings; use crate::lsp::config::SpecifierSettings; use crate::lsp::config::WorkspaceSettings; + use crate::lsp::documents::Documents; use crate::lsp::documents::LanguageId; use crate::lsp::language_server::StateSnapshot; use std::path::Path; use std::path::PathBuf; + use std::sync::Arc; use test_util::TempDir; fn mock_state_snapshot( fixtures: &[(&str, &str, i32, LanguageId)], location: &Path, + maybe_import_map: Option<(&str, &str)>, ) -> StateSnapshot { let mut documents = Documents::new(location); for (specifier, source, version, language_id) in fixtures { @@ -901,8 +981,17 @@ mod tests { (*source).into(), ); } + let maybe_import_map = maybe_import_map.map(|(base, json_string)| { + let base_url = ModuleSpecifier::parse(base).unwrap(); + let result = import_map::parse_from_json(&base_url, json_string).unwrap(); + if !result.diagnostics.is_empty() { + panic!("unexpected import map diagnostics"); + } + Arc::new(result.import_map) + }); StateSnapshot { documents, + maybe_import_map, ..Default::default() } } @@ -924,9 +1013,11 @@ mod tests { fn setup( temp_dir: &TempDir, sources: &[(&str, &str, i32, LanguageId)], + maybe_import_map: Option<(&str, &str)>, ) -> (StateSnapshot, PathBuf) { let location = temp_dir.path().join("deps"); - let state_snapshot = mock_state_snapshot(sources, &location); + let state_snapshot = + mock_state_snapshot(sources, &location, maybe_import_map); (state_snapshot, location) } @@ -945,6 +1036,7 @@ let c: number = "a"; 1, LanguageId::TypeScript, )], + None, ); let snapshot = Arc::new(snapshot); let ts_server = TsServer::new(Default::default()); @@ -969,7 +1061,7 @@ let c: number = "a"; .await .unwrap(); assert_eq!(get_diagnostics_for_single(diagnostics).len(), 4); - let diagnostics = generate_deps_diagnostics( + let diagnostics = generate_deno_diagnostics( &snapshot, &enabled_config, Default::default(), @@ -1010,7 +1102,7 @@ let c: number = "a"; .await .unwrap(); assert_eq!(get_diagnostics_for_single(diagnostics).len(), 0); - let diagnostics = generate_deps_diagnostics( + let diagnostics = generate_deno_diagnostics( &snapshot, &disabled_config, Default::default(), @@ -1039,6 +1131,7 @@ let c: number = "a"; 1, LanguageId::TypeScript, )], + None, ); let snapshot = Arc::new(snapshot); let ts_server = TsServer::new(Default::default()); @@ -1053,4 +1146,128 @@ let c: number = "a"; // should be none because it's cancelled assert_eq!(diagnostics.len(), 0); } + + #[tokio::test] + async fn test_deno_diagnostics_with_import_map() { + let temp_dir = TempDir::new(); + let (snapshot, _) = setup( + &temp_dir, + &[ + ("file:///std/testing/asserts.ts", "export function assert() {}", 1, LanguageId::TypeScript), + ("file:///a/file.ts", "import { assert } from \"../std/testing/asserts.ts\";\n\nassert();\n", 1, LanguageId::TypeScript), + ], + Some(("file:///a/import-map.json", r#"{ + "imports": { + "/~/std/": "../std/" + } + }"#)), + ); + let config = mock_config(); + let token = CancellationToken::new(); + let actual = generate_deno_diagnostics(&snapshot, &config, token).await; + assert_eq!(actual.len(), 2); + for (specifier, _, diagnostics) in actual { + match specifier.as_str() { + "file:///std/testing/asserts.ts" => { + assert_eq!(json!(diagnostics), json!([])) + } + "file:///a/file.ts" => assert_eq!( + json!(diagnostics), + json!([ + { + "range": { + "start": { + "line": 0, + "character": 23 + }, + "end": { + "line": 0, + "character": 50 + } + }, + "severity": 4, + "code": "import-map-remap", + "source": "deno", + "message": "The import specifier can be remapped to \"/~/std/testing/asserts.ts\" which will resolve it via the active import map.", + "data": { + "from": "../std/testing/asserts.ts", + "to": "/~/std/testing/asserts.ts" + } + } + ]) + ), + _ => unreachable!("unexpected specifier {}", specifier), + } + } + } + + #[test] + fn test_get_code_action_import_map_remap() { + let specifier = ModuleSpecifier::parse("file:///a/file.ts").unwrap(); + let result = DenoDiagnostic::get_code_action(&specifier, &lsp::Diagnostic { + range: lsp::Range { + start: lsp::Position { line: 0, character: 23 }, + end: lsp::Position { line: 0, character: 50 }, + }, + severity: Some(lsp::DiagnosticSeverity::HINT), + code: Some(lsp::NumberOrString::String("import-map-remap".to_string())), + source: Some("deno".to_string()), + message: "The import specifier can be remapped to \"/~/std/testing/asserts.ts\" which will resolve it via the active import map.".to_string(), + data: Some(json!({ + "from": "../std/testing/asserts.ts", + "to": "/~/std/testing/asserts.ts" + })), + ..Default::default() + }); + assert!(result.is_ok()); + let actual = result.unwrap(); + assert_eq!( + json!(actual), + json!({ + "title": "Update \"../std/testing/asserts.ts\" to \"/~/std/testing/asserts.ts\" to use import map.", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 0, + "character": 23 + }, + "end": { + "line": 0, + "character": 50 + } + }, + "severity": 4, + "code": "import-map-remap", + "source": "deno", + "message": "The import specifier can be remapped to \"/~/std/testing/asserts.ts\" which will resolve it via the active import map.", + "data": { + "from": "../std/testing/asserts.ts", + "to": "/~/std/testing/asserts.ts" + } + } + ], + "edit": { + "changes": { + "file:///a/file.ts": [ + { + "range": { + "start": { + "line": 0, + "character": 23 + }, + "end": { + "line": 0, + "character": 50 + } + }, + "newText": "\"/~/std/testing/asserts.ts\"" + } + ] + } + } + }) + ); + } } diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 05d23bf008..2e787134b2 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -84,6 +84,7 @@ pub struct StateSnapshot { pub assets: AssetsSnapshot, pub cache_metadata: cache::CacheMetadata, pub documents: Documents, + pub maybe_import_map: Option>, pub root_uri: Option, } @@ -425,6 +426,7 @@ impl Inner { assets: self.assets.snapshot(), cache_metadata: self.cache_metadata.clone(), documents: self.documents.clone(), + maybe_import_map: self.maybe_import_map.clone(), root_uri: self.config.root_uri.clone(), }) }