feat: suggest adding a "node:" prefix for bare specifiers that look like built-in Node modules (#17519)

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(handle_check_error(
-                return Some(Err(custom_error(
+                error.clone().into(),
-                  get_error_class_name(&error.clone().into()),
+                Some(range),
-                  format!("{}\n    at {}", error, range),
+              ));
-                )));
-              }
-              return Some(Err(error.clone().into()));
             }
           }
           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(handle_check_error(
-                    return Some(Err(custom_error(
+                    error.clone().into(),
-                      get_error_class_name(&error.clone().into()),
+                    Some(range),
-                      format!("{}\n    at {}", error, range),
+                  ));
-                    )));
-                  }
-                  return Some(Err(error.clone().into()));
                 }
               }
             }
           }
         }
         ModuleEntry::Error(error) => {
-          if !roots.contains(specifier) {
+          let maybe_range = if roots.contains(specifier) {
-            if let Some(range) = self.referrer_map.get(specifier) {
+            None
-              if !range.specifier.as_str().contains("$deno") {
+          } else {
-                let message = error.to_string();
+            self.referrer_map.get(specifier)
-                return Some(Err(custom_error(
+          };
-                  get_error_class_name(&error.clone().into()),
+          return Some(handle_check_error(
-                  format!("{}\n    at {}", message, range),
+            error.clone().into(),
-                )));
+            maybe_range.map(|r| &**r),
-              }
+          ));
-            }
-          }
-          return Some(Err(error.clone().into()));
         }
         _ => {}
       }
@@ -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::<ResolutionError>() {
+    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
+}

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,8 +796,11 @@ 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<lsp::NumberOrString>) -> bool {
+  pub fn is_fixable(diagnostic: &lsp_types::Diagnostic) -> bool {
-    if let Some(lsp::NumberOrString::String(code)) = code {
+    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"
@@ -775,6 +810,7 @@ impl DenoDiagnostic {
             | "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 {

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,

cli/tests/integration/lsp_tests.rs

@@ -4436,14 +4436,8 @@ fn lsp_completions_node_specifier() {
     json!([
       {
         "range": {
-          "start": {
+          "start": { "line": 0, "character": 15 },
-            "line": 0,
+          "end": { "line": 0, "character": 34 },
-            "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": {
+              "start": { "line": 0, "character": 16 },
-                "line": 0,
+              "end": { "line": 0, "character": 33 },
-                "character": 16
             },
-              "end": {
+            "text": "fs"
-                "line": 0,
-                "character": 33
-              }
-            },
-            "text": "node: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::<Vec<_>>();
+
+  // 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": {
+          "start": { "line": 0, "character": 15 },
-            "line": 0,
+          "end": { "line": 0, "character": 24 }
-            "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": {
+              "start": { "line": 2, "character": 0 },
-                "line": 2,
+              "end": { "line": 2, "character": 0 }
-                "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": {
+        "position": { "line": 2, "character": 3 },
-          "line": 2,
-          "character": 3
-        },
         "context": {
           "triggerKind": 2,
           "triggerCharacter": "."

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![(
+  envs: env_vars_for_npm_tests_no_sync_download(),
-    "DENO_NODE_COMPAT_URL".to_string(),
-    test_util::std_file_url()
-  )],
   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![(
+  envs: env_vars_for_npm_tests_no_sync_download(),
-    "DENO_NODE_COMPAT_URL".to_string(),
-    test_util::std_file_url()
-  )],
   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,
+});

cli/tests/testdata/jsx/deno.lock

@@ -0,0 +1,6 @@
+{
+  "version": "2",
+  "remote": {
+    "http://localhost:4545/jsx/jsx-dev-runtime/index.ts": "183c5bf1cfb82b15fc1e8cca15593d4816035759532d851abd4476df378c8412"
+  }
+}

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.

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.

cli/tests/testdata/run/node_prefix_missing/main.ts

@@ -0,0 +1,3 @@
+import fs from "fs";
+
+console.log(fs.writeFile);

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