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`.

cli/lsp/documents.rs

@@ -1074,7 +1074,7 @@ impl Documents {
                 &specifier,
                 referrer,
                 NodeResolutionMode::Types,
-                &mut PermissionsContainer::allow_all(),
+                &PermissionsContainer::allow_all(),
               )
               .ok()
               .flatten(),
@@ -1461,7 +1461,7 @@ fn node_resolve_npm_req_ref(
         .resolve_npm_req_reference(
           &npm_req_ref,
           NodeResolutionMode::Types,
-          &mut PermissionsContainer::allow_all(),
+          &PermissionsContainer::allow_all(),
         )
         .ok()
         .flatten(),

cli/module_loader.rs

@@ -450,10 +450,10 @@ impl ModuleLoader for CliModuleLoader {
     referrer: &str,
     kind: ResolutionKind,
   ) -> Result<ModuleSpecifier, AnyError> {
-    let mut permissions = if matches!(kind, ResolutionKind::DynamicImport) {
-      self.dynamic_permissions.clone()
+    let permissions = if matches!(kind, ResolutionKind::DynamicImport) {
+      &self.dynamic_permissions
     } else {
-      self.root_permissions.clone()
+      &self.root_permissions
     };
 
     // TODO(bartlomieju): ideally we shouldn't need to call `current_dir()` on each
@@ -469,7 +469,7 @@ impl ModuleLoader for CliModuleLoader {
             specifier,
             referrer,
             NodeResolutionMode::Execution,
-            &mut permissions,
+            permissions,
           ))
           .with_context(|| {
             format!("Could not resolve '{specifier}' from '{referrer}'.")
@@ -494,7 +494,7 @@ impl ModuleLoader for CliModuleLoader {
                 self.node_resolver.resolve_npm_reference(
                   &module.nv_reference,
                   NodeResolutionMode::Execution,
-                  &mut permissions,
+                  permissions,
                 ),
               )
               .with_context(|| {
@@ -556,7 +556,7 @@ impl ModuleLoader for CliModuleLoader {
               self.node_resolver.resolve_npm_req_reference(
                 &reference,
                 NodeResolutionMode::Execution,
-                &mut permissions,
+                permissions,
               ),
             )
             .with_context(|| format!("Could not resolve '{reference}'."));

cli/npm/resolvers/common.rs

@@ -47,7 +47,7 @@ pub trait NpmPackageFsResolver: Send + Sync {
 
   fn ensure_read_permission(
     &self,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
     path: &Path,
   ) -> Result<(), AnyError>;
 }
@@ -90,7 +90,7 @@ pub async fn cache_packages(
 }
 
 pub fn ensure_registry_read_permission(
-  permissions: &mut dyn NodePermissions,
+  permissions: &dyn NodePermissions,
   registry_path: &Path,
   path: &Path,
 ) -> Result<(), AnyError> {

cli/npm/resolvers/global.rs

@@ -126,7 +126,7 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver {
 
   fn ensure_read_permission(
     &self,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
     path: &Path,
   ) -> Result<(), AnyError> {
     let registry_path = self.cache.registry_folder(&self.registry_url);

cli/npm/resolvers/local.rs

@@ -206,7 +206,7 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver {
 
   fn ensure_read_permission(
     &self,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
     path: &Path,
   ) -> Result<(), AnyError> {
     ensure_registry_read_permission(

cli/npm/resolvers/mod.rs

@@ -262,7 +262,7 @@ impl NpmResolver for CliNpmResolver {
 
   fn ensure_read_permission(
     &self,
-    permissions: &mut dyn NodePermissions,
+    permissions: &dyn NodePermissions,
     path: &Path,
   ) -> Result<(), AnyError> {
     self.fs_resolver.ensure_read_permission(permissions, path)

cli/tsc/mod.rs

@@ -639,7 +639,7 @@ fn resolve_graph_specifier_types(
         let maybe_resolution = node_resolver.resolve_npm_reference(
           &module.nv_reference,
           NodeResolutionMode::Types,
-          &mut PermissionsContainer::allow_all(),
+          &PermissionsContainer::allow_all(),
         )?;
         Ok(Some(NodeResolution::into_specifier_and_media_type(
           maybe_resolution,
@@ -679,7 +679,7 @@ fn resolve_non_graph_specifier_types(
           specifier,
           referrer,
           NodeResolutionMode::Types,
-          &mut PermissionsContainer::allow_all(),
+          &PermissionsContainer::allow_all(),
         )
         .ok()
         .flatten(),
@@ -692,7 +692,7 @@ fn resolve_non_graph_specifier_types(
     let maybe_resolution = node_resolver.resolve_npm_req_reference(
       &npm_ref,
       NodeResolutionMode::Types,
-      &mut PermissionsContainer::allow_all(),
+      &PermissionsContainer::allow_all(),
     )?;
     Ok(Some(NodeResolution::into_specifier_and_media_type(
       maybe_resolution,

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>;
 }

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"))?;
 

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)?;

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(

runtime/build.rs

@@ -122,10 +122,7 @@ mod startup_snapshot {
   }
 
   impl deno_node::NodePermissions for Permissions {
-    fn check_read(
-      &mut self,
-      _p: &Path,
-    ) -> Result<(), deno_core::error::AnyError> {
+    fn check_read(&self, _p: &Path) -> Result<(), deno_core::error::AnyError> {
       unreachable!("snapshotting!")
     }
   }

runtime/permissions/mod.rs

@@ -1872,7 +1872,7 @@ impl PermissionsContainer {
 
 impl deno_node::NodePermissions for PermissionsContainer {
   #[inline(always)]
-  fn check_read(&mut self, path: &Path) -> Result<(), AnyError> {
+  fn check_read(&self, path: &Path) -> Result<(), AnyError> {
     self.0.lock().read.check(path, None)
   }
 }