refactor(ext/node): enforce interior mutable for `NodePermissions` to remove clones (#18831)

We can make `NodePermissions` rely on interior mutability (which the
`PermissionsContainer` is already doing) in order to not have to clone
everything all the time. This also reduces the chance of an accidental
`borrow` while `borrrow_mut`.

4 files changed, 41 insertions, 44 deletions

diff --git a/ext/node/lib.rs b/ext/node/lib.rs
index e63c73537..84530423f 100644
--- a/ext/node/lib.rs
+++ b/ext/node/lib.rs
@@ -44,13 +44,13 @@ pub trait NodeEnv {
 }
 
 pub trait NodePermissions {
-  fn check_read(&mut self, path: &Path) -> Result<(), AnyError>;
+  fn check_read(&self, path: &Path) -> Result<(), AnyError>;
 }
 
 pub(crate) struct AllowAllNodePermissions;
 
 impl NodePermissions for AllowAllNodePermissions {
-  fn check_read(&mut self, _path: &Path) -> Result<(), AnyError> {
+  fn check_read(&self, _path: &Path) -> Result<(), AnyError> {
     Ok(())
   }
 }
@@ -164,7 +164,7 @@ pub trait NpmResolver: std::fmt::Debug + Send + Sync {
 
   fn ensure_read_permission(
     &self,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
     path: &Path,
   ) -> Result<(), AnyError>;
 }
diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs
index 513b3f589..34eac8475 100644
--- a/ext/node/ops/require.rs
+++ b/ext/node/ops/require.rs
@@ -32,11 +32,8 @@ fn ensure_read_permission<P>(
 where
   P: NodePermissions + 'static,
 {
-  let resolver = {
-    let resolver = state.borrow::<Arc<dyn NpmResolver>>();
-    resolver.clone()
-  };
-  let permissions = state.borrow_mut::<P>();
+  let resolver = state.borrow::<Arc<dyn NpmResolver>>();
+  let permissions = state.borrow::<P>();
   resolver.ensure_read_permission(permissions, file_path)
 }
 
@@ -98,7 +95,7 @@ pub fn op_require_node_module_paths<Env>(
 where
   Env: NodeEnv + 'static,
 {
-  let fs = state.borrow::<Arc<dyn NodeFs>>().clone();
+  let fs = state.borrow::<Arc<dyn NodeFs>>();
   // Guarantee that "from" is absolute.
   let from = deno_core::resolve_path(
     &from,
@@ -267,7 +264,7 @@ where
 {
   let path = PathBuf::from(path);
   ensure_read_permission::<Env::P>(state, &path)?;
-  let fs = state.borrow::<Arc<dyn NodeFs>>().clone();
+  let fs = state.borrow::<Arc<dyn NodeFs>>();
   if let Ok(metadata) = fs.metadata(&path) {
     if metadata.is_file {
       return Ok(0);
@@ -289,7 +286,7 @@ where
 {
   let path = PathBuf::from(request);
   ensure_read_permission::<Env::P>(state, &path)?;
-  let fs = state.borrow::<Arc<dyn NodeFs>>().clone();
+  let fs = state.borrow::<Arc<dyn NodeFs>>();
   let mut canonicalized_path = fs.canonicalize(&path)?;
   if cfg!(windows) {
     canonicalized_path = PathBuf::from(
@@ -358,7 +355,7 @@ where
 
   if let Some(parent_id) = maybe_parent_id {
     if parent_id == "<repl>" || parent_id == "internal/preload" {
-      let fs = state.borrow::<Arc<dyn NodeFs>>().clone();
+      let fs = state.borrow::<Arc<dyn NodeFs>>();
       if let Ok(cwd) = fs.current_dir() {
         ensure_read_permission::<Env::P>(state, &cwd)?;
         return Ok(Some(cwd.to_string_lossy().to_string()));
@@ -381,8 +378,8 @@ where
     return Ok(None);
   }
 
-  let node_resolver = state.borrow::<Rc<NodeResolver>>().clone();
-  let permissions = state.borrow_mut::<Env::P>();
+  let node_resolver = state.borrow::<Rc<NodeResolver>>();
+  let permissions = state.borrow::<Env::P>();
   let pkg = node_resolver
     .get_package_scope_config(
       &Url::from_file_path(parent_path.unwrap()).unwrap(),
@@ -441,7 +438,7 @@ where
 {
   let file_path = PathBuf::from(file_path);
   ensure_read_permission::<Env::P>(state, &file_path)?;
-  let fs = state.borrow::<Arc<dyn NodeFs>>().clone();
+  let fs = state.borrow::<Arc<dyn NodeFs>>();
   Ok(fs.read_to_string(&file_path)?)
 }
 
@@ -469,10 +466,10 @@ fn op_require_resolve_exports<Env>(
 where
   Env: NodeEnv + 'static,
 {
-  let fs = state.borrow::<Arc<dyn NodeFs>>().clone();
-  let npm_resolver = state.borrow::<Arc<dyn NpmResolver>>().clone();
-  let node_resolver = state.borrow::<Rc<NodeResolver>>().clone();
-  let permissions = state.borrow_mut::<Env::P>();
+  let fs = state.borrow::<Arc<dyn NodeFs>>();
+  let npm_resolver = state.borrow::<Arc<dyn NpmResolver>>();
+  let node_resolver = state.borrow::<Rc<NodeResolver>>();
+  let permissions = state.borrow::<Env::P>();
 
   let pkg_path = if npm_resolver
     .in_npm_package_at_path(&PathBuf::from(&modules_path))
@@ -524,8 +521,8 @@ where
     state,
     PathBuf::from(&filename).parent().unwrap(),
   )?;
-  let node_resolver = state.borrow::<Rc<NodeResolver>>().clone();
-  let permissions = state.borrow_mut::<Env::P>();
+  let node_resolver = state.borrow::<Rc<NodeResolver>>();
+  let permissions = state.borrow::<Env::P>();
   node_resolver.get_closest_package_json(
     &Url::from_file_path(filename).unwrap(),
     permissions,
@@ -540,8 +537,8 @@ fn op_require_read_package_scope<Env>(
 where
   Env: NodeEnv + 'static,
 {
-  let node_resolver = state.borrow::<Rc<NodeResolver>>().clone();
-  let permissions = state.borrow_mut::<Env::P>();
+  let node_resolver = state.borrow::<Rc<NodeResolver>>();
+  let permissions = state.borrow::<Env::P>();
   let package_json_path = PathBuf::from(package_json_path);
   node_resolver
     .load_package_json(permissions, package_json_path)
@@ -559,8 +556,8 @@ where
 {
   let parent_path = PathBuf::from(&parent_filename);
   ensure_read_permission::<Env::P>(state, &parent_path)?;
-  let node_resolver = state.borrow::<Rc<NodeResolver>>().clone();
-  let permissions = state.borrow_mut::<Env::P>();
+  let node_resolver = state.borrow::<Rc<NodeResolver>>();
+  let permissions = state.borrow::<Env::P>();
   let pkg = node_resolver
     .load_package_json(permissions, parent_path.join("package.json"))?;
 
diff --git a/ext/node/package_json.rs b/ext/node/package_json.rs
index 0e34897e3..940e32631 100644
--- a/ext/node/package_json.rs
+++ b/ext/node/package_json.rs
@@ -65,7 +65,7 @@ impl PackageJson {
   pub fn load(
     fs: &dyn NodeFs,
     resolver: &dyn NpmResolver,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
     path: PathBuf,
   ) -> Result<PackageJson, AnyError> {
     resolver.ensure_read_permission(permissions, &path)?;
diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs
index e5db6b3ac..0c90fffb6 100644
--- a/ext/node/resolution.rs
+++ b/ext/node/resolution.rs
@@ -127,7 +127,7 @@ impl NodeResolver {
     specifier: &str,
     referrer: &ModuleSpecifier,
     mode: NodeResolutionMode,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<Option<NodeResolution>, AnyError> {
     // Note: if we are here, then the referrer is an esm module
     // TODO(bartlomieju): skipped "policy" part as we don't plan to support it
@@ -201,7 +201,7 @@ impl NodeResolver {
     referrer: &ModuleSpecifier,
     conditions: &[&str],
     mode: NodeResolutionMode,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<Option<ModuleSpecifier>, AnyError> {
     // note: if we're here, the referrer is an esm module
     let url = if should_be_treated_as_relative_or_absolute_path(specifier) {
@@ -305,7 +305,7 @@ impl NodeResolver {
     &self,
     reference: &NpmPackageReqReference,
     mode: NodeResolutionMode,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<Option<NodeResolution>, AnyError> {
     let reference = self
       .npm_resolver
@@ -317,7 +317,7 @@ impl NodeResolver {
     &self,
     reference: &NpmPackageNvReference,
     mode: NodeResolutionMode,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<Option<NodeResolution>, AnyError> {
     let package_folder = self
       .npm_resolver
@@ -367,8 +367,8 @@ impl NodeResolver {
       .npm_resolver
       .resolve_package_folder_from_deno_module(pkg_nv)?;
     let package_json_path = package_folder.join("package.json");
-    let package_json = self
-      .load_package_json(&mut AllowAllNodePermissions, package_json_path)?;
+    let package_json =
+      self.load_package_json(&AllowAllNodePermissions, package_json_path)?;
 
     Ok(match package_json.bin {
       Some(Value::String(_)) => vec![pkg_nv.name.to_string()],
@@ -392,8 +392,8 @@ impl NodeResolver {
       .npm_resolver
       .resolve_package_folder_from_deno_module(&pkg_nv)?;
     let package_json_path = package_folder.join("package.json");
-    let package_json = self
-      .load_package_json(&mut AllowAllNodePermissions, package_json_path)?;
+    let package_json =
+      self.load_package_json(&AllowAllNodePermissions, package_json_path)?;
     let bin = match &package_json.bin {
       Some(bin) => bin,
       None => bail!(
@@ -420,7 +420,7 @@ impl NodeResolver {
       Ok(NodeResolution::Esm(url))
     } else if url_str.ends_with(".js") || url_str.ends_with(".d.ts") {
       let package_config =
-        self.get_closest_package_json(&url, &mut AllowAllNodePermissions)?;
+        self.get_closest_package_json(&url, &AllowAllNodePermissions)?;
       if package_config.typ == "module" {
         Ok(NodeResolution::Esm(url))
       } else {
@@ -444,7 +444,7 @@ impl NodeResolver {
     referrer_kind: NodeModuleKind,
     conditions: &[&str],
     mode: NodeResolutionMode,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<Option<PathBuf>, AnyError> {
     let package_json_path = package_dir.join("package.json");
     let referrer = ModuleSpecifier::from_directory_path(package_dir).unwrap();
@@ -537,7 +537,7 @@ impl NodeResolver {
     referrer_kind: NodeModuleKind,
     conditions: &[&str],
     mode: NodeResolutionMode,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<PathBuf, AnyError> {
     if name == "#" || name.starts_with("#/") || name.ends_with('/') {
       let reason = "is not a valid internal imports specifier name";
@@ -638,7 +638,7 @@ impl NodeResolver {
     internal: bool,
     conditions: &[&str],
     mode: NodeResolutionMode,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<PathBuf, AnyError> {
     if !subpath.is_empty() && !pattern && !target.ends_with('/') {
       return Err(throw_invalid_package_target(
@@ -747,7 +747,7 @@ impl NodeResolver {
     internal: bool,
     conditions: &[&str],
     mode: NodeResolutionMode,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<Option<PathBuf>, AnyError> {
     if let Some(target) = target.as_str() {
       return self
@@ -871,7 +871,7 @@ impl NodeResolver {
     referrer_kind: NodeModuleKind,
     conditions: &[&str],
     mode: NodeResolutionMode,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<PathBuf, AnyError> {
     if package_exports.contains_key(&package_subpath)
       && package_subpath.find('*').is_none()
@@ -975,7 +975,7 @@ impl NodeResolver {
     referrer_kind: NodeModuleKind,
     conditions: &[&str],
     mode: NodeResolutionMode,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<Option<PathBuf>, AnyError> {
     let (package_name, package_subpath, _is_scoped) =
       parse_package_name(specifier, referrer)?;
@@ -1055,7 +1055,7 @@ impl NodeResolver {
   pub(super) fn get_package_scope_config(
     &self,
     referrer: &ModuleSpecifier,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<PackageJson, AnyError> {
     let root_folder = self
       .npm_resolver
@@ -1067,7 +1067,7 @@ impl NodeResolver {
   pub(super) fn get_closest_package_json(
     &self,
     url: &ModuleSpecifier,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
   ) -> Result<PackageJson, AnyError> {
     let package_json_path = self.get_closest_package_json_path(url)?;
     self.load_package_json(permissions, package_json_path)
@@ -1099,7 +1099,7 @@ impl NodeResolver {
 
   pub(super) fn load_package_json(
     &self,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
     package_json_path: PathBuf,
   ) -> Result<PackageJson, AnyError> {
     PackageJson::load(