fix(cli): Don't statically error on dynamic unmapped bare specifiers (d…

…enoland#10618)

Fixes denoland#10168
Fixes denoland#10615
Fixes denoland#10616

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<String>),
+ 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<Option<Url>, ImportMapError> {
+ ) -> Result<Url, ImportMapError> {
  let as_url: Option<Url> =
  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,27 +459,26 @@ 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()),
+ ))
  }
 }
 
 #[cfg(test)]
 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 {

cli/lsp/analysis.rs

@@ -214,13 +214,7 @@ pub fn resolve_import(
  maybe_import_map: &Option<ImportMap>,
 ) -> 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
  };

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::<ModuleResolutionError>() {
- Some(ModuleResolutionError::ImportPrefixMissing(_, _)) => None,
- _ => {
- return Err(any_error);
- }
+ Some(ModuleResolutionError::ImportPrefixMissing(..)) => None,
+ _ => match any_error.downcast_ref::<ImportMapError>() {
+ Some(ImportMapError::UnmappedBareSpecifier(..)) => None,
+ _ => {
+ return Err(any_error);
+ }
+ },
  }
  }
  };
@@ -447,10 +451,8 @@ impl Module {
  ) -> Result<ModuleSpecifier, AnyError> {
  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
  };

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);
  }
  }
 

cli/tests/092_import_map_unmapped_bare_specifier.ts

@@ -0,0 +1 @@
+await import("unmapped");

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]

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"

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:https:///[WILDCARD]/error_012_bad_dynamic_import_specifier.ts:2:22

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:https:///[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:https:///[WILDCARD]/error_014_catch_dynamic_import_error.js:10:5
 Caught error thrown by dynamically imported module.
 Error: An error

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"

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",

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!(""),
  }
  ),