From da6cf9cde292171cf2663738d89db39c39869783 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 19 Dec 2023 03:34:35 -0500 Subject: [PATCH 01/10] refactor(util-schemas): add `thiserror` --- Cargo.lock | 1 + crates/cargo-util-schemas/Cargo.toml | 1 + 2 files changed, 2 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 045c01588ef..b92ba687f70 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -444,6 +444,7 @@ dependencies = [ "serde", "serde-untagged", "serde-value", + "thiserror", "toml", "unicode-xid", "url", diff --git a/crates/cargo-util-schemas/Cargo.toml b/crates/cargo-util-schemas/Cargo.toml index 32a13c26394..3676bffb5b2 100644 --- a/crates/cargo-util-schemas/Cargo.toml +++ b/crates/cargo-util-schemas/Cargo.toml @@ -14,6 +14,7 @@ semver.workspace = true serde = { workspace = true, features = ["derive"] } serde-untagged.workspace = true serde-value.workspace = true +thiserror.workspace = true toml.workspace = true unicode-xid.workspace = true url.workspace = true From d7b811b686cec5e74a83f84eaf6cf533915b853e Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 19 Dec 2023 05:42:18 -0500 Subject: [PATCH 02/10] refactor(util-schemas): error type for `PartialVersion` --- crates/cargo-util-schemas/src/core/mod.rs | 1 + .../src/core/partial_version.rs | 31 +++++++++++++------ 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/crates/cargo-util-schemas/src/core/mod.rs b/crates/cargo-util-schemas/src/core/mod.rs index aad6d1488f3..e0cd0953894 100644 --- a/crates/cargo-util-schemas/src/core/mod.rs +++ b/crates/cargo-util-schemas/src/core/mod.rs @@ -4,5 +4,6 @@ mod source_kind; pub use package_id_spec::PackageIdSpec; pub use partial_version::PartialVersion; +pub use partial_version::PartialVersionError; pub use source_kind::GitReference; pub use source_kind::SourceKind; diff --git a/crates/cargo-util-schemas/src/core/partial_version.rs b/crates/cargo-util-schemas/src/core/partial_version.rs index 9670bd87764..a7a96e386c5 100644 --- a/crates/cargo-util-schemas/src/core/partial_version.rs +++ b/crates/cargo-util-schemas/src/core/partial_version.rs @@ -80,11 +80,11 @@ impl From for PartialVersion { } impl std::str::FromStr for PartialVersion { - type Err = anyhow::Error; + type Err = PartialVersionError; fn from_str(value: &str) -> Result { if is_req(value) { - anyhow::bail!("unexpected version requirement, expected a version like \"1.32\"") + return Err(PartialVersionError::VersionReq); } match semver::Version::parse(value) { Ok(ver) => Ok(ver.into()), @@ -92,15 +92,11 @@ impl std::str::FromStr for PartialVersion { // HACK: Leverage `VersionReq` for partial version parsing let mut version_req = match semver::VersionReq::parse(value) { Ok(req) => req, - Err(_) if value.contains('-') => { - anyhow::bail!( - "unexpected prerelease field, expected a version like \"1.32\"" - ) - } + Err(_) if value.contains('-') => return Err(PartialVersionError::Prerelease), Err(_) if value.contains('+') => { - anyhow::bail!("unexpected build field, expected a version like \"1.32\"") + return Err(PartialVersionError::BuildMetadata) } - Err(_) => anyhow::bail!("expected a version like \"1.32\""), + Err(_) => return Err(PartialVersionError::Unexpected), }; assert_eq!(version_req.comparators.len(), 1, "guaranteed by is_req"); let comp = version_req.comparators.pop().unwrap(); @@ -163,6 +159,23 @@ impl<'de> serde::Deserialize<'de> for PartialVersion { } } +/// Error parsing a [`PartialVersion`]. +#[non_exhaustive] +#[derive(Debug, thiserror::Error)] +pub enum PartialVersionError { + #[error("unexpected version requirement, expected a version like \"1.32\"")] + VersionReq, + + #[error("unexpected prerelease field, expected a version like \"1.32\"")] + Prerelease, + + #[error("unexpected build field, expected a version like \"1.32\"")] + BuildMetadata, + + #[error("expected a version like \"1.32\"")] + Unexpected, +} + fn is_req(value: &str) -> bool { let Some(first) = value.chars().next() else { return false; From 90017c0747555391fa82ab01616d8288a557afb6 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 19 Dec 2023 16:59:41 -0500 Subject: [PATCH 03/10] refactor(util-schemas): error type for `RustVerion` --- crates/cargo-util-schemas/src/manifest.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/crates/cargo-util-schemas/src/manifest.rs b/crates/cargo-util-schemas/src/manifest.rs index eb6d4ea4852..c2fa7187104 100644 --- a/crates/cargo-util-schemas/src/manifest.rs +++ b/crates/cargo-util-schemas/src/manifest.rs @@ -18,6 +18,7 @@ use serde_untagged::UntaggedEnumVisitor; use crate::core::PackageIdSpec; use crate::core::PartialVersion; +use crate::core::PartialVersionError; use crate::restricted_names; /// This type is used to deserialize `Cargo.toml` files. @@ -1334,15 +1335,15 @@ impl std::ops::Deref for RustVersion { } impl std::str::FromStr for RustVersion { - type Err = anyhow::Error; + type Err = RustVersionError; fn from_str(value: &str) -> Result { let partial = value.parse::()?; if partial.pre.is_some() { - anyhow::bail!("unexpected prerelease field, expected a version like \"1.32\"") + return Err(RustVersionError::Prerelease); } if partial.build.is_some() { - anyhow::bail!("unexpected prerelease field, expected a version like \"1.32\"") + return Err(RustVersionError::BuildMetadata); } Ok(Self(partial)) } @@ -1366,6 +1367,20 @@ impl Display for RustVersion { } } +/// Error parsing a [`PartialVersion`]. +#[non_exhaustive] +#[derive(Debug, thiserror::Error)] +pub enum RustVersionError { + #[error("unexpected prerelease field, expected a version like \"1.32\"")] + Prerelease, + + #[error("unexpected build field, expected a version like \"1.32\"")] + BuildMetadata, + + #[error(transparent)] + PartialVersion(#[from] PartialVersionError), +} + #[derive(Copy, Clone, Debug)] pub struct InvalidCargoFeatures {} From a3267bfa2923ea50424e69ef8a01b1bafef1f4ab Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 19 Dec 2023 03:40:38 -0500 Subject: [PATCH 04/10] refactor(util-schemas): error type for restricted_names --- crates/cargo-util-schemas/src/manifest.rs | 13 +- .../src/restricted_names.rs | 156 ++++++++++-------- tests/testsuite/features.rs | 6 +- tests/testsuite/new.rs | 3 +- tests/testsuite/profile_custom.rs | 3 +- 5 files changed, 100 insertions(+), 81 deletions(-) diff --git a/crates/cargo-util-schemas/src/manifest.rs b/crates/cargo-util-schemas/src/manifest.rs index c2fa7187104..c23dd01a14c 100644 --- a/crates/cargo-util-schemas/src/manifest.rs +++ b/crates/cargo-util-schemas/src/manifest.rs @@ -10,7 +10,6 @@ use std::fmt::{self, Display, Write}; use std::path::PathBuf; use std::str; -use anyhow::Result; use serde::de::{self, IntoDeserializer as _, Unexpected}; use serde::ser; use serde::{Deserialize, Serialize}; @@ -21,6 +20,8 @@ use crate::core::PartialVersion; use crate::core::PartialVersionError; use crate::restricted_names; +pub use crate::restricted_names::NameValidationError; + /// This type is used to deserialize `Cargo.toml` files. #[derive(Debug, Deserialize, Serialize)] #[serde(rename_all = "kebab-case")] @@ -1145,7 +1146,7 @@ macro_rules! str_newtype { } impl<'a> std::str::FromStr for $name { - type Err = anyhow::Error; + type Err = restricted_names::NameValidationError; fn from_str(value: &str) -> Result { Self::new(value.to_owned()) @@ -1174,7 +1175,7 @@ str_newtype!(PackageName); impl> PackageName { /// Validated package name - pub fn new(name: T) -> Result { + pub fn new(name: T) -> Result { restricted_names::validate_package_name(name.as_ref(), "package name", "")?; Ok(Self(name)) } @@ -1196,7 +1197,7 @@ str_newtype!(RegistryName); impl> RegistryName { /// Validated registry name - pub fn new(name: T) -> Result { + pub fn new(name: T) -> Result { restricted_names::validate_package_name(name.as_ref(), "registry name", "")?; Ok(Self(name)) } @@ -1206,7 +1207,7 @@ str_newtype!(ProfileName); impl> ProfileName { /// Validated profile name - pub fn new(name: T) -> Result { + pub fn new(name: T) -> Result { restricted_names::validate_profile_name(name.as_ref())?; Ok(Self(name)) } @@ -1216,7 +1217,7 @@ str_newtype!(FeatureName); impl> FeatureName { /// Validated feature name - pub fn new(name: T) -> Result { + pub fn new(name: T) -> Result { restricted_names::validate_feature_name(name.as_ref())?; Ok(Self(name)) } diff --git a/crates/cargo-util-schemas/src/restricted_names.rs b/crates/cargo-util-schemas/src/restricted_names.rs index 2d22ce4f2f7..a92a3e25e05 100644 --- a/crates/cargo-util-schemas/src/restricted_names.rs +++ b/crates/cargo-util-schemas/src/restricted_names.rs @@ -1,7 +1,32 @@ //! Helpers for validating and checking names like package and crate names. -use anyhow::bail; -use anyhow::Result; +type Result = std::result::Result; + +/// Error validating names in Cargo. +#[non_exhaustive] +#[derive(Debug, thiserror::Error)] +pub enum NameValidationError { + #[error("{0} cannot be empty")] + Empty(&'static str), + + #[error("invalid character `{ch}` in {what}: `{name}`, {reason}")] + InvalidCharacter { + ch: char, + what: &'static str, + name: String, + reason: &'static str, + }, + + #[error( + "profile name `{name}` is reserved\n{help}\n\ + See https://doc.rust-lang.org/cargo/reference/profiles.html \ + for more on configuring profiles." + )] + ProfileNameReservedKeyword { name: String, help: &'static str }, + + #[error("feature named `{0}` is not allowed to start with `dep:`")] + FeatureNameStartsWithDepColon(String), +} /// Check the base requirements for a package name. /// @@ -9,46 +34,41 @@ use anyhow::Result; /// level of sanity. Note that package names have other restrictions /// elsewhere. `cargo new` has a few restrictions, such as checking for /// reserved names. crates.io has even more restrictions. -pub fn validate_package_name(name: &str, what: &str, help: &str) -> Result<()> { +pub fn validate_package_name(name: &str, what: &'static str, help: &str) -> Result<()> { if name.is_empty() { - bail!("{what} cannot be empty"); + return Err(NameValidationError::Empty(what)); } let mut chars = name.chars(); if let Some(ch) = chars.next() { if ch.is_digit(10) { // A specific error for a potentially common case. - bail!( - "the name `{}` cannot be used as a {}, \ - the name cannot start with a digit{}", - name, + return Err(NameValidationError::InvalidCharacter { + ch, what, - help - ); + name: name.into(), + reason: "the name cannot start with a digit", + }); } if !(unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_') { - bail!( - "invalid character `{}` in {}: `{}`, \ - the first character must be a Unicode XID start character \ - (most letters or `_`){}", + return Err(NameValidationError::InvalidCharacter { ch, what, - name, - help - ); + name: name.into(), + reason: "the first character must be a Unicode XID start character \ + (most letters or `_`)", + }); } } for ch in chars { if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-') { - bail!( - "invalid character `{}` in {}: `{}`, \ - characters must be Unicode XID characters \ - (numbers, `-`, `_`, or most letters){}", + return Err(NameValidationError::InvalidCharacter { ch, what, - name, - help - ); + name: name.into(), + reason: "characters must be Unicode XID characters \ + (numbers, `-`, `_`, or most letters)", + }); } } Ok(()) @@ -83,37 +103,28 @@ pub fn validate_profile_name(name: &str) -> Result<()> { .chars() .find(|ch| !ch.is_alphanumeric() && *ch != '_' && *ch != '-') { - bail!( - "invalid character `{}` in profile name `{}`\n\ - Allowed characters are letters, numbers, underscore, and hyphen.", + return Err(NameValidationError::InvalidCharacter { ch, - name - ); + what: "profile name", + name: name.into(), + reason: "allowed characters are letters, numbers, underscore, and hyphen", + }); } - const SEE_DOCS: &str = "See https://doc.rust-lang.org/cargo/reference/profiles.html \ - for more on configuring profiles."; - let lower_name = name.to_lowercase(); if lower_name == "debug" { - bail!( - "profile name `{}` is reserved\n\ - To configure the default development profile, use the name `dev` \ - as in [profile.dev]\n\ - {}", - name, - SEE_DOCS - ); + return Err(NameValidationError::ProfileNameReservedKeyword { + name: name.into(), + help: "To configure the default development profile, \ + use the name `dev` as in [profile.dev]", + }); } if lower_name == "build-override" { - bail!( - "profile name `{}` is reserved\n\ - To configure build dependency settings, use [profile.dev.build-override] \ - and [profile.release.build-override]\n\ - {}", - name, - SEE_DOCS - ); + return Err(NameValidationError::ProfileNameReservedKeyword { + name: name.into(), + help: "To configure build dependency settings, use [profile.dev.build-override] \ + and [profile.release.build-override]", + }); } // These are some arbitrary reservations. We have no plans to use @@ -144,46 +155,55 @@ pub fn validate_profile_name(name: &str) -> Result<()> { | "uninstall" ) || lower_name.starts_with("cargo") { - bail!( - "profile name `{}` is reserved\n\ - Please choose a different name.\n\ - {}", - name, - SEE_DOCS - ); + return Err(NameValidationError::ProfileNameReservedKeyword { + name: name.into(), + help: "Please choose a different name.", + }); } Ok(()) } pub fn validate_feature_name(name: &str) -> Result<()> { + let what = "feature name"; if name.is_empty() { - bail!("feature name cannot be empty"); + return Err(NameValidationError::Empty(what)); } if name.starts_with("dep:") { - bail!("feature named `{name}` is not allowed to start with `dep:`",); + return Err(NameValidationError::FeatureNameStartsWithDepColon( + name.into(), + )); } if name.contains('/') { - bail!("feature named `{name}` is not allowed to contain slashes",); + return Err(NameValidationError::InvalidCharacter { + ch: '/', + what, + name: name.into(), + reason: "feature name is not allowed to contain slashes", + }); } let mut chars = name.chars(); if let Some(ch) = chars.next() { if !(unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_' || ch.is_digit(10)) { - bail!( - "invalid character `{ch}` in feature `{name}`, \ - the first character must be a Unicode XID start character or digit \ - (most letters or `_` or `0` to `9`)", - ); + return Err(NameValidationError::InvalidCharacter { + ch, + what, + name: name.into(), + reason: "the first character must be a Unicode XID start character or digit \ + (most letters or `_` or `0` to `9`)", + }); } } for ch in chars { if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-' || ch == '+' || ch == '.') { - bail!( - "invalid character `{ch}` in feature `{name}`, \ - characters must be Unicode XID characters, '-', `+`, or `.` \ - (numbers, `+`, `-`, `_`, `.`, or most letters)", - ); + return Err(NameValidationError::InvalidCharacter { + ch, + what, + name: name.into(), + reason: "characters must be Unicode XID characters, '-', `+`, or `.` \ + (numbers, `+`, `-`, `_`, `.`, or most letters)", + }); } } Ok(()) diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs index febdf52fed8..221de106fa8 100644 --- a/tests/testsuite/features.rs +++ b/tests/testsuite/features.rs @@ -2063,7 +2063,7 @@ Caused by: | 8 | \"+foo\" = [] | ^^^^^^ - invalid character `+` in feature `+foo`, \ + invalid character `+` in feature name: `+foo`, \ the first character must be a Unicode XID start character or digit \ (most letters or `_` or `0` to `9`) ", @@ -2094,7 +2094,7 @@ Caused by: | 8 | \"a&b\" = [] | ^^^^^ - invalid character `&` in feature `a&b`, \ + invalid character `&` in feature name: `a&b`, \ characters must be Unicode XID characters, '-', `+`, or `.` \ (numbers, `+`, `-`, `_`, `.`, or most letters) ", @@ -2131,7 +2131,7 @@ Caused by: | 7 | \"foo/bar\" = [] | ^^^^^^^^^ - feature named `foo/bar` is not allowed to contain slashes + invalid character `/` in feature name: `foo/bar`, feature name is not allowed to contain slashes ", ) .run(); diff --git a/tests/testsuite/new.rs b/tests/testsuite/new.rs index a34169e9dbb..2f56aa677d7 100644 --- a/tests/testsuite/new.rs +++ b/tests/testsuite/new.rs @@ -348,8 +348,7 @@ fn explicit_invalid_name_not_suggested() { .with_status(101) .with_stderr( "\ -[ERROR] the name `10-invalid` cannot be used as a package name, \ -the name cannot start with a digit\n\ +[ERROR] invalid character `1` in package name: `10-invalid`, the name cannot start with a digit If you need a binary with the name \"10-invalid\", use a valid package name, \ and set the binary name to be different from the package. \ This can be done by setting the binary filename to `src/bin/10-invalid.rs` \ diff --git a/tests/testsuite/profile_custom.rs b/tests/testsuite/profile_custom.rs index cf9828d3794..c3374874d8c 100644 --- a/tests/testsuite/profile_custom.rs +++ b/tests/testsuite/profile_custom.rs @@ -90,8 +90,7 @@ Caused by: | 7 | [profile.'.release-lto'] | ^^^^^^^^^^^^^^ - invalid character `.` in profile name `.release-lto` - Allowed characters are letters, numbers, underscore, and hyphen. + invalid character `.` in profile name: `.release-lto`, allowed characters are letters, numbers, underscore, and hyphen ", ) .run(); From 1e577614c5177cdde44379fbbfee2c656365be5f Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 19 Dec 2023 13:25:49 -0500 Subject: [PATCH 05/10] refactor(util-schemas): error type for `PackageIdSpec` --- crates/cargo-util-schemas/src/core/mod.rs | 1 + .../src/core/package_id_spec.rs | 69 ++++++++++++------- src/cargo/ops/cargo_compile/packages.rs | 2 +- src/cargo/ops/tree/mod.rs | 4 +- 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/crates/cargo-util-schemas/src/core/mod.rs b/crates/cargo-util-schemas/src/core/mod.rs index e0cd0953894..e8a878aa77c 100644 --- a/crates/cargo-util-schemas/src/core/mod.rs +++ b/crates/cargo-util-schemas/src/core/mod.rs @@ -3,6 +3,7 @@ mod partial_version; mod source_kind; pub use package_id_spec::PackageIdSpec; +pub use package_id_spec::PackageIdSpecError; pub use partial_version::PartialVersion; pub use partial_version::PartialVersionError; pub use source_kind::GitReference; diff --git a/crates/cargo-util-schemas/src/core/package_id_spec.rs b/crates/cargo-util-schemas/src/core/package_id_spec.rs index e0a706750af..9b5908b42ba 100644 --- a/crates/cargo-util-schemas/src/core/package_id_spec.rs +++ b/crates/cargo-util-schemas/src/core/package_id_spec.rs @@ -1,7 +1,5 @@ use std::fmt; -use anyhow::bail; -use anyhow::Result; use semver::Version; use serde::{de, ser}; use url::Url; @@ -11,6 +9,8 @@ use crate::core::PartialVersion; use crate::core::SourceKind; use crate::manifest::PackageName; +type Result = std::result::Result; + /// Some or all of the data required to identify a package: /// /// 1. the package name (a `String`, required) @@ -83,12 +83,10 @@ impl PackageIdSpec { if abs.exists() { let maybe_url = Url::from_file_path(abs) .map_or_else(|_| "a file:// URL".to_string(), |url| url.to_string()); - bail!( - "package ID specification `{}` looks like a file path, \ - maybe try {}", - spec, - maybe_url - ); + return Err(PackageIdSpecError::MaybeFilePath { + spec: spec.into(), + maybe_url, + }); } } let mut parts = spec.splitn(2, [':', '@']); @@ -119,14 +117,14 @@ impl PackageIdSpec { } "registry" => { if url.query().is_some() { - bail!("cannot have a query string in a pkgid: {url}") + return Err(PackageIdSpecError::UnexpectedQueryString(url)); } kind = Some(SourceKind::Registry); url = strip_url_protocol(&url); } "sparse" => { if url.query().is_some() { - bail!("cannot have a query string in a pkgid: {url}") + return Err(PackageIdSpecError::UnexpectedQueryString(url)); } kind = Some(SourceKind::SparseRegistry); // Leave `sparse` as part of URL, see `SourceId::new` @@ -134,19 +132,19 @@ impl PackageIdSpec { } "path" => { if url.query().is_some() { - bail!("cannot have a query string in a pkgid: {url}") + return Err(PackageIdSpecError::UnexpectedQueryString(url)); } if scheme != "file" { - anyhow::bail!("`path+{scheme}` is unsupported; `path+file` and `file` schemes are supported"); + return Err(PackageIdSpecError::UnsupportedPathPlusScheme(scheme.into())); } kind = Some(SourceKind::Path); url = strip_url_protocol(&url); } - kind => anyhow::bail!("unsupported source protocol: {kind}"), + kind => return Err(PackageIdSpecError::UnsupportedProtocol(kind.into())), } } else { if url.query().is_some() { - bail!("cannot have a query string in a pkgid: {url}") + return Err(PackageIdSpecError::UnexpectedQueryString(url)); } } @@ -154,16 +152,9 @@ impl PackageIdSpec { url.set_fragment(None); let (name, version) = { - let mut path = url - .path_segments() - .ok_or_else(|| anyhow::format_err!("pkgid urls must have a path: {}", url))?; - let path_name = path.next_back().ok_or_else(|| { - anyhow::format_err!( - "pkgid urls must have at least one path \ - component: {}", - url - ) - })?; + let Some(path_name) = url.path_segments().and_then(|mut p| p.next_back()) else { + return Err(PackageIdSpecError::MissingUrlPath(url)); + }; match frag { Some(fragment) => match fragment.split_once([':', '@']) { Some((name, part)) => { @@ -259,7 +250,7 @@ impl fmt::Display for PackageIdSpec { } impl ser::Serialize for PackageIdSpec { - fn serialize(&self, s: S) -> Result + fn serialize(&self, s: S) -> std::result::Result where S: ser::Serializer, { @@ -268,7 +259,7 @@ impl ser::Serialize for PackageIdSpec { } impl<'de> de::Deserialize<'de> for PackageIdSpec { - fn deserialize(d: D) -> Result + fn deserialize(d: D) -> std::result::Result where D: de::Deserializer<'de>, { @@ -277,6 +268,32 @@ impl<'de> de::Deserialize<'de> for PackageIdSpec { } } +/// Error parsing a [`PackageIdSpec`]. +#[non_exhaustive] +#[derive(Debug, thiserror::Error)] +pub enum PackageIdSpecError { + #[error("unsupported source protocol: {0}")] + UnsupportedProtocol(String), + + #[error("`path+{0}` is unsupported; `path+file` and `file` schemes are supported")] + UnsupportedPathPlusScheme(String), + + #[error("cannot have a query string in a pkgid: {0}")] + UnexpectedQueryString(Url), + + #[error("pkgid urls must have at least one path component: {0}")] + MissingUrlPath(Url), + + #[error("package ID specification `{spec}` looks like a file path, maybe try {maybe_url}")] + MaybeFilePath { spec: String, maybe_url: String }, + + #[error(transparent)] + NameValidation(#[from] crate::restricted_names::NameValidationError), + + #[error(transparent)] + PartialVersion(#[from] crate::core::PartialVersionError), +} + #[cfg(test)] mod tests { use super::PackageIdSpec; diff --git a/src/cargo/ops/cargo_compile/packages.rs b/src/cargo/ops/cargo_compile/packages.rs index 192bfcb3a3f..0472cc83716 100644 --- a/src/cargo/ops/cargo_compile/packages.rs +++ b/src/cargo/ops/cargo_compile/packages.rs @@ -72,7 +72,7 @@ impl Packages { let mut specs = packages .iter() .map(|p| PackageIdSpec::parse(p)) - .collect::>>()?; + .collect::, _>>()?; if !patterns.is_empty() { let matched_pkgs = ws .members() diff --git a/src/cargo/ops/tree/mod.rs b/src/cargo/ops/tree/mod.rs index 6928ec5f94b..79a69a66a4f 100644 --- a/src/cargo/ops/tree/mod.rs +++ b/src/cargo/ops/tree/mod.rs @@ -186,7 +186,7 @@ pub fn build_and_print(ws: &Workspace<'_>, opts: &TreeOptions) -> CargoResult<() opts.invert .iter() .map(|p| PackageIdSpec::parse(p)) - .collect::>>()? + .collect::, _>>()? }; let root_ids = ws_resolve.targeted_resolve.specs_to_ids(&root_specs)?; let root_indexes = graph.indexes_from_ids(&root_ids); @@ -207,7 +207,7 @@ pub fn build_and_print(ws: &Workspace<'_>, opts: &TreeOptions) -> CargoResult<() let pkgs_to_prune = opts .pkgs_to_prune .iter() - .map(|p| PackageIdSpec::parse(p)) + .map(|p| PackageIdSpec::parse(p).map_err(Into::into)) .map(|r| { // Provide an error message if pkgid is not within the resolved // dependencies graph. From f1216495bb426a172586be5dcf05a6451d14c985 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 19 Dec 2023 12:24:05 -0500 Subject: [PATCH 06/10] refactor(util-schemas): remove anyhow --- Cargo.lock | 1 - crates/cargo-util-schemas/Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b92ba687f70..77148c1b090 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -439,7 +439,6 @@ dependencies = [ name = "cargo-util-schemas" version = "0.1.1" dependencies = [ - "anyhow", "semver", "serde", "serde-untagged", diff --git a/crates/cargo-util-schemas/Cargo.toml b/crates/cargo-util-schemas/Cargo.toml index 3676bffb5b2..f81a4dd8844 100644 --- a/crates/cargo-util-schemas/Cargo.toml +++ b/crates/cargo-util-schemas/Cargo.toml @@ -9,7 +9,6 @@ repository.workspace = true description = "Deserialization schemas for Cargo" [dependencies] -anyhow.workspace = true semver.workspace = true serde = { workspace = true, features = ["derive"] } serde-untagged.workspace = true From d1e1c3d0aaf497aea48372d5337b618a361d0623 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 19 Dec 2023 15:20:36 -0500 Subject: [PATCH 07/10] refactor(util-schemas): remove unused arg --- crates/cargo-util-schemas/src/manifest.rs | 4 ++-- crates/cargo-util-schemas/src/restricted_names.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/cargo-util-schemas/src/manifest.rs b/crates/cargo-util-schemas/src/manifest.rs index c23dd01a14c..1b6722a1b00 100644 --- a/crates/cargo-util-schemas/src/manifest.rs +++ b/crates/cargo-util-schemas/src/manifest.rs @@ -1176,7 +1176,7 @@ str_newtype!(PackageName); impl> PackageName { /// Validated package name pub fn new(name: T) -> Result { - restricted_names::validate_package_name(name.as_ref(), "package name", "")?; + restricted_names::validate_package_name(name.as_ref(), "package name")?; Ok(Self(name)) } } @@ -1198,7 +1198,7 @@ str_newtype!(RegistryName); impl> RegistryName { /// Validated registry name pub fn new(name: T) -> Result { - restricted_names::validate_package_name(name.as_ref(), "registry name", "")?; + restricted_names::validate_package_name(name.as_ref(), "registry name")?; Ok(Self(name)) } } diff --git a/crates/cargo-util-schemas/src/restricted_names.rs b/crates/cargo-util-schemas/src/restricted_names.rs index a92a3e25e05..c256fc0b0af 100644 --- a/crates/cargo-util-schemas/src/restricted_names.rs +++ b/crates/cargo-util-schemas/src/restricted_names.rs @@ -34,7 +34,7 @@ pub enum NameValidationError { /// level of sanity. Note that package names have other restrictions /// elsewhere. `cargo new` has a few restrictions, such as checking for /// reserved names. crates.io has even more restrictions. -pub fn validate_package_name(name: &str, what: &'static str, help: &str) -> Result<()> { +pub fn validate_package_name(name: &str, what: &'static str) -> Result<()> { if name.is_empty() { return Err(NameValidationError::Empty(what)); } From a0201cd46531b54aecd264b376a1fa21a3102362 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 19 Dec 2023 17:57:53 -0500 Subject: [PATCH 08/10] refactor(util-schemas): make fn in `restricted_names` crate private pub --- crates/cargo-util-schemas/src/restricted_names.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/cargo-util-schemas/src/restricted_names.rs b/crates/cargo-util-schemas/src/restricted_names.rs index c256fc0b0af..ab8aaee18b8 100644 --- a/crates/cargo-util-schemas/src/restricted_names.rs +++ b/crates/cargo-util-schemas/src/restricted_names.rs @@ -34,7 +34,7 @@ pub enum NameValidationError { /// level of sanity. Note that package names have other restrictions /// elsewhere. `cargo new` has a few restrictions, such as checking for /// reserved names. crates.io has even more restrictions. -pub fn validate_package_name(name: &str, what: &'static str) -> Result<()> { +pub(crate) fn validate_package_name(name: &str, what: &'static str) -> Result<()> { if name.is_empty() { return Err(NameValidationError::Empty(what)); } @@ -75,7 +75,7 @@ pub fn validate_package_name(name: &str, what: &'static str) -> Result<()> { } /// Ensure a package name is [valid][validate_package_name] -pub fn sanitize_package_name(name: &str, placeholder: char) -> String { +pub(crate) fn sanitize_package_name(name: &str, placeholder: char) -> String { let mut slug = String::new(); let mut chars = name.chars(); while let Some(ch) = chars.next() { @@ -98,7 +98,7 @@ pub fn sanitize_package_name(name: &str, placeholder: char) -> String { } /// Validate dir-names and profile names according to RFC 2678. -pub fn validate_profile_name(name: &str) -> Result<()> { +pub(crate) fn validate_profile_name(name: &str) -> Result<()> { if let Some(ch) = name .chars() .find(|ch| !ch.is_alphanumeric() && *ch != '_' && *ch != '-') @@ -164,7 +164,7 @@ pub fn validate_profile_name(name: &str) -> Result<()> { Ok(()) } -pub fn validate_feature_name(name: &str) -> Result<()> { +pub(crate) fn validate_feature_name(name: &str) -> Result<()> { let what = "feature name"; if name.is_empty() { return Err(NameValidationError::Empty(what)); From f9e726b05672abfc1ddf5e1be3ac43967d9749d0 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 19 Dec 2023 18:58:46 -0500 Subject: [PATCH 09/10] refactor(util-schemas): make error enum private --- .../src/core/package_id_spec.rs | 40 +++++++++--- .../src/core/partial_version.rs | 17 +++-- crates/cargo-util-schemas/src/manifest.rs | 16 +++-- .../src/restricted_names.rs | 65 +++++++++++-------- 4 files changed, 90 insertions(+), 48 deletions(-) diff --git a/crates/cargo-util-schemas/src/core/package_id_spec.rs b/crates/cargo-util-schemas/src/core/package_id_spec.rs index 9b5908b42ba..7584771b3ee 100644 --- a/crates/cargo-util-schemas/src/core/package_id_spec.rs +++ b/crates/cargo-util-schemas/src/core/package_id_spec.rs @@ -6,8 +6,10 @@ use url::Url; use crate::core::GitReference; use crate::core::PartialVersion; +use crate::core::PartialVersionError; use crate::core::SourceKind; use crate::manifest::PackageName; +use crate::restricted_names::NameValidationError; type Result = std::result::Result; @@ -83,10 +85,11 @@ impl PackageIdSpec { if abs.exists() { let maybe_url = Url::from_file_path(abs) .map_or_else(|_| "a file:// URL".to_string(), |url| url.to_string()); - return Err(PackageIdSpecError::MaybeFilePath { + return Err(ErrorKind::MaybeFilePath { spec: spec.into(), maybe_url, - }); + } + .into()); } } let mut parts = spec.splitn(2, [':', '@']); @@ -117,14 +120,14 @@ impl PackageIdSpec { } "registry" => { if url.query().is_some() { - return Err(PackageIdSpecError::UnexpectedQueryString(url)); + return Err(ErrorKind::UnexpectedQueryString(url).into()); } kind = Some(SourceKind::Registry); url = strip_url_protocol(&url); } "sparse" => { if url.query().is_some() { - return Err(PackageIdSpecError::UnexpectedQueryString(url)); + return Err(ErrorKind::UnexpectedQueryString(url).into()); } kind = Some(SourceKind::SparseRegistry); // Leave `sparse` as part of URL, see `SourceId::new` @@ -132,19 +135,19 @@ impl PackageIdSpec { } "path" => { if url.query().is_some() { - return Err(PackageIdSpecError::UnexpectedQueryString(url)); + return Err(ErrorKind::UnexpectedQueryString(url).into()); } if scheme != "file" { - return Err(PackageIdSpecError::UnsupportedPathPlusScheme(scheme.into())); + return Err(ErrorKind::UnsupportedPathPlusScheme(scheme.into()).into()); } kind = Some(SourceKind::Path); url = strip_url_protocol(&url); } - kind => return Err(PackageIdSpecError::UnsupportedProtocol(kind.into())), + kind => return Err(ErrorKind::UnsupportedProtocol(kind.into()).into()), } } else { if url.query().is_some() { - return Err(PackageIdSpecError::UnexpectedQueryString(url)); + return Err(ErrorKind::UnexpectedQueryString(url).into()); } } @@ -153,7 +156,7 @@ impl PackageIdSpec { let (name, version) = { let Some(path_name) = url.path_segments().and_then(|mut p| p.next_back()) else { - return Err(PackageIdSpecError::MissingUrlPath(url)); + return Err(ErrorKind::MissingUrlPath(url).into()); }; match frag { Some(fragment) => match fragment.split_once([':', '@']) { @@ -269,9 +272,26 @@ impl<'de> de::Deserialize<'de> for PackageIdSpec { } /// Error parsing a [`PackageIdSpec`]. +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub struct PackageIdSpecError(#[from] ErrorKind); + +impl From for PackageIdSpecError { + fn from(value: PartialVersionError) -> Self { + ErrorKind::PartialVersion(value).into() + } +} + +impl From for PackageIdSpecError { + fn from(value: NameValidationError) -> Self { + ErrorKind::NameValidation(value).into() + } +} + +/// Non-public error kind for [`PackageIdSpecError`]. #[non_exhaustive] #[derive(Debug, thiserror::Error)] -pub enum PackageIdSpecError { +enum ErrorKind { #[error("unsupported source protocol: {0}")] UnsupportedProtocol(String), diff --git a/crates/cargo-util-schemas/src/core/partial_version.rs b/crates/cargo-util-schemas/src/core/partial_version.rs index a7a96e386c5..b9c1db82ed1 100644 --- a/crates/cargo-util-schemas/src/core/partial_version.rs +++ b/crates/cargo-util-schemas/src/core/partial_version.rs @@ -84,7 +84,7 @@ impl std::str::FromStr for PartialVersion { fn from_str(value: &str) -> Result { if is_req(value) { - return Err(PartialVersionError::VersionReq); + return Err(ErrorKind::VersionReq.into()); } match semver::Version::parse(value) { Ok(ver) => Ok(ver.into()), @@ -92,11 +92,9 @@ impl std::str::FromStr for PartialVersion { // HACK: Leverage `VersionReq` for partial version parsing let mut version_req = match semver::VersionReq::parse(value) { Ok(req) => req, - Err(_) if value.contains('-') => return Err(PartialVersionError::Prerelease), - Err(_) if value.contains('+') => { - return Err(PartialVersionError::BuildMetadata) - } - Err(_) => return Err(PartialVersionError::Unexpected), + Err(_) if value.contains('-') => return Err(ErrorKind::Prerelease.into()), + Err(_) if value.contains('+') => return Err(ErrorKind::BuildMetadata.into()), + Err(_) => return Err(ErrorKind::Unexpected.into()), }; assert_eq!(version_req.comparators.len(), 1, "guaranteed by is_req"); let comp = version_req.comparators.pop().unwrap(); @@ -160,9 +158,14 @@ impl<'de> serde::Deserialize<'de> for PartialVersion { } /// Error parsing a [`PartialVersion`]. +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub struct PartialVersionError(#[from] ErrorKind); + +/// Non-public error kind for [`PartialVersionError`]. #[non_exhaustive] #[derive(Debug, thiserror::Error)] -pub enum PartialVersionError { +enum ErrorKind { #[error("unexpected version requirement, expected a version like \"1.32\"")] VersionReq, diff --git a/crates/cargo-util-schemas/src/manifest.rs b/crates/cargo-util-schemas/src/manifest.rs index 1b6722a1b00..0bf921418c0 100644 --- a/crates/cargo-util-schemas/src/manifest.rs +++ b/crates/cargo-util-schemas/src/manifest.rs @@ -1339,12 +1339,13 @@ impl std::str::FromStr for RustVersion { type Err = RustVersionError; fn from_str(value: &str) -> Result { - let partial = value.parse::()?; + let partial = value.parse::(); + let partial = partial.map_err(RustVersionErrorKind::PartialVersion)?; if partial.pre.is_some() { - return Err(RustVersionError::Prerelease); + return Err(RustVersionErrorKind::Prerelease.into()); } if partial.build.is_some() { - return Err(RustVersionError::BuildMetadata); + return Err(RustVersionErrorKind::BuildMetadata.into()); } Ok(Self(partial)) } @@ -1368,10 +1369,15 @@ impl Display for RustVersion { } } -/// Error parsing a [`PartialVersion`]. +/// Error parsing a [`RustVersion`]. +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub struct RustVersionError(#[from] RustVersionErrorKind); + +/// Non-public error kind for [`RustVersionError`]. #[non_exhaustive] #[derive(Debug, thiserror::Error)] -pub enum RustVersionError { +enum RustVersionErrorKind { #[error("unexpected prerelease field, expected a version like \"1.32\"")] Prerelease, diff --git a/crates/cargo-util-schemas/src/restricted_names.rs b/crates/cargo-util-schemas/src/restricted_names.rs index ab8aaee18b8..79d01ac9b88 100644 --- a/crates/cargo-util-schemas/src/restricted_names.rs +++ b/crates/cargo-util-schemas/src/restricted_names.rs @@ -3,9 +3,14 @@ type Result = std::result::Result; /// Error validating names in Cargo. +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub struct NameValidationError(#[from] ErrorKind); + +/// Non-public error kind for [`NameValidationError`]. #[non_exhaustive] #[derive(Debug, thiserror::Error)] -pub enum NameValidationError { +enum ErrorKind { #[error("{0} cannot be empty")] Empty(&'static str), @@ -36,39 +41,42 @@ pub enum NameValidationError { /// reserved names. crates.io has even more restrictions. pub(crate) fn validate_package_name(name: &str, what: &'static str) -> Result<()> { if name.is_empty() { - return Err(NameValidationError::Empty(what)); + return Err(ErrorKind::Empty(what).into()); } let mut chars = name.chars(); if let Some(ch) = chars.next() { if ch.is_digit(10) { // A specific error for a potentially common case. - return Err(NameValidationError::InvalidCharacter { + return Err(ErrorKind::InvalidCharacter { ch, what, name: name.into(), reason: "the name cannot start with a digit", - }); + } + .into()); } if !(unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_') { - return Err(NameValidationError::InvalidCharacter { + return Err(ErrorKind::InvalidCharacter { ch, what, name: name.into(), reason: "the first character must be a Unicode XID start character \ (most letters or `_`)", - }); + } + .into()); } } for ch in chars { if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-') { - return Err(NameValidationError::InvalidCharacter { + return Err(ErrorKind::InvalidCharacter { ch, what, name: name.into(), reason: "characters must be Unicode XID characters \ (numbers, `-`, `_`, or most letters)", - }); + } + .into()); } } Ok(()) @@ -103,28 +111,31 @@ pub(crate) fn validate_profile_name(name: &str) -> Result<()> { .chars() .find(|ch| !ch.is_alphanumeric() && *ch != '_' && *ch != '-') { - return Err(NameValidationError::InvalidCharacter { + return Err(ErrorKind::InvalidCharacter { ch, what: "profile name", name: name.into(), reason: "allowed characters are letters, numbers, underscore, and hyphen", - }); + } + .into()); } let lower_name = name.to_lowercase(); if lower_name == "debug" { - return Err(NameValidationError::ProfileNameReservedKeyword { + return Err(ErrorKind::ProfileNameReservedKeyword { name: name.into(), help: "To configure the default development profile, \ use the name `dev` as in [profile.dev]", - }); + } + .into()); } if lower_name == "build-override" { - return Err(NameValidationError::ProfileNameReservedKeyword { + return Err(ErrorKind::ProfileNameReservedKeyword { name: name.into(), help: "To configure build dependency settings, use [profile.dev.build-override] \ and [profile.release.build-override]", - }); + } + .into()); } // These are some arbitrary reservations. We have no plans to use @@ -155,10 +166,11 @@ pub(crate) fn validate_profile_name(name: &str) -> Result<()> { | "uninstall" ) || lower_name.starts_with("cargo") { - return Err(NameValidationError::ProfileNameReservedKeyword { + return Err(ErrorKind::ProfileNameReservedKeyword { name: name.into(), help: "Please choose a different name.", - }); + } + .into()); } Ok(()) @@ -167,43 +179,44 @@ pub(crate) fn validate_profile_name(name: &str) -> Result<()> { pub(crate) fn validate_feature_name(name: &str) -> Result<()> { let what = "feature name"; if name.is_empty() { - return Err(NameValidationError::Empty(what)); + return Err(ErrorKind::Empty(what).into()); } if name.starts_with("dep:") { - return Err(NameValidationError::FeatureNameStartsWithDepColon( - name.into(), - )); + return Err(ErrorKind::FeatureNameStartsWithDepColon(name.into()).into()); } if name.contains('/') { - return Err(NameValidationError::InvalidCharacter { + return Err(ErrorKind::InvalidCharacter { ch: '/', what, name: name.into(), reason: "feature name is not allowed to contain slashes", - }); + } + .into()); } let mut chars = name.chars(); if let Some(ch) = chars.next() { if !(unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_' || ch.is_digit(10)) { - return Err(NameValidationError::InvalidCharacter { + return Err(ErrorKind::InvalidCharacter { ch, what, name: name.into(), reason: "the first character must be a Unicode XID start character or digit \ (most letters or `_` or `0` to `9`)", - }); + } + .into()); } } for ch in chars { if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-' || ch == '+' || ch == '.') { - return Err(NameValidationError::InvalidCharacter { + return Err(ErrorKind::InvalidCharacter { ch, what, name: name.into(), reason: "characters must be Unicode XID characters, '-', `+`, or `.` \ (numbers, `+`, `-`, `_`, `.`, or most letters)", - }); + } + .into()); } } Ok(()) From 0b0e78fa3fdc069da9bf8d8c67e9963b46b62630 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 19 Dec 2023 15:36:35 -0500 Subject: [PATCH 10/10] chore: bump cargo-util-schemas to 0.2.0 --- Cargo.lock | 2 +- Cargo.toml | 2 +- crates/cargo-util-schemas/Cargo.toml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 77148c1b090..781a3e1ab1b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -437,7 +437,7 @@ dependencies = [ [[package]] name = "cargo-util-schemas" -version = "0.1.1" +version = "0.2.0" dependencies = [ "semver", "serde", diff --git a/Cargo.toml b/Cargo.toml index 88a5e0fcc35..fdbdd4ed2c5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ cargo-platform = { path = "crates/cargo-platform", version = "0.1.4" } cargo-test-macro = { path = "crates/cargo-test-macro" } cargo-test-support = { path = "crates/cargo-test-support" } cargo-util = { version = "0.2.6", path = "crates/cargo-util" } -cargo-util-schemas = { version = "0.1.0", path = "crates/cargo-util-schemas" } +cargo-util-schemas = { version = "0.2.0", path = "crates/cargo-util-schemas" } cargo_metadata = "0.18.1" clap = "4.4.10" color-print = "0.3.5" diff --git a/crates/cargo-util-schemas/Cargo.toml b/crates/cargo-util-schemas/Cargo.toml index f81a4dd8844..19170618e11 100644 --- a/crates/cargo-util-schemas/Cargo.toml +++ b/crates/cargo-util-schemas/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cargo-util-schemas" -version = "0.1.1" +version = "0.2.0" rust-version.workspace = true edition.workspace = true license.workspace = true