refactor(ext/node): add NodeEnv::Fs associated type (denoland#18484)

This commit adds associated type to "NodeEnv" trait, called "Fs".

The "Fs" type has a trait bound on "NodeFs", which specifies APIs
required for all ops and resolution APIs to function.

A "RealFs" implementation of "NodeFs" is exported from the "deno_node"
crate, that provides a default implementation for the trait.

All code in "deno_node" extension was changed to use the "NodeFs" trait
to handle file system operations, instead of relying on APIs from the
standard library.

cli/node/mod.rs

@@ -31,6 +31,7 @@ use deno_runtime::deno_node::NodePermissions;
 use deno_runtime::deno_node::NodeResolutionMode;
 use deno_runtime::deno_node::PackageJson;
 use deno_runtime::deno_node::PathClean;
+use deno_runtime::deno_node::RealFs;
 use deno_runtime::deno_node::RequireNpmResolver;
 use deno_runtime::deno_node::DEFAULT_CONDITIONS;
 use deno_runtime::permissions::PermissionsContainer;
@@ -227,10 +228,11 @@ pub fn node_resolve(
  let path = url.to_file_path().unwrap();
  // todo(16370): the module kind is not correct here. I think we need
  // typescript to tell us if the referrer is esm or cjs
- let path = match path_to_declaration_path(path, NodeModuleKind::Esm) {
- Some(path) => path,
- None => return Ok(None),
- };
+ let path =
+ match path_to_declaration_path::<RealFs>(path, NodeModuleKind::Esm) {
+ Some(path) => path,
+ None => return Ok(None),
+ };
  ModuleSpecifier::from_file_path(path).unwrap()
  }
  };
@@ -273,7 +275,8 @@ pub fn node_resolve_npm_reference(
  let resolved_path = match mode {
  NodeResolutionMode::Execution => resolved_path,
  NodeResolutionMode::Types => {
- match path_to_declaration_path(resolved_path, node_module_kind) {
+ match path_to_declaration_path::<RealFs>(resolved_path, node_module_kind)
+ {
  Some(path) => path,
  None => return Ok(None),
  }
@@ -312,7 +315,7 @@ pub fn node_resolve_binary_commands(
  let package_folder =
  npm_resolver.resolve_package_folder_from_deno_module(pkg_nv)?;
  let package_json_path = package_folder.join("package.json");
- let package_json = PackageJson::load(
+ let package_json = PackageJson::load::<RealFs>(
  npm_resolver,
  &mut PermissionsContainer::allow_all(),
  package_json_path,
@@ -335,7 +338,7 @@ pub fn node_resolve_binary_export(
  let package_folder =
  npm_resolver.resolve_package_folder_from_deno_module(pkg_nv)?;
  let package_json_path = package_folder.join("package.json");
- let package_json = PackageJson::load(
+ let package_json = PackageJson::load::<RealFs>(
  npm_resolver,
  &mut PermissionsContainer::allow_all(),
  package_json_path,
@@ -424,10 +427,13 @@ fn package_config_resolve(
 ) -> Result<Option<PathBuf>, AnyError> {
  let package_json_path = package_dir.join("package.json");
  let referrer = ModuleSpecifier::from_directory_path(package_dir).unwrap();
- let package_config =
- PackageJson::load(npm_resolver, permissions, package_json_path.clone())?;
+ let package_config = PackageJson::load::<RealFs>(
+ npm_resolver,
+ permissions,
+ package_json_path.clone(),
+ )?;
  if let Some(exports) = &package_config.exports {
- let result = package_exports_resolve(
+ let result = package_exports_resolve::<RealFs>(
  &package_json_path,
  package_subpath.to_string(),
  exports,
@@ -443,7 +449,7 @@ fn package_config_resolve(
  Err(exports_err) => {
  if mode.is_types() && package_subpath == "." {
  if let Ok(Some(path)) =
- legacy_main_resolve(&package_config, referrer_kind, mode)
+ legacy_main_resolve::<RealFs>(&package_config, referrer_kind, mode)
  {
  return Ok(Some(path));
  } else {
@@ -455,7 +461,7 @@ fn package_config_resolve(
  }
  }
  if package_subpath == "." {
- return legacy_main_resolve(&package_config, referrer_kind, mode);
+ return legacy_main_resolve::<RealFs>(&package_config, referrer_kind, mode);
  }
 
  Ok(Some(package_dir.join(package_subpath)))
@@ -469,7 +475,7 @@ pub fn url_to_node_resolution(
  if url_str.starts_with("http") {
  Ok(NodeResolution::Esm(url))
  } else if url_str.ends_with(".js") || url_str.ends_with(".d.ts") {
- let package_config = get_closest_package_json(
+ let package_config = get_closest_package_json::<RealFs>(
  &url,
  npm_resolver,
  &mut PermissionsContainer::allow_all(),
@@ -556,7 +562,7 @@ fn module_resolve(
  // todo(dsherret): the node module kind is not correct and we
  // should use the value provided by typescript instead
  let declaration_path =
- path_to_declaration_path(file_path, NodeModuleKind::Esm);
+ path_to_declaration_path::<RealFs>(file_path, NodeModuleKind::Esm);
  declaration_path.map(|declaration_path| {
  ModuleSpecifier::from_file_path(declaration_path).unwrap()
  })
@@ -565,7 +571,7 @@ fn module_resolve(
  }
  } else if specifier.starts_with('#') {
  Some(
- package_imports_resolve(
+ package_imports_resolve::<RealFs>(
  specifier,
  referrer,
  NodeModuleKind::Esm,
@@ -579,7 +585,7 @@ fn module_resolve(
  } else if let Ok(resolved) = Url::parse(specifier) {
  Some(resolved)
  } else {
- package_resolve(
+ package_resolve::<RealFs>(
  specifier,
  referrer,
  NodeModuleKind::Esm,
@@ -821,11 +827,14 @@ fn resolve(
 
  let package_json_path = module_dir.join("package.json");
  if package_json_path.exists() {
- let package_json =
- PackageJson::load(npm_resolver, permissions, package_json_path.clone())?;
+ let package_json = PackageJson::load::<RealFs>(
+ npm_resolver,
+ permissions,
+ package_json_path.clone(),
+ )?;
 
  if let Some(exports) = &package_json.exports {
- return package_exports_resolve(
+ return package_exports_resolve::<deno_node::RealFs>(
  &package_json_path,
  package_subpath,
  exports,
@@ -846,8 +855,11 @@ fn resolve(
  // subdir might have a package.json that specifies the entrypoint
  let package_json_path = d.join("package.json");
  if package_json_path.exists() {
- let package_json =
- PackageJson::load(npm_resolver, permissions, package_json_path)?;
+ let package_json = PackageJson::load::<RealFs>(
+ npm_resolver,
+ permissions,
+ package_json_path,
+ )?;
  if let Some(main) = package_json.main(NodeModuleKind::Cjs) {
  return Ok(d.join(main).clean());
  }

cli/npm/resolvers/local.rs

@@ -159,9 +159,9 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver {
  if sub_dir.is_dir() {
  // if doing types resolution, only resolve the package if it specifies a types property
  if mode.is_types() && !name.starts_with("@types/") {
- let package_json = PackageJson::load_skip_read_permission(
- sub_dir.join("package.json"),
- )?;
+ let package_json = PackageJson::load_skip_read_permission::<
+ deno_runtime::deno_node::RealFs,
+ >(sub_dir.join("package.json"))?;
  if package_json.types.is_some() {
  return Ok(sub_dir);
  }

ext/node/clippy.toml

@@ -0,0 +1,21 @@
+disallowed-methods = [
+ { path = "std::env::current_dir", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::path::Path::exists", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::canonicalize", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::copy", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::create_dir", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::create_dir_all", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::hard_link", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::metadata", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::read", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::read_dir", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::read_link", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::read_to_string", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::remove_dir", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::remove_dir_all", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::remove_file", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::rename", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::set_permissions", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::symlink_metadata", reason = "File system operations should be done using NodeFs trait" },
+ { path = "std::fs::write", reason = "File system operations should be done using NodeFs trait" },
+]

ext/node/lib.rs

@@ -6,6 +6,7 @@ use deno_core::op;
 use deno_core::JsRuntime;
 use once_cell::sync::Lazy;
 use std::collections::HashSet;
+use std::io;
 use std::path::Path;
 use std::path::PathBuf;
 use std::rc::Rc;
@@ -41,18 +42,36 @@ pub use resolution::DEFAULT_CONDITIONS;
 
 pub trait NodeEnv {
  type P: NodePermissions;
- // TODO(bartlomieju):
- // type Fs: NodeFs;
+ type Fs: NodeFs;
 }
 
 pub trait NodePermissions {
  fn check_read(&mut self, path: &Path) -> Result<(), AnyError>;
 }
 
-// TODO(bartlomieju):
-// pub trait NodeFs {
-// fn current_dir() -> Result<PathBuf, AnyError>;
-// }
+pub trait NodeFs {
+ fn current_dir() -> io::Result<PathBuf>;
+ fn metadata<P: AsRef<Path>>(path: P) -> io::Result<std::fs::Metadata>;
+ fn read_to_string<P: AsRef<Path>>(path: P) -> io::Result<String>;
+}
+
+pub struct RealFs;
+impl NodeFs for RealFs {
+ fn current_dir() -> io::Result<PathBuf> {
+ #[allow(clippy::disallowed_methods)]
+ std::env::current_dir()
+ }
+
+ fn metadata<P: AsRef<Path>>(path: P) -> io::Result<std::fs::Metadata> {
+ #[allow(clippy::disallowed_methods)]
+ std::fs::metadata(path)
+ }
+
+ fn read_to_string<P: AsRef<Path>>(path: P) -> io::Result<String> {
+ #[allow(clippy::disallowed_methods)]
+ std::fs::read_to_string(path)
+ }
+}
 
 pub trait RequireNpmResolver {
  fn resolve_package_folder_from_package(

ext/node/ops.rs

@@ -14,6 +14,7 @@ use std::path::PathBuf;
 use std::rc::Rc;
 
 use crate::NodeEnv;
+use crate::NodeFs;
 
 use super::resolution;
 use super::NodeModuleKind;
@@ -98,7 +99,7 @@ where
  // Guarantee that "from" is absolute.
  let from = deno_core::resolve_path(
  &from,
- &std::env::current_dir().context("Unable to get CWD")?,
+ &(Env::Fs::current_dir()).context("Unable to get CWD")?,
  )
  .unwrap()
  .to_file_path()
@@ -263,7 +264,7 @@ where
 {
  let path = PathBuf::from(path);
  ensure_read_permission::<Env::P>(state, &path)?;
- if let Ok(metadata) = std::fs::metadata(&path) {
+ if let Ok(metadata) = Env::Fs::metadata(&path) {
  if metadata.is_file() {
  return Ok(0);
  } else {
@@ -352,7 +353,7 @@ where
 
  if let Some(parent_id) = maybe_parent_id {
  if parent_id == "<repl>" || parent_id == "internal/preload" {
- if let Ok(cwd) = std::env::current_dir() {
+ if let Ok(cwd) = Env::Fs::current_dir() {
  ensure_read_permission::<Env::P>(state, &cwd)?;
  return Ok(Some(cwd.to_string_lossy().to_string()));
  }
@@ -376,7 +377,7 @@ where
 
  let resolver = state.borrow::<Rc<dyn RequireNpmResolver>>().clone();
  let permissions = state.borrow_mut::<Env::P>();
- let pkg = resolution::get_package_scope_config(
+ let pkg = resolution::get_package_scope_config::<Env::Fs>(
  &Url::from_file_path(parent_path.unwrap()).unwrap(),
  &*resolver,
  permissions,
@@ -407,7 +408,7 @@ where
 
  let referrer = deno_core::url::Url::from_file_path(&pkg.path).unwrap();
  if let Some(exports) = &pkg.exports {
- resolution::package_exports_resolve(
+ resolution::package_exports_resolve::<Env::Fs>(
  &pkg.path,
  expansion,
  exports,
@@ -434,7 +435,7 @@ where
 {
  let file_path = PathBuf::from(file_path);
  ensure_read_permission::<Env::P>(state, &file_path)?;
- Ok(std::fs::read_to_string(file_path)?)
+ Ok(Env::Fs::read_to_string(file_path)?)
 }
 
 #[op]
@@ -471,15 +472,15 @@ where
  } else {
  path_resolve(vec![modules_path, name])
  };
- let pkg = PackageJson::load(
+ let pkg = PackageJson::load::<Env::Fs>(
  &*resolver,
  permissions,
  PathBuf::from(&pkg_path).join("package.json"),
  )?;
 
  if let Some(exports) = &pkg.exports {
  let referrer = Url::from_file_path(parent_path).unwrap();
- resolution::package_exports_resolve(
+ resolution::package_exports_resolve::<Env::Fs>(
  &pkg.path,
  format!(".{expansion}"),
  exports,
@@ -510,7 +511,7 @@ where
  )?;
  let resolver = state.borrow::<Rc<dyn RequireNpmResolver>>().clone();
  let permissions = state.borrow_mut::<Env::P>();
- resolution::get_closest_package_json(
+ resolution::get_closest_package_json::<Env::Fs>(
  &Url::from_file_path(filename).unwrap(),
  &*resolver,
  permissions,
@@ -528,7 +529,7 @@ where
  let resolver = state.borrow::<Rc<dyn RequireNpmResolver>>().clone();
  let permissions = state.borrow_mut::<Env::P>();
  let package_json_path = PathBuf::from(package_json_path);
- PackageJson::load(&*resolver, permissions, package_json_path).ok()
+ PackageJson::load::<Env::Fs>(&*resolver, permissions, package_json_path).ok()
 }
 
 #[op]
@@ -544,7 +545,7 @@ where
  ensure_read_permission::<Env::P>(state, &parent_path)?;
  let resolver = state.borrow::<Rc<dyn RequireNpmResolver>>().clone();
  let permissions = state.borrow_mut::<Env::P>();
- let pkg = PackageJson::load(
+ let pkg = PackageJson::load::<Env::Fs>(
  &*resolver,
  permissions,
  parent_path.join("package.json"),
@@ -553,7 +554,7 @@ where
  if pkg.imports.is_some() {
  let referrer =
  deno_core::url::Url::from_file_path(&parent_filename).unwrap();
- let r = resolution::package_imports_resolve(
+ let r = resolution::package_imports_resolve::<Env::Fs>(
  &request,
  &referrer,
  NodeModuleKind::Cjs,

ext/node/package_json.rs

@@ -1,5 +1,6 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
+use crate::NodeFs;
 use crate::NodeModuleKind;
 use crate::NodePermissions;
 
@@ -61,16 +62,16 @@ impl PackageJson {
  }
  }
 
- pub fn load(
+ pub fn load<Fs: NodeFs>(
  resolver: &dyn RequireNpmResolver,
  permissions: &mut dyn NodePermissions,
  path: PathBuf,
  ) -> Result<PackageJson, AnyError> {
  resolver.ensure_read_permission(permissions, &path)?;
- Self::load_skip_read_permission(path)
+ Self::load_skip_read_permission::<Fs>(path)
  }
 
- pub fn load_skip_read_permission(
+ pub fn load_skip_read_permission<Fs: NodeFs>(
  path: PathBuf,
  ) -> Result<PackageJson, AnyError> {
  assert!(path.is_absolute());
@@ -79,7 +80,7 @@ impl PackageJson {
  return Ok(CACHE.with(|cache| cache.borrow()[&path].clone()));
  }
 
- let source = match std::fs::read_to_string(&path) {
+ let source = match Fs::read_to_string(&path) {
  Ok(source) => source,
  Err(err) if err.kind() == ErrorKind::NotFound => {
  return Ok(PackageJson::empty(path));