refactor: use sys_traits (#557)

Author: dsherret

.clippy.toml

@@ -2,6 +2,44 @@
 # away swc's non-zero-indexed based positioning
 disallowed-methods = [
   "swc_common::Spanned::span",
+  { path = "std::env::current_dir", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::Path::exists", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::Path::canonicalize", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::Path::is_dir", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::Path::is_file", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::Path::is_symlink", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::Path::metadata", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::Path::read_dir", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::Path::read_link", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::Path::symlink_metadata", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::Path::try_exists", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::PathBuf::exists", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::PathBuf::canonicalize", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::PathBuf::is_dir", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::PathBuf::is_file", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::PathBuf::is_symlink", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::PathBuf::metadata", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::PathBuf::read_dir", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::PathBuf::read_link", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::PathBuf::symlink_metadata", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::path::PathBuf::try_exists", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::canonicalize", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::copy", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::create_dir", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::create_dir_all", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::hard_link", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::metadata", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::read", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::read_dir", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::read_link", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::read_to_string", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::remove_dir", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::remove_dir_all", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::remove_file", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::rename", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::set_permissions", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::symlink_metadata", reason = "File system operations should be done using the sys_traits crate" },
+  { path = "std::fs::write", reason = "File system operations should be done using the sys_traits crate" }
 ]
 disallowed-types = [
   "swc_common::BytePos",

Cargo.lock

@@ -14,6 +14,7 @@ members = ["lib"]
 
 [workspace.dependencies]
 deno_unsync = { version = "0.4.0", default-features = false }
+sys_traits = "0.1.0"
 
 [lib]
 name = "deno_graph"
@@ -44,6 +45,7 @@ capacity_builder = "0.5.0"
 data-url = "0.3.0"
 deno_ast = { version = "0.44.0", features = ["dep_analysis", "emit"] }
 deno_unsync.workspace = true
+deno_path_util = "0.3.0"
 deno_semver = "0.7.1"
 encoding_rs = "0.8.33"
 futures = "0.3.26"
@@ -57,6 +59,7 @@ regex = "1.10.2"
 serde = { version = "1.0.130", features = ["derive", "rc"] }
 serde_json = { version = "1.0.67", features = ["preserve_order"] }
 sha2 = "^0.10.0"
+sys_traits.workspace = true
 thiserror = "2.0.3"
 twox-hash = { version = "1.6.3", optional = true }
 url = { version = "2.2.2", features = ["serde"] }
@@ -74,6 +77,7 @@ tempfile = "3.4.0"
 tokio = { version = "1.10.1", features = ["macros", "rt-multi-thread", "sync"] }
 deno_terminal = "0.1.1"
 env_logger = "0.11.3"
+sys_traits = { workspace = true, features = ["memory"] }
 
 [profile.release]
 codegen-units = 1

Cargo.toml

@@ -23,6 +23,7 @@ js-sys = "0.3.68"
 serde = { version = "1.0.130", features = ["derive", "rc"] }
 serde_json = { version = "1.0.67", features = ["preserve_order"] }
 serde-wasm-bindgen = "0.5.0"
+sys_traits = { workspace = true, features = ["real", "wasm"] }
 wasm-bindgen = { version = "=0.2.91" }
 wasm-bindgen-futures = { version = "=0.4.41" }
 

lib/Cargo.toml

@@ -273,6 +273,10 @@ pub async fn js_create_graph(
       BuildOptions {
         is_dynamic: false,
         resolver: maybe_resolver.as_ref().map(|r| r as &dyn Resolver),
+        // todo(dsherret): actually implement this for Wasm users
+        // and don't just use a RealSys here as it would be better
+        // to have a way for users to provide their own file system
+        // via the JS API.
         file_system: &NullFileSystem,
         jsr_url_provider: Default::default(),
         npm_resolver: None,

lib/lib.rs

@@ -73,7 +73,10 @@ use std::collections::HashMap;
 use std::collections::HashSet;
 use std::collections::VecDeque;
 use std::fmt;
+use std::path::Path;
 use std::sync::Arc;
+use sys_traits::FileType;
+use sys_traits::FsDirEntry;
 use thiserror::Error;
 use url::Url;
 use wasm::wasm_module_to_dts;
@@ -1207,7 +1210,6 @@ pub struct BuildFastCheckTypeGraphOptions<'a> {
   pub workspace_fast_check: WorkspaceFastCheckOption<'a>,
 }
 
-#[derive(Default)]
 pub struct BuildOptions<'a> {
   pub is_dynamic: bool,
   /// Additional imports that should be brought into the scope of
@@ -1217,7 +1219,7 @@ pub struct BuildOptions<'a> {
   pub imports: Vec<ReferrerImports>,
   pub executor: &'a dyn Executor,
   pub locker: Option<&'a mut dyn Locker>,
-  pub file_system: &'a dyn FileSystem,
+  pub file_system: &'a FileSystem,
   pub jsr_url_provider: &'a dyn JsrUrlProvider,
   /// Whether to pass through JSR specifiers to the resolver instead of
   /// resolving them. This is useful in cases where you want to mark JSR
@@ -1229,6 +1231,24 @@ pub struct BuildOptions<'a> {
   pub resolver: Option<&'a dyn Resolver>,
 }
 
+impl<'a> Default for BuildOptions<'a> {
+  fn default() -> Self {
+    Self {
+      is_dynamic: false,
+      imports: Default::default(),
+      executor: Default::default(),
+      locker: None,
+      file_system: &NullFileSystem,
+      jsr_url_provider: Default::default(),
+      passthrough_jsr_specifiers: false,
+      module_analyzer: Default::default(),
+      npm_resolver: None,
+      reporter: None,
+      resolver: None,
+    }
+  }
+}
+
 #[derive(Debug, Copy, Clone)]
 pub enum ModuleEntryRef<'a> {
   Module(&'a Module),
@@ -2433,7 +2453,7 @@ pub(crate) struct ParseModuleOptions {
 /// With the provided information, parse a module and return its "module slot"
 #[allow(clippy::result_large_err)]
 pub(crate) fn parse_module(
-  file_system: &dyn FileSystem,
+  file_system: &FileSystem,
   jsr_url_provider: &dyn JsrUrlProvider,
   maybe_resolver: Option<&dyn Resolver>,
   maybe_npm_resolver: Option<&dyn NpmResolver>,
@@ -2493,7 +2513,7 @@ pub(crate) fn parse_js_module_from_module_info(
   maybe_headers: Option<&HashMap<String, String>>,
   module_info: ModuleInfo,
   source: Arc<str>,
-  file_system: &dyn FileSystem,
+  file_system: &FileSystem,
   jsr_url_provider: &dyn JsrUrlProvider,
   maybe_resolver: Option<&dyn Resolver>,
   maybe_npm_resolver: Option<&dyn NpmResolver>,
@@ -2847,7 +2867,7 @@ fn parse_wasm_module_from_module_info(
   module_info: ModuleInfo,
   source: Arc<[u8]>,
   source_dts: Arc<str>,
-  file_system: &dyn FileSystem,
+  file_system: &FileSystem,
   jsr_url_provider: &dyn JsrUrlProvider,
   maybe_resolver: Option<&dyn Resolver>,
   maybe_npm_resolver: Option<&dyn NpmResolver>,
@@ -2880,7 +2900,7 @@ fn fill_module_dependencies(
   dependencies: Vec<DependencyDescriptor>,
   module_specifier: &ModuleSpecifier,
   module_dependencies: &mut IndexMap<String, Dependency>,
-  file_system: &dyn FileSystem,
+  file_system: &FileSystem,
   jsr_url_provider: &dyn JsrUrlProvider,
   maybe_resolver: Option<&dyn Resolver>,
   maybe_npm_resolver: Option<&dyn NpmResolver>,
@@ -3079,7 +3099,7 @@ fn analyze_dynamic_arg_template_parts(
   referrer: &Url,
   referrer_range: &PositionRange,
   import_attributes: &ImportAttributes,
-  file_system: &dyn FileSystem,
+  file_system: &FileSystem,
 ) -> Vec<String> {
   fn resolve_initial_dir_path(
     parts: &[DynamicTemplatePart],
@@ -3169,18 +3189,58 @@ fn analyze_dynamic_arg_template_parts(
   if is_fs_root_specifier(&dir_path) {
     return specifiers;
   }
+  let Ok(dir_path) = deno_path_util::url_to_file_path(&dir_path) else {
+    return specifiers;
+  };
   let mut pending_dirs = VecDeque::from([dir_path]);
+  let handle_err = |path: &Path, err: &std::io::Error| {
+    if matches!(
+      err.kind(),
+      std::io::ErrorKind::PermissionDenied | std::io::ErrorKind::NotFound
+    ) {
+      return;
+    }
+    // For now, errors are swallowed and not stored in the graph.
+    // If we decide to represent these in the graph, we'll need to
+    // figure out what to do with errors like directory errors.
+    // Additionally, take note that these are dynamic import errors,
+    // so they shouldn't be eagerly surfaced.
+    log::warn!(
+      "Graph failed resolving '{}'. {:#}\n    at {}:{}:{}",
+      path.display(),
+      err,
+      referrer,
+      referrer_range.start.line + 1,
+      referrer_range.start.character + 1,
+    );
+  };
   while let Some(dir_path) = pending_dirs.pop_front() {
-    let entries = file_system.read_dir(&dir_path);
+    let entries = match file_system.fs_read_dir_boxed(&dir_path) {
+      Ok(entries) => entries,
+      Err(err) => {
+        handle_err(&dir_path, &err);
+        continue;
+      }
+    };
     for entry in entries {
-      match entry.kind {
-        DirEntryKind::File => {
-          let url = &entry.url;
-          if matching_media_types.contains(&MediaType::from_specifier(url)) {
-            if url == referrer {
+      let entry = match entry {
+        Ok(entry) => entry,
+        Err(err) => {
+          handle_err(&dir_path, &err);
+          continue;
+        }
+      };
+      let path = entry.path();
+      match entry.file_type() {
+        Ok(FileType::File) => {
+          let Ok(url) = deno_path_util::url_from_file_path(&path) else {
+            continue;
+          };
+          if matching_media_types.contains(&MediaType::from_specifier(&url)) {
+            if url == *referrer {
               continue; // found itself, so skip
             }
-            if let Some(specifier) = referrer.make_relative(url) {
+            if let Some(specifier) = referrer.make_relative(&url) {
               let specifier = if !specifier.starts_with("../") {
                 format!("./{}", specifier)
               } else {
@@ -3210,38 +3270,25 @@ fn analyze_dynamic_arg_template_parts(
             }
           }
         }
-        DirEntryKind::Dir => {
+        Ok(FileType::Dir) => {
           // ignore hidden directories and any node_modules/vendor folders
-          let is_allowed_dir = entry
-            .url
-            .path()
-            .rsplit('/')
-            .find(|c| !c.is_empty())
+          let is_allowed_dir = path
+            .file_name()
             .map(|c| {
-              !c.starts_with('.') && c != "node_modules" && c != "vendor"
+              !c.to_string_lossy().starts_with('.')
+                && c != "node_modules"
+                && c != "vendor"
             })
             .unwrap_or(true);
           if is_allowed_dir {
-            pending_dirs.push_back(entry.url);
+            pending_dirs.push_back(path.into_owned());
           }
         }
-        DirEntryKind::Symlink => {
+        Ok(_) => {
           // ignore
         }
-        DirEntryKind::Error(err) => {
-          // For now, errors are swallowed and not stored in the graph.
-          // If we decide to represent these in the graph, we'll need to
-          // figure out what to do with errors like directory errors.
-          // Additionally, take note that these are dynamic import errors,
-          // so they shouldn't be eagerly surfaced.
-          log::warn!(
-            "Graph failed resolving '{}'. {:#}\n    at {}:{}:{}",
-            entry.url,
-            err,
-            referrer,
-            referrer_range.start.line + 1,
-            referrer_range.start.character + 1,
-          );
+        Err(err) => {
+          handle_err(&path, &err);
         }
       };
     }
@@ -3466,7 +3513,7 @@ impl FillPassMode {
 struct Builder<'a, 'graph> {
   in_dynamic_branch: bool,
   was_dynamic_root: bool,
-  file_system: &'a dyn FileSystem,
+  file_system: &'a FileSystem,
   jsr_url_provider: &'a dyn JsrUrlProvider,
   passthrough_jsr_specifiers: bool,
   loader: &'a dyn Loader,

src/graph.rs

@@ -115,7 +115,7 @@ pub struct ParseModuleOptions<'a> {
   pub specifier: ModuleSpecifier,
   pub maybe_headers: Option<HashMap<String, String>>,
   pub content: Arc<[u8]>,
-  pub file_system: &'a dyn FileSystem,
+  pub file_system: &'a FileSystem,
   pub jsr_url_provider: &'a dyn JsrUrlProvider,
   pub maybe_resolver: Option<&'a dyn Resolver>,
   pub maybe_npm_resolver: Option<&'a dyn NpmResolver>,
@@ -158,7 +158,7 @@ pub struct ParseModuleFromAstOptions<'a> {
   pub specifier: ModuleSpecifier,
   pub maybe_headers: Option<&'a HashMap<String, String>>,
   pub parsed_source: &'a deno_ast::ParsedSource,
-  pub file_system: &'a dyn FileSystem,
+  pub file_system: &'a FileSystem,
   pub jsr_url_provider: &'a dyn JsrUrlProvider,
   pub maybe_resolver: Option<&'a dyn Resolver>,
   pub maybe_npm_resolver: Option<&'a dyn NpmResolver>,