From 658ec2aaf9c7e0d0b4ded4e97a3d89dc2fa25806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Mon, 25 Nov 2019 15:33:23 +0100 Subject: [PATCH] better error message for missing module (#3402) --- cli/file_fetcher.rs | 77 +++++++++++-------- cli/global_state.rs | 3 +- cli/js/compiler.ts | 7 +- cli/lib.rs | 4 +- cli/ops/compiler.rs | 15 +++- cli/state.rs | 3 +- cli/tests/error_004_missing_module.ts.out | 2 +- .../error_005_missing_dynamic_import.ts.out | 2 +- cli/tests/error_006_import_ext_failure.ts.out | 2 +- core/modules.rs | 13 +++- 10 files changed, 81 insertions(+), 47 deletions(-) diff --git a/cli/file_fetcher.rs b/cli/file_fetcher.rs index 79ea3ab81f..b156e98fd5 100644 --- a/cli/file_fetcher.rs +++ b/cli/file_fetcher.rs @@ -134,6 +134,7 @@ impl SourceFileFetcher { pub fn fetch_source_file_async( self: &Self, specifier: &ModuleSpecifier, + maybe_referrer: Option, ) -> Pin> { let module_url = specifier.as_url().to_owned(); debug!("fetch_source_file. specifier {} ", &module_url); @@ -156,12 +157,16 @@ impl SourceFileFetcher { .then(move |result| { let mut out = match result.map_err(|err| { if err.kind() == ErrorKind::NotFound { - // For NotFound, change the message to something better. - DenoError::new( - ErrorKind::NotFound, - format!("Cannot resolve module \"{}\"", module_url.to_string()), - ) - .into() + let msg = if let Some(referrer) = maybe_referrer { + format!( + "Cannot resolve module \"{}\" from \"{}\"", + module_url.to_string(), + referrer + ) + } else { + format!("Cannot resolve module \"{}\"", module_url.to_string()) + }; + DenoError::new(ErrorKind::NotFound, msg).into() } else { err } @@ -1011,10 +1016,12 @@ mod tests { ); // first download - tokio_util::run(fetcher.fetch_source_file_async(&specifier).then(|r| { - assert!(r.is_ok()); - futures::future::ok(()) - })); + tokio_util::run(fetcher.fetch_source_file_async(&specifier, None).then( + |r| { + assert!(r.is_ok()); + futures::future::ok(()) + }, + )); let result = fs::File::open(&headers_file_name); assert!(result.is_ok()); @@ -1026,10 +1033,12 @@ mod tests { // download file again, it should use already fetched file even though `use_disk_cache` is set to // false, this can be verified using source header file creation timestamp (should be // the same as after first download) - tokio_util::run(fetcher.fetch_source_file_async(&specifier).then(|r| { - assert!(r.is_ok()); - futures::future::ok(()) - })); + tokio_util::run(fetcher.fetch_source_file_async(&specifier, None).then( + |r| { + assert!(r.is_ok()); + futures::future::ok(()) + }, + )); let result = fs::File::open(&headers_file_name); assert!(result.is_ok()); @@ -1443,20 +1452,24 @@ mod tests { // Test failure case. let specifier = ModuleSpecifier::resolve_url(file_url!("/baddir/hello.ts")).unwrap(); - tokio_util::run(fetcher.fetch_source_file_async(&specifier).then(|r| { - assert!(r.is_err()); - futures::future::ok(()) - })); + tokio_util::run(fetcher.fetch_source_file_async(&specifier, None).then( + |r| { + assert!(r.is_err()); + futures::future::ok(()) + }, + )); let p = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) .join("js/main.ts") .to_owned(); let specifier = ModuleSpecifier::resolve_url_or_path(p.to_str().unwrap()).unwrap(); - tokio_util::run(fetcher.fetch_source_file_async(&specifier).then(|r| { - assert!(r.is_ok()); - futures::future::ok(()) - })); + tokio_util::run(fetcher.fetch_source_file_async(&specifier, None).then( + |r| { + assert!(r.is_ok()); + futures::future::ok(()) + }, + )); } #[test] @@ -1467,20 +1480,24 @@ mod tests { // Test failure case. let specifier = ModuleSpecifier::resolve_url(file_url!("/baddir/hello.ts")).unwrap(); - tokio_util::run(fetcher.fetch_source_file_async(&specifier).then(|r| { - assert!(r.is_err()); - futures::future::ok(()) - })); + tokio_util::run(fetcher.fetch_source_file_async(&specifier, None).then( + |r| { + assert!(r.is_err()); + futures::future::ok(()) + }, + )); let p = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) .join("js/main.ts") .to_owned(); let specifier = ModuleSpecifier::resolve_url_or_path(p.to_str().unwrap()).unwrap(); - tokio_util::run(fetcher.fetch_source_file_async(&specifier).then(|r| { - assert!(r.is_ok()); - futures::future::ok(()) - })); + tokio_util::run(fetcher.fetch_source_file_async(&specifier, None).then( + |r| { + assert!(r.is_ok()); + futures::future::ok(()) + }, + )); } #[test] diff --git a/cli/global_state.rs b/cli/global_state.rs index 38aff2093d..fd4cf80471 100644 --- a/cli/global_state.rs +++ b/cli/global_state.rs @@ -124,13 +124,14 @@ impl ThreadSafeGlobalState { pub fn fetch_compiled_module( self: &Self, module_specifier: &ModuleSpecifier, + maybe_referrer: Option, ) -> impl Future> { let state1 = self.clone(); let state2 = self.clone(); self .file_fetcher - .fetch_source_file_async(&module_specifier) + .fetch_source_file_async(&module_specifier, maybe_referrer) .and_then(move |out| match out.media_type { msg::MediaType::Unknown => state1.js_compiler.compile_async(&out), msg::MediaType::Json => state1.json_compiler.compile_async(&out), diff --git a/cli/js/compiler.ts b/cli/js/compiler.ts index 4ad4ae8a43..35e3325826 100644 --- a/cli/js/compiler.ts +++ b/cli/js/compiler.ts @@ -171,7 +171,8 @@ class SourceFile { /** Cache the source file to be able to be retrieved by `moduleSpecifier` and * `containingFile`. */ - cache(moduleSpecifier: string, containingFile: string): void { + cache(moduleSpecifier: string, containingFile?: string): void { + containingFile = containingFile || ""; let innerCache = SourceFile._specifierCache.get(containingFile); if (!innerCache) { innerCache = new Map(); @@ -269,7 +270,7 @@ function fetchAsset(name: string): string { /** Ops to Rust to resolve and fetch modules meta data. */ function fetchSourceFiles( specifiers: string[], - referrer: string + referrer?: string ): Promise { util.log("compiler::fetchSourceFiles", { specifiers, referrer }); return sendAsync(dispatch.OP_FETCH_SOURCE_FILES, { @@ -286,7 +287,7 @@ function fetchSourceFiles( * that should be actually resolved. */ async function processImports( specifiers: Array<[string, string]>, - referrer = "" + referrer?: string ): Promise { if (!specifiers.length) { return []; diff --git a/cli/lib.rs b/cli/lib.rs index df9c3db54c..4161474225 100644 --- a/cli/lib.rs +++ b/cli/lib.rs @@ -176,7 +176,7 @@ async fn print_file_info(worker: Worker, module_specifier: ModuleSpecifier) { let maybe_source_file = global_state_ .file_fetcher - .fetch_source_file_async(&module_specifier) + .fetch_source_file_async(&module_specifier, None) .await; if let Err(err) = maybe_source_file { println!("{}", err); @@ -197,7 +197,7 @@ async fn print_file_info(worker: Worker, module_specifier: ModuleSpecifier) { let maybe_compiled = global_state_ .clone() - .fetch_compiled_module(&module_specifier) + .fetch_compiled_module(&module_specifier, None) .await; if let Err(e) = maybe_compiled { debug!("compiler error exiting!"); diff --git a/cli/ops/compiler.rs b/cli/ops/compiler.rs index fdb62ca321..b45f6d9372 100644 --- a/cli/ops/compiler.rs +++ b/cli/ops/compiler.rs @@ -51,7 +51,7 @@ fn op_cache( #[derive(Deserialize)] struct FetchSourceFilesArgs { specifiers: Vec, - referrer: String, + referrer: Option, } fn op_fetch_source_files( @@ -65,14 +65,23 @@ fn op_fetch_source_files( // to this. Need a test to demonstrate the hole. let is_dyn_import = false; + let (referrer, ref_specifier) = if let Some(referrer) = args.referrer { + let specifier = ModuleSpecifier::resolve_url(&referrer) + .expect("Referrer is not a valid specifier"); + (referrer, Some(specifier)) + } else { + // main script import + (".".to_string(), None) + }; + let mut futures = vec![]; for specifier in &args.specifiers { let resolved_specifier = - state.resolve(specifier, &args.referrer, false, is_dyn_import)?; + state.resolve(specifier, &referrer, false, is_dyn_import)?; let fut = state .global_state .file_fetcher - .fetch_source_file_async(&resolved_specifier); + .fetch_source_file_async(&resolved_specifier, ref_specifier.clone()); futures.push(fut); } diff --git a/cli/state.rs b/cli/state.rs index 5ce4328937..e99bff08ea 100644 --- a/cli/state.rs +++ b/cli/state.rs @@ -178,12 +178,13 @@ impl Loader for ThreadSafeState { fn load( &self, module_specifier: &ModuleSpecifier, + maybe_referrer: Option, ) -> Pin> { self.metrics.resolve_count.fetch_add(1, Ordering::SeqCst); let module_url_specified = module_specifier.to_string(); let fut = self .global_state - .fetch_compiled_module(module_specifier) + .fetch_compiled_module(module_specifier, maybe_referrer) .map_ok(|compiled_module| deno::SourceCodeInfo { // Real module name, might be different from initial specifier // due to redirections. diff --git a/cli/tests/error_004_missing_module.ts.out b/cli/tests/error_004_missing_module.ts.out index 7a5f509380..a9be1419a6 100644 --- a/cli/tests/error_004_missing_module.ts.out +++ b/cli/tests/error_004_missing_module.ts.out @@ -1,4 +1,4 @@ -[WILDCARD]error: Uncaught NotFound: Cannot resolve module "[WILDCARD]/bad-module.ts" +[WILDCARD]error: Uncaught NotFound: Cannot resolve module "[WILDCARD]/bad-module.ts" from "[WILDCARD]/error_004_missing_module.ts" [WILDCARD]dispatch_json.ts:[WILDCARD] at DenoError ([WILDCARD]errors.ts:[WILDCARD]) at unwrapResponse ([WILDCARD]dispatch_json.ts:[WILDCARD]) diff --git a/cli/tests/error_005_missing_dynamic_import.ts.out b/cli/tests/error_005_missing_dynamic_import.ts.out index 7a5f509380..24031a61c9 100644 --- a/cli/tests/error_005_missing_dynamic_import.ts.out +++ b/cli/tests/error_005_missing_dynamic_import.ts.out @@ -1,4 +1,4 @@ -[WILDCARD]error: Uncaught NotFound: Cannot resolve module "[WILDCARD]/bad-module.ts" +[WILDCARD]error: Uncaught NotFound: Cannot resolve module "[WILDCARD]/bad-module.ts" from "[WILDCARD]/error_005_missing_dynamic_import.ts" [WILDCARD]dispatch_json.ts:[WILDCARD] at DenoError ([WILDCARD]errors.ts:[WILDCARD]) at unwrapResponse ([WILDCARD]dispatch_json.ts:[WILDCARD]) diff --git a/cli/tests/error_006_import_ext_failure.ts.out b/cli/tests/error_006_import_ext_failure.ts.out index d88477df86..deda91c2f7 100644 --- a/cli/tests/error_006_import_ext_failure.ts.out +++ b/cli/tests/error_006_import_ext_failure.ts.out @@ -1,4 +1,4 @@ -[WILDCARD]error: Uncaught NotFound: Cannot resolve module "[WILDCARD]/non-existent" +[WILDCARD]error: Uncaught NotFound: Cannot resolve module "[WILDCARD]/non-existent" from "[WILDCARD]/error_006_import_ext_failure.ts" [WILDCARD]dispatch_json.ts:[WILDCARD] at DenoError ([WILDCARD]errors.ts:[WILDCARD]) at unwrapResponse ([WILDCARD]dispatch_json.ts:[WILDCARD]) diff --git a/core/modules.rs b/core/modules.rs index 9f3434a4f7..dd45f554d9 100644 --- a/core/modules.rs +++ b/core/modules.rs @@ -48,6 +48,7 @@ pub trait Loader: Send + Sync { fn load( &self, module_specifier: &ModuleSpecifier, + maybe_referrer: Option, ) -> Pin>; } @@ -154,7 +155,7 @@ impl RecursiveLoad { // integrated into one thing. self .pending - .push(self.loader.load(&module_specifier).boxed()); + .push(self.loader.load(&module_specifier, None).boxed()); self.state = State::LoadingRoot; Ok(()) @@ -166,6 +167,8 @@ impl RecursiveLoad { referrer: &str, parent_id: deno_mod, ) -> Result<(), ErrBox> { + let referrer_specifier = ModuleSpecifier::resolve_url(referrer) + .expect("Referrer should be a valid specifier"); let module_specifier = self.loader.resolve( specifier, referrer, @@ -181,9 +184,10 @@ impl RecursiveLoad { if !modules.is_registered(module_name) && !self.is_pending.contains(&module_specifier) { - self - .pending - .push(self.loader.load(&module_specifier).boxed()); + let fut = self + .loader + .load(&module_specifier, Some(referrer_specifier.clone())); + self.pending.push(fut.boxed()); self.is_pending.insert(module_specifier); } @@ -739,6 +743,7 @@ mod tests { fn load( &self, module_specifier: &ModuleSpecifier, + _maybe_referrer: Option, ) -> Pin> { let mut loads = self.loads.lock().unwrap(); loads.push(module_specifier.to_string());