fix(cli): ModuleGraph2 properly handles redirects (#7981)

cli/file_fetcher.rs

@@ -32,9 +32,6 @@ use std::sync::Arc;
 use std::sync::Mutex;
 
 /// Structure representing local or remote file.
-///
-/// In case of remote file `url` might be different than originally requested URL, if so
-/// `redirect_source_url` will contain original URL and `url` will be equal to final location.
 #[derive(Debug, Clone)]
 pub struct SourceFile {
   pub url: Url,

cli/module_graph2.rs

@@ -161,7 +161,6 @@ fn get_version(source: &str, version: &str, config: &[u8]) -> String {
 struct Module {
   dependencies: DependencyMap,
   is_dirty: bool,
-  is_hydrated: bool,
   is_parsed: bool,
   maybe_emit: Option<Emit>,
   maybe_emit_path: Option<(PathBuf, Option<PathBuf>)>,
@@ -180,7 +179,6 @@ impl Default for Module {
     Module {
       dependencies: HashMap::new(),
       is_dirty: false,
-      is_hydrated: false,
       is_parsed: false,
       maybe_emit: None,
       maybe_emit_path: None,
@@ -189,7 +187,7 @@ impl Default for Module {
       maybe_types: None,
       maybe_version: None,
       media_type: MediaType::Unknown,
-      specifier: ModuleSpecifier::resolve_url("https://deno.land/x/").unwrap(),
+      specifier: ModuleSpecifier::resolve_url("file:///example.js").unwrap(),
       source: "".to_string(),
       source_path: PathBuf::new(),
     }
@@ -198,51 +196,49 @@ impl Default for Module {
 
 impl Module {
   pub fn new(
-    specifier: ModuleSpecifier,
+    cached_module: CachedModule,
     maybe_import_map: Option<Rc<RefCell<ImportMap>>>,
   ) -> Self {
-    Module {
+    let mut module = Module {
-      specifier,
+      specifier: cached_module.specifier,
       maybe_import_map,
-      ..Module::default()
+      media_type: cached_module.media_type,
-    }
+      source: cached_module.source,
-  }
+      source_path: cached_module.source_path,
-
+      maybe_emit: cached_module.maybe_emit,
-  /// Return `true` if the current hash of the module matches the stored
+      maybe_emit_path: cached_module.maybe_emit_path,
-  /// version.
+      maybe_version: cached_module.maybe_version,
-  pub fn emit_valid(&self, config: &[u8]) -> bool {
+      is_dirty: false,
-    if let Some(version) = self.maybe_version.clone() {
+      ..Self::default()
-      version == get_version(&self.source, version::DENO, config)
+    };
-    } else {
+    if module.maybe_import_map.is_none() {
-      false
-    }
-  }
-
-  pub fn hydrate(&mut self, cached_module: CachedModule) {
-    self.media_type = cached_module.media_type;
-    self.source = cached_module.source;
-    self.source_path = cached_module.source_path;
-    self.maybe_emit = cached_module.maybe_emit;
-    self.maybe_emit_path = cached_module.maybe_emit_path;
-    if self.maybe_import_map.is_none() {
       if let Some(dependencies) = cached_module.maybe_dependencies {
-        self.dependencies = dependencies;
+        module.dependencies = dependencies;
-        self.is_parsed = true;
+        module.is_parsed = true;
       }
     }
-    self.maybe_types = if let Some(ref specifier) = cached_module.maybe_types {
+    module.maybe_types = if let Some(ref specifier) = cached_module.maybe_types
+    {
       Some((
         specifier.clone(),
-        self
+        module
           .resolve_import(&specifier, None)
           .expect("could not resolve module"),
       ))
     } else {
       None
     };
-    self.maybe_version = cached_module.maybe_version;
+    module
-    self.is_dirty = false;
+  }
-    self.is_hydrated = true;
+
+  /// Return `true` if the current hash of the module matches the stored
+  /// version.
+  pub fn is_emit_valid(&self, config: &[u8]) -> bool {
+    if let Some(version) = self.maybe_version.clone() {
+      version == get_version(&self.source, version::DENO, config)
+    } else {
+      false
+    }
   }
 
   pub fn parse(&mut self) -> Result<(), AnyError> {
@@ -415,6 +411,7 @@ pub struct Graph2 {
   handler: Rc<RefCell<dyn SpecifierHandler>>,
   maybe_ts_build_info: Option<String>,
   modules: HashMap<ModuleSpecifier, Module>,
+  redirects: HashMap<ModuleSpecifier, ModuleSpecifier>,
   roots: Vec<ModuleSpecifier>,
 }
 
@@ -429,10 +426,40 @@ impl Graph2 {
       handler,
       maybe_ts_build_info: None,
       modules: HashMap::new(),
+      redirects: HashMap::new(),
       roots: Vec::new(),
     }
   }
 
+  fn contains_module(&self, specifier: &ModuleSpecifier) -> bool {
+    let s = self.resolve_specifier(specifier);
+    self.modules.contains_key(s)
+  }
+
+  /// Update the handler with any modules that are marked as _dirty_ and update
+  /// any build info if present.
+  fn flush(&mut self) -> Result<(), AnyError> {
+    let mut handler = self.handler.borrow_mut();
+    for (_, module) in self.modules.iter_mut() {
+      if module.is_dirty {
+        if let Some(emit) = &module.maybe_emit {
+          handler.set_cache(&module.specifier, emit)?;
+        }
+        if let Some(version) = &module.maybe_version {
+          handler.set_version(&module.specifier, version.clone())?;
+        }
+        module.is_dirty = false;
+      }
+    }
+    for root_specifier in self.roots.iter() {
+      if let Some(ts_build_info) = &self.maybe_ts_build_info {
+        handler.set_ts_build_info(root_specifier, ts_build_info.to_owned())?;
+      }
+    }
+
+    Ok(())
+  }
+
   fn get_info(
     &self,
     specifier: &ModuleSpecifier,
@@ -440,7 +467,7 @@ impl Graph2 {
     totals: &mut HashMap<ModuleSpecifier, usize>,
   ) -> ModuleInfo {
     let not_seen = seen.insert(specifier.clone());
-    let module = self.modules.get(specifier).unwrap();
+    let module = self.get_module(specifier).unwrap();
     let mut deps = Vec::new();
     let mut total_size = None;
 
@@ -511,6 +538,32 @@ impl Graph2 {
     ModuleInfoMap::new(map)
   }
 
+  pub fn get_media_type(
+    &self,
+    specifier: &ModuleSpecifier,
+  ) -> Option<MediaType> {
+    if let Some(module) = self.get_module(specifier) {
+      Some(module.media_type)
+    } else {
+      None
+    }
+  }
+
+  fn get_module(&self, specifier: &ModuleSpecifier) -> Option<&Module> {
+    let s = self.resolve_specifier(specifier);
+    self.modules.get(s)
+  }
+
+  /// Get the source for a given module specifier.  If the module is not part
+  /// of the graph, the result will be `None`.
+  pub fn get_source(&self, specifier: &ModuleSpecifier) -> Option<String> {
+    if let Some(module) = self.get_module(specifier) {
+      Some(module.source.clone())
+    } else {
+      None
+    }
+  }
+
   /// Return a structure which provides information about the module graph and
   /// the relationship of the modules in the graph.  This structure is used to
   /// provide information for the `info` subcommand.
@@ -520,7 +573,7 @@ impl Graph2 {
     }
 
     let module = self.roots[0].clone();
-    let m = self.modules.get(&module).unwrap();
+    let m = self.get_module(&module).unwrap();
 
     let mut seen = HashSet::new();
     let mut totals = HashMap::new();
@@ -548,51 +601,6 @@ impl Graph2 {
     })
   }
 
-  /// Update the handler with any modules that are marked as _dirty_ and update
-  /// any build info if present.
-  fn flush(&mut self) -> Result<(), AnyError> {
-    let mut handler = self.handler.borrow_mut();
-    for (_, module) in self.modules.iter_mut() {
-      if module.is_dirty {
-        if let Some(emit) = &module.maybe_emit {
-          handler.set_cache(&module.specifier, emit)?;
-        }
-        if let Some(version) = &module.maybe_version {
-          handler.set_version(&module.specifier, version.clone())?;
-        }
-        module.is_dirty = false;
-      }
-    }
-    for root_specifier in self.roots.iter() {
-      if let Some(ts_build_info) = &self.maybe_ts_build_info {
-        handler.set_ts_build_info(root_specifier, ts_build_info.to_owned())?;
-      }
-    }
-
-    Ok(())
-  }
-
-  pub fn get_media_type(
-    &self,
-    specifier: &ModuleSpecifier,
-  ) -> Option<MediaType> {
-    if let Some(module) = self.modules.get(specifier) {
-      Some(module.media_type)
-    } else {
-      None
-    }
-  }
-
-  /// Get the source for a given module specifier.  If the module is not part
-  /// of the graph, the result will be `None`.
-  pub fn get_source(&self, specifier: &ModuleSpecifier) -> Option<String> {
-    if let Some(module) = self.modules.get(specifier) {
-      Some(module.source.clone())
-    } else {
-      None
-    }
-  }
-
   /// Verify the subresource integrity of the graph based upon the optional
   /// lockfile, updating the lockfile with any missing resources.  This will
   /// error if any of the resources do not match their lock status.
@@ -624,10 +632,10 @@ impl Graph2 {
     specifier: &str,
     referrer: &ModuleSpecifier,
   ) -> Result<ModuleSpecifier, AnyError> {
-    if !self.modules.contains_key(referrer) {
+    if !self.contains_module(referrer) {
       return Err(MissingSpecifier(referrer.to_owned()).into());
     }
-    let module = self.modules.get(referrer).unwrap();
+    let module = self.get_module(referrer).unwrap();
     if !module.dependencies.contains_key(specifier) {
       return Err(
         MissingDependency(referrer.to_owned(), specifier.to_owned()).into(),
@@ -647,13 +655,13 @@ impl Graph2 {
           MissingDependency(referrer.to_owned(), specifier.to_owned()).into(),
         );
       };
-    if !self.modules.contains_key(&resolved_specifier) {
+    if !self.contains_module(&resolved_specifier) {
       return Err(
         MissingDependency(referrer.to_owned(), resolved_specifier.to_string())
           .into(),
       );
     }
-    let dep_module = self.modules.get(&resolved_specifier).unwrap();
+    let dep_module = self.get_module(&resolved_specifier).unwrap();
     // In the case that there is a X-TypeScript-Types or a triple-slash types,
     // then the `maybe_types` specifier will be populated and we should use that
     // instead.
@@ -666,6 +674,29 @@ impl Graph2 {
     Ok(result)
   }
 
+  /// Takes a module specifier and returns the "final" specifier, accounting for
+  /// any redirects that may have occurred.
+  fn resolve_specifier<'a>(
+    &'a self,
+    specifier: &'a ModuleSpecifier,
+  ) -> &'a ModuleSpecifier {
+    let mut s = specifier;
+    let mut seen = HashSet::new();
+    seen.insert(s.clone());
+    while let Some(redirect) = self.redirects.get(s) {
+      if !seen.insert(redirect.clone()) {
+        eprintln!("An infinite loop of module redirections detected.\n  Original specifier: {}", specifier);
+        break;
+      }
+      s = redirect;
+      if seen.len() > 5 {
+        eprintln!("An excessive number of module redirections detected.\n  Original specifier: {}", specifier);
+        break;
+      }
+    }
+    s
+  }
+
   /// Transpile (only transform) the graph, updating any emitted modules
   /// with the specifier handler.  The result contains any performance stats
   /// from the compiler and optionally any user provided configuration compiler
@@ -724,7 +755,7 @@ impl Graph2 {
       }
       let config = ts_config.as_bytes();
       // skip modules that already have a valid emit
-      if module.maybe_emit.is_some() && module.emit_valid(&config) {
+      if module.maybe_emit.is_some() && module.is_emit_valid(&config) {
         continue;
       }
       if module.maybe_parsed_module.is_none() {
@@ -794,9 +825,8 @@ impl GraphBuilder2 {
   /// module into the graph.
   fn visit(&mut self, cached_module: CachedModule) -> Result<(), AnyError> {
     let specifier = cached_module.specifier.clone();
-    let mut module =
+    let requested_specifier = cached_module.requested_specifier.clone();
-      Module::new(specifier.clone(), self.maybe_import_map.clone());
+    let mut module = Module::new(cached_module, self.maybe_import_map.clone());
-    module.hydrate(cached_module);
     if !module.is_parsed {
       let has_types = module.maybe_types.is_some();
       module.parse()?;
@@ -821,6 +851,12 @@ impl GraphBuilder2 {
     if let Some((_, specifier)) = module.maybe_types.as_ref() {
       self.fetch(specifier)?;
     }
+    if specifier != requested_specifier {
+      self
+        .graph
+        .redirects
+        .insert(requested_specifier, specifier.clone());
+    }
     self.graph.modules.insert(specifier, module);
 
     Ok(())
@@ -921,6 +957,7 @@ pub mod tests {
 
       Ok(CachedModule {
         source,
+        requested_specifier: specifier.clone(),
         source_path,
         specifier,
         media_type,
@@ -1014,7 +1051,7 @@ pub mod tests {
       maybe_version,
       ..Module::default()
     };
-    assert!(module.emit_valid(b""));
+    assert!(module.is_emit_valid(b""));
 
     let source = "console.log(42);".to_string();
     let old_source = "console.log(43);";
@@ -1024,7 +1061,7 @@ pub mod tests {
       maybe_version,
       ..Module::default()
     };
-    assert!(!module.emit_valid(b""));
+    assert!(!module.is_emit_valid(b""));
 
     let source = "console.log(42);".to_string();
     let maybe_version = Some(get_version(&source, "0.0.0", b""));
@@ -1033,14 +1070,14 @@ pub mod tests {
       maybe_version,
       ..Module::default()
     };
-    assert!(!module.emit_valid(b""));
+    assert!(!module.is_emit_valid(b""));
 
     let source = "console.log(42);".to_string();
     let module = Module {
       source,
       ..Module::default()
     };
-    assert!(!module.emit_valid(b""));
+    assert!(!module.is_emit_valid(b""));
   }
 
   #[test]

cli/specifier_handler.rs

@@ -33,6 +33,7 @@ pub struct CachedModule {
   pub maybe_types: Option<String>,
   pub maybe_version: Option<String>,
   pub media_type: MediaType,
+  pub requested_specifier: ModuleSpecifier,
   pub source: String,
   pub source_path: PathBuf,
   pub specifier: ModuleSpecifier,
@@ -41,6 +42,7 @@ pub struct CachedModule {
 #[cfg(test)]
 impl Default for CachedModule {
   fn default() -> Self {
+    let specifier = ModuleSpecifier::resolve_url("file:///example.js").unwrap();
     CachedModule {
       maybe_dependencies: None,
       maybe_emit: None,
@@ -48,10 +50,10 @@ impl Default for CachedModule {
       maybe_types: None,
       maybe_version: None,
       media_type: MediaType::Unknown,
+      requested_specifier: specifier.clone(),
       source: "".to_string(),
       source_path: PathBuf::new(),
-      specifier: ModuleSpecifier::resolve_url("https://deno.land/x/mod.ts")
+      specifier,
-        .unwrap(),
     }
   }
 }
@@ -192,16 +194,16 @@ impl FetchHandler {
 }
 
 impl SpecifierHandler for FetchHandler {
-  fn fetch(&mut self, specifier: ModuleSpecifier) -> FetchFuture {
+  fn fetch(&mut self, requested_specifier: ModuleSpecifier) -> FetchFuture {
     let permissions = self.permissions.clone();
     let file_fetcher = self.file_fetcher.clone();
     let disk_cache = self.disk_cache.clone();
 
     async move {
       let source_file = file_fetcher
-        .fetch_source_file(&specifier, None, permissions)
+        .fetch_source_file(&requested_specifier, None, permissions)
         .await?;
-      let url = source_file.url;
+      let url = source_file.url.clone();
       let filename = disk_cache.get_cache_filename_with_extension(&url, "meta");
       let maybe_version = if let Ok(bytes) = disk_cache.get(&filename) {
         if let Ok(compiled_file_metadata) =
@@ -232,6 +234,7 @@ impl SpecifierHandler for FetchHandler {
         maybe_emit_path =
           Some((disk_cache.location.join(emit_path), maybe_map_path));
       };
+      let specifier = ModuleSpecifier::from(url);
 
       Ok(CachedModule {
         maybe_dependencies: None,
@@ -240,6 +243,7 @@ impl SpecifierHandler for FetchHandler {
         maybe_types: source_file.types_header,
         maybe_version,
         media_type: source_file.media_type,
+        requested_specifier,
         source: source_file.source_code,
         source_path: source_file.filename,
         specifier,

cli/tests/017_import_redirect_info.out

@@ -0,0 +1,6 @@
+local: [WILDCARD]017_import_redirect.ts
+type: TypeScript
+deps: 1 unique (total [WILDCARD]B)
+
+file:///[WILDCARD]cli/tests/017_import_redirect.ts ([WILDCARD])
+└── http://gist.githubusercontent.com/ry/f12b2aa3409e6b52645bc346a9e22929/raw/79318f239f51d764384a8bded8d7c6a833610dde/print_hello.ts ([WILDCARD])

cli/tests/integration_tests.rs

@@ -1572,6 +1572,16 @@ itest!(_017_import_redirect {
   output: "017_import_redirect.ts.out",
 });
 
+itest!(_017_import_redirect_nocheck {
+  args: "run --quiet --reload --no-check 017_import_redirect.ts",
+  output: "017_import_redirect.ts.out",
+});
+
+itest!(_017_import_redirect_info {
+  args: "info --quiet --reload 017_import_redirect.ts",
+  output: "017_import_redirect_info.out",
+});
+
 itest!(_018_async_catch {
   args: "run --quiet --reload 018_async_catch.ts",
   output: "018_async_catch.ts.out",