refactor(ext/node): combine `deno_node::Fs` with `deno_fs::FileSystem` (#18991)

7 files changed, 68 insertions, 131 deletions

diff --git a/ext/node/Cargo.toml b/ext/node/Cargo.toml
index 38c8474dc..6a897a9a1 100644
--- a/ext/node/Cargo.toml
+++ b/ext/node/Cargo.toml
@@ -18,6 +18,7 @@ aes.workspace = true
 cbc.workspace = true
 data-encoding = "2.3.3"
 deno_core.workspace = true
+deno_fs.workspace = true
 deno_media_type.workspace = true
 deno_npm.workspace = true
 deno_semver.workspace = true
diff --git a/ext/node/analyze.rs b/ext/node/analyze.rs
index 2622ce8da..bad0906c5 100644
--- a/ext/node/analyze.rs
+++ b/ext/node/analyze.rs
@@ -13,7 +13,6 @@ use once_cell::sync::Lazy;
 
 use deno_core::error::AnyError;
 
-use crate::NodeFs;
 use crate::NodeModuleKind;
 use crate::NodePermissions;
 use crate::NodeResolutionMode;
@@ -67,7 +66,7 @@ pub trait CjsEsmCodeAnalyzer {
 
 pub struct NodeCodeTranslator<TCjsEsmCodeAnalyzer: CjsEsmCodeAnalyzer> {
   cjs_esm_code_analyzer: TCjsEsmCodeAnalyzer,
-  fs: Arc<dyn NodeFs>,
+  fs: Arc<dyn deno_fs::FileSystem>,
   node_resolver: Arc<NodeResolver>,
   npm_resolver: Arc<dyn NpmResolver>,
 }
@@ -77,7 +76,7 @@ impl<TCjsEsmCodeAnalyzer: CjsEsmCodeAnalyzer>
 {
   pub fn new(
     cjs_esm_code_analyzer: TCjsEsmCodeAnalyzer,
-    fs: Arc<dyn NodeFs>,
+    fs: Arc<dyn deno_fs::FileSystem>,
     node_resolver: Arc<NodeResolver>,
     npm_resolver: Arc<dyn NpmResolver>,
   ) -> Self {
@@ -161,6 +160,7 @@ impl<TCjsEsmCodeAnalyzer: CjsEsmCodeAnalyzer>
       let reexport_file_text = self
         .fs
         .read_to_string(&resolved_reexport)
+        .map_err(AnyError::from)
         .with_context(|| {
           format!(
             "Could not find '{}' ({}) referenced from {}",
diff --git a/ext/node/clippy.toml b/ext/node/clippy.toml
index 94796f5a7..31d9d7d47 100644
--- a/ext/node/clippy.toml
+++ b/ext/node/clippy.toml
@@ -1,40 +1,40 @@
 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::path::Path::canonicalize", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::Path::is_dir", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::Path::is_file", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::Path::is_symlink", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::Path::metadata", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::Path::read_dir", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::Path::read_link", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::Path::symlink_metadata", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::Path::try_exists", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::PathBuf::exists", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::PathBuf::canonicalize", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::PathBuf::is_dir", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::PathBuf::is_file", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::PathBuf::is_symlink", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::PathBuf::metadata", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::PathBuf::read_dir", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::PathBuf::read_link", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::PathBuf::symlink_metadata", reason = "File system operations should be done using NodeFs trait" },
-  { path = "std::path::PathBuf::try_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" },
+  { path = "std::env::current_dir", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::Path::exists", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::Path::canonicalize", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::Path::is_dir", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::Path::is_file", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::Path::is_symlink", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::Path::metadata", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::Path::read_dir", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::Path::read_link", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::Path::symlink_metadata", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::Path::try_exists", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::PathBuf::exists", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::PathBuf::canonicalize", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::PathBuf::is_dir", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::PathBuf::is_file", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::PathBuf::is_symlink", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::PathBuf::metadata", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::PathBuf::read_dir", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::PathBuf::read_link", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::PathBuf::symlink_metadata", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::path::PathBuf::try_exists", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::canonicalize", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::copy", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::create_dir", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::create_dir_all", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::hard_link", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::metadata", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::read", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::read_dir", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::read_link", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::read_to_string", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::remove_dir", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::remove_dir_all", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::remove_file", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::rename", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::set_permissions", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::symlink_metadata", reason = "File system operations should be done using FileSystem trait" },
+  { path = "std::fs::write", reason = "File system operations should be done using FileSystem trait" },
 ]
diff --git a/ext/node/lib.rs b/ext/node/lib.rs
index 128f3a2fe..03ec730d8 100644
--- a/ext/node/lib.rs
+++ b/ext/node/lib.rs
@@ -14,7 +14,6 @@ use deno_semver::npm::NpmPackageReq;
 use deno_semver::npm::NpmPackageReqReference;
 use once_cell::sync::Lazy;
 use std::collections::HashSet;
-use std::io;
 use std::path::Path;
 use std::path::PathBuf;
 use std::rc::Rc;
@@ -51,71 +50,6 @@ impl NodePermissions for AllowAllNodePermissions {
   }
 }
 
-#[derive(Default, Clone)]
-pub struct NodeFsMetadata {
-  pub is_file: bool,
-  pub is_dir: bool,
-}
-
-pub trait NodeFs: std::fmt::Debug + Send + Sync {
-  fn current_dir(&self) -> io::Result<PathBuf>;
-  fn metadata(&self, path: &Path) -> io::Result<NodeFsMetadata>;
-  fn is_file(&self, path: &Path) -> bool;
-  fn is_dir(&self, path: &Path) -> bool;
-  fn exists(&self, path: &Path) -> bool;
-  fn read_to_string(&self, path: &Path) -> io::Result<String>;
-  fn canonicalize(&self, path: &Path) -> io::Result<PathBuf>;
-}
-
-#[derive(Debug)]
-pub struct RealFs;
-
-impl NodeFs for RealFs {
-  fn current_dir(&self) -> io::Result<PathBuf> {
-    #[allow(clippy::disallowed_methods)]
-    std::env::current_dir()
-  }
-
-  fn metadata(&self, path: &Path) -> io::Result<NodeFsMetadata> {
-    #[allow(clippy::disallowed_methods)]
-    std::fs::metadata(path).map(|metadata| {
-      // on most systems, calling is_file() and is_dir() is cheap
-      // and returns information already found in the metadata object
-      NodeFsMetadata {
-        is_file: metadata.is_file(),
-        is_dir: metadata.is_dir(),
-      }
-    })
-  }
-
-  fn exists(&self, path: &Path) -> bool {
-    #[allow(clippy::disallowed_methods)]
-    std::fs::metadata(path).is_ok()
-  }
-
-  fn is_file(&self, path: &Path) -> bool {
-    #[allow(clippy::disallowed_methods)]
-    std::fs::metadata(path)
-      .map(|m| m.is_file())
-      .unwrap_or(false)
-  }
-
-  fn is_dir(&self, path: &Path) -> bool {
-    #[allow(clippy::disallowed_methods)]
-    std::fs::metadata(path).map(|m| m.is_dir()).unwrap_or(false)
-  }
-
-  fn read_to_string(&self, path: &Path) -> io::Result<String> {
-    #[allow(clippy::disallowed_methods)]
-    std::fs::read_to_string(path)
-  }
-
-  fn canonicalize(&self, path: &Path) -> io::Result<PathBuf> {
-    #[allow(clippy::disallowed_methods)]
-    std::path::Path::canonicalize(path)
-  }
-}
-
 pub trait NpmResolver: std::fmt::Debug + Send + Sync {
   /// Resolves an npm package folder path from an npm package referrer.
   fn resolve_package_folder_from_package(
@@ -516,10 +450,10 @@ deno_core::extension!(deno_node,
   ],
   options = {
     maybe_npm_resolver: Option<Arc<dyn NpmResolver>>,
-    fs: Option<Arc<dyn NodeFs>>,
+    fs: Arc<dyn deno_fs::FileSystem>,
   },
   state = |state, options| {
-    let fs = options.fs.unwrap_or_else(|| Arc::new(RealFs));
+    let fs = options.fs;
     state.put(fs.clone());
     if let Some(npm_resolver) = options.maybe_npm_resolver {
       state.put(npm_resolver.clone());
diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs
index 4a2b97187..972815995 100644
--- a/ext/node/ops/require.rs
+++ b/ext/node/ops/require.rs
@@ -16,7 +16,6 @@ use std::rc::Rc;
 use std::sync::Arc;
 
 use crate::resolution;
-use crate::NodeFs;
 use crate::NodeModuleKind;
 use crate::NodePermissions;
 use crate::NodeResolutionMode;
@@ -94,11 +93,11 @@ pub fn op_require_node_module_paths<P>(
 where
   P: NodePermissions + 'static,
 {
-  let fs = state.borrow::<Arc<dyn NodeFs>>();
+  let fs = state.borrow::<Arc<dyn deno_fs::FileSystem>>();
   // Guarantee that "from" is absolute.
   let from = deno_core::resolve_path(
     &from,
-    &(fs.current_dir()).context("Unable to get CWD")?,
+    &(fs.cwd().map_err(AnyError::from)).context("Unable to get CWD")?,
   )
   .unwrap()
   .to_file_path()
@@ -263,8 +262,8 @@ where
 {
   let path = PathBuf::from(path);
   ensure_read_permission::<P>(state, &path)?;
-  let fs = state.borrow::<Arc<dyn NodeFs>>();
-  if let Ok(metadata) = fs.metadata(&path) {
+  let fs = state.borrow::<Arc<dyn deno_fs::FileSystem>>();
+  if let Ok(metadata) = fs.stat_sync(&path) {
     if metadata.is_file {
       return Ok(0);
     } else {
@@ -285,8 +284,9 @@ where
 {
   let path = PathBuf::from(request);
   ensure_read_permission::<P>(state, &path)?;
-  let fs = state.borrow::<Arc<dyn NodeFs>>();
-  let canonicalized_path = deno_core::strip_unc_prefix(fs.canonicalize(&path)?);
+  let fs = state.borrow::<Arc<dyn deno_fs::FileSystem>>();
+  let canonicalized_path =
+    deno_core::strip_unc_prefix(fs.realpath_sync(&path)?);
   Ok(canonicalized_path.to_string_lossy().to_string())
 }
 
@@ -346,8 +346,8 @@ where
 
   if let Some(parent_id) = maybe_parent_id {
     if parent_id == "<repl>" || parent_id == "internal/preload" {
-      let fs = state.borrow::<Arc<dyn NodeFs>>();
-      if let Ok(cwd) = fs.current_dir() {
+      let fs = state.borrow::<Arc<dyn deno_fs::FileSystem>>();
+      if let Ok(cwd) = fs.cwd() {
         ensure_read_permission::<P>(state, &cwd)?;
         return Ok(Some(cwd.to_string_lossy().to_string()));
       }
@@ -429,7 +429,7 @@ where
 {
   let file_path = PathBuf::from(file_path);
   ensure_read_permission::<P>(state, &file_path)?;
-  let fs = state.borrow::<Arc<dyn NodeFs>>();
+  let fs = state.borrow::<Arc<dyn deno_fs::FileSystem>>();
   Ok(fs.read_to_string(&file_path)?)
 }
 
@@ -457,7 +457,7 @@ fn op_require_resolve_exports<P>(
 where
   P: NodePermissions + 'static,
 {
-  let fs = state.borrow::<Arc<dyn NodeFs>>();
+  let fs = state.borrow::<Arc<dyn deno_fs::FileSystem>>();
   let npm_resolver = state.borrow::<Arc<dyn NpmResolver>>();
   let node_resolver = state.borrow::<Rc<NodeResolver>>();
   let permissions = state.borrow::<P>();
diff --git a/ext/node/package_json.rs b/ext/node/package_json.rs
index 940e32631..95ca8b561 100644
--- a/ext/node/package_json.rs
+++ b/ext/node/package_json.rs
@@ -1,6 +1,5 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
-use crate::NodeFs;
 use crate::NodeModuleKind;
 use crate::NodePermissions;
 
@@ -63,7 +62,7 @@ impl PackageJson {
   }
 
   pub fn load(
-    fs: &dyn NodeFs,
+    fs: &dyn deno_fs::FileSystem,
     resolver: &dyn NpmResolver,
     permissions: &dyn NodePermissions,
     path: PathBuf,
@@ -73,7 +72,7 @@ impl PackageJson {
   }
 
   pub fn load_skip_read_permission(
-    fs: &dyn NodeFs,
+    fs: &dyn deno_fs::FileSystem,
     path: PathBuf,
   ) -> Result<PackageJson, AnyError> {
     assert!(path.is_absolute());
@@ -90,7 +89,7 @@ impl PackageJson {
       Err(err) => bail!(
         "Error loading package.json at {}. {:#}",
         path.display(),
-        err
+        AnyError::from(err),
       ),
     };
 
diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs
index 046c774fa..71b988c19 100644
--- a/ext/node/resolution.rs
+++ b/ext/node/resolution.rs
@@ -19,7 +19,6 @@ use deno_semver::npm::NpmPackageReqReference;
 
 use crate::errors;
 use crate::AllowAllNodePermissions;
-use crate::NodeFs;
 use crate::NodePermissions;
 use crate::NpmResolver;
 use crate::PackageJson;
@@ -107,12 +106,15 @@ impl NodeResolution {
 
 #[derive(Debug)]
 pub struct NodeResolver {
-  fs: Arc<dyn NodeFs>,
+  fs: Arc<dyn deno_fs::FileSystem>,
   npm_resolver: Arc<dyn NpmResolver>,
 }
 
 impl NodeResolver {
-  pub fn new(fs: Arc<dyn NodeFs>, npm_resolver: Arc<dyn NpmResolver>) -> Self {
+  pub fn new(
+    fs: Arc<dyn deno_fs::FileSystem>,
+    npm_resolver: Arc<dyn NpmResolver>,
+  ) -> Self {
     Self { fs, npm_resolver }
   }
 
@@ -280,8 +282,9 @@ impl NodeResolver {
       p_str.to_string()
     };
 
-    let (is_dir, is_file) = if let Ok(stats) = self.fs.metadata(Path::new(&p)) {
-      (stats.is_dir, stats.is_file)
+    let (is_dir, is_file) = if let Ok(stats) = self.fs.stat_sync(Path::new(&p))
+    {
+      (stats.is_directory, stats.is_file)
     } else {
       (false, false)
     };
@@ -491,7 +494,7 @@ impl NodeResolver {
     referrer_kind: NodeModuleKind,
   ) -> Option<PathBuf> {
     fn probe_extensions(
-      fs: &dyn NodeFs,
+      fs: &dyn deno_fs::FileSystem,
       path: &Path,
       referrer_kind: NodeModuleKind,
     ) -> Option<PathBuf> {
@@ -1079,7 +1082,7 @@ impl NodeResolver {
   ) -> Result<PathBuf, AnyError> {
     let file_path = url.to_file_path().unwrap();
     let current_dir = deno_core::strip_unc_prefix(
-      self.fs.canonicalize(file_path.parent().unwrap())?,
+      self.fs.realpath_sync(file_path.parent().unwrap())?,
     );
     let mut current_dir = current_dir.as_path();
     let package_json_path = current_dir.join("package.json");