From 913e2875c1c31d5ffbc9c0c9ed0e8c63f6143024 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Thu, 30 Mar 2023 03:20:31 +0200 Subject: refactor(ext/node): add NodeEnv::Fs associated type (#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. --- ext/node/clippy.toml | 21 +++++++++++ ext/node/lib.rs | 31 +++++++++++++---- ext/node/ops.rs | 25 +++++++------- ext/node/package_json.rs | 9 ++--- ext/node/resolution.rs | 90 ++++++++++++++++++++++++++---------------------- 5 files changed, 112 insertions(+), 64 deletions(-) create mode 100644 ext/node/clippy.toml (limited to 'ext') diff --git a/ext/node/clippy.toml b/ext/node/clippy.toml new file mode 100644 index 000000000..3ce5624b3 --- /dev/null +++ b/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" }, +] diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 1693a82ef..f3bdb7e5b 100644 --- a/ext/node/lib.rs +++ b/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; -// } +pub trait NodeFs { + fn current_dir() -> io::Result; + fn metadata>(path: P) -> io::Result; + fn read_to_string>(path: P) -> io::Result; +} + +pub struct RealFs; +impl NodeFs for RealFs { + fn current_dir() -> io::Result { + #[allow(clippy::disallowed_methods)] + std::env::current_dir() + } + + fn metadata>(path: P) -> io::Result { + #[allow(clippy::disallowed_methods)] + std::fs::metadata(path) + } + + fn read_to_string>(path: P) -> io::Result { + #[allow(clippy::disallowed_methods)] + std::fs::read_to_string(path) + } +} pub trait RequireNpmResolver { fn resolve_package_folder_from_package( diff --git a/ext/node/ops.rs b/ext/node/ops.rs index 3dbab33c1..5ecf70f3e 100644 --- a/ext/node/ops.rs +++ b/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::(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 == "" || parent_id == "internal/preload" { - if let Ok(cwd) = std::env::current_dir() { + if let Ok(cwd) = Env::Fs::current_dir() { ensure_read_permission::(state, &cwd)?; return Ok(Some(cwd.to_string_lossy().to_string())); } @@ -376,7 +377,7 @@ where let resolver = state.borrow::>().clone(); let permissions = state.borrow_mut::(); - let pkg = resolution::get_package_scope_config( + let pkg = resolution::get_package_scope_config::( &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::( &pkg.path, expansion, exports, @@ -434,7 +435,7 @@ where { let file_path = PathBuf::from(file_path); ensure_read_permission::(state, &file_path)?; - Ok(std::fs::read_to_string(file_path)?) + Ok(Env::Fs::read_to_string(file_path)?) } #[op] @@ -471,7 +472,7 @@ where } else { path_resolve(vec![modules_path, name]) }; - let pkg = PackageJson::load( + let pkg = PackageJson::load::( &*resolver, permissions, PathBuf::from(&pkg_path).join("package.json"), @@ -479,7 +480,7 @@ where if let Some(exports) = &pkg.exports { let referrer = Url::from_file_path(parent_path).unwrap(); - resolution::package_exports_resolve( + resolution::package_exports_resolve::( &pkg.path, format!(".{expansion}"), exports, @@ -510,7 +511,7 @@ where )?; let resolver = state.borrow::>().clone(); let permissions = state.borrow_mut::(); - resolution::get_closest_package_json( + resolution::get_closest_package_json::( &Url::from_file_path(filename).unwrap(), &*resolver, permissions, @@ -528,7 +529,7 @@ where let resolver = state.borrow::>().clone(); let permissions = state.borrow_mut::(); let package_json_path = PathBuf::from(package_json_path); - PackageJson::load(&*resolver, permissions, package_json_path).ok() + PackageJson::load::(&*resolver, permissions, package_json_path).ok() } #[op] @@ -544,7 +545,7 @@ where ensure_read_permission::(state, &parent_path)?; let resolver = state.borrow::>().clone(); let permissions = state.borrow_mut::(); - let pkg = PackageJson::load( + let pkg = PackageJson::load::( &*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::( &request, &referrer, NodeModuleKind::Cjs, diff --git a/ext/node/package_json.rs b/ext/node/package_json.rs index b0816dd85..60f50ad78 100644 --- a/ext/node/package_json.rs +++ b/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( resolver: &dyn RequireNpmResolver, permissions: &mut dyn NodePermissions, path: PathBuf, ) -> Result { resolver.ensure_read_permission(permissions, &path)?; - Self::load_skip_read_permission(path) + Self::load_skip_read_permission::(path) } - pub fn load_skip_read_permission( + pub fn load_skip_read_permission( path: PathBuf, ) -> Result { 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)); diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs index 950485a1d..16b1efba6 100644 --- a/ext/node/resolution.rs +++ b/ext/node/resolution.rs @@ -15,6 +15,7 @@ use regex::Regex; use crate::errors; use crate::package_json::PackageJson; use crate::path::PathClean; +use crate::NodeFs; use crate::NodePermissions; use crate::RequireNpmResolver; @@ -40,11 +41,11 @@ impl NodeResolutionMode { } /// Checks if the resolved file has a corresponding declaration file. -pub fn path_to_declaration_path( +pub fn path_to_declaration_path( path: PathBuf, referrer_kind: NodeModuleKind, ) -> Option { - fn probe_extensions( + fn probe_extensions( path: &Path, referrer_kind: NodeModuleKind, ) -> Option { @@ -52,11 +53,11 @@ pub fn path_to_declaration_path( NodeModuleKind::Cjs => with_known_extension(path, "d.cts"), NodeModuleKind::Esm => with_known_extension(path, "d.mts"), }; - if specific_dts_path.exists() { + if Fs::metadata(&specific_dts_path).is_ok() { return Some(specific_dts_path); } let dts_path = with_known_extension(path, "d.ts"); - if dts_path.exists() { + if Fs::metadata(&dts_path).is_ok() { Some(dts_path) } else { None @@ -70,11 +71,13 @@ pub fn path_to_declaration_path( { return Some(path); } - if let Some(path) = probe_extensions(&path, referrer_kind) { + if let Some(path) = probe_extensions::(&path, referrer_kind) { return Some(path); } if path.is_dir() { - if let Some(path) = probe_extensions(&path.join("index"), referrer_kind) { + if let Some(path) = + probe_extensions::(&path.join("index"), referrer_kind) + { return Some(path); } } @@ -182,7 +185,7 @@ fn pattern_key_compare(a: &str, b: &str) -> i32 { 0 } -pub fn package_imports_resolve( +pub fn package_imports_resolve( name: &str, referrer: &ModuleSpecifier, referrer_kind: NodeModuleKind, @@ -201,13 +204,13 @@ pub fn package_imports_resolve( } let package_config = - get_package_scope_config(referrer, npm_resolver, permissions)?; + get_package_scope_config::(referrer, npm_resolver, permissions)?; let mut package_json_path = None; if package_config.exists { package_json_path = Some(package_config.path.clone()); if let Some(imports) = &package_config.imports { if imports.contains_key(name) && !name.contains('*') { - let maybe_resolved = resolve_package_target( + let maybe_resolved = resolve_package_target::( package_json_path.as_ref().unwrap(), imports.get(name).unwrap().to_owned(), "".to_string(), @@ -250,7 +253,7 @@ pub fn package_imports_resolve( if !best_match.is_empty() { let target = imports.get(best_match).unwrap().to_owned(); - let maybe_resolved = resolve_package_target( + let maybe_resolved = resolve_package_target::( package_json_path.as_ref().unwrap(), target, best_match_subpath.unwrap(), @@ -315,7 +318,7 @@ fn throw_invalid_subpath( } #[allow(clippy::too_many_arguments)] -fn resolve_package_target_string( +fn resolve_package_target_string( target: String, subpath: String, match_: String, @@ -354,7 +357,7 @@ fn resolve_package_target_string( }; let package_json_url = ModuleSpecifier::from_file_path(package_json_path).unwrap(); - return match package_resolve( + return match package_resolve::( &export_target, &package_json_url, referrer_kind, @@ -425,7 +428,7 @@ fn resolve_package_target_string( } #[allow(clippy::too_many_arguments)] -fn resolve_package_target( +fn resolve_package_target( package_json_path: &Path, target: Value, subpath: String, @@ -440,7 +443,7 @@ fn resolve_package_target( permissions: &mut dyn NodePermissions, ) -> Result, AnyError> { if let Some(target) = target.as_str() { - return resolve_package_target_string( + return resolve_package_target_string::( target.to_string(), subpath, package_subpath, @@ -456,7 +459,7 @@ fn resolve_package_target( ) .map(|path| { if mode.is_types() { - path_to_declaration_path(path, referrer_kind) + path_to_declaration_path::(path, referrer_kind) } else { Some(path) } @@ -468,7 +471,7 @@ fn resolve_package_target( let mut last_error = None; for target_item in target_arr { - let resolved_result = resolve_package_target( + let resolved_result = resolve_package_target::( package_json_path, target_item.to_owned(), subpath.clone(), @@ -518,7 +521,7 @@ fn resolve_package_target( { let condition_target = target_obj.get(key).unwrap().to_owned(); - let resolved = resolve_package_target( + let resolved = resolve_package_target::( package_json_path, condition_target, subpath.clone(), @@ -566,7 +569,7 @@ fn throw_exports_not_found( } #[allow(clippy::too_many_arguments)] -pub fn package_exports_resolve( +pub fn package_exports_resolve( package_json_path: &Path, package_subpath: String, package_exports: &Map, @@ -582,7 +585,7 @@ pub fn package_exports_resolve( && !package_subpath.ends_with('/') { let target = package_exports.get(&package_subpath).unwrap().to_owned(); - let resolved = resolve_package_target( + let resolved = resolve_package_target::( package_json_path, target, "".to_string(), @@ -642,7 +645,7 @@ pub fn package_exports_resolve( if !best_match.is_empty() { let target = package_exports.get(best_match).unwrap().to_owned(); - let maybe_resolved = resolve_package_target( + let maybe_resolved = resolve_package_target::( package_json_path, target, best_match_subpath.unwrap(), @@ -725,7 +728,7 @@ fn parse_package_name( Ok((package_name, package_subpath, is_scoped)) } -pub fn package_resolve( +pub fn package_resolve( specifier: &str, referrer: &ModuleSpecifier, referrer_kind: NodeModuleKind, @@ -739,12 +742,12 @@ pub fn package_resolve( // ResolveSelf let package_config = - get_package_scope_config(referrer, npm_resolver, permissions)?; + get_package_scope_config::(referrer, npm_resolver, permissions)?; if package_config.exists && package_config.name.as_ref() == Some(&package_name) { if let Some(exports) = &package_config.exports { - return package_exports_resolve( + return package_exports_resolve::( &package_config.path, package_subpath, exports, @@ -781,9 +784,9 @@ pub fn package_resolve( // Package match. let package_json = - PackageJson::load(npm_resolver, permissions, package_json_path)?; + PackageJson::load::(npm_resolver, permissions, package_json_path)?; if let Some(exports) = &package_json.exports { - return package_exports_resolve( + return package_exports_resolve::( &package_json.path, package_subpath, exports, @@ -797,21 +800,21 @@ pub fn package_resolve( .map(Some); } if package_subpath == "." { - return legacy_main_resolve(&package_json, referrer_kind, mode); + return legacy_main_resolve::(&package_json, referrer_kind, mode); } let file_path = package_json.path.parent().unwrap().join(&package_subpath); if mode.is_types() { let maybe_declaration_path = - path_to_declaration_path(file_path, referrer_kind); + path_to_declaration_path::(file_path, referrer_kind); Ok(maybe_declaration_path) } else { Ok(Some(file_path)) } } -pub fn get_package_scope_config( +pub fn get_package_scope_config( referrer: &ModuleSpecifier, npm_resolver: &dyn RequireNpmResolver, permissions: &mut dyn NodePermissions, @@ -819,26 +822,27 @@ pub fn get_package_scope_config( let root_folder = npm_resolver .resolve_package_folder_from_path(&referrer.to_file_path().unwrap())?; let package_json_path = root_folder.join("package.json"); - PackageJson::load(npm_resolver, permissions, package_json_path) + PackageJson::load::(npm_resolver, permissions, package_json_path) } -pub fn get_closest_package_json( +pub fn get_closest_package_json( url: &ModuleSpecifier, npm_resolver: &dyn RequireNpmResolver, permissions: &mut dyn NodePermissions, ) -> Result { - let package_json_path = get_closest_package_json_path(url, npm_resolver)?; - PackageJson::load(npm_resolver, permissions, package_json_path) + let package_json_path = + get_closest_package_json_path::(url, npm_resolver)?; + PackageJson::load::(npm_resolver, permissions, package_json_path) } -fn get_closest_package_json_path( +fn get_closest_package_json_path( url: &ModuleSpecifier, npm_resolver: &dyn RequireNpmResolver, ) -> Result { let file_path = url.to_file_path().unwrap(); let mut current_dir = file_path.parent().unwrap(); let package_json_path = current_dir.join("package.json"); - if package_json_path.exists() { + if Fs::metadata(&package_json_path).is_ok() { return Ok(package_json_path); } let root_pkg_folder = npm_resolver @@ -846,7 +850,7 @@ fn get_closest_package_json_path( while current_dir.starts_with(&root_pkg_folder) { current_dir = current_dir.parent().unwrap(); let package_json_path = current_dir.join("package.json"); - if package_json_path.exists() { + if Fs::metadata(&package_json_path).is_ok() { return Ok(package_json_path); } } @@ -854,15 +858,15 @@ fn get_closest_package_json_path( bail!("did not find package.json in {}", root_pkg_folder.display()) } -fn file_exists(path: &Path) -> bool { - if let Ok(stats) = std::fs::metadata(path) { +fn file_exists(path: &Path) -> bool { + if let Ok(stats) = Fs::metadata(path) { stats.is_file() } else { false } } -pub fn legacy_main_resolve( +pub fn legacy_main_resolve( package_json: &PackageJson, referrer_kind: NodeModuleKind, mode: NodeResolutionMode, @@ -875,7 +879,9 @@ pub fn legacy_main_resolve( // a corresponding declaration file if let Some(main) = package_json.main(referrer_kind) { let main = package_json.path.parent().unwrap().join(main).clean(); - if let Some(path) = path_to_declaration_path(main, referrer_kind) { + if let Some(path) = + path_to_declaration_path::(main, referrer_kind) + { return Ok(Some(path)); } } @@ -888,7 +894,7 @@ pub fn legacy_main_resolve( if let Some(main) = maybe_main { let guess = package_json.path.parent().unwrap().join(main).clean(); - if file_exists(&guess) { + if file_exists::(&guess) { return Ok(Some(guess)); } @@ -917,7 +923,7 @@ pub fn legacy_main_resolve( .unwrap() .join(format!("{main}{ending}")) .clean(); - if file_exists(&guess) { + if file_exists::(&guess) { // TODO(bartlomieju): emitLegacyIndexDeprecation() return Ok(Some(guess)); } @@ -940,7 +946,7 @@ pub fn legacy_main_resolve( .unwrap() .join(index_file_name) .clean(); - if file_exists(&guess) { + if file_exists::(&guess) { // TODO(bartlomieju): emitLegacyIndexDeprecation() return Ok(Some(guess)); } -- cgit v1.2.3