fix(lsp): show dependency errors for repeated imports #18807
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -857,24 +857,24 @@ impl DenoDiagnostic { | |
| } | ||
|
|
||
| fn diagnose_resolution( | ||
| diagnostics_: &mut Vec<lsp::Diagnostic>, | ||
| snapshot: &language_server::StateSnapshot, | ||
| resolution: &Resolution, | ||
| is_dynamic: bool, | ||
| maybe_assert_type: Option<&str>, | ||
| ranges: Vec<lsp::Range>, | ||
| ) { | ||
| let mut diagnostics = vec![]; | ||
| match resolution { | ||
| Resolution::Ok(resolved) => { | ||
| let specifier = &resolved.specifier; | ||
| // 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)); | ||
| } | ||
| } | ||
| 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(), | ||
| }); | ||
| } | ||
| 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())), | ||
| // The module is missing an assertion type, diagnostic | ||
| 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())); | ||
| } | ||
| } | ||
| } 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())); | ||
| } 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(), | ||
| )); | ||
| } | ||
| } | ||
| } else { | ||
|
|
@@ -958,17 +945,21 @@ fn diagnose_resolution( | |
| "blob" => DenoDiagnostic::NoCacheBlob, | ||
| _ => DenoDiagnostic::NoCache(specifier.clone()), | ||
| }; | ||
| 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())) | ||
| } | ||
| _ => (), | ||
| } | ||
| 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 | ||
|
|
@@ -1005,17 +996,43 @@ fn diagnose_dependency( | |
| diagnose_resolution( | ||
| diagnostics, | ||
| snapshot, | ||
| if dependency.maybe_code.is_none() { | ||
| &dependency.maybe_type | ||
| } else { | ||
| &dependency.maybe_code | ||
| }, | ||
| dependency.is_dynamic, | ||
| dependency.maybe_assert_type.as_deref(), | ||
| dependency | ||
| .imports | ||
| .iter() | ||
| .map(|i| documents::to_lsp_range(&i.range)) | ||
| .collect(), | ||
| ); | ||
| // 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!(), | ||
dsherret marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }; | ||
| 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 +1393,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"; | ||
|
Contributor
There was a problem hiding this comment. Maybe try adding a test with multiple bad
Contributor
Author
There was a problem hiding this comment. Only one |
||
| "#, | ||
| 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 ../", | ||
| } | ||
| ]) | ||
| ); | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.