diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 243fd6eef4..3cf3b0d0b0 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -27,7 +27,9 @@ use deno_graph::ModuleGraph; use deno_graph::ModuleGraphError; use deno_graph::ModuleKind; use deno_graph::Range; +use deno_graph::ResolutionError; use deno_graph::Resolved; +use deno_graph::SpecifierError; use deno_runtime::permissions::PermissionsContainer; use std::collections::BTreeMap; use std::collections::HashMap; @@ -346,13 +348,10 @@ impl GraphData { if check_types { if let Some(Resolved::Err(error)) = maybe_types { let range = error.range(); - if !range.specifier.as_str().contains("$deno") { - return Some(Err(custom_error( - get_error_class_name(&error.clone().into()), - format!("{}\n at {}", error, range), - ))); - } - return Some(Err(error.clone().into())); + return Some(handle_check_error( + error.clone().into(), + Some(range), + )); } } for (_, dep) in dependencies.iter() { @@ -365,31 +364,25 @@ impl GraphData { for resolved in resolutions { if let Resolved::Err(error) = resolved { let range = error.range(); - if !range.specifier.as_str().contains("$deno") { - return Some(Err(custom_error( - get_error_class_name(&error.clone().into()), - format!("{}\n at {}", error, range), - ))); - } - return Some(Err(error.clone().into())); + return Some(handle_check_error( + error.clone().into(), + Some(range), + )); } } } } } ModuleEntry::Error(error) => { - if !roots.contains(specifier) { - if let Some(range) = self.referrer_map.get(specifier) { - if !range.specifier.as_str().contains("$deno") { - let message = error.to_string(); - return Some(Err(custom_error( - get_error_class_name(&error.clone().into()), - format!("{}\n at {}", message, range), - ))); - } - } - } - return Some(Err(error.clone().into())); + let maybe_range = if roots.contains(specifier) { + None + } else { + self.referrer_map.get(specifier) + }; + return Some(handle_check_error( + error.clone().into(), + maybe_range.map(|r| &**r), + )); } _ => {} } @@ -629,3 +622,42 @@ pub fn error_for_any_npm_specifier( Ok(()) } } + +fn handle_check_error( + error: AnyError, + maybe_range: Option<&deno_graph::Range>, +) -> Result<(), AnyError> { + let mut message = if let Some(err) = error.downcast_ref::() { + enhanced_resolution_error_message(err) + } else { + format!("{}", error) + }; + + if let Some(range) = maybe_range { + if !range.specifier.as_str().contains("$deno") { + message.push_str(&format!("\n at {}", range)); + } + } + + Err(custom_error(get_error_class_name(&error), message)) +} + +/// Adds more explanatory information to a resolution error. +pub fn enhanced_resolution_error_message(error: &ResolutionError) -> String { + let mut message = format!("{}", error); + + if let ResolutionError::InvalidSpecifier { + error: SpecifierError::ImportPrefixMissing(specifier, _), + .. + } = error + { + if crate::node::resolve_builtin_node_module(specifier).is_ok() { + message.push_str(&format!( + "\nIf you want to use a built-in Node module, add a \"node:\" prefix (ex. \"node:{}\").", + specifier + )); + } + } + + message +} diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 4faff2faea..9f3c409ccb 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -13,6 +13,7 @@ use super::tsc; use super::tsc::TsServer; use crate::args::LintOptions; +use crate::graph_util::enhanced_resolution_error_message; use crate::node; use crate::npm::NpmPackageReference; use crate::tools::lint::get_configured_rules; @@ -25,7 +26,9 @@ use deno_core::serde::Deserialize; use deno_core::serde_json; use deno_core::serde_json::json; use deno_core::ModuleSpecifier; +use deno_graph::ResolutionError; use deno_graph::Resolved; +use deno_graph::SpecifierError; use deno_lint::rules::LintRule; use deno_runtime::tokio_util::create_basic_runtime; use log::error; @@ -572,6 +575,12 @@ struct DiagnosticDataSpecifier { pub specifier: ModuleSpecifier, } +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct DiagnosticDataStrSpecifier { + pub specifier: String, +} + #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] struct DiagnosticDataRedirect { @@ -621,9 +630,6 @@ pub enum DenoDiagnostic { impl DenoDiagnostic { fn code(&self) -> &str { - use deno_graph::ResolutionError; - use deno_graph::SpecifierError; - match self { Self::DenoWarn(_) => "deno-warn", Self::ImportMapRemap { .. } => "import-map-remap", @@ -749,6 +755,32 @@ impl DenoDiagnostic { ..Default::default() } } + "import-prefix-missing" => { + // if an import-prefix-missing diagnostic ends up here, then that currently + // will only ever occur for a possible "node:" specifier, so don't bother + // checking if it's actually a "node:"" specifier + let data = diagnostic + .data + .clone() + .ok_or_else(|| anyhow!("Diagnostic is missing data"))?; + let data: DiagnosticDataStrSpecifier = serde_json::from_value(data)?; + lsp::CodeAction { + title: format!("Update specifier to node:{}", data.specifier), + 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!("\"node:{}\"", data.specifier), + range: diagnostic.range, + }], + )])), + ..Default::default() + }), + ..Default::default() + } + } _ => { return Err(anyhow!( "Unsupported diagnostic code (\"{}\") provided.", @@ -764,17 +796,21 @@ impl DenoDiagnostic { /// Given a reference to the code from an LSP diagnostic, determine if the /// diagnostic is fixable or not - pub fn is_fixable(code: &Option) -> bool { - if let Some(lsp::NumberOrString::String(code)) = code { - matches!( - code.as_str(), - "import-map-remap" - | "no-cache" - | "no-cache-npm" - | "no-cache-data" - | "no-assert-type" - | "redirect" - ) + pub fn is_fixable(diagnostic: &lsp_types::Diagnostic) -> bool { + if let Some(lsp::NumberOrString::String(code)) = &diagnostic.code { + if code == "import-prefix-missing" { + diagnostic.data.is_some() + } else { + matches!( + code.as_str(), + "import-map-remap" + | "no-cache" + | "no-cache-npm" + | "no-cache-data" + | "no-assert-type" + | "redirect" + ) + } } else { false } @@ -794,7 +830,22 @@ impl DenoDiagnostic { Self::NoCacheNpm(pkg_ref, specifier) => (lsp::DiagnosticSeverity::ERROR, format!("Uncached or missing npm package: \"{}\".", pkg_ref.req), Some(json!({ "specifier": specifier }))), Self::NoLocal(specifier) => (lsp::DiagnosticSeverity::ERROR, format!("Unable to load a local module: \"{}\".\n Please check the file path.", specifier), None), Self::Redirect { from, to} => (lsp::DiagnosticSeverity::INFORMATION, format!("The import of \"{}\" was redirected to \"{}\".", from, to), Some(json!({ "specifier": from, "redirect": to }))), - Self::ResolutionError(err) => (lsp::DiagnosticSeverity::ERROR, err.to_string(), None), + Self::ResolutionError(err) => ( + lsp::DiagnosticSeverity::ERROR, + enhanced_resolution_error_message(err), + if let ResolutionError::InvalidSpecifier { + error: SpecifierError::ImportPrefixMissing(specifier, _), + .. + } = err { + if crate::node::resolve_builtin_node_module(specifier).is_ok() { + Some(json!({ "specifier": specifier })) + } else { + None + } + } else { + None + }, + ), Self::InvalidNodeSpecifier(specifier) => (lsp::DiagnosticSeverity::ERROR, format!("Unknown Node built-in module: {}", specifier.path()), None), }; lsp::Diagnostic { diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 68f2447c15..0941fe0fba 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -1368,7 +1368,7 @@ impl Inner { _ => false, }, "deno-lint" => matches!(&d.code, Some(_)), - "deno" => diagnostics::DenoDiagnostic::is_fixable(&d.code), + "deno" => diagnostics::DenoDiagnostic::is_fixable(d), _ => false, }, None => false, diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index 4f08ad84bf..f7c0e61375 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -4436,14 +4436,8 @@ fn lsp_completions_node_specifier() { json!([ { "range": { - "start": { - "line": 0, - "character": 15 - }, - "end": { - "line": 0, - "character": 34 - } + "start": { "line": 0, "character": 15 }, + "end": { "line": 0, "character": 34 }, }, "severity": 1, "code": "resolver-error", @@ -4453,7 +4447,7 @@ fn lsp_completions_node_specifier() { ]) ); - // update to have node:fs import + // update to have fs import client .write_notification( "textDocument/didChange", @@ -4465,22 +4459,109 @@ fn lsp_completions_node_specifier() { "contentChanges": [ { "range": { - "start": { - "line": 0, - "character": 16 - }, - "end": { - "line": 0, - "character": 33 - } + "start": { "line": 0, "character": 16 }, + "end": { "line": 0, "character": 33 }, }, - "text": "node:fs" + "text": "fs" } ] }), ) .unwrap(); let diagnostics = read_diagnostics(&mut client); + let diagnostics = diagnostics + .with_file_and_source("file:///a/file.ts", "deno") + .diagnostics + .into_iter() + .filter(|d| { + d.code + == Some(lsp::NumberOrString::String( + "import-prefix-missing".to_string(), + )) + }) + .collect::>(); + + // get the quick fixes + let (maybe_res, maybe_err) = client + .write_request( + "textDocument/codeAction", + json!({ + "textDocument": { + "uri": "file:///a/file.ts" + }, + "range": { + "start": { "line": 0, "character": 16 }, + "end": { "line": 0, "character": 18 }, + }, + "context": { + "diagnostics": json!(diagnostics), + "only": [ + "quickfix" + ] + } + }), + ) + .unwrap(); + assert!(maybe_err.is_none()); + assert_eq!( + maybe_res, + Some(json!([{ + "title": "Update specifier to node:fs", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { "line": 0, "character": 15 }, + "end": { "line": 0, "character": 19 } + }, + "severity": 1, + "code": "import-prefix-missing", + "source": "deno", + "message": "Relative import path \"fs\" not prefixed with / or ./ or ../\nIf you want to use a built-in Node module, add a \"node:\" prefix (ex. \"node:fs\").", + "data": { + "specifier": "fs" + }, + } + ], + "edit": { + "changes": { + "file:///a/file.ts": [ + { + "range": { + "start": { "line": 0, "character": 15 }, + "end": { "line": 0, "character": 19 } + }, + "newText": "\"node:fs\"" + } + ] + } + } + }])) + ); + + // update to have node:fs import + client + .write_notification( + "textDocument/didChange", + json!({ + "textDocument": { + "uri": "file:///a/file.ts", + "version": 3, + }, + "contentChanges": [ + { + "range": { + "start": { "line": 0, "character": 15 }, + "end": { "line": 0, "character": 19 }, + }, + "text": "\"node:fs\"", + } + ] + }), + ) + .unwrap(); + + let diagnostics = read_diagnostics(&mut client); let cache_diagnostics = diagnostics .with_file_and_source("file:///a/file.ts", "deno") .diagnostics @@ -4495,14 +4576,8 @@ fn lsp_completions_node_specifier() { json!([ { "range": { - "start": { - "line": 0, - "character": 15 - }, - "end": { - "line": 0, - "character": 24 - } + "start": { "line": 0, "character": 15 }, + "end": { "line": 0, "character": 24 } }, "data": { "specifier": "npm:@types/node", @@ -4539,19 +4614,13 @@ fn lsp_completions_node_specifier() { json!({ "textDocument": { "uri": "file:///a/file.ts", - "version": 2 + "version": 4 }, "contentChanges": [ { "range": { - "start": { - "line": 2, - "character": 0 - }, - "end": { - "line": 2, - "character": 0 - } + "start": { "line": 2, "character": 0 }, + "end": { "line": 2, "character": 0 } }, "text": "fs." } @@ -4568,10 +4637,7 @@ fn lsp_completions_node_specifier() { "textDocument": { "uri": "file:///a/file.ts" }, - "position": { - "line": 2, - "character": 3 - }, + "position": { "line": 2, "character": 3 }, "context": { "triggerKind": 2, "triggerCharacter": "." diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index 4c1f6363a1..65d567658a 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -12,6 +12,7 @@ use tokio::task::LocalSet; use trust_dns_client::serialize::txt::Lexer; use trust_dns_client::serialize::txt::Parser; use util::assert_contains; +use util::env_vars_for_npm_tests_no_sync_download; itest!(stdout_write_all { args: "run --quiet run/stdout_write_all.ts", @@ -3749,22 +3750,23 @@ fn stdio_streams_are_locked_in_permission_prompt() { }); } -itest!(run_node_builtin_modules_ts { +itest!(node_builtin_modules_ts { args: "run --quiet run/node_builtin_modules/mod.ts", output: "run/node_builtin_modules/mod.ts.out", - envs: vec![( - "DENO_NODE_COMPAT_URL".to_string(), - test_util::std_file_url() - )], + envs: env_vars_for_npm_tests_no_sync_download(), exit_code: 0, }); -itest!(run_node_builtin_modules_js { +itest!(node_builtin_modules_js { args: "run --quiet run/node_builtin_modules/mod.js", output: "run/node_builtin_modules/mod.js.out", - envs: vec![( - "DENO_NODE_COMPAT_URL".to_string(), - test_util::std_file_url() - )], + envs: env_vars_for_npm_tests_no_sync_download(), exit_code: 0, }); + +itest!(node_prefix_missing { + args: "run --quiet run/node_prefix_missing/main.ts", + output: "run/node_prefix_missing/main.ts.out", + envs: env_vars_for_npm_tests_no_sync_download(), + exit_code: 1, +}); diff --git a/cli/tests/testdata/jsx/deno.lock b/cli/tests/testdata/jsx/deno.lock new file mode 100644 index 0000000000..011e8fe108 --- /dev/null +++ b/cli/tests/testdata/jsx/deno.lock @@ -0,0 +1,6 @@ +{ + "version": "2", + "remote": { + "http://localhost:4545/jsx/jsx-dev-runtime/index.ts": "183c5bf1cfb82b15fc1e8cca15593d4816035759532d851abd4476df378c8412" + } +} \ No newline at end of file diff --git a/cli/tests/testdata/run/035_cached_only_flag.out b/cli/tests/testdata/run/035_cached_only_flag.out index 3bda398b6e..f677ec9153 100644 --- a/cli/tests/testdata/run/035_cached_only_flag.out +++ b/cli/tests/testdata/run/035_cached_only_flag.out @@ -1,4 +1 @@ error: Specifier not found in cache: "http://127.0.0.1:4545/run/019_media_types.ts", --cached-only is specified. - -Caused by: - Specifier not found in cache: "http://127.0.0.1:4545/run/019_media_types.ts", --cached-only is specified. diff --git a/cli/tests/testdata/run/052_no_remote_flag.out b/cli/tests/testdata/run/052_no_remote_flag.out index f511f6d94e..2f5950ee89 100644 --- a/cli/tests/testdata/run/052_no_remote_flag.out +++ b/cli/tests/testdata/run/052_no_remote_flag.out @@ -1,4 +1 @@ error: A remote specifier was requested: "http://127.0.0.1:4545/run/019_media_types.ts", but --no-remote is specified. - -Caused by: - A remote specifier was requested: "http://127.0.0.1:4545/run/019_media_types.ts", but --no-remote is specified. diff --git a/cli/tests/testdata/run/node_prefix_missing/main.ts b/cli/tests/testdata/run/node_prefix_missing/main.ts new file mode 100644 index 0000000000..c5c1885a2f --- /dev/null +++ b/cli/tests/testdata/run/node_prefix_missing/main.ts @@ -0,0 +1,3 @@ +import fs from "fs"; + +console.log(fs.writeFile); diff --git a/cli/tests/testdata/run/node_prefix_missing/main.ts.out b/cli/tests/testdata/run/node_prefix_missing/main.ts.out new file mode 100644 index 0000000000..fd19ed55f7 --- /dev/null +++ b/cli/tests/testdata/run/node_prefix_missing/main.ts.out @@ -0,0 +1,3 @@ +error: Relative import path "fs" not prefixed with / or ./ or ../ +If you want to use a built-in Node module, add a "node:" prefix (ex. "node:fs"). + at file:///[WILDCARD]/main.ts:1:16