refactor(ext/fs): abstract FS via FileSystem trait (denoland#18599)

This commit abstracts out the specifics of the underlying system calls FS operations behind a new `FileSystem` and `File` trait in the `ext/fs` extension. This allows other embedders to re-use ext/fs, but substituting in a different FS backend. This is likely not the final form of these traits. Eventually they will be entirely `deno_core::Resource` agnostic, and will live in a seperate crate. --------- Co-authored-by: Bartek Iwańczuk <[email protected]>
HurricanKai · Apr 12, 2023 · e9f41be · e9f41be
1 parent 4fab252
commit e9f41be
Show file tree

Hide file tree

Showing 15 changed files with 3,350 additions and 2,488 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/cli/build.rs b/cli/build.rs
@@ -8,6 +8,7 @@ use deno_core::Extension;
 use deno_core::ExtensionFileSource;
 use deno_core::ExtensionFileSourceCode;
 use deno_runtime::deno_cache::SqliteBackedCache;
+use deno_runtime::deno_fs::StdFs;
 use deno_runtime::deno_kv::sqlite::SqliteDbHandler;
 use deno_runtime::permissions::PermissionsContainer;
 use deno_runtime::*;
@@ -361,7 +362,7 @@ fn create_cli_snapshot(snapshot_path: PathBuf) {
  deno_napi::deno_napi::init_ops::<PermissionsContainer>(),
  deno_http::deno_http::init_ops(),
  deno_io::deno_io::init_ops(Default::default()),
- deno_fs::deno_fs::init_ops::<PermissionsContainer>(false),
+ deno_fs::deno_fs::init_ops::<_, PermissionsContainer>(false, StdFs),
  deno_node::deno_node::init_ops::<deno_runtime::RuntimeNodeEnv>(None),
  cli::init_ops_and_esm(), // NOTE: This needs to be init_ops_and_esm!
  ];

diff --git a/core/extensions.rs b/core/extensions.rs
@@ -180,7 +180,7 @@ macro_rules! extension {
  $(, deps = [ $( $dep:ident ),* ] )?
  $(, parameters = [ $( $param:ident : $type:ident ),+ ] )?
  $(, ops_fn = $ops_symbol:ident $( < $ops_param:ident > )? )?
- $(, ops = [ $( $(#[$m:meta])* $( $op:ident )::+ $( < $op_param:ident > )? ),+ $(,)? ] )?
+ $(, ops = [ $( $(#[$m:meta])* $( $op:ident )::+ $( < $( $op_param:ident ),* > )? ),+ $(,)? ] )?
  $(, esm_entry_point = $esm_entry_point:literal )?
  $(, esm = [ $( dir $dir_esm:literal , )? $( $esm:literal ),* $(,)? ] )?
  $(, esm_setup_script = $esm_setup_script:expr )?
@@ -235,7 +235,7 @@ macro_rules! extension {
  ext.ops(vec![
  $(
  $( #[ $m ] )*
- $( $op )::+ :: decl $( :: <$op_param> )? ()
+ $( $op )::+ :: decl $( :: < $($op_param),* > )? ()
  ),+
  ]);
  )?
@@ -267,11 +267,11 @@ macro_rules! extension {
  }
 
  #[allow(dead_code)]
- pub fn init_js_only $( < $( $param : $type + 'static ),+ > )? () -> $crate::Extension {
+ pub fn init_js_only $( < $( $param : $type + 'static ),* > )? () -> $crate::Extension {
  let mut ext = Self::ext();
  // If esm or JS was specified, add JS files
  Self::with_js(&mut ext);
- Self::with_ops $( ::<($( $param ),+)> )?(&mut ext);
+ Self::with_ops $( ::< $( $param ),+ > )?(&mut ext);
  Self::with_customizer(&mut ext);
  ext.take()
  }
@@ -281,17 +281,17 @@ macro_rules! extension {
  let mut ext = Self::ext();
  // If esm or JS was specified, add JS files
  Self::with_js(&mut ext);
- Self::with_ops $( ::<($( $param ),+)> )?(&mut ext);
- Self::with_state_and_middleware $( ::<($( $param ),+)> )?(&mut ext, $( $( $options_id , )* )? );
+ Self::with_ops $( ::< $( $param ),+ > )?(&mut ext);
+ Self::with_state_and_middleware $( ::< $( $param ),+ > )?(&mut ext, $( $( $options_id , )* )? );
  Self::with_customizer(&mut ext);
  ext.take()
  }
 
  #[allow(dead_code)]
  pub fn init_ops $( < $( $param : $type + 'static ),+ > )? ( $( $( $options_id : $options_type ),* )? ) -> $crate::Extension {
  let mut ext = Self::ext();
- Self::with_ops $( ::<($( $param ),+)> )?(&mut ext);
- Self::with_state_and_middleware $( ::<($( $param ),+)> )?(&mut ext, $( $( $options_id , )* )? );
+ Self::with_ops $( ::< $( $param ),+ > )?(&mut ext);
+ Self::with_state_and_middleware $( ::< $( $param ),+ > )?(&mut ext, $( $( $options_id , )* )? );
  Self::with_customizer(&mut ext);
  ext.take()
  }

diff --git a/ext/fs/30_fs.js b/ext/fs/30_fs.js
@@ -85,43 +85,50 @@ function chdir(directory) {
 }
 
 function makeTempDirSync(options = {}) {
- return ops.op_make_temp_dir_sync(options);
+ return ops.op_make_temp_dir_sync(options.dir, options.prefix, options.suffix);
 }
 
 function makeTempDir(options = {}) {
- return core.opAsync("op_make_temp_dir_async", options);
+ return core.opAsync(
+ "op_make_temp_dir_async",
+ options.dir,
+ options.prefix,
+ options.suffix,
+ );
 }
 
 function makeTempFileSync(options = {}) {
- return ops.op_make_temp_file_sync(options);
+ return ops.op_make_temp_file_sync(
+ options.dir,
+ options.prefix,
+ options.suffix,
+ );
 }
 
 function makeTempFile(options = {}) {
- return core.opAsync("op_make_temp_file_async", options);
-}
-
-function mkdirArgs(path, options) {
- const args = { path: pathFromURL(path), recursive: false };
- if (options != null) {
- if (typeof options.recursive == "boolean") {
- args.recursive = options.recursive;
- }
- if (options.mode) {
- args.mode = options.mode;
- }
- }
- return args;
+ return core.opAsync(
+ "op_make_temp_file_async",
+ options.dir,
+ options.prefix,
+ options.suffix,
+ );
 }
 
 function mkdirSync(path, options) {
- ops.op_mkdir_sync(mkdirArgs(path, options));
+ ops.op_mkdir_sync(
+ pathFromURL(path),
+ options?.recursive ?? false,
+ options?.mode,
+ );
 }
 
-async function mkdir(
- path,
- options,
-) {
- await core.opAsync2("op_mkdir_async", mkdirArgs(path, options));
+async function mkdir(path, options) {
+ await core.opAsync(
+ "op_mkdir_async",
+ pathFromURL(path),
+ options?.recursive ?? false,
+ options?.mode,
+ );
 }
 
 function readDirSync(path) {
@@ -306,44 +313,29 @@ async function fstat(rid) {
 }
 
 async function lstat(path) {
- const res = await core.opAsync("op_stat_async", {
- path: pathFromURL(path),
- lstat: true,
- });
+ const res = await core.opAsync("op_lstat_async", pathFromURL(path));
  return parseFileInfo(res);
 }
 
 function lstatSync(path) {
- ops.op_stat_sync(
- pathFromURL(path),
- true,
- statBuf,
- );
+ ops.op_lstat_sync(pathFromURL(path), statBuf);
  return statStruct(statBuf);
 }
 
 async function stat(path) {
- const res = await core.opAsync("op_stat_async", {
- path: pathFromURL(path),
- lstat: false,
- });
+ const res = await core.opAsync("op_stat_async", pathFromURL(path));
  return parseFileInfo(res);
 }
 
 function statSync(path) {
- ops.op_stat_sync(
- pathFromURL(path),
- false,
- statBuf,
- );
+ ops.op_stat_sync(pathFromURL(path), statBuf);
  return statStruct(statBuf);
 }
 
 function coerceLen(len) {
  if (len == null || len < 0) {
  return 0;
  }
-
  return len;
 }
 
@@ -518,27 +510,25 @@ function seekSync(
  offset,
  whence,
 ) {
- return ops.op_seek_sync({ rid, offset, whence });
+ return ops.op_seek_sync(rid, offset, whence);
 }
 
 function seek(
  rid,
  offset,
  whence,
 ) {
- return core.opAsync("op_seek_async", { rid, offset, whence });
+ return core.opAsync("op_seek_async", rid, offset, whence);
 }
 
 function openSync(
  path,
  options,
 ) {
  if (options) checkOpenOptions(options);
- const mode = options?.mode;
  const rid = ops.op_open_sync(
  pathFromURL(path),
  options,
- mode,
  );
 
  return new FsFile(rid);
@@ -549,12 +539,10 @@ async function open(
  options,
 ) {
  if (options) checkOpenOptions(options);
- const mode = options?.mode;
  const rid = await core.opAsync(
  "op_open_async",
  pathFromURL(path),
  options,
- mode,
  );
 
  return new FsFile(rid);
@@ -679,7 +667,7 @@ function checkOpenOptions(options) {
 const File = FsFile;
 
 function readFileSync(path) {
- return ops.op_readfile_sync(pathFromURL(path));
+ return ops.op_read_file_sync(pathFromURL(path));
 }
 
 async function readFile(path, options) {
@@ -694,7 +682,7 @@ async function readFile(path, options) {
 
  try {
  const read = await core.opAsync(
- "op_readfile_async",
+ "op_read_file_async",
  pathFromURL(path),
  cancelRid,
  );
@@ -710,7 +698,7 @@ async function readFile(path, options) {
 }
 
 function readTextFileSync(path) {
- return ops.op_readfile_text_sync(pathFromURL(path));
+ return ops.op_read_file_text_sync(pathFromURL(path));
 }
 
 async function readTextFile(path, options) {
@@ -725,7 +713,7 @@ async function readTextFile(path, options) {
 
  try {
  const read = await core.opAsync(
- "op_readfile_text_async",
+ "op_read_file_text_async",
  pathFromURL(path),
  cancelRid,
  );

diff --git a/ext/fs/Cargo.toml b/ext/fs/Cargo.toml
@@ -14,13 +14,14 @@ description = "Ops for interacting with the file system"
 path = "lib.rs"
 
 [dependencies]
+async-trait.workspace = true
 deno_core.workspace = true
-deno_crypto.workspace = true
 deno_io.workspace = true
 filetime = "0.2.16"
 fs3 = "0.5.0"
 libc.workspace = true
 log.workspace = true
+rand.workspace = true
 serde.workspace = true
 tokio.workspace = true
 

diff --git a/ext/fs/clippy.toml b/ext/fs/clippy.toml
@@ -0,0 +1,45 @@
+disallowed-methods = [
+ { path = "std::env::current_dir", reason = "File system operations should be done using FileSystem 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::env::set_current_dir", reason = "File system operations should be done using FileSystem trait" },
+ { path = "std::env::temp_dir", 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_all", 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::DirBuilder::new", 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::OpenOptions::new", 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::read", 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_dir", 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" },
+ { path = "std::path::Path::canonicalize", 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" },
+]