From 897142bac821dcfac241615fc063e17e42f79bb9 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Sun, 23 Apr 2023 11:39:41 +0100 Subject: [PATCH 1/4] fix(lsp): show resolution errors for repeated imports --- cli/lsp/diagnostics.rs | 161 ++++++++++++++++++++++++++++++----------- 1 file changed, 117 insertions(+), 44 deletions(-) diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 965075a2d7509c..0a269858be2e61 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -857,24 +857,24 @@ impl DenoDiagnostic { } fn diagnose_resolution( - diagnostics: &mut Vec, + diagnostics_: &mut Vec, snapshot: &language_server::StateSnapshot, resolution: &Resolution, is_dynamic: bool, maybe_assert_type: Option<&str>, + ranges: Vec, ) { + let mut diagnostics = vec![]; match resolution { Resolution::Ok(resolved) => { let specifier = &resolved.specifier; - let range = documents::to_lsp_range(&resolved.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) = snapshot.cache_metadata.get(specifier) { if let Some(message) = metadata.get(&cache::MetadataKey::Warning).cloned() { - diagnostics - .push(DenoDiagnostic::DenoWarn(message).to_lsp_diagnostic(&range)); + diagnostics.push(DenoDiagnostic::DenoWarn(message)); } } if let Some(doc) = snapshot.documents.get(specifier) { @@ -883,13 +883,10 @@ fn diagnose_resolution( // diagnostic that indicates this. This then allows us to issue a code // action to replace the specifier with the final redirected one. if doc_specifier != specifier { - diagnostics.push( - DenoDiagnostic::Redirect { - from: specifier.clone(), - to: doc_specifier.clone(), - } - .to_lsp_diagnostic(&range), - ); + diagnostics.push(DenoDiagnostic::Redirect { + from: specifier.clone(), + to: doc_specifier.clone(), + }); } if doc.media_type() == MediaType::Json { match maybe_assert_type { @@ -900,13 +897,10 @@ fn diagnose_resolution( // not provide a potentially incorrect diagnostic. None if is_dynamic => (), // The module has an incorrect assertion type, diagnostic - Some(assert_type) => diagnostics.push( - DenoDiagnostic::InvalidAssertType(assert_type.to_string()) - .to_lsp_diagnostic(&range), - ), + Some(assert_type) => diagnostics + .push(DenoDiagnostic::InvalidAssertType(assert_type.to_string())), // The module is missing an assertion type, diagnostic - None => diagnostics - .push(DenoDiagnostic::NoAssertType.to_lsp_diagnostic(&range)), + None => diagnostics.push(DenoDiagnostic::NoAssertType), } } } else if let Ok(pkg_ref) = @@ -918,19 +912,15 @@ fn diagnose_resolution( .resolve_pkg_id_from_pkg_req(&pkg_ref.req) .is_err() { - diagnostics.push( - DenoDiagnostic::NoCacheNpm(pkg_ref, specifier.clone()) - .to_lsp_diagnostic(&range), - ); + diagnostics + .push(DenoDiagnostic::NoCacheNpm(pkg_ref, specifier.clone())); } } } else if let Some(module_name) = specifier.as_str().strip_prefix("node:") { if deno_node::resolve_builtin_node_module(module_name).is_err() { - diagnostics.push( - DenoDiagnostic::InvalidNodeSpecifier(specifier.clone()) - .to_lsp_diagnostic(&range), - ); + diagnostics + .push(DenoDiagnostic::InvalidNodeSpecifier(specifier.clone())); } else if let Some(npm_resolver) = &snapshot.maybe_npm_resolver { // check that a @types/node package exists in the resolver let types_node_ref = @@ -939,13 +929,10 @@ fn diagnose_resolution( .resolve_pkg_id_from_pkg_req(&types_node_ref.req) .is_err() { - diagnostics.push( - DenoDiagnostic::NoCacheNpm( - types_node_ref, - ModuleSpecifier::parse("npm:@types/node").unwrap(), - ) - .to_lsp_diagnostic(&range), - ); + diagnostics.push(DenoDiagnostic::NoCacheNpm( + types_node_ref, + ModuleSpecifier::parse("npm:@types/node").unwrap(), + )); } } } else { @@ -958,17 +945,21 @@ fn diagnose_resolution( "blob" => DenoDiagnostic::NoCacheBlob, _ => DenoDiagnostic::NoCache(specifier.clone()), }; - diagnostics.push(deno_diagnostic.to_lsp_diagnostic(&range)); + 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()) - .to_lsp_diagnostic(&documents::to_lsp_range(err.range())), - ), + Resolution::Err(err) => { + diagnostics.push(DenoDiagnostic::ResolutionError(*err.clone())) + } _ => (), } + for range in ranges { + for diagnostic in &diagnostics { + diagnostics_.push(diagnostic.to_lsp_diagnostic(&range)); + } + } } /// Generate diagnostics related to a dependency. The dependency is analyzed to @@ -1008,14 +999,36 @@ fn diagnose_dependency( &dependency.maybe_code, dependency.is_dynamic, dependency.maybe_assert_type.as_deref(), + dependency + .imports + .iter() + .map(|i| documents::to_lsp_range(&i.range)) + .collect(), ); - diagnose_resolution( - diagnostics, - snapshot, - &dependency.maybe_type, - dependency.is_dynamic, - dependency.maybe_assert_type.as_deref(), - ); + // TODO(nayeemrmn): This is a crude way of detecting `@deno-types` which has + // a different specifier and therefore needs a separate call to + // `diagnose_resolution()`. It would be much cleaner if that were modelled as + // a separate dependency: https://github.com/denoland/deno_graph/issues/247. + if !dependency.maybe_type.is_none() + && !dependency + .imports + .iter() + .any(|i| dependency.maybe_type.includes(&i.range.start).is_some()) + { + let range = match &dependency.maybe_type { + Resolution::Ok(resolved) => documents::to_lsp_range(&resolved.range), + Resolution::Err(error) => documents::to_lsp_range(error.range()), + Resolution::None => unreachable!(), + }; + diagnose_resolution( + diagnostics, + snapshot, + &dependency.maybe_type, + dependency.is_dynamic, + dependency.maybe_assert_type.as_deref(), + vec![range], + ); + } } /// Generate diagnostics that come from Deno module resolution logic (like @@ -1376,4 +1389,64 @@ let c: number = "a"; }) ); } + + #[tokio::test] + async fn duplicate_diagnostics_for_duplicate_imports() { + let temp_dir = TempDir::new(); + let (snapshot, _) = setup( + &temp_dir, + &[( + "file:///a.ts", + r#" + import "bad.js"; + import "bad.js"; + "#, + 1, + LanguageId::TypeScript, + )], + None, + ); + let config = mock_config(); + let token = CancellationToken::new(); + let actual = generate_deno_diagnostics(&snapshot, &config, token).await; + assert_eq!(actual.len(), 1); + let (_, _, diagnostics) = actual.first().unwrap(); + assert_eq!( + json!(diagnostics), + json!([ + { + "range": { + "start": { + "line": 1, + "character": 15 + }, + "end": { + "line": 1, + "character": 23 + } + }, + "severity": 1, + "code": "import-prefix-missing", + "source": "deno", + "message": "Relative import path \"bad.js\" not prefixed with / or ./ or ../", + }, + { + "range": { + "start": { + "line": 2, + "character": 15 + }, + "end": { + "line": 2, + "character": 23 + } + }, + "severity": 1, + "code": "import-prefix-missing", + "source": "deno", + "message": "Relative import path \"bad.js\" not prefixed with / or ./ or ../", + } + ]) + ); + } } From 9c8ef00de6f193490020acd5f0d263bb1bc69085 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Sun, 23 Apr 2023 12:36:07 +0100 Subject: [PATCH 2/4] fix type-only deps --- cli/lsp/diagnostics.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 0a269858be2e61..237d6598d59812 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -996,7 +996,11 @@ fn diagnose_dependency( diagnose_resolution( diagnostics, snapshot, - &dependency.maybe_code, + if dependency.maybe_code.is_none() { + &dependency.maybe_type + } else { + &dependency.maybe_code + }, dependency.is_dynamic, dependency.maybe_assert_type.as_deref(), dependency From 2c536cfc211b81ba00d82bd62fd2c242e4859735 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 24 Apr 2023 12:32:29 +0100 Subject: [PATCH 3/4] bump deno_graph --- Cargo.lock | 4 ++-- cli/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 666741cf5dee7c..e2480d54c75692 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -982,9 +982,9 @@ dependencies = [ [[package]] name = "deno_graph" -version = "0.48.0" +version = "0.48.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "57683392402015acc8f20cc3623035f6b2a2c49f1728eef93536c712adafb2c2" +checksum = "dcdbc17bfe49a41dd596ba2a96106b3eae3bd0812e1b63a6fe5883166c1b6fef" dependencies = [ "anyhow", "data-url", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index ebd8583304eac6..a806b709348297 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -44,7 +44,7 @@ deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "dep_gra deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] } deno_doc = "0.62.0" deno_emit = "0.20.0" -deno_graph = "=0.48.0" +deno_graph = "=0.48.1" deno_lint = { version = "0.44.0", features = ["docs"] } deno_lockfile.workspace = true deno_npm.workspace = true From b452511b0446f751eac3e3951e91cccfe3f58e9f Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 25 Apr 2023 00:21:35 +0100 Subject: [PATCH 4/4] review --- cli/lsp/diagnostics.rs | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 237d6598d59812..b650d8e558b0ec 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -857,7 +857,7 @@ impl DenoDiagnostic { } fn diagnose_resolution( - diagnostics_: &mut Vec, + lsp_diagnostics: &mut Vec, snapshot: &language_server::StateSnapshot, resolution: &Resolution, is_dynamic: bool, @@ -957,7 +957,7 @@ fn diagnose_resolution( } for range in ranges { for diagnostic in &diagnostics { - diagnostics_.push(diagnostic.to_lsp_diagnostic(&range)); + lsp_diagnostics.push(diagnostic.to_lsp_diagnostic(&range)); } } } @@ -1402,6 +1402,7 @@ let c: number = "a"; &[( "file:///a.ts", r#" + // @deno-types="bad.d.ts" import "bad.js"; import "bad.js"; "#, @@ -1421,11 +1422,11 @@ let c: number = "a"; { "range": { "start": { - "line": 1, + "line": 2, "character": 15 }, "end": { - "line": 1, + "line": 2, "character": 23 } }, @@ -1437,11 +1438,11 @@ let c: number = "a"; { "range": { "start": { - "line": 2, + "line": 3, "character": 15 }, "end": { - "line": 2, + "line": 3, "character": 23 } }, @@ -1449,7 +1450,23 @@ let c: number = "a"; "code": "import-prefix-missing", "source": "deno", "message": "Relative import path \"bad.js\" not prefixed with / or ./ or ../", - } + }, + { + "range": { + "start": { + "line": 1, + "character": 23 + }, + "end": { + "line": 1, + "character": 33 + } + }, + "severity": 1, + "code": "import-prefix-missing", + "source": "deno", + "message": "Relative import path \"bad.d.ts\" not prefixed with / or ./ or ../", + }, ]) ); }