From 339165bd9565806374fa842dfc217dcc5ebabac5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Thu, 6 Apr 2023 15:08:14 +0200 Subject: [PATCH] refactor(ext/node): add more methods to 'NodeFs' trait (#18604) Added more methods to `ext/node/clippy.toml` that are not allowed to be used in the crate. Prerequisite for https://github.com/denoland/deno/pull/18544 --- ext/node/clippy.toml | 19 +++++++++++++++++++ ext/node/lib.rs | 24 ++++++++++++++++++++++-- ext/node/ops.rs | 6 +++--- ext/node/resolution.rs | 24 ++++++++---------------- 4 files changed, 52 insertions(+), 21 deletions(-) diff --git a/ext/node/clippy.toml b/ext/node/clippy.toml index 3ce5624b35..94796f5a70 100644 --- a/ext/node/clippy.toml +++ b/ext/node/clippy.toml @@ -1,6 +1,25 @@ 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" }, diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 12d7b0b1e2..04fd07cab6 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -52,8 +52,11 @@ pub trait NodePermissions { pub trait NodeFs { fn current_dir() -> io::Result; - fn metadata>(path: P) -> io::Result; + fn is_file>(path: P) -> bool; + fn is_dir>(path: P) -> bool; + fn exists>(path: P) -> bool; fn read_to_string>(path: P) -> io::Result; + fn canonicalize>(path: P) -> io::Result; } pub struct RealFs; @@ -63,15 +66,32 @@ impl NodeFs for RealFs { std::env::current_dir() } - fn metadata>(path: P) -> io::Result { + fn exists>(path: P) -> bool { + #[allow(clippy::disallowed_methods)] + std::fs::metadata(path).is_ok() + } + + fn is_file>(path: P) -> bool { #[allow(clippy::disallowed_methods)] std::fs::metadata(path) + .map(|m| m.is_file()) + .unwrap_or(false) + } + + fn is_dir>(path: P) -> bool { + #[allow(clippy::disallowed_methods)] + std::fs::metadata(path).map(|m| m.is_dir()).unwrap_or(false) } fn read_to_string>(path: P) -> io::Result { #[allow(clippy::disallowed_methods)] std::fs::read_to_string(path) } + + fn canonicalize>(path: P) -> io::Result { + #[allow(clippy::disallowed_methods)] + std::path::Path::canonicalize(path.as_ref()) + } } pub trait RequireNpmResolver { diff --git a/ext/node/ops.rs b/ext/node/ops.rs index 5ecf70f3e9..456c0dd51e 100644 --- a/ext/node/ops.rs +++ b/ext/node/ops.rs @@ -264,8 +264,8 @@ where { let path = PathBuf::from(path); ensure_read_permission::(state, &path)?; - if let Ok(metadata) = Env::Fs::metadata(&path) { - if metadata.is_file() { + if Env::Fs::exists(&path) { + if Env::Fs::is_file(&path) { return Ok(0); } else { return Ok(1); @@ -285,7 +285,7 @@ where { let path = PathBuf::from(request); ensure_read_permission::(state, &path)?; - let mut canonicalized_path = path.canonicalize()?; + let mut canonicalized_path = Env::Fs::canonicalize(&path)?; if cfg!(windows) { canonicalized_path = PathBuf::from( canonicalized_path diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs index 16b1efba67..3f9c5da264 100644 --- a/ext/node/resolution.rs +++ b/ext/node/resolution.rs @@ -53,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 Fs::metadata(&specific_dts_path).is_ok() { + if Fs::exists(&specific_dts_path) { return Some(specific_dts_path); } let dts_path = with_known_extension(path, "d.ts"); - if Fs::metadata(&dts_path).is_ok() { + if Fs::exists(&dts_path) { Some(dts_path) } else { None @@ -74,7 +74,7 @@ pub fn path_to_declaration_path( if let Some(path) = probe_extensions::(&path, referrer_kind) { return Some(path); } - if path.is_dir() { + if Fs::is_dir(&path) { if let Some(path) = probe_extensions::(&path.join("index"), referrer_kind) { @@ -842,7 +842,7 @@ fn get_closest_package_json_path( 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 Fs::metadata(&package_json_path).is_ok() { + if Fs::exists(&package_json_path) { return Ok(package_json_path); } let root_pkg_folder = npm_resolver @@ -850,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 Fs::metadata(&package_json_path).is_ok() { + if Fs::exists(&package_json_path) { return Ok(package_json_path); } } @@ -858,14 +858,6 @@ 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) = Fs::metadata(path) { - stats.is_file() - } else { - false - } -} - pub fn legacy_main_resolve( package_json: &PackageJson, referrer_kind: NodeModuleKind, @@ -894,7 +886,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 Fs::is_file(&guess) { return Ok(Some(guess)); } @@ -923,7 +915,7 @@ pub fn legacy_main_resolve( .unwrap() .join(format!("{main}{ending}")) .clean(); - if file_exists::(&guess) { + if Fs::is_file(&guess) { // TODO(bartlomieju): emitLegacyIndexDeprecation() return Ok(Some(guess)); } @@ -946,7 +938,7 @@ pub fn legacy_main_resolve( .unwrap() .join(index_file_name) .clean(); - if file_exists::(&guess) { + if Fs::is_file(&guess) { // TODO(bartlomieju): emitLegacyIndexDeprecation() return Ok(Some(guess)); }