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 =
-      state.resolve(specifier, &referrer, is_main, is_dyn_import);
+    let resolved_specifier = state.resolve(specifier, &referrer, is_main);
     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);
-      import_specifiers.push(specifier.to_rust_string_lossy(scope));
+      let import_specifier =
+        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
-      .get_cached_specifier(specifier, &referrer)
-      .expect("Module should already be resolved");
+      .loader
+      .resolve(specifier, referrer, false)
+      .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
-      .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) {
-        load.add_import(module_specifier, referrer_specifier.clone());
+    let imports = self.modules.get_children(module_id).unwrap();
+
+    for module_specifier in imports {
+      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
-      .modules
-      .get_info(mod_a)
-      .unwrap()
-      .import_specifiers
-      .clone();
-    let specifier_b = "./b.js".to_string();
-    assert_eq!(imports, vec![specifier_b.clone()]);
+    let imports = isolate.modules.get_children(mod_a);
+    assert_eq!(
+      imports,
+      Some(&vec![ModuleSpecifier::resolve_url("file:///b.js").unwrap()])
+    );
     let mod_b = isolate
       .mod_new(false, "file:///b.js", "export function b() { return 'b' }")
       .unwrap();
-    let imports = isolate
-      .modules
-      .get_info(mod_b)
-      .unwrap()
-      .import_specifiers
-      .clone();
+    let imports = isolate.modules.get_children(mod_b).unwrap();
     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 {