From 3a33510bd4a169aba00393c2b7e88bf7fa0cad06 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 31 May 2021 01:20:34 +0100 Subject: [PATCH] fix(cli): Don't statically error on dynamic unmapped bare specifiers (#10618) Fixes #10168 Fixes #10615 Fixes #10616 --- cli/import_map.rs | 63 ++++++++++--------- cli/lsp/analysis.rs | 8 +-- cli/module_graph.rs | 18 +++--- cli/module_loader.rs | 7 +-- .../092_import_map_unmapped_bare_specifier.ts | 1 + ..._import_map_unmapped_bare_specifier.ts.out | 4 ++ .../error_011_bad_module_specifier.ts.out | 2 +- ...or_012_bad_dynamic_import_specifier.ts.out | 2 +- ...rror_014_catch_dynamic_import_error.js.out | 4 +- cli/tests/error_type_definitions.ts.out | 2 +- cli/tests/integration_tests.rs | 6 ++ core/module_specifier.rs | 4 +- 12 files changed, 66 insertions(+), 55 deletions(-) create mode 100644 cli/tests/092_import_map_unmapped_bare_specifier.ts create mode 100644 cli/tests/092_import_map_unmapped_bare_specifier.ts.out diff --git a/cli/import_map.rs b/cli/import_map.rs index d18633545f..f2126bed92 100644 --- a/cli/import_map.rs +++ b/cli/import_map.rs @@ -14,11 +14,25 @@ use std::error::Error; use std::fmt; #[derive(Debug)] -pub struct ImportMapError(String); +pub enum ImportMapError { + UnmappedBareSpecifier(String, Option), + Other(String), +} impl fmt::Display for ImportMapError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.pad(&self.0) + match self { + ImportMapError::UnmappedBareSpecifier(specifier, maybe_referrer) => write!( + f, + "Relative import path \"{}\" not prefixed with / or ./ or ../ and not in import map{}", + specifier, + match maybe_referrer { + Some(referrer) => format!(" from \"{}\"", referrer), + None => format!(""), + } + ), + ImportMapError::Other(message) => f.pad(message), + } } } @@ -51,7 +65,7 @@ impl ImportMap { let v: Value = match serde_json::from_str(json_string) { Ok(v) => v, Err(_) => { - return Err(ImportMapError( + return Err(ImportMapError::Other( "Unable to parse import map JSON".to_string(), )); } @@ -60,7 +74,7 @@ impl ImportMap { match v { Value::Object(_) => {} _ => { - return Err(ImportMapError( + return Err(ImportMapError::Other( "Import map JSON must be an object".to_string(), )); } @@ -70,7 +84,7 @@ impl ImportMap { let normalized_imports = match &v.get("imports") { Some(imports_map) => { if !imports_map.is_object() { - return Err(ImportMapError( + return Err(ImportMapError::Other( "Import map's 'imports' must be an object".to_string(), )); } @@ -84,7 +98,7 @@ impl ImportMap { let normalized_scopes = match &v.get("scopes") { Some(scope_map) => { if !scope_map.is_object() { - return Err(ImportMapError( + return Err(ImportMapError::Other( "Import map's 'scopes' must be an object".to_string(), )); } @@ -252,7 +266,7 @@ impl ImportMap { // Order is preserved because of "preserve_order" feature of "serde_json". for (scope_prefix, potential_specifier_map) in scope_map.iter() { if !potential_specifier_map.is_object() { - return Err(ImportMapError(format!( + return Err(ImportMapError::Other(format!( "The value for the {:?} scope prefix must be an object", scope_prefix ))); @@ -341,7 +355,7 @@ impl ImportMap { if let Some(address) = maybe_address { return Ok(Some(address.clone())); } else { - return Err(ImportMapError(format!( + return Err(ImportMapError::Other(format!( "Blocked by null entry for \"{:?}\"", normalized_specifier ))); @@ -367,7 +381,7 @@ impl ImportMap { } if maybe_address.is_none() { - return Err(ImportMapError(format!( + return Err(ImportMapError::Other(format!( "Blocked by null entry for \"{:?}\"", specifier_key ))); @@ -383,7 +397,7 @@ impl ImportMap { let url = match resolution_result.join(after_prefix) { Ok(url) => url, Err(_) => { - return Err(ImportMapError(format!( + return Err(ImportMapError::Other(format!( "Failed to resolve the specifier \"{:?}\" as its after-prefix portion \"{:?}\" could not be URL-parsed relative to the URL prefix \"{:?}\" mapped to by the prefix \"{:?}\"", @@ -396,7 +410,7 @@ impl ImportMap { }; if !url.as_str().starts_with(resolution_result.as_str()) { - return Err(ImportMapError(format!( + return Err(ImportMapError::Other(format!( "The specifier \"{:?}\" backtracks above its prefix \"{:?}\"", normalized_specifier, specifier_key ))); @@ -417,7 +431,7 @@ impl ImportMap { &self, specifier: &str, referrer: &str, - ) -> Result, ImportMapError> { + ) -> Result { let as_url: Option = ImportMap::try_url_like_specifier(specifier, referrer); let normalized_specifier = if let Some(url) = as_url.as_ref() { @@ -434,7 +448,7 @@ impl ImportMap { )?; // match found in scopes map - if scopes_match.is_some() { + if let Some(scopes_match) = scopes_match { return Ok(scopes_match); } @@ -445,19 +459,19 @@ impl ImportMap { )?; // match found in import map - if imports_match.is_some() { + if let Some(imports_match) = imports_match { return Ok(imports_match); } // The specifier was able to be turned into a URL, but wasn't remapped into anything. - if as_url.is_some() { + if let Some(as_url) = as_url { return Ok(as_url); } - Err(ImportMapError(format!( - "Unmapped bare specifier {:?}", - specifier - ))) + Err(ImportMapError::UnmappedBareSpecifier( + specifier.to_string(), + Some(referrer.to_string()), + )) } } @@ -465,7 +479,6 @@ impl ImportMap { mod tests { use super::*; - use deno_core::resolve_import; use std::path::Path; use std::path::PathBuf; use walkdir::WalkDir; @@ -652,15 +665,7 @@ mod tests { let maybe_resolved = import_map .resolve(&given_specifier, &base_url) .ok() - .map(|maybe_resolved| { - if let Some(specifier) = maybe_resolved { - specifier.to_string() - } else { - resolve_import(&given_specifier, &base_url) - .unwrap() - .to_string() - } - }); + .map(|url| url.to_string()); assert_eq!(expected_specifier, &maybe_resolved, "{}", test.name); } TestKind::Parse { diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 4ef4a6e22a..bd3ce799ae 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -214,13 +214,7 @@ pub fn resolve_import( maybe_import_map: &Option, ) -> ResolvedDependency { let maybe_mapped = if let Some(import_map) = maybe_import_map { - if let Ok(maybe_specifier) = - import_map.resolve(specifier, referrer.as_str()) - { - maybe_specifier - } else { - None - } + import_map.resolve(specifier, referrer.as_str()).ok() } else { None }; diff --git a/cli/module_graph.rs b/cli/module_graph.rs index 368df0a742..5bfa52e892 100644 --- a/cli/module_graph.rs +++ b/cli/module_graph.rs @@ -12,6 +12,7 @@ use crate::config_file::IgnoredCompilerOptions; use crate::config_file::TsConfig; use crate::diagnostics::Diagnostics; use crate::import_map::ImportMap; +use crate::import_map::ImportMapError; use crate::info; use crate::lockfile::Lockfile; use crate::media_type::MediaType; @@ -397,10 +398,13 @@ impl Module { Ok(specifier) => Some(specifier), Err(any_error) => { match any_error.downcast_ref::() { - Some(ModuleResolutionError::ImportPrefixMissing(_, _)) => None, - _ => { - return Err(any_error); - } + Some(ModuleResolutionError::ImportPrefixMissing(..)) => None, + _ => match any_error.downcast_ref::() { + Some(ImportMapError::UnmappedBareSpecifier(..)) => None, + _ => { + return Err(any_error); + } + }, } } }; @@ -447,10 +451,8 @@ impl Module { ) -> Result { let maybe_resolve = if let Some(import_map) = self.maybe_import_map.clone() { - import_map - .lock() - .unwrap() - .resolve(specifier, self.specifier.as_str())? + let import_map = import_map.lock().unwrap(); + Some(import_map.resolve(specifier, self.specifier.as_str())?) } else { None }; diff --git a/cli/module_loader.rs b/cli/module_loader.rs index acf7625065..349e72393e 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -83,10 +83,9 @@ impl ModuleLoader for CliModuleLoader { if !is_main { if let Some(import_map) = &self.import_map { - let result = import_map.resolve(specifier, referrer)?; - if let Some(r) = result { - return Ok(r); - } + return import_map + .resolve(specifier, referrer) + .map_err(AnyError::from); } } diff --git a/cli/tests/092_import_map_unmapped_bare_specifier.ts b/cli/tests/092_import_map_unmapped_bare_specifier.ts new file mode 100644 index 0000000000..87684430dd --- /dev/null +++ b/cli/tests/092_import_map_unmapped_bare_specifier.ts @@ -0,0 +1 @@ +await import("unmapped"); diff --git a/cli/tests/092_import_map_unmapped_bare_specifier.ts.out b/cli/tests/092_import_map_unmapped_bare_specifier.ts.out new file mode 100644 index 0000000000..1a55e352b8 --- /dev/null +++ b/cli/tests/092_import_map_unmapped_bare_specifier.ts.out @@ -0,0 +1,4 @@ +[WILDCARD]error: Uncaught (in promise) TypeError: Relative import path "unmapped" not prefixed with / or ./ or ../ and not in import map from "[WILDCARD]" +await import("unmapped"); +^ + at [WILDCARD] diff --git a/cli/tests/error_011_bad_module_specifier.ts.out b/cli/tests/error_011_bad_module_specifier.ts.out index e6f9b2321b..713072191e 100644 --- a/cli/tests/error_011_bad_module_specifier.ts.out +++ b/cli/tests/error_011_bad_module_specifier.ts.out @@ -1 +1 @@ -[WILDCARD]error: relative import path "bad-module.ts" not prefixed with / or ./ or ../ Imported from "[WILDCARD]/error_011_bad_module_specifier.ts" +[WILDCARD]error: Relative import path "bad-module.ts" not prefixed with / or ./ or ../ from "[WILDCARD]/error_011_bad_module_specifier.ts" diff --git a/cli/tests/error_012_bad_dynamic_import_specifier.ts.out b/cli/tests/error_012_bad_dynamic_import_specifier.ts.out index 45bce82616..0d0b168a48 100644 --- a/cli/tests/error_012_bad_dynamic_import_specifier.ts.out +++ b/cli/tests/error_012_bad_dynamic_import_specifier.ts.out @@ -1,5 +1,5 @@ Check [WILDCARD]error_012_bad_dynamic_import_specifier.ts -error: Uncaught (in promise) TypeError: relative import path "bad-module.ts" not prefixed with / or ./ or ../ Imported from "[WILDCARD]/error_012_bad_dynamic_import_specifier.ts" +error: Uncaught (in promise) TypeError: Relative import path "bad-module.ts" not prefixed with / or ./ or ../ from "[WILDCARD]/error_012_bad_dynamic_import_specifier.ts" const _badModule = await import("bad-module.ts"); ^ at async file:///[WILDCARD]/error_012_bad_dynamic_import_specifier.ts:2:22 diff --git a/cli/tests/error_014_catch_dynamic_import_error.js.out b/cli/tests/error_014_catch_dynamic_import_error.js.out index 4f133c834f..60de400db9 100644 --- a/cli/tests/error_014_catch_dynamic_import_error.js.out +++ b/cli/tests/error_014_catch_dynamic_import_error.js.out @@ -1,8 +1,8 @@ Caught direct dynamic import error. -TypeError: relative import path "does not exist" not prefixed with / or ./ or ../ Imported from "[WILDCARD]/error_014_catch_dynamic_import_error.js" +TypeError: Relative import path "does not exist" not prefixed with / or ./ or ../ from "[WILDCARD]/error_014_catch_dynamic_import_error.js" at async file:///[WILDCARD]/error_014_catch_dynamic_import_error.js:3:5 Caught indirect direct dynamic import error. -TypeError: relative import path "does not exist either" not prefixed with / or ./ or ../ Imported from "[WILDCARD]/indirect_import_error.js" +TypeError: Relative import path "does not exist either" not prefixed with / or ./ or ../ from "[WILDCARD]/indirect_import_error.js" at async file:///[WILDCARD]/error_014_catch_dynamic_import_error.js:10:5 Caught error thrown by dynamically imported module. Error: An error diff --git a/cli/tests/error_type_definitions.ts.out b/cli/tests/error_type_definitions.ts.out index 32c3c9b525..304ec1bdfe 100644 --- a/cli/tests/error_type_definitions.ts.out +++ b/cli/tests/error_type_definitions.ts.out @@ -1 +1 @@ -[WILDCARD]error: relative import path "baz" not prefixed with / or ./ or ../ Imported from "[WILDCARD]/type_definitions/bar.d.ts" +[WILDCARD]error: Relative import path "baz" not prefixed with / or ./ or ../ from "[WILDCARD]/type_definitions/bar.d.ts" diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index 5188c60ba5..5af533dab2 100644 --- a/cli/tests/integration_tests.rs +++ b/cli/tests/integration_tests.rs @@ -3133,6 +3133,12 @@ console.log("finish"); exit_code: 1, }); + itest!(_092_import_map_unmapped_bare_specifier { + args: "run --import-map import_maps/import_map.json 092_import_map_unmapped_bare_specifier.ts", + output: "092_import_map_unmapped_bare_specifier.ts.out", + exit_code: 1, + }); + itest!(dynamic_import_permissions_remote_remote { args: "run --quiet --reload --allow-net=localhost:4545 dynamic_import/permissions_remote_remote.ts", output: "dynamic_import/permissions_remote_remote.ts.out", diff --git a/core/module_specifier.rs b/core/module_specifier.rs index dc6b4d6bf6..4de8750736 100644 --- a/core/module_specifier.rs +++ b/core/module_specifier.rs @@ -39,10 +39,10 @@ impl fmt::Display for ModuleResolutionError { InvalidPath(ref path) => write!(f, "invalid module path: {:?}", path), ImportPrefixMissing(ref specifier, ref maybe_referrer) => write!( f, - "relative import path \"{}\" not prefixed with / or ./ or ../{}", + "Relative import path \"{}\" not prefixed with / or ./ or ../{}", specifier, match maybe_referrer { - Some(referrer) => format!(" Imported from \"{}\"", referrer), + Some(referrer) => format!(" from \"{}\"", referrer), None => format!(""), } ),