From 5ff2b9ca9ff6e7b0dfbb47db36e14649e82e3d96 Mon Sep 17 00:00:00 2001 From: Marek Kaput Date: Wed, 20 Sep 2023 16:12:17 +0200 Subject: [PATCH] Fix all cases when std canonicalization was used instead of dunce (#695) --- clippy.toml | 8 +++ scarb/src/core/manifest/toml_manifest.rs | 25 +++------ scarb/src/internal/fsx.rs | 7 +++ scarb/src/internal/serdex.rs | 3 +- scarb/src/lib.rs | 1 + scarb/src/manifest_editor/add.rs | 3 +- scarb/src/manifest_editor/mod.rs | 3 +- scarb/src/ops/manifest.rs | 4 +- scarb/src/ops/new.rs | 7 +-- scarb/src/ops/workspace.rs | 4 +- scarb/tests/metadata.rs | 68 ++++++------------------ scarb/tests/workspace.rs | 18 ++++--- utils/scarb-test-support/src/fsx.rs | 2 +- 13 files changed, 58 insertions(+), 95 deletions(-) create mode 100644 clippy.toml diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 000000000..2e0a5d5d8 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,8 @@ +disallowed-methods = [ + { path = "std::fs::canonicalize", reason = "use fsx::canonicalize or fsx::canonicalize_utf8" }, + { path = "std::path::Path::canonicalize", reason = "use fsx::canonicalize or fsx::canonicalize_utf8" }, + { path = "camino::Utf8Path::canonicalize", reason = "use fsx::canonicalize or fsx::canonicalize_utf8" }, + { path = "camino::Utf8Path::canonicalize_utf8", reason = "use fsx::canonicalize or fsx::canonicalize_utf8" }, + { path = "camino::Utf8PathBuf::canonicalize", reason = "use fsx::canonicalize or fsx::canonicalize_utf8" }, + { path = "camino::Utf8PathBuf::canonicalize_utf8", reason = "use fsx::canonicalize or fsx::canonicalize_utf8" }, +] diff --git a/scarb/src/core/manifest/toml_manifest.rs b/scarb/src/core/manifest/toml_manifest.rs index e9cbc8b17..43ff29c05 100644 --- a/scarb/src/core/manifest/toml_manifest.rs +++ b/scarb/src/core/manifest/toml_manifest.rs @@ -22,7 +22,6 @@ use crate::core::package::PackageId; use crate::core::source::{GitReference, SourceId}; use crate::core::{DependencyVersionReq, ManifestBuilder, ManifestCompilerConfig, PackageName}; use crate::internal::fsx; -use crate::internal::fsx::PathBufUtf8Ext; use crate::internal::serdex::{toml_merge, RelativeUtf8PathBuf}; use crate::internal::to_version::ToVersion; use crate::{DEFAULT_SOURCE_PATH, MANIFEST_FILE_NAME}; @@ -621,10 +620,8 @@ impl TomlManifest { let source_path = target .source_path .clone() - .map(|p| root.as_std_path().to_path_buf().join(p)) - .map(fsx::canonicalize) - .transpose()? - .map(|p| p.try_into_utf8()) + .map(|p| root.join_os(p)) + .map(fsx::canonicalize_utf8) .transpose()? .unwrap_or(default_source_path.to_path_buf()); @@ -767,18 +764,12 @@ pub fn readme_for_package( fn abs_canonical_path(prefix: &Utf8Path, readme: Option<&Utf8Path>) -> Result> { match readme { None => Ok(None), - Some(readme) => prefix - .parent() - .unwrap() - .join(readme) - .canonicalize_utf8() - .map(Some) - .with_context(|| { - format!( - "failed to find the readme at {}", - prefix.parent().unwrap().join(readme) - ) - }), + Some(readme) => { + let path = prefix.parent().unwrap().join(readme); + let path = fsx::canonicalize_utf8(&path) + .with_context(|| format!("failed to find the readme at {path}"))?; + Ok(Some(path)) + } } } diff --git a/scarb/src/internal/fsx.rs b/scarb/src/internal/fsx.rs index 4e00d3592..456304f03 100644 --- a/scarb/src/internal/fsx.rs +++ b/scarb/src/internal/fsx.rs @@ -8,6 +8,8 @@ use anyhow::{anyhow, Context, Result}; use camino::{Utf8Path, Utf8PathBuf}; /// Equivalent to [`fs::canonicalize`] with better error messages. +/// +/// Uses [`dunce`] to generate more familiar paths on Windows. pub fn canonicalize(p: impl AsRef) -> Result { return inner(p.as_ref()); @@ -17,6 +19,11 @@ pub fn canonicalize(p: impl AsRef) -> Result { } } +/// Equivalent to [`fs::canonicalize`], but for Utf-8 paths, with better error messages. +pub fn canonicalize_utf8(p: impl AsRef) -> Result { + canonicalize(p)?.try_into_utf8() +} + /// Equivalent to [`fs::create_dir_all`] with better error messages. pub fn create_dir_all(p: impl AsRef) -> Result<()> { return inner(p.as_ref()); diff --git a/scarb/src/internal/serdex.rs b/scarb/src/internal/serdex.rs index 521d78ba6..95b127bc1 100644 --- a/scarb/src/internal/serdex.rs +++ b/scarb/src/internal/serdex.rs @@ -3,7 +3,6 @@ use camino::{Utf8Path, Utf8PathBuf}; use serde::{Deserialize, Serialize}; use crate::internal::fsx; -use crate::internal::fsx::PathUtf8Ext; /// Merge two `toml::Value` serializable structs. pub fn toml_merge<'de, T, S>(target: &T, source: &S) -> anyhow::Result @@ -32,7 +31,7 @@ pub struct RelativeUtf8PathBuf(Utf8PathBuf); impl RelativeUtf8PathBuf { pub fn relative_to_directory(&self, root: &Utf8Path) -> Result { - fsx::canonicalize(root.join(&self.0))?.try_to_utf8() + fsx::canonicalize_utf8(root.join(&self.0)) } pub fn relative_to_file(&self, file: &Utf8Path) -> Result { diff --git a/scarb/src/lib.rs b/scarb/src/lib.rs index c7948c069..eb44b1d36 100644 --- a/scarb/src/lib.rs +++ b/scarb/src/lib.rs @@ -2,6 +2,7 @@ //! //! [cairo]: https://cairo-lang.org/ +#![deny(clippy::disallowed_methods)] #![deny(rustdoc::broken_intra_doc_links)] #![deny(rustdoc::private_intra_doc_links)] #![warn(rust_2018_idioms)] diff --git a/scarb/src/manifest_editor/add.rs b/scarb/src/manifest_editor/add.rs index 85058bc97..cf0906364 100644 --- a/scarb/src/manifest_editor/add.rs +++ b/scarb/src/manifest_editor/add.rs @@ -7,7 +7,6 @@ use url::Url; use crate::core::{GitReference, PackageName}; use crate::internal::fsx; -use crate::internal::fsx::PathBufUtf8Ext; use crate::sources::canonical_url::CanonicalUrl; use super::tomlx::get_table_mut; @@ -115,7 +114,7 @@ impl Dep { }); let source: Box = if let Some(path) = op.path { - let path = fsx::canonicalize(path)?.try_into_utf8()?; + let path = fsx::canonicalize_utf8(path)?; let path = path_value(ctx.manifest_path, &path); Box::new(PathSource { version, path }) diff --git a/scarb/src/manifest_editor/mod.rs b/scarb/src/manifest_editor/mod.rs index 89de8d8d3..4dcf273e2 100644 --- a/scarb/src/manifest_editor/mod.rs +++ b/scarb/src/manifest_editor/mod.rs @@ -12,7 +12,6 @@ pub use remove::RemoveDependency; use crate::core::Config; use crate::internal::fsx; -use crate::internal::fsx::PathBufUtf8Ext; mod add; mod dep_id; @@ -40,7 +39,7 @@ pub fn edit( ops: Vec>, opts: EditManifestOptions<'_>, ) -> Result<()> { - let manifest_path = fsx::canonicalize(manifest_path)?.try_into_utf8()?; + let manifest_path = fsx::canonicalize_utf8(manifest_path)?; let original_raw_manifest = fsx::read_to_string(&manifest_path)?; let mut doc = Document::from_str(&original_raw_manifest) diff --git a/scarb/src/ops/manifest.rs b/scarb/src/ops/manifest.rs index 1390d1f36..9924e44dc 100644 --- a/scarb/src/ops/manifest.rs +++ b/scarb/src/ops/manifest.rs @@ -5,7 +5,7 @@ use camino::{Utf8Path, Utf8PathBuf}; use crate::core::manifest::TomlManifest; use crate::internal::fsx; -use crate::internal::fsx::{PathBufUtf8Ext, PathUtf8Ext}; +use crate::internal::fsx::PathBufUtf8Ext; use crate::MANIFEST_FILE_NAME; #[tracing::instrument(level = "debug")] @@ -15,7 +15,7 @@ pub fn find_manifest_path(user_override: Option<&Utf8Path>) -> Result { - let pwd = fsx::canonicalize(env::current_dir()?)?.try_to_utf8()?; + let pwd = fsx::canonicalize_utf8(env::current_dir()?)?; let accept_all = |_| Ok(true); let manifest_path = try_find_manifest_of_pwd(pwd.clone(), accept_all)? .unwrap_or_else(|| pwd.join(MANIFEST_FILE_NAME)); diff --git a/scarb/src/ops/new.rs b/scarb/src/ops/new.rs index 741baf8ab..7fea49cea 100644 --- a/scarb/src/ops/new.rs +++ b/scarb/src/ops/new.rs @@ -5,7 +5,6 @@ use itertools::Itertools; use crate::core::{Config, PackageName}; use crate::internal::fsx; -use crate::internal::fsx::PathBufUtf8Ext; use crate::internal::restricted_names; use crate::{ops, DEFAULT_SOURCE_PATH, DEFAULT_TARGET_DIR_NAME, MANIFEST_FILE_NAME}; @@ -126,11 +125,7 @@ fn mk( // Create project directory in case we are called from `new` op. fsx::create_dir_all(&path)?; - let canonical_path = if let Ok(canonicalize_path) = fsx::canonicalize(&path) { - canonicalize_path.try_into_utf8()? - } else { - path - }; + let canonical_path = fsx::canonicalize_utf8(&path).unwrap_or(path); init_vcs(&canonical_path, version_control)?; write_vcs_ignore(&canonical_path, config, version_control)?; diff --git a/scarb/src/ops/workspace.rs b/scarb/src/ops/workspace.rs index d414c6687..9d20c53c5 100644 --- a/scarb/src/ops/workspace.rs +++ b/scarb/src/ops/workspace.rs @@ -165,9 +165,7 @@ fn find_member_paths( // Look for manifest file, continuing if it does not exist. let path = path.join(MANIFEST_FILE_NAME); if path.is_file() { - let path = fsx::canonicalize(path)?; - let path = path.try_into_utf8()?; - + let path = fsx::canonicalize_utf8(path)?; paths.push(path) } else { config.ui().warn(format!( diff --git a/scarb/tests/metadata.rs b/scarb/tests/metadata.rs index 4594e5130..ec4c23b23 100644 --- a/scarb/tests/metadata.rs +++ b/scarb/tests/metadata.rs @@ -2,12 +2,13 @@ use std::collections::BTreeMap; use assert_fs::prelude::*; use indoc::indoc; +use serde_json::json; + use scarb_metadata::{Cfg, ManifestMetadataBuilder, Metadata, PackageMetadata}; use scarb_test_support::command::{CommandExt, Scarb}; -use scarb_test_support::fsx::PathBufUtf8Ext; +use scarb_test_support::fsx; use scarb_test_support::project_builder::ProjectBuilder; use scarb_test_support::workspace_builder::WorkspaceBuilder; -use serde_json::json; fn packages_by_name(meta: Metadata) -> BTreeMap { meta.packages @@ -285,10 +286,7 @@ fn manifest_targets_and_metadata() { .license(Some("MIT License".to_string())) .license_file(Some("./license.md".to_string())) .readme( - t.join("README.md") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.join("README.md")) .unwrap() .into_string() ) @@ -617,10 +615,7 @@ fn infer_readme_simple() { .manifest_metadata .readme, Some( - t.join("README") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.join("README")) .unwrap() .into_string() ) @@ -643,10 +638,7 @@ fn infer_readme_simple() { .manifest_metadata .readme, Some( - t.join("README.txt") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.join("README.txt")) .unwrap() .into_string() ) @@ -669,10 +661,7 @@ fn infer_readme_simple() { .manifest_metadata .readme, Some( - t.join("README.md") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.join("README.md")) .unwrap() .into_string() ) @@ -711,10 +700,7 @@ fn infer_readme_simple() { .manifest_metadata .readme, Some( - t.join("a/b/c/MEREAD.md") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.join("a/b/c/MEREAD.md")) .unwrap() .into_string() ) @@ -797,10 +783,7 @@ fn infer_readme_simple_bool() { .manifest_metadata .readme, Some( - t.join("README.md") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.join("README.md")) .unwrap() .into_string() ) @@ -986,10 +969,7 @@ fn infer_readme_workspace() { assert_eq!( packages.get("hello").unwrap().manifest_metadata.readme, Some( - t.join("MEREAD.md") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.join("MEREAD.md")) .unwrap() .into_string() ) @@ -997,10 +977,7 @@ fn infer_readme_workspace() { assert_eq!( packages.get("t7").unwrap().manifest_metadata.readme, Some( - t.join("MEREAD.md") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.join("MEREAD.md")) .unwrap() .into_string() ) @@ -1008,10 +985,7 @@ fn infer_readme_workspace() { assert_eq!( packages.get("t1").unwrap().manifest_metadata.readme, Some( - t.join("MEREAD.md") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.join("MEREAD.md")) .unwrap() .into_string() ) @@ -1019,11 +993,7 @@ fn infer_readme_workspace() { assert_eq!( packages.get("t2").unwrap().manifest_metadata.readme, Some( - t.child("t2") - .join("README.md") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.child("t2").join("README.md")) .unwrap() .into_string() ) @@ -1031,11 +1001,7 @@ fn infer_readme_workspace() { assert_eq!( packages.get("t3").unwrap().manifest_metadata.readme, Some( - t.child("t3") - .join("README.txt") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.child("t3").join("README.txt")) .unwrap() .into_string() ) @@ -1043,11 +1009,7 @@ fn infer_readme_workspace() { assert_eq!( packages.get("t4").unwrap().manifest_metadata.readme, Some( - t.child("t4") - .join("TEST.txt") - .canonicalize() - .unwrap() - .try_into_utf8() + fsx::canonicalize_utf8(t.child("t4").join("TEST.txt")) .unwrap() .into_string() ) diff --git a/scarb/tests/workspace.rs b/scarb/tests/workspace.rs index 11d1febf4..b5060c20e 100644 --- a/scarb/tests/workspace.rs +++ b/scarb/tests/workspace.rs @@ -1,9 +1,10 @@ use assert_fs::fixture::{PathChild, PathCreateDir}; use assert_fs::TempDir; use indoc::indoc; -use scarb_metadata::Metadata; +use scarb_metadata::Metadata; use scarb_test_support::command::{CommandExt, Scarb}; +use scarb_test_support::fsx; use scarb_test_support::project_builder::ProjectBuilder; use scarb_test_support::workspace_builder::WorkspaceBuilder; @@ -79,11 +80,14 @@ fn unify_target_dir() { assert_eq!(root_metadata.target_dir, pkg_metadata.target_dir); assert_eq!( - root_metadata - .target_dir - .unwrap() - .to_owned() - .into_std_path_buf(), - t.child("target").canonicalize().unwrap() + fsx::canonicalize( + root_metadata + .target_dir + .unwrap() + .to_owned() + .into_std_path_buf() + ) + .unwrap(), + fsx::canonicalize(t.child("target")).unwrap() ); } diff --git a/utils/scarb-test-support/src/fsx.rs b/utils/scarb-test-support/src/fsx.rs index 8b1b0b6d0..feee93470 100644 --- a/utils/scarb-test-support/src/fsx.rs +++ b/utils/scarb-test-support/src/fsx.rs @@ -7,7 +7,7 @@ use camino::Utf8Path; use itertools::Itertools; use serde::de::DeserializeOwned; -pub use internal_fsx::{PathBufUtf8Ext, PathUtf8Ext}; +pub use internal_fsx::{canonicalize, canonicalize_utf8, PathBufUtf8Ext, PathUtf8Ext}; #[allow(unused)] #[path = "../../../scarb/src/internal/fsx.rs"]