fix(check): npm resolution errors to tsc diagnostics (#28174)

Closes https://github.com/denoland/deno/issues/27188

cli/graph_util.rs

@@ -40,6 +40,7 @@ use deno_runtime::deno_permissions::PermissionsContainer;
 use deno_semver::jsr::JsrDepPackageReq;
 use deno_semver::package::PackageNv;
 use deno_semver::SmallStackString;
+use sys_traits::FsMetadata;
 
 use crate::args::config_to_deno_graph_workspace_member;
 use crate::args::deno_json::TsConfigResolver;
@@ -151,6 +152,25 @@ pub fn graph_walk_errors<'a>(
   roots: &'a [ModuleSpecifier],
   options: GraphWalkErrorsOptions<'a>,
 ) -> impl Iterator<Item = JsErrorBox> + 'a {
+  fn should_ignore_error(
+    sys: &CliSys,
+    graph_kind: GraphKind,
+    error: &ModuleGraphError,
+  ) -> bool {
+    if graph_kind == GraphKind::TypesOnly
+      && matches!(
+        error,
+        ModuleGraphError::ModuleError(ModuleError::UnsupportedMediaType(..))
+      )
+    {
+      return true;
+    }
+
+    // surface these as typescript diagnostics instead
+    graph_kind.include_types()
+      && has_module_graph_error_for_tsc_diagnostic(sys, error)
+  }
+
   graph
     .walk(
       roots.iter(),
@@ -163,6 +183,11 @@ pub fn graph_walk_errors<'a>(
     )
     .errors()
     .flat_map(|error| {
+      if should_ignore_error(sys, graph.graph_kind(), &error) {
+        log::debug!("Ignoring: {}", error);
+        return None;
+      }
+
       let is_root = match &error {
         ModuleGraphError::ResolutionError(_)
         | ModuleGraphError::TypesResolutionError(_) => false,
@@ -180,32 +205,94 @@ pub fn graph_walk_errors<'a>(
         },
       );
 
-      if graph.graph_kind() == GraphKind::TypesOnly
-        && matches!(
-          error,
-          ModuleGraphError::ModuleError(ModuleError::UnsupportedMediaType(..))
-        )
-      {
-        log::debug!("Ignoring: {}", message);
-        return None;
-      }
-
-      if graph.graph_kind().include_types()
-        && (message.contains(RUN_WITH_SLOPPY_IMPORTS_MSG)
-          || matches!(
-            error,
-            ModuleGraphError::ModuleError(ModuleError::Missing(..))
-          ))
-      {
-        // ignore and let typescript surface this as a diagnostic instead
-        log::debug!("Ignoring: {}", message);
-        return None;
-      }
-
       Some(JsErrorBox::new(error.get_class(), message))
     })
 }
 
+fn has_module_graph_error_for_tsc_diagnostic(
+  sys: &CliSys,
+  error: &ModuleGraphError,
+) -> bool {
+  match error {
+    ModuleGraphError::ModuleError(error) => {
+      module_error_for_tsc_diagnostic(sys, error).is_some()
+    }
+    ModuleGraphError::ResolutionError(error) => {
+      resolution_error_for_tsc_diagnostic(error).is_some()
+    }
+    ModuleGraphError::TypesResolutionError(error) => {
+      resolution_error_for_tsc_diagnostic(error).is_some()
+    }
+  }
+}
+
+pub struct ModuleNotFoundGraphErrorRef<'a> {
+  pub specifier: &'a ModuleSpecifier,
+  pub maybe_range: Option<&'a deno_graph::Range>,
+}
+
+pub fn module_error_for_tsc_diagnostic<'a>(
+  sys: &CliSys,
+  error: &'a ModuleError,
+) -> Option<ModuleNotFoundGraphErrorRef<'a>> {
+  match error {
+    ModuleError::Missing(specifier, maybe_range) => {
+      Some(ModuleNotFoundGraphErrorRef {
+        specifier,
+        maybe_range: maybe_range.as_ref(),
+      })
+    }
+    ModuleError::LoadingErr(
+      specifier,
+      maybe_range,
+      ModuleLoadError::Loader(_),
+    ) => {
+      if let Ok(path) = deno_path_util::url_to_file_path(specifier) {
+        if sys.fs_is_dir_no_err(path) {
+          return Some(ModuleNotFoundGraphErrorRef {
+            specifier,
+            maybe_range: maybe_range.as_ref(),
+          });
+        }
+      }
+      None
+    }
+    _ => None,
+  }
+}
+
+pub struct ModuleNotFoundNodeResolutionErrorRef<'a> {
+  pub specifier: &'a str,
+  pub maybe_range: Option<&'a deno_graph::Range>,
+}
+
+pub fn resolution_error_for_tsc_diagnostic(
+  error: &ResolutionError,
+) -> Option<ModuleNotFoundNodeResolutionErrorRef> {
+  match error {
+    ResolutionError::ResolverError {
+      error,
+      specifier,
+      range,
+    } => match error.as_ref() {
+      ResolveError::Other(error) => {
+        // would be nice if there were an easier way of doing this
+        let text = error.to_string();
+        if text.contains("[ERR_MODULE_NOT_FOUND]") {
+          Some(ModuleNotFoundNodeResolutionErrorRef {
+            specifier,
+            maybe_range: Some(range),
+          })
+        } else {
+          None
+        }
+      }
+      _ => None,
+    },
+    _ => None,
+  }
+}
+
 #[derive(Debug, PartialEq, Eq)]
 pub enum EnhanceGraphErrorMode {
   ShowRange,

cli/tools/check.rs

@@ -15,9 +15,7 @@ use deno_core::error::AnyError;
 use deno_core::url::Url;
 use deno_error::JsErrorBox;
 use deno_graph::Module;
-use deno_graph::ModuleError;
 use deno_graph::ModuleGraph;
-use deno_graph::ModuleLoadError;
 use deno_lib::util::hash::FastInsecureHasher;
 use deno_semver::npm::NpmPackageNvReference;
 use deno_terminal::colors;
@@ -38,6 +36,8 @@ use crate::cache::Caches;
 use crate::cache::TypeCheckCache;
 use crate::factory::CliFactory;
 use crate::graph_util::maybe_additional_sloppy_imports_message;
+use crate::graph_util::module_error_for_tsc_diagnostic;
+use crate::graph_util::resolution_error_for_tsc_diagnostic;
 use crate::graph_util::BuildFastCheckGraphOptions;
 use crate::graph_util::ModuleGraphBuilder;
 use crate::node::CliNodeResolver;
@@ -722,7 +722,7 @@ impl<'a> GraphWalker<'a> {
             self
               .missing_diagnostics
               .push(tsc::Diagnostic::from_missing_error(
-                specifier,
+                specifier.as_str(),
                 None,
                 maybe_additional_sloppy_imports_message(self.sys, specifier),
               ));
@@ -763,36 +763,23 @@ impl<'a> GraphWalker<'a> {
       let module = match self.graph.try_get(specifier) {
         Ok(Some(module)) => module,
         Ok(None) => continue,
-        Err(ModuleError::Missing(specifier, maybe_range)) => {
+        Err(err) => {
           if !is_dynamic {
-            self
-              .missing_diagnostics
-              .push(tsc::Diagnostic::from_missing_error(
-                specifier,
-                maybe_range.as_ref(),
-                maybe_additional_sloppy_imports_message(self.sys, specifier),
-              ));
+            if let Some(err) = module_error_for_tsc_diagnostic(self.sys, err) {
+              self.missing_diagnostics.push(
+                tsc::Diagnostic::from_missing_error(
+                  err.specifier.as_str(),
+                  err.maybe_range,
+                  maybe_additional_sloppy_imports_message(
+                    self.sys,
+                    err.specifier,
+                  ),
+                ),
+              );
+            }
           }
           continue;
         }
-        Err(ModuleError::LoadingErr(
-          specifier,
-          maybe_range,
-          ModuleLoadError::Loader(_),
-        )) => {
-          // these will be errors like attempting to load a directory
-          if !is_dynamic {
-            self
-              .missing_diagnostics
-              .push(tsc::Diagnostic::from_missing_error(
-                specifier,
-                maybe_range.as_ref(),
-                maybe_additional_sloppy_imports_message(self.sys, specifier),
-              ));
-          }
-          continue;
-        }
-        Err(_) => continue,
       };
       if is_dynamic && !self.seen.insert(specifier) {
         continue;
@@ -831,11 +818,26 @@ impl<'a> GraphWalker<'a> {
       if let Some(deps) = maybe_module_dependencies {
         for dep in deps.values() {
           // walk both the code and type dependencies
-          if let Some(specifier) = dep.get_code() {
-            self.handle_specifier(specifier, dep.is_dynamic);
+          for resolution in [&dep.maybe_type, &dep.maybe_code] {
+            match resolution {
+              deno_graph::Resolution::Ok(resolution) => {
+                self.handle_specifier(&resolution.specifier, dep.is_dynamic);
+              }
+              deno_graph::Resolution::Err(resolution_error) => {
+                if let Some(err) =
+                  resolution_error_for_tsc_diagnostic(resolution_error)
+                {
+                  self.missing_diagnostics.push(
+                    tsc::Diagnostic::from_missing_error(
+                      err.specifier,
+                      err.maybe_range,
+                      None,
+                    ),
+                  );
+                }
+              }
+              deno_graph::Resolution::None => {}
             }
-          if let Some(specifier) = dep.get_type() {
-            self.handle_specifier(specifier, dep.is_dynamic);
           }
         }
       }

cli/tsc/diagnostics.rs

@@ -152,7 +152,7 @@ pub struct Diagnostic {
 
 impl Diagnostic {
   pub fn from_missing_error(
-    specifier: &ModuleSpecifier,
+    specifier: &str,
     maybe_range: Option<&deno_graph::Range>,
     additional_message: Option<String>,
   ) -> Self {

resolvers/node/analyze.rs

@@ -20,6 +20,7 @@ use sys_traits::FsMetadata;
 use sys_traits::FsRead;
 use url::Url;
 
+use crate::errors::ModuleNotFoundError;
 use crate::resolution::NodeResolverRc;
 use crate::InNpmPackageChecker;
 use crate::IsBuiltInNodeModuleChecker;
@@ -473,7 +474,12 @@ impl<
       last = parent;
     }
 
-    Err(not_found(specifier, referrer_path))
+    Err(JsErrorBox::from_err(ModuleNotFoundError {
+      specifier: UrlOrPath::Path(PathBuf::from(specifier)),
+      typ: "module",
+      maybe_referrer: Some(UrlOrPath::Path(referrer_path.to_path_buf())),
+      suggested_ext: None,
+    }))
   }
 
   fn file_extension_probe(
@@ -509,7 +515,12 @@ impl<
         }
       }
     }
-    Err(not_found(&p.to_string_lossy(), referrer))
+    Err(JsErrorBox::from_err(ModuleNotFoundError {
+      specifier: UrlOrPath::Path(p),
+      maybe_referrer: Some(UrlOrPath::Path(referrer.to_path_buf())),
+      typ: "module",
+      suggested_ext: None,
+    }))
   }
 }
 
@@ -670,15 +681,6 @@ fn parse_specifier(specifier: &str) -> Option<(String, String)> {
   Some((package_name, package_subpath))
 }
 
-fn not_found(path: &str, referrer: &Path) -> JsErrorBox {
-  let msg = format!(
-    "[ERR_MODULE_NOT_FOUND] Cannot find module \"{}\" imported from \"{}\"",
-    path,
-    referrer.to_string_lossy()
-  );
-  JsErrorBox::from_err(std::io::Error::new(std::io::ErrorKind::NotFound, msg))
-}
-
 fn to_double_quote_string(text: &str) -> String {
   // serde can handle this for us
   serde_json::to_string(text).unwrap()

tests/specs/check/module_not_found_in_npm_pkg/__test__.jsonc

@@ -0,0 +1,5 @@
+{
+  "args": "check main.ts",
+  "output": "check.out",
+  "exitCode": 1
+}

tests/specs/check/module_not_found_in_npm_pkg/check.out

@@ -0,0 +1,10 @@
+Check file:///[WILDLINE]/main.ts
+TS2307 [ERROR]: Cannot find module 'package'.
+    at file:///[WILDLINE]/main.ts:1:22
+
+TS2307 [ERROR]: Cannot find module 'package/missing-js'.
+    at file:///[WILDLINE]/main.ts:2:23
+
+Found 2 errors.
+
+error: Type checking failed.

tests/specs/check/module_not_found_in_npm_pkg/main.ts

@@ -0,0 +1,4 @@
+import { Test } from "package";
+import { Other } from "package/missing-js";
+
+console.log(Test, Other);

tests/specs/check/module_not_found_in_npm_pkg/node_modules/package/package.json

@@ -0,0 +1,12 @@
+{
+  "exports": {
+    ".": {
+      "types": "./types.d.ts",
+      "default": "./index.js"
+    },
+    "./missing-js": {
+      "types": "./missing.d.ts",
+      "default": "./missing.js"
+    }
+  }
+}

tests/specs/check/module_not_found_in_npm_pkg/package.json

@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "package": "*"
+  }
+}

tests/specs/node/cjs_analysis_multiple_errors/main.out

@@ -1,3 +1,3 @@
 [# Since this package has multiple cjs export errors and the resolution is done in ]
 [# parallel, we want to ensure the output is deterministic when there's multiple errors]
-error: [ERR_MODULE_NOT_FOUND] Cannot find module "[WILDLINE]not_exists.js" imported from "[WILDLINE]a.js"
+error: [ERR_MODULE_NOT_FOUND] Cannot find module '[WILDLINE]not_exists.js' imported from '[WILDLINE]a.js'