From 2554893851809539836d8690d6bcc8e7bd54428d Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 18 Feb 2025 14:31:06 -0500 Subject: [PATCH 1/2] fix(check): npm resolution errors to tsc diagnostics --- cli/graph_util.rs | 129 +++++++++++++++--- cli/tools/check.rs | 70 +++++----- cli/tsc/diagnostics.rs | 2 +- resolvers/node/analyze.rs | 24 ++-- .../__test__.jsonc | 5 + .../module_not_found_in_npm_pkg/check.out | 10 ++ .../check/module_not_found_in_npm_pkg/main.ts | 4 + .../node_modules/package/index.js | 0 .../node_modules/package/package.json | 12 ++ .../module_not_found_in_npm_pkg/package.json | 5 + 10 files changed, 194 insertions(+), 67 deletions(-) create mode 100644 tests/specs/check/module_not_found_in_npm_pkg/__test__.jsonc create mode 100644 tests/specs/check/module_not_found_in_npm_pkg/check.out create mode 100644 tests/specs/check/module_not_found_in_npm_pkg/main.ts create mode 100644 tests/specs/check/module_not_found_in_npm_pkg/node_modules/package/index.js create mode 100644 tests/specs/check/module_not_found_in_npm_pkg/node_modules/package/package.json create mode 100644 tests/specs/check/module_not_found_in_npm_pkg/package.json diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 861fa664bff05f..c5afdb1aa0d5b5 100644 --- a/cli/graph_util.rs +++ b/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 + '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,30 +205,92 @@ pub fn graph_walk_errors<'a>( }, ); - if graph.graph_kind() == GraphKind::TypesOnly - && matches!( - error, - ModuleGraphError::ModuleError(ModuleError::UnsupportedMediaType(..)) - ) - { - log::debug!("Ignoring: {}", message); - return None; - } + Some(JsErrorBox::new(error.get_class(), message)) + }) +} - 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; +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> { + 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, + } +} - Some(JsErrorBox::new(error.get_class(), message)) - }) +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 { + 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)] diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 861aa874333516..d510470848be77 100644 --- a/cli/tools/check.rs +++ b/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), - )); - } - 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), - )); + 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(_) => 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); - } - if let Some(specifier) = dep.get_type() { - 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 => {} + } } } } diff --git a/cli/tsc/diagnostics.rs b/cli/tsc/diagnostics.rs index 73d20159423888..0810b657ea834a 100644 --- a/cli/tsc/diagnostics.rs +++ b/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, ) -> Self { diff --git a/resolvers/node/analyze.rs b/resolvers/node/analyze.rs index dd1e7dbc41cba9..6ad1bf9bd7ced1 100644 --- a/resolvers/node/analyze.rs +++ b/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() diff --git a/tests/specs/check/module_not_found_in_npm_pkg/__test__.jsonc b/tests/specs/check/module_not_found_in_npm_pkg/__test__.jsonc new file mode 100644 index 00000000000000..25b1a7c3249506 --- /dev/null +++ b/tests/specs/check/module_not_found_in_npm_pkg/__test__.jsonc @@ -0,0 +1,5 @@ +{ + "args": "check main.ts", + "output": "check.out", + "exitCode": 1 +} diff --git a/tests/specs/check/module_not_found_in_npm_pkg/check.out b/tests/specs/check/module_not_found_in_npm_pkg/check.out new file mode 100644 index 00000000000000..dba0055a6c81a5 --- /dev/null +++ b/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. diff --git a/tests/specs/check/module_not_found_in_npm_pkg/main.ts b/tests/specs/check/module_not_found_in_npm_pkg/main.ts new file mode 100644 index 00000000000000..7a3ae3b77d01b8 --- /dev/null +++ b/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); diff --git a/tests/specs/check/module_not_found_in_npm_pkg/node_modules/package/index.js b/tests/specs/check/module_not_found_in_npm_pkg/node_modules/package/index.js new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/tests/specs/check/module_not_found_in_npm_pkg/node_modules/package/package.json b/tests/specs/check/module_not_found_in_npm_pkg/node_modules/package/package.json new file mode 100644 index 00000000000000..ca4e6d0d5eb140 --- /dev/null +++ b/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" + } + } +} diff --git a/tests/specs/check/module_not_found_in_npm_pkg/package.json b/tests/specs/check/module_not_found_in_npm_pkg/package.json new file mode 100644 index 00000000000000..f0b3ee21dd5975 --- /dev/null +++ b/tests/specs/check/module_not_found_in_npm_pkg/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "package": "*" + } +} From 867171ecd56cb6373f187edf7df47e0a0ebc423f Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 18 Feb 2025 14:48:45 -0500 Subject: [PATCH 2/2] fix --- tests/specs/node/cjs_analysis_multiple_errors/main.out | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/specs/node/cjs_analysis_multiple_errors/main.out b/tests/specs/node/cjs_analysis_multiple_errors/main.out index cc8077950b32a2..b7f15ce63bd0ea 100644 --- a/tests/specs/node/cjs_analysis_multiple_errors/main.out +++ b/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'