Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(lsp): show dependency errors for repeated imports #18807

Merged
merged 6 commits into from
Apr 24, 2023
Merged

fix(lsp): show dependency errors for repeated imports #18807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
897142b
fix(lsp): show resolution errors for repeated imports
nayeemrmn Apr 23, 2023
9c8ef00
fix type-only deps
nayeemrmn Apr 23, 2023
9705414
Merge remote-tracking branch 'upstream/main' into lsp-diagnostics-for…
nayeemrmn Apr 24, 2023
2c536cf
bump deno_graph
nayeemrmn Apr 24, 2023
ed45620
Merge remote-tracking branch 'upstream/main' into lsp-diagnostics-for…
nayeemrmn Apr 24, 2023
b452511
review
nayeemrmn Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cli/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
184 changes: 139 additions & 45 deletions cli/lsp/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,24 +857,24 @@ impl DenoDiagnostic {
}

fn diagnose_resolution(
diagnostics: &mut Vec<lsp::Diagnostic>,
lsp_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;
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) {
Expand All @@ -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 {
Expand All @@ -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) =
Expand All @@ -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 =
Expand All @@ -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 {
Expand All @@ -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 {
lsp_diagnostics.push(diagnostic.to_lsp_diagnostic(&range));
}
}
}

/// Generate diagnostics related to a dependency. The dependency is analyzed to
Expand Down Expand Up @@ -1005,17 +996,43 @@ fn diagnose_dependency(
diagnose_resolution(
diagnostics,
snapshot,
&dependency.maybe_code,
dependency.is_dynamic,
dependency.maybe_assert_type.as_deref(),
);
diagnose_resolution(
diagnostics,
snapshot,
&dependency.maybe_type,
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
Expand Down Expand Up @@ -1376,4 +1393,81 @@ 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#"
// @deno-types="bad.d.ts"
import "bad.js";
import "bad.js";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe try adding a test with multiple bad @deno-types directives as well?

Copy link
Collaborator Author

@nayeemrmn nayeemrmn Apr 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one @deno-types directive is stored per JS dep, that would be addressed by denoland/deno_graph#272 Edit: actually no it should ignore @deno-types for deps which already have one attached.

"#,
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": 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 ../",
},
{
"range": {
"start": {
"line": 3,
"character": 15
},
"end": {
"line": 3,
"character": 23
}
},
"severity": 1,
"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 ../",
},
])
);
}
}