refactor: remove deno_graph::Locker usage (denoland#16877)

This is just a straight refactor and doesn't make any improvements to the code that could now be made. Closes denoland#16493
DavidDeSimone · Dec 6, 2022 · c03e0f3 · c03e0f3
1 parent 3e47a27
commit c03e0f3
Show file tree

Hide file tree

Showing 15 changed files with 70 additions and 104 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -89,7 +89,7 @@ libc = "=0.2.126"
 log = "=0.4.17"
 lzzzz = "1.0"
 notify = "=5.0.0"
-once_cell = "=1.14.0"
+once_cell = "=1.16.0"
 os_pipe = "=1.0.1"
 parking_lot = "0.12.0"
 percent-encoding = "=2.2.0"

diff --git a/cli/Cargo.toml b/cli/Cargo.toml
@@ -43,9 +43,9 @@ winres.workspace = true
 [dependencies]
 deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "dep_graph", "module_specifier", "proposal", "react", "sourcemap", "transforms", "typescript", "view", "visit"] }
 deno_core.workspace = true
-deno_doc = "0.50.0"
-deno_emit = "0.11.0"
-deno_graph = "0.38.0"
+deno_doc = "0.51.0"
+deno_emit = "0.12.0"
+deno_graph = "0.39.0"
 deno_lint = { version = "0.35.0", features = ["docs"] }
 deno_runtime.workspace = true
 deno_task_shell = "0.8.1"
@@ -66,7 +66,7 @@ dprint-plugin-markdown = "=0.14.3"
 dprint-plugin-typescript = "=0.78.0"
 encoding_rs.workspace = true
 env_logger = "=0.9.0"
-eszip = "=0.30.0"
+eszip = "=0.31.0"
 fancy-regex = "=0.10.0"
 flate2.workspace = true
 http.workspace = true

diff --git a/cli/args/lockfile.rs b/cli/args/lockfile.rs
@@ -2,18 +2,13 @@
 
 use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
-use deno_core::parking_lot::Mutex;
 use deno_core::serde::Deserialize;
 use deno_core::serde::Serialize;
 use deno_core::serde_json;
-use deno_core::ModuleSpecifier;
 use log::debug;
-use std::cell::RefCell;
 use std::collections::BTreeMap;
 use std::io::Write;
 use std::path::PathBuf;
-use std::rc::Rc;
-use std::sync::Arc;
 
 use crate::args::ConfigFile;
 use crate::npm::NpmPackageId;
@@ -96,15 +91,6 @@ pub struct Lockfile {
 }
 
 impl Lockfile {
-  pub fn as_maybe_locker(
-    lockfile: Option<Arc<Mutex<Lockfile>>>,
-  ) -> Option<Rc<RefCell<dyn deno_graph::source::Locker>>> {
-    lockfile.as_ref().map(|lf| {
-      Rc::new(RefCell::new(Locker(Some(lf.clone()))))
-        as Rc<RefCell<dyn deno_graph::source::Locker>>
-    })
-  }
-
   pub fn discover(
     flags: &Flags,
     maybe_config_file: Option<&ConfigFile>,
@@ -342,33 +328,6 @@ Use \"--lock-write\" flag to regenerate the lockfile at \"{}\".",
   }
 }
 
-#[derive(Debug)]
-pub struct Locker(Option<Arc<Mutex<Lockfile>>>);
-
-impl deno_graph::source::Locker for Locker {
-  fn check_or_insert(
-    &mut self,
-    specifier: &ModuleSpecifier,
-    source: &str,
-  ) -> bool {
-    if let Some(lock_file) = &self.0 {
-      let mut lock_file = lock_file.lock();
-      lock_file.check_or_insert_remote(specifier.as_str(), source)
-    } else {
-      true
-    }
-  }
-
-  fn get_checksum(&self, content: &str) -> String {
-    util::checksum::gen(&[content.as_bytes()])
-  }
-
-  fn get_filename(&self) -> Option<String> {
-    let lock_file = self.0.as_ref()?.lock();
-    lock_file.filename.to_str().map(|s| s.to_string())
-  }
-}
-
 #[cfg(test)]
 mod tests {
   use super::*;

diff --git a/cli/errors.rs b/cli/errors.rs
@@ -26,8 +26,7 @@ fn get_diagnostic_class(_: &Diagnostic) -> &'static str {
 fn get_module_graph_error_class(err: &ModuleGraphError) -> &'static str {
   match err {
     ModuleGraphError::LoadingErr(_, err) => get_error_class_name(err.as_ref()),
-    ModuleGraphError::InvalidSource(_, _)
-    | ModuleGraphError::InvalidTypeAssertion { .. } => "SyntaxError",
+    ModuleGraphError::InvalidTypeAssertion { .. } => "SyntaxError",
     ModuleGraphError::ParseErr(_, diagnostic) => {
       get_diagnostic_class(diagnostic)
     }

diff --git a/cli/graph_util.rs b/cli/graph_util.rs
@@ -1,5 +1,6 @@
 // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license.
 
+use crate::args::Lockfile;
 use crate::args::TsTypeLib;
 use crate::colors;
 use crate::errors::get_error_class_name;
@@ -80,23 +81,23 @@ impl GraphData {
     let mut has_npm_specifier_in_graph = false;
 
     for (specifier, result) in graph.specifiers() {
-      if NpmPackageReference::from_specifier(&specifier).is_ok() {
+      if NpmPackageReference::from_specifier(specifier).is_ok() {
         has_npm_specifier_in_graph = true;
         continue;
       }
 
-      if !reload && self.modules.contains_key(&specifier) {
+      if !reload && self.modules.contains_key(specifier) {
         continue;
       }
 
-      if let Some(found) = graph.redirects.get(&specifier) {
+      if let Some(found) = graph.redirects.get(specifier) {
         let module_entry = ModuleEntry::Redirect(found.clone());
         self.modules.insert(specifier.clone(), module_entry);
         continue;
       }
       match result {
         Ok((_, _, media_type)) => {
-          let module = graph.get(&specifier).unwrap();
+          let module = graph.get(specifier).unwrap();
           let code = match &module.maybe_source {
             Some(source) => source.clone(),
             None => continue,
@@ -134,11 +135,11 @@ impl GraphData {
             checked_libs: Default::default(),
             maybe_types,
           };
-          self.modules.insert(specifier, module_entry);
+          self.modules.insert(specifier.clone(), module_entry);
         }
         Err(error) => {
-          let module_entry = ModuleEntry::Error(error);
-          self.modules.insert(specifier, module_entry);
+          let module_entry = ModuleEntry::Error(error.clone());
+          self.modules.insert(specifier.clone(), module_entry);
         }
       }
     }
@@ -475,10 +476,23 @@ pub fn graph_valid(
     .unwrap()
 }
 
-/// Calls `graph.lock()` and exits on errors.
-pub fn graph_lock_or_exit(graph: &ModuleGraph) {
-  if let Err(err) = graph.lock() {
-    log::error!("{} {}", colors::red("error:"), err);
-    std::process::exit(10);
+/// Checks the lockfile against the graph and and exits on errors.
+pub fn graph_lock_or_exit(graph: &ModuleGraph, lockfile: &mut Lockfile) {
+  for module in graph.modules() {
+    if let Some(source) = &module.maybe_source {
+      if !lockfile.check_or_insert_remote(module.specifier.as_str(), source) {
+        let err = format!(
+          concat!(
+            "The source code is invalid, as it does not match the expected hash in the lock file.\n",
+            "  Specifier: {}\n",
+            "  Lock file: {}",
+          ),
+          module.specifier,
+          lockfile.filename.display(),
+        );
+        log::error!("{} {}", colors::red("error:"), err);
+        std::process::exit(10);
+      }
+    }
   }
 }
diff --git a/cli/main.rs b/cli/main.rs
@@ -61,7 +61,6 @@ use crate::util::display;
 use crate::util::file_watcher::ResolutionResult;
 
 use args::CliOptions;
-use args::Lockfile;
 use deno_ast::MediaType;
 use deno_core::anyhow::bail;
 use deno_core::error::generic_error;
@@ -315,7 +314,6 @@ async fn create_graph_and_maybe_check(
     Permissions::allow_all(),
     Permissions::allow_all(),
   );
-  let maybe_locker = Lockfile::as_maybe_locker(ps.lockfile.clone());
   let maybe_imports = ps.options.to_maybe_imports()?;
   let maybe_cli_resolver = CliResolver::maybe_new(
     ps.options.to_maybe_jsx_import_source_config(),
@@ -332,7 +330,6 @@ async fn create_graph_and_maybe_check(
         is_dynamic: false,
         imports: maybe_imports,
         resolver: maybe_graph_resolver,
-        locker: maybe_locker,
         module_analyzer: Some(&*analyzer),
         reporter: None,
       },
@@ -353,7 +350,9 @@ async fn create_graph_and_maybe_check(
   ps.npm_resolver
     .add_package_reqs(graph_data.npm_package_reqs().clone())
     .await?;
-  graph_lock_or_exit(&graph);
+  if let Some(lockfile) = &ps.lockfile {
+    graph_lock_or_exit(&graph, &mut lockfile.lock());
+  }
 
   if ps.options.type_check_mode() != TypeCheckMode::None {
     let ts_config_result =
@@ -433,7 +432,6 @@ async fn bundle_command(
 
       let mut paths_to_watch: Vec<PathBuf> = graph
         .specifiers()
-        .iter()
         .filter_map(|(_, r)| {
           r.as_ref().ok().and_then(|(s, _, _)| s.to_file_path().ok())
         })
@@ -533,9 +531,8 @@ fn error_for_any_npm_specifier(
 ) -> Result<(), AnyError> {
   let first_npm_specifier = graph
     .specifiers()
-    .values()
-    .filter_map(|r| match r {
-      Ok((specifier, kind, _)) if *kind == deno_graph::ModuleKind::External => {
+    .filter_map(|(_, r)| match r {
+      Ok((specifier, kind, _)) if kind == deno_graph::ModuleKind::External => {
         Some(specifier.clone())
       }
       _ => None,

diff --git a/cli/npm/resolution/specifier.rs b/cli/npm/resolution/specifier.rs
@@ -947,7 +947,6 @@ mod tests {
         is_dynamic: false,
         imports: None,
         resolver: None,
-        locker: None,
         module_analyzer: Some(&analyzer),
         reporter: None,
       },

diff --git a/cli/proc_state.rs b/cli/proc_state.rs
@@ -329,7 +329,6 @@ impl ProcState {
       root_permissions.clone(),
       dynamic_permissions.clone(),
     );
-    let maybe_locker = Lockfile::as_maybe_locker(self.lockfile.clone());
     let maybe_imports = self.options.to_maybe_imports()?;
     let maybe_resolver =
       self.maybe_resolver.as_ref().map(|r| r.as_graph_resolver());
@@ -383,16 +382,16 @@ impl ProcState {
         is_dynamic,
         imports: maybe_imports,
         resolver: maybe_resolver,
-        locker: maybe_locker,
         module_analyzer: Some(&*analyzer),
         reporter: maybe_file_watcher_reporter,
       },
     )
     .await;
 
-    // If there was a locker, validate the integrity of all the modules in the
-    // locker.
-    graph_lock_or_exit(&graph);
+    // If there is a lockfile, validate the integrity of all the modules.
+    if let Some(lockfile) = &self.lockfile {
+      graph_lock_or_exit(&graph, &mut lockfile.lock());
+    }
 
     // Determine any modules that have already been emitted this session and
     // should be skipped.
@@ -639,7 +638,6 @@ impl ProcState {
     roots: Vec<(ModuleSpecifier, ModuleKind)>,
     loader: &mut dyn Loader,
   ) -> Result<deno_graph::ModuleGraph, AnyError> {
-    let maybe_locker = Lockfile::as_maybe_locker(self.lockfile.clone());
     let maybe_imports = self.options.to_maybe_imports()?;
 
     let maybe_cli_resolver = CliResolver::maybe_new(
@@ -657,7 +655,6 @@ impl ProcState {
         is_dynamic: false,
         imports: maybe_imports,
         resolver: maybe_graph_resolver,
-        locker: maybe_locker,
         module_analyzer: Some(&*analyzer),
         reporter: None,
       },

diff --git a/cli/tools/doc.rs b/cli/tools/doc.rs
@@ -49,7 +49,6 @@ pub async fn print_docs(
         is_dynamic: false,
         imports: None,
         resolver: None,
-        locker: None,
         module_analyzer: Some(&analyzer),
         reporter: None,
       },