refactor: Modules and Loader trait (#3791)

* move is_dyn_import argument from Loader::resolve to Loader::load - it was always kind of strange that resolve() checks permissions.
* change argument type from &str to &ModuleSpecifier where applicable

cli/lib.rs

@@ -205,7 +205,6 @@ async fn print_file_info(
     eprintln!("\n{}", e.to_string());
     std::process::exit(1);
   }
-  let compiled = maybe_compiled.unwrap();
   if out.media_type == msg::MediaType::TypeScript
     || (out.media_type == msg::MediaType::JavaScript
       && global_state_.ts_compiler.compile_js)
@@ -235,7 +234,7 @@ async fn print_file_info(
   }
 
   let isolate = worker.isolate.try_lock().unwrap();
-  if let Some(deps) = isolate.modules.deps(&compiled.name) {
+  if let Some(deps) = isolate.modules.deps(&module_specifier) {
     println!("{}{}", colors::bold("deps:\n".to_string()), deps.name);
     if let Some(ref depsdeps) = deps.deps {
       for d in depsdeps {

cli/ops/compiler.rs

@@ -58,11 +58,6 @@ fn op_resolve_modules(
   _data: Option<ZeroCopyBuf>,
 ) -> Result<JsonOp, ErrBox> {
   let args: SpecifiersReferrerArgs = serde_json::from_value(args)?;
-
-  // TODO(ry) Maybe a security hole. Only the compiler worker should have access
-  // to this. Need a test to demonstrate the hole.
-  let is_dyn_import = false;
-
   let (referrer, is_main) = if let Some(referrer) = args.referrer {
     (referrer, false)
   } else {
@@ -72,8 +67,7 @@ fn op_resolve_modules(
   let mut specifiers = vec![];
 
   for specifier in &args.specifiers {
-    let resolved_specifier =
+    let resolved_specifier = state.resolve(specifier, &referrer, is_main);
-      state.resolve(specifier, &referrer, is_main, is_dyn_import);
     match resolved_specifier {
       Ok(ms) => specifiers.push(ms.as_str().to_owned()),
       Err(err) => return Err(err),

cli/state.rs

@@ -167,7 +167,6 @@ impl Loader for ThreadSafeState {
     specifier: &str,
     referrer: &str,
     is_main: bool,
-    is_dyn_import: bool,
   ) -> Result<ModuleSpecifier, ErrBox> {
     if !is_main {
       if let Some(import_map) = &self.import_map {
@@ -180,10 +179,6 @@ impl Loader for ThreadSafeState {
     let module_specifier =
       ModuleSpecifier::resolve_import(specifier, referrer)?;
 
-    if is_dyn_import {
-      self.check_dyn_import(&module_specifier)?;
-    }
-
     Ok(module_specifier)
   }
 
@@ -192,7 +187,15 @@ impl Loader for ThreadSafeState {
     &self,
     module_specifier: &ModuleSpecifier,
     maybe_referrer: Option<ModuleSpecifier>,
+    is_dyn_import: bool,
   ) -> Pin<Box<deno_core::SourceCodeInfoFuture>> {
+    if is_dyn_import {
+      if let Err(e) = self.check_dyn_import(&module_specifier) {
+        return async move { Err(e) }.boxed();
+      }
+    }
+
+    // TODO(bartlomieju): incrementing resolve_count here has no sense...
     self.metrics.resolve_count.fetch_add(1, Ordering::SeqCst);
     let module_url_specified = module_specifier.to_string();
     let fut = self

cli/tests/054_info_local_imports.out

@@ -0,0 +1,9 @@
+local: [WILDCARD]005_more_imports.ts
+type: TypeScript
+compiled: [WILDCARD]005_more_imports.ts.js
+map: [WILDCARD]005_more_imports.ts.js.map
+deps:
+file://[WILDCARD]/005_more_imports.ts
+  └─┬ file://[WILDCARD]/subdir/mod1.ts
+    └─┬ file://[WILDCARD]/subdir/subdir2/mod2.ts
+      └── file://[WILDCARD]/subdir/print_hello.ts

cli/tests/integration_tests.rs

@@ -385,6 +385,12 @@ itest!(_052_no_remote_flag {
   http_server: true,
 });
 
+itest!(_054_info_local_imports {
+  args: "info 005_more_imports.ts",
+  output: "054_info_local_imports.out",
+  exit_code: 0,
+});
+
 itest!(lock_check_ok {
   args: "run --lock=lock_check_ok.json http://127.0.0.1:4545/cli/tests/003_relative_import.ts",
   output: "003_relative_import.ts.out",

core/es_isolate.rs

@@ -178,10 +178,13 @@ impl EsIsolate {
     let module = maybe_module.unwrap();
     let id = module.get_identity_hash();
 
-    let mut import_specifiers: Vec<String> = vec![];
+    let mut import_specifiers: Vec<ModuleSpecifier> = vec![];
     for i in 0..module.get_module_requests_length() {
-      let specifier = module.get_module_request(i);
+      let import_specifier =
-      import_specifiers.push(specifier.to_rust_string_lossy(scope));
+        module.get_module_request(i).to_rust_string_lossy(scope);
+      let module_specifier =
+        self.loader.resolve(&import_specifier, name, false)?;
+      import_specifiers.push(module_specifier);
     }
 
     let mut handle = v8::Global::<v8::Module>::new();
@@ -284,12 +287,10 @@ impl EsIsolate {
     referrer_id: ModuleId,
   ) -> ModuleId {
     let referrer = self.modules.get_name(referrer_id).unwrap();
-    // We should have already resolved and Ready this module, so
-    // resolve() will not fail this time.
     let specifier = self
-      .modules
+      .loader
-      .get_cached_specifier(specifier, &referrer)
+      .resolve(specifier, referrer, false)
-      .expect("Module should already be resolved");
+      .expect("Module should have been already resolved");
     self.modules.get_id(specifier.as_str()).unwrap_or(0)
   }
 
@@ -480,26 +481,12 @@ impl EsIsolate {
     };
 
     // Now we must iterate over all imports of the module and load them.
-    let imports = self
+    let imports = self.modules.get_children(module_id).unwrap();
-      .modules
-      .get_info(module_id)
-      .unwrap()
-      .import_specifiers
-      .clone();
-    for import in imports {
-      let module_specifier = self.loader.resolve(
-        &import,
-        referrer_name,
-        false,
-        load.is_dynamic_import(),
-      )?;
-      self
-        .modules
-        .cache_specifier(&import, referrer_name, &module_specifier);
-      let module_name = module_specifier.as_str();
 
-      if !self.modules.is_registered(module_name) {
+    for module_specifier in imports {
-        load.add_import(module_specifier, referrer_specifier.clone());
+      if !self.modules.is_registered(module_specifier) {
+        load
+          .add_import(module_specifier.to_owned(), referrer_specifier.clone());
       }
     }
 
@@ -589,7 +576,6 @@ pub mod tests {
         specifier: &str,
         referrer: &str,
         _is_main: bool,
-        _is_dyn_import: bool,
       ) -> Result<ModuleSpecifier, ErrBox> {
         self.count.fetch_add(1, Ordering::Relaxed);
         assert_eq!(specifier, "./b.js");
@@ -602,6 +588,7 @@ pub mod tests {
         &self,
         _module_specifier: &ModuleSpecifier,
         _maybe_referrer: Option<ModuleSpecifier>,
+        _is_dyn_import: bool,
       ) -> Pin<Box<SourceCodeInfoFuture>> {
         unreachable!()
       }
@@ -653,35 +640,20 @@ pub mod tests {
       .unwrap();
     assert_eq!(dispatch_count.load(Ordering::Relaxed), 0);
 
-    let imports = isolate
+    let imports = isolate.modules.get_children(mod_a);
-      .modules
+    assert_eq!(
-      .get_info(mod_a)
+      imports,
-      .unwrap()
+      Some(&vec![ModuleSpecifier::resolve_url("file:///b.js").unwrap()])
-      .import_specifiers
+    );
-      .clone();
-    let specifier_b = "./b.js".to_string();
-    assert_eq!(imports, vec![specifier_b.clone()]);
     let mod_b = isolate
       .mod_new(false, "file:///b.js", "export function b() { return 'b' }")
       .unwrap();
-    let imports = isolate
+    let imports = isolate.modules.get_children(mod_b).unwrap();
-      .modules
-      .get_info(mod_b)
-      .unwrap()
-      .import_specifiers
-      .clone();
     assert_eq!(imports.len(), 0);
 
-    let module_specifier =
-      ModuleSpecifier::resolve_import(&specifier_b, &specifier_a).unwrap();
-    isolate.modules.cache_specifier(
-      &specifier_b,
-      &specifier_a,
-      &module_specifier,
-    );
     js_check(isolate.mod_instantiate(mod_b));
     assert_eq!(dispatch_count.load(Ordering::Relaxed), 0);
-    assert_eq!(resolve_count.load(Ordering::SeqCst), 0);
+    assert_eq!(resolve_count.load(Ordering::SeqCst), 1);
 
     js_check(isolate.mod_instantiate(mod_a));
     assert_eq!(dispatch_count.load(Ordering::Relaxed), 0);
@@ -703,7 +675,6 @@ pub mod tests {
         specifier: &str,
         referrer: &str,
         _is_main: bool,
-        _is_dyn_import: bool,
       ) -> Result<ModuleSpecifier, ErrBox> {
         self.count.fetch_add(1, Ordering::Relaxed);
         assert_eq!(specifier, "/foo.js");
@@ -716,6 +687,7 @@ pub mod tests {
         &self,
         _module_specifier: &ModuleSpecifier,
         _maybe_referrer: Option<ModuleSpecifier>,
+        _is_dyn_import: bool,
       ) -> Pin<Box<SourceCodeInfoFuture>> {
         async { Err(ErrBox::from(io::Error::from(io::ErrorKind::NotFound))) }
           .boxed()
@@ -849,7 +821,6 @@ pub mod tests {
         specifier: &str,
         referrer: &str,
         _is_main: bool,
-        _is_dyn_import: bool,
       ) -> Result<ModuleSpecifier, ErrBox> {
         let c = self.resolve_count.fetch_add(1, Ordering::Relaxed);
         match c {
@@ -866,6 +837,7 @@ pub mod tests {
         &self,
         specifier: &ModuleSpecifier,
         _maybe_referrer: Option<ModuleSpecifier>,
+        _is_dyn_import: bool,
       ) -> Pin<Box<SourceCodeInfoFuture>> {
         self.load_count.fetch_add(1, Ordering::Relaxed);
         let info = SourceCodeInfo {

core/modules.rs

@@ -28,19 +28,25 @@ pub trait Loader: Send + Sync {
   /// When implementing an spec-complaint VM, this should be exactly the
   /// algorithm described here:
   /// https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier
+  ///
+  /// `is_main` can be used to resolve from current working directory or
+  /// apply import map for child imports.
   fn resolve(
     &self,
     specifier: &str,
     referrer: &str,
     is_main: bool,
-    is_dyn_import: bool,
   ) -> Result<ModuleSpecifier, ErrBox>;
 
   /// Given ModuleSpecifier, load its source code.
+  ///
+  /// `is_dyn_import` can be used to check permissions or deny
+  /// dynamic imports altogether.
   fn load(
     &self,
     module_specifier: &ModuleSpecifier,
     maybe_referrer: Option<ModuleSpecifier>,
+    is_dyn_import: bool,
   ) -> Pin<Box<SourceCodeInfoFuture>>;
 }
 
@@ -121,10 +127,10 @@ impl RecursiveModuleLoad {
   fn add_root(&mut self) -> Result<(), ErrBox> {
     let module_specifier = match self.state {
       LoadState::ResolveMain(ref specifier, _) => {
-        self.loader.resolve(specifier, ".", true, false)?
+        self.loader.resolve(specifier, ".", true)?
       }
       LoadState::ResolveImport(ref specifier, ref referrer) => {
-        self.loader.resolve(specifier, referrer, false, true)?
+        self.loader.resolve(specifier, referrer, false)?
       }
 
       _ => unreachable!(),
@@ -139,7 +145,10 @@ impl RecursiveModuleLoad {
         })
         .boxed()
       }
-      _ => self.loader.load(&module_specifier, None).boxed(),
+      _ => self
+        .loader
+        .load(&module_specifier, None, self.is_dynamic_import())
+        .boxed(),
     };
 
     self.pending.push(load_fut);
@@ -154,7 +163,10 @@ impl RecursiveModuleLoad {
     referrer: ModuleSpecifier,
   ) {
     if !self.is_pending.contains(&specifier) {
-      let fut = self.loader.load(&specifier, Some(referrer));
+      let fut =
+        self
+          .loader
+          .load(&specifier, Some(referrer), self.is_dynamic_import());
       self.pending.push(fut.boxed());
       self.is_pending.insert(specifier);
     }
@@ -192,7 +204,7 @@ pub struct ModuleInfo {
   pub main: bool,
   pub name: String,
   pub handle: v8::Global<v8::Module>,
-  pub import_specifiers: Vec<String>,
+  pub import_specifiers: Vec<ModuleSpecifier>,
 }
 
 /// A symbolic module entity.
@@ -264,7 +276,6 @@ impl ModuleNameMap {
 pub struct Modules {
   pub(crate) info: HashMap<ModuleId, ModuleInfo>,
   by_name: ModuleNameMap,
-  pub(crate) specifier_cache: HashMap<(String, String), ModuleSpecifier>,
 }
 
 impl Modules {
@@ -272,7 +283,6 @@ impl Modules {
     Self {
       info: HashMap::new(),
       by_name: ModuleNameMap::new(),
-      specifier_cache: HashMap::new(),
     }
   }
 
@@ -280,20 +290,16 @@ impl Modules {
     self.by_name.get(name)
   }
 
-  pub fn get_children(&self, id: ModuleId) -> Option<&Vec<String>> {
+  pub fn get_children(&self, id: ModuleId) -> Option<&Vec<ModuleSpecifier>> {
     self.info.get(&id).map(|i| &i.import_specifiers)
   }
 
-  pub fn get_children2(&self, name: &str) -> Option<&Vec<String>> {
-    self.get_id(name).and_then(|id| self.get_children(id))
-  }
-
   pub fn get_name(&self, id: ModuleId) -> Option<&String> {
     self.info.get(&id).map(|i| &i.name)
   }
 
-  pub fn is_registered(&self, name: &str) -> bool {
+  pub fn is_registered(&self, specifier: &ModuleSpecifier) -> bool {
-    self.by_name.get(name).is_some()
+    self.by_name.get(&specifier.to_string()).is_some()
   }
 
   pub fn register(
@@ -302,7 +308,7 @@ impl Modules {
     name: &str,
     main: bool,
     handle: v8::Global<v8::Module>,
-    import_specifiers: Vec<String>,
+    import_specifiers: Vec<ModuleSpecifier>,
   ) {
     let name = String::from(name);
     debug!("register_complete {}", name);
@@ -334,30 +340,8 @@ impl Modules {
     self.info.get(&id)
   }
 
-  pub fn cache_specifier(
+  pub fn deps(&self, module_specifier: &ModuleSpecifier) -> Option<Deps> {
-    &mut self,
+    Deps::new(self, module_specifier)
-    specifier: &str,
-    referrer: &str,
-    resolved_specifier: &ModuleSpecifier,
-  ) {
-    self.specifier_cache.insert(
-      (specifier.to_string(), referrer.to_string()),
-      resolved_specifier.to_owned(),
-    );
-  }
-
-  pub fn get_cached_specifier(
-    &self,
-    specifier: &str,
-    referrer: &str,
-  ) -> Option<&ModuleSpecifier> {
-    self
-      .specifier_cache
-      .get(&(specifier.to_string(), referrer.to_string()))
-  }
-
-  pub fn deps(&self, url: &str) -> Option<Deps> {
-    Deps::new(self, url)
   }
 }
 
@@ -373,9 +357,12 @@ pub struct Deps {
 }
 
 impl Deps {
-  fn new(modules: &Modules, module_name: &str) -> Option<Deps> {
+  fn new(
+    modules: &Modules,
+    module_specifier: &ModuleSpecifier,
+  ) -> Option<Deps> {
     let mut seen = HashSet::new();
-    Self::helper(&mut seen, "".to_string(), true, modules, module_name)
+    Self::helper(&mut seen, "".to_string(), true, modules, module_specifier)
   }
 
   fn helper(
@@ -383,35 +370,37 @@ impl Deps {
     prefix: String,
     is_last: bool,
     modules: &Modules,
-    name: &str, // TODO(ry) rename url
+    module_specifier: &ModuleSpecifier,
   ) -> Option<Deps> {
-    if seen.contains(name) {
+    let name = module_specifier.to_string();
+    if seen.contains(&name) {
       Some(Deps {
-        name: name.to_string(),
+        name,
         prefix,
         deps: None,
         is_last,
       })
     } else {
-      let children = modules.get_children2(name)?;
+      let mod_id = modules.get_id(&name)?;
+      let children = modules.get_children(mod_id).unwrap();
       seen.insert(name.to_string());
       let child_count = children.len();
       let deps: Vec<Deps> = children
         .iter()
         .enumerate()
-        .map(|(index, dep_name)| {
+        .map(|(index, dep_specifier)| {
           let new_is_last = index == child_count - 1;
           let mut new_prefix = prefix.clone();
           new_prefix.push(if is_last { ' ' } else { '│' });
           new_prefix.push(' ');
 
-          Self::helper(seen, new_prefix, new_is_last, modules, dep_name)
+          Self::helper(seen, new_prefix, new_is_last, modules, dep_specifier)
         })
         // If any of the children are missing, return None.
         .collect::<Option<_>>()?;
 
       Some(Deps {
-        name: name.to_string(),
+        name,
         prefix,
         deps: Some(deps),
         is_last,
@@ -565,7 +554,6 @@ mod tests {
       specifier: &str,
       referrer: &str,
       _is_root: bool,
-      _is_dyn_import: bool,
     ) -> Result<ModuleSpecifier, ErrBox> {
       let referrer = if referrer == "." {
         "file:///"
@@ -592,6 +580,7 @@ mod tests {
       &self,
       module_specifier: &ModuleSpecifier,
       _maybe_referrer: Option<ModuleSpecifier>,
+      _is_dyn_import: bool,
     ) -> Pin<Box<SourceCodeInfoFuture>> {
       let mut loads = self.loads.lock().unwrap();
       loads.push(module_specifier.to_string());
@@ -666,10 +655,19 @@ mod tests {
     let d_id = modules.get_id("file:///d.js").unwrap();
     assert_eq!(
       modules.get_children(a_id),
-      Some(&vec!["/b.js".to_string(), "/c.js".to_string()])
+      Some(&vec![
+        ModuleSpecifier::resolve_url("file:///b.js").unwrap(),
+        ModuleSpecifier::resolve_url("file:///c.js").unwrap()
+      ])
+    );
+    assert_eq!(
+      modules.get_children(b_id),
+      Some(&vec![ModuleSpecifier::resolve_url("file:///c.js").unwrap()])
+    );
+    assert_eq!(
+      modules.get_children(c_id),
+      Some(&vec![ModuleSpecifier::resolve_url("file:///d.js").unwrap()])
     );
-    assert_eq!(modules.get_children(b_id), Some(&vec!["/c.js".to_string()]));
-    assert_eq!(modules.get_children(c_id), Some(&vec!["/d.js".to_string()]));
     assert_eq!(modules.get_children(d_id), Some(&vec![]));
   }
 
@@ -719,12 +717,16 @@ mod tests {
 
       assert_eq!(
         modules.get_children(circular1_id),
-        Some(&vec!["/circular2.js".to_string()])
+        Some(&vec![
+          ModuleSpecifier::resolve_url("file:///circular2.js").unwrap()
+        ])
       );
 
       assert_eq!(
         modules.get_children(circular2_id),
-        Some(&vec!["/circular3.js".to_string()])
+        Some(&vec![
+          ModuleSpecifier::resolve_url("file:///circular3.js").unwrap()
+        ])
       );
 
       assert!(modules.get_id("file:///circular3.js").is_some());
@@ -732,8 +734,8 @@ mod tests {
       assert_eq!(
         modules.get_children(circular3_id),
         Some(&vec![
-          "/circular1.js".to_string(),
+          ModuleSpecifier::resolve_url("file:///circular1.js").unwrap(),
-          "/circular2.js".to_string()
+          ModuleSpecifier::resolve_url("file:///circular2.js").unwrap()
         ])
       );
     }
@@ -923,17 +925,27 @@ mod tests {
 
     assert_eq!(
       modules.get_children(main_id),
-      Some(&vec!["/b.js".to_string(), "/c.js".to_string()])
+      Some(&vec![
+        ModuleSpecifier::resolve_url("file:///b.js").unwrap(),
+        ModuleSpecifier::resolve_url("file:///c.js").unwrap()
+      ])
+    );
+    assert_eq!(
+      modules.get_children(b_id),
+      Some(&vec![ModuleSpecifier::resolve_url("file:///c.js").unwrap()])
+    );
+    assert_eq!(
+      modules.get_children(c_id),
+      Some(&vec![ModuleSpecifier::resolve_url("file:///d.js").unwrap()])
     );
-    assert_eq!(modules.get_children(b_id), Some(&vec!["/c.js".to_string()]));
-    assert_eq!(modules.get_children(c_id), Some(&vec!["/d.js".to_string()]));
     assert_eq!(modules.get_children(d_id), Some(&vec![]));
   }
 
   #[test]
   fn empty_deps() {
     let modules = Modules::new();
-    assert!(modules.deps("foo").is_none());
+    let specifier = ModuleSpecifier::resolve_url("file:///foo").unwrap();
+    assert!(modules.deps(&specifier).is_none());
   }
 
   /* TODO(bartlomieju): reenable