core: return useful error when import path has no prefix like ./

denoland · Jun 30, 2019 · 32cde32 · 32cde32
1 parent 9d18f97
commit 32cde32
Show file tree

Hide file tree

Showing 6 changed files with 119 additions and 53 deletions.
diff --git a/cli/deno_error.rs b/cli/deno_error.rs
@@ -7,6 +7,7 @@ use crate::resolve_addr::ResolveAddrError;
 use crate::source_maps::apply_source_map;
 use crate::source_maps::SourceMapGetter;
 use deno::JSError;
+use deno::ModuleResolutionError;
 use hyper;
 #[cfg(unix)]
 use nix::{errno::Errno, Error as UnixError};
@@ -30,6 +31,7 @@ enum Repr {
   UrlErr(url::ParseError),
   HyperErr(hyper::Error),
   ImportMapErr(import_map::ImportMapError),
+  ModuleResolutionErr(ModuleResolutionError),
   Diagnostic(diagnostics::Diagnostic),
   JSError(JSError),
 }
@@ -42,6 +44,24 @@ pub fn new(kind: ErrorKind, msg: String) -> DenoError {
 }
 
 impl DenoError {
+  fn url_error_kind(err: url::ParseError) -> ErrorKind {
+    use url::ParseError::*;
+    match err {
+      EmptyHost => ErrorKind::EmptyHost,
+      IdnaError => ErrorKind::IdnaError,
+      InvalidPort => ErrorKind::InvalidPort,
+      InvalidIpv4Address => ErrorKind::InvalidIpv4Address,
+      InvalidIpv6Address => ErrorKind::InvalidIpv6Address,
+      InvalidDomainCharacter => ErrorKind::InvalidDomainCharacter,
+      RelativeUrlWithoutBase => ErrorKind::RelativeUrlWithoutBase,
+      RelativeUrlWithCannotBeABaseBase => {
+        ErrorKind::RelativeUrlWithCannotBeABaseBase
+      }
+      SetHostOnCannotBeABaseUrl => ErrorKind::SetHostOnCannotBeABaseUrl,
+      Overflow => ErrorKind::Overflow,
+    }
+  }
+
   pub fn kind(&self) -> ErrorKind {
     match self.repr {
       Repr::Simple(kind, ref _msg) => kind,
@@ -70,23 +90,7 @@ impl DenoError {
           _ => unreachable!(),
         }
       }
-      Repr::UrlErr(ref err) => {
-        use url::ParseError::*;
-        match err {
-          EmptyHost => ErrorKind::EmptyHost,
-          IdnaError => ErrorKind::IdnaError,
-          InvalidPort => ErrorKind::InvalidPort,
-          InvalidIpv4Address => ErrorKind::InvalidIpv4Address,
-          InvalidIpv6Address => ErrorKind::InvalidIpv6Address,
-          InvalidDomainCharacter => ErrorKind::InvalidDomainCharacter,
-          RelativeUrlWithoutBase => ErrorKind::RelativeUrlWithoutBase,
-          RelativeUrlWithCannotBeABaseBase => {
-            ErrorKind::RelativeUrlWithCannotBeABaseBase
-          }
-          SetHostOnCannotBeABaseUrl => ErrorKind::SetHostOnCannotBeABaseUrl,
-          Overflow => ErrorKind::Overflow,
-        }
-      }
+      Repr::UrlErr(err) => Self::url_error_kind(err),
       Repr::HyperErr(ref err) => {
         // For some reason hyper::errors::Kind is private.
         if err.is_parse() {
@@ -102,6 +106,13 @@ impl DenoError {
         }
       }
       Repr::ImportMapErr(ref _err) => ErrorKind::ImportMapError,
+      Repr::ModuleResolutionErr(err) => {
+        use ModuleResolutionError::*;
+        match err {
+          InvalidUrl(err) | InvalidBaseUrl(err) => Self::url_error_kind(err),
+          ImportPathPrefixMissing => ErrorKind::ImportPathPrefixMissing,
+        }
+      }
       Repr::Diagnostic(ref _err) => ErrorKind::Diagnostic,
       Repr::JSError(ref _err) => ErrorKind::JSError,
     }
@@ -126,6 +137,7 @@ impl fmt::Display for DenoError {
       Repr::UrlErr(ref err) => err.fmt(f),
       Repr::HyperErr(ref err) => err.fmt(f),
       Repr::ImportMapErr(ref err) => f.pad(&err.msg),
+      Repr::ModuleResolutionErr(ref err) => err.fmt(f),
       Repr::Diagnostic(ref err) => err.fmt(f),
       Repr::JSError(ref err) => JSErrorColor(err).fmt(f),
     }
@@ -140,6 +152,7 @@ impl std::error::Error for DenoError {
       Repr::UrlErr(ref err) => err.description(),
       Repr::HyperErr(ref err) => err.description(),
       Repr::ImportMapErr(ref err) => &err.msg,
+      Repr::ModuleResolutionErr(ref err) => err.description(),
       Repr::Diagnostic(ref err) => &err.items[0].message,
       Repr::JSError(ref err) => &err.description(),
     }
@@ -152,6 +165,7 @@ impl std::error::Error for DenoError {
       Repr::UrlErr(ref err) => Some(err),
       Repr::HyperErr(ref err) => Some(err),
       Repr::ImportMapErr(ref _err) => None,
+      Repr::ModuleResolutionErr(ref err) => err.source(),
       Repr::Diagnostic(ref _err) => None,
       Repr::JSError(ref err) => Some(err),
     }
@@ -241,6 +255,14 @@ impl From<import_map::ImportMapError> for DenoError {
   }
 }
 
+impl From<ModuleResolutionError> for DenoError {
+  fn from(err: ModuleResolutionError) -> Self {
+    Self {
+      repr: Repr::ModuleResolutionErr(err),
+    }
+  }
+}
+
 impl From<diagnostics::Diagnostic> for DenoError {
   fn from(diagnostic: diagnostics::Diagnostic) -> Self {
     Self {

diff --git a/cli/msg.fbs b/cli/msg.fbs
@@ -141,6 +141,7 @@ enum ErrorKind: byte {
   NoAsyncSupport,
   NoSyncSupport,
   ImportMapError,
+  ImportPathPrefixMissing,
 
   // other kinds
   Diagnostic,

diff --git a/core/lib.rs b/core/lib.rs
@@ -17,7 +17,7 @@ pub use crate::isolate::*;
 pub use crate::js_errors::*;
 pub use crate::libdeno::deno_mod;
 pub use crate::libdeno::PinnedBuf;
-pub use crate::module_specifier::ModuleSpecifier;
+pub use crate::module_specifier::*;
 pub use crate::modules::*;
 
 pub fn v8_version() -> &'static str {

diff --git a/core/module_specifier.rs b/core/module_specifier.rs
@@ -1,6 +1,40 @@
+use std::error::Error;
 use std::fmt;
+use url::ParseError;
 use url::Url;
 
+/// Error indicating the reason resolving a module specifier failed.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum ModuleResolutionError {
+  InvalidUrl(ParseError),
+  InvalidBaseUrl(ParseError),
+  ImportPathPrefixMissing,
+}
+use ModuleResolutionError::*;
+
+impl Error for ModuleResolutionError {
+  fn source(&self) -> Option<&(dyn Error + 'static)> {
+    match self {
+      InvalidUrl(ref err) | InvalidBaseUrl(ref err) => Some(err),
+      _ => None,
+    }
+  }
+}
+
+impl fmt::Display for ModuleResolutionError {
+  fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    match self {
+      InvalidUrl(ref err) => write!(f, "invalid URL: {}", err),
+      InvalidBaseUrl(ref err) => {
+        write!(f, "invalid base URL for relative import: {}", err)
+      }
+      ImportPathPrefixMissing => {
+        write!(f, "relative import path not prefixed with / or ./ or ../")
+      }
+    }
+  }
+}
+
 #[derive(Debug, Clone, PartialEq)]
 /// Resolved module specifier
 pub struct ModuleSpecifier(Url);
@@ -15,32 +49,39 @@ impl ModuleSpecifier {
   pub fn resolve(
     specifier: &str,
     base: &str,
-  ) -> Result<ModuleSpecifier, url::ParseError> {
-    // 1. Apply the URL parser to specifier. If the result is not failure, return
-    //    the result.
-    // let specifier = parse_local_or_remote(specifier)?.to_string();
-    if let Ok(url) = Url::parse(specifier) {
-      return Ok(ModuleSpecifier(url));
-    }
+  ) -> Result<ModuleSpecifier, ModuleResolutionError> {
+    let url = match Url::parse(specifier) {
+      // 1. Apply the URL parser to specifier.
+      //    If the result is not failure, return he result.
+      Ok(url) => url,
 
-    // 2. If specifier does not start with the character U+002F SOLIDUS (/), the
-    //    two-character sequence U+002E FULL STOP, U+002F SOLIDUS (./), or the
-    //    three-character sequence U+002E FULL STOP, U+002E FULL STOP, U+002F
-    //    SOLIDUS (../), return failure.
-    if !specifier.starts_with('/')
-      && !specifier.starts_with("./")
-      && !specifier.starts_with("../")
-    {
-      // TODO This is (probably) not the correct error to return here.
-      // TODO: This error is very not-user-friendly
-      return Err(url::ParseError::RelativeUrlWithCannotBeABaseBase);
-    }
+      // 2. If specifier does not start with the character U+002F SOLIDUS (/),
+      //    the two-character sequence U+002E FULL STOP, U+002F SOLIDUS (./),
+      //    or the three-character sequence U+002E FULL STOP, U+002E FULL STOP,
+      //    U+002F SOLIDUS (../), return failure.
+      Err(ParseError::RelativeUrlWithoutBase)
+        if !(specifier.starts_with('/')
+          || specifier.starts_with("./")
+          || specifier.starts_with("../")) =>
+      {
+        Err(ImportPathPrefixMissing)?
+      }
 
-    // 3. Return the result of applying the URL parser to specifier with base URL
-    //    as the base URL.
-    let base_url = Url::parse(base)?;
-    let u = base_url.join(&specifier)?;
-    Ok(ModuleSpecifier(u))
+      // 3. Return the result of applying the URL parser to specifier with base
+      //    URL as the base URL.
+      Err(ParseError::RelativeUrlWithoutBase) => {
+        let base = Url::parse(base).map_err(InvalidBaseUrl)?;
+        base.join(&specifier).map_err(InvalidUrl)?
+      }
+
+      // If parsing the specifier as a URL failed for a different reason than
+      // it being relative, always return the original error. We don't want to
+      // return `ImportPathPrefixMissing` or `InvalidBaseUrl` if the real
+      // problem lies somewhere else.
+      Err(err) => Err(InvalidUrl(err))?,
+    };
+
+    Ok(ModuleSpecifier(url))
   }
 
   /// Takes a string representing a path or URL to a module, but of the type
@@ -51,15 +92,17 @@ impl ModuleSpecifier {
   /// directory and returns an absolute URL.
   pub fn resolve_root(
     root_specifier: &str,
-  ) -> Result<ModuleSpecifier, url::ParseError> {
-    if let Ok(url) = Url::parse(root_specifier) {
-      Ok(ModuleSpecifier(url))
-    } else {
-      let cwd = std::env::current_dir().unwrap();
-      let base = Url::from_directory_path(cwd).unwrap();
-      let url = base.join(root_specifier)?;
-      Ok(ModuleSpecifier(url))
-    }
+  ) -> Result<ModuleSpecifier, ModuleResolutionError> {
+    let url = match Url::parse(root_specifier) {
+      Ok(url) => url,
+      Err(..) => {
+        let cwd = std::env::current_dir().unwrap();
+        let base = Url::from_directory_path(cwd).unwrap();
+        base.join(&root_specifier).map_err(InvalidUrl)?
+      }
+    };
+
+    Ok(ModuleSpecifier(url))
   }
 }
 

diff --git a/tests/error_011_bad_module_specifier.ts.out b/tests/error_011_bad_module_specifier.ts.out
@@ -1,4 +1,4 @@
-[WILDCARD]error: Uncaught RelativeUrlWithCannotBeABaseBase: relative URL with a cannot-be-a-base base
+[WILDCARD]error: Uncaught ImportPathPrefixMissing: relative import path not prefixed with / or ./ or ../
 [WILDCARD] js/errors.ts:[WILDCARD]
     at DenoError (js/errors.ts:[WILDCARD])
     at maybeError (js/errors.ts:[WILDCARD])

diff --git a/tests/error_012_bad_dynamic_import_specifier.ts.out b/tests/error_012_bad_dynamic_import_specifier.ts.out
@@ -1,4 +1,4 @@
-[WILDCARD]error: Uncaught RelativeUrlWithCannotBeABaseBase: relative URL with a cannot-be-a-base base
+[WILDCARD]error: Uncaught ImportPathPrefixMissing: relative import path not prefixed with / or ./ or ../
 [WILDCARD] js/errors.ts:[WILDCARD]
     at DenoError (js/errors.ts:[WILDCARD])
     at maybeError (js/errors.ts:[WILDCARD])