From b0ce24a79df6a93ebc2807179b343967b7e48f24 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 12 Dec 2023 10:33:07 -0600 Subject: [PATCH 01/18] refactor: Consolidate feature name validation logic --- src/cargo/core/summary.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 8dfdf0dbfb4..2bf19763239 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -191,18 +191,6 @@ fn build_feature_map( // Validate features are listed properly. for (feature, fvs) in &map { - if feature.starts_with("dep:") { - bail!( - "feature named `{}` is not allowed to start with `dep:`", - feature - ); - } - if feature.contains('/') { - bail!( - "feature named `{}` is not allowed to contain slashes", - feature - ); - } validate_feature_name(pkg_id, feature)?; for fv in fvs { // Find data for the referenced dependency... @@ -434,6 +422,13 @@ fn validate_feature_name(pkg_id: PackageId, name: &str) -> CargoResult<()> { if name.is_empty() { bail!("feature name cannot be empty"); } + + if name.starts_with("dep:") { + bail!("feature named `{name}` is not allowed to start with `dep:`",); + } + if name.contains('/') { + bail!("feature named `{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)) { From f0bc0d2ae2d2a50e7c212d05095c81be5eabac86 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 12 Dec 2023 10:35:01 -0600 Subject: [PATCH 02/18] refactor: Consolidate name validation logicc --- src/cargo/core/summary.rs | 73 +----------------------------- src/cargo/util/restricted_names.rs | 73 ++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 72 deletions(-) diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 2bf19763239..3c6e869a802 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -1,5 +1,6 @@ use crate::core::{Dependency, PackageId, SourceId}; use crate::util::interning::InternedString; +use crate::util::restricted_names::validate_feature_name; use crate::util::CargoResult; use crate::util_schemas::manifest::RustVersion; use anyhow::bail; @@ -417,75 +418,3 @@ impl fmt::Display for FeatureValue { } pub type FeatureMap = BTreeMap>; - -fn validate_feature_name(pkg_id: PackageId, name: &str) -> CargoResult<()> { - if name.is_empty() { - bail!("feature name cannot be empty"); - } - - if name.starts_with("dep:") { - bail!("feature named `{name}` is not allowed to start with `dep:`",); - } - if name.contains('/') { - bail!("feature named `{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 `{}` in feature `{}` in package {}, \ - the first character must be a Unicode XID start character or digit \ - (most letters or `_` or `0` to `9`)", - ch, - name, - pkg_id - ); - } - } - for ch in chars { - if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-' || ch == '+' || ch == '.') { - bail!( - "invalid character `{}` in feature `{}` in package {}, \ - characters must be Unicode XID characters, '-', `+`, or `.` \ - (numbers, `+`, `-`, `_`, `.`, or most letters)", - ch, - name, - pkg_id - ); - } - } - Ok(()) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::sources::CRATES_IO_INDEX; - use crate::util::into_url::IntoUrl; - - use crate::core::SourceId; - - #[test] - fn valid_feature_names() { - let loc = CRATES_IO_INDEX.into_url().unwrap(); - let source_id = SourceId::for_registry(&loc).unwrap(); - let pkg_id = PackageId::try_new("foo", "1.0.0", source_id).unwrap(); - - assert!(validate_feature_name(pkg_id, "c++17").is_ok()); - assert!(validate_feature_name(pkg_id, "128bit").is_ok()); - assert!(validate_feature_name(pkg_id, "_foo").is_ok()); - assert!(validate_feature_name(pkg_id, "feat-name").is_ok()); - assert!(validate_feature_name(pkg_id, "feat_name").is_ok()); - assert!(validate_feature_name(pkg_id, "foo.bar").is_ok()); - - assert!(validate_feature_name(pkg_id, "+foo").is_err()); - assert!(validate_feature_name(pkg_id, "-foo").is_err()); - assert!(validate_feature_name(pkg_id, ".foo").is_err()); - assert!(validate_feature_name(pkg_id, "foo:bar").is_err()); - assert!(validate_feature_name(pkg_id, "foo?").is_err()); - assert!(validate_feature_name(pkg_id, "?foo").is_err()); - assert!(validate_feature_name(pkg_id, "ⒶⒷⒸ").is_err()); - assert!(validate_feature_name(pkg_id, "a¼").is_err()); - assert!(validate_feature_name(pkg_id, "").is_err()); - } -} diff --git a/src/cargo/util/restricted_names.rs b/src/cargo/util/restricted_names.rs index df0152ecb1e..2e2fa83e274 100644 --- a/src/cargo/util/restricted_names.rs +++ b/src/cargo/util/restricted_names.rs @@ -1,5 +1,6 @@ //! Helpers for validating and checking names like package and crate names. +use crate::core::PackageId; use crate::util::CargoResult; use anyhow::bail; use std::path::Path; @@ -203,3 +204,75 @@ pub fn validate_profile_name(name: &str) -> CargoResult<()> { Ok(()) } + +pub fn validate_feature_name(pkg_id: PackageId, name: &str) -> CargoResult<()> { + if name.is_empty() { + bail!("feature name cannot be empty"); + } + + if name.starts_with("dep:") { + bail!("feature named `{name}` is not allowed to start with `dep:`",); + } + if name.contains('/') { + bail!("feature named `{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 `{}` in feature `{}` in package {}, \ + the first character must be a Unicode XID start character or digit \ + (most letters or `_` or `0` to `9`)", + ch, + name, + pkg_id + ); + } + } + for ch in chars { + if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-' || ch == '+' || ch == '.') { + bail!( + "invalid character `{}` in feature `{}` in package {}, \ + characters must be Unicode XID characters, '-', `+`, or `.` \ + (numbers, `+`, `-`, `_`, `.`, or most letters)", + ch, + name, + pkg_id + ); + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::sources::CRATES_IO_INDEX; + use crate::util::into_url::IntoUrl; + + use crate::core::SourceId; + + #[test] + fn valid_feature_names() { + let loc = CRATES_IO_INDEX.into_url().unwrap(); + let source_id = SourceId::for_registry(&loc).unwrap(); + let pkg_id = PackageId::try_new("foo", "1.0.0", source_id).unwrap(); + + assert!(validate_feature_name(pkg_id, "c++17").is_ok()); + assert!(validate_feature_name(pkg_id, "128bit").is_ok()); + assert!(validate_feature_name(pkg_id, "_foo").is_ok()); + assert!(validate_feature_name(pkg_id, "feat-name").is_ok()); + assert!(validate_feature_name(pkg_id, "feat_name").is_ok()); + assert!(validate_feature_name(pkg_id, "foo.bar").is_ok()); + + assert!(validate_feature_name(pkg_id, "+foo").is_err()); + assert!(validate_feature_name(pkg_id, "-foo").is_err()); + assert!(validate_feature_name(pkg_id, ".foo").is_err()); + assert!(validate_feature_name(pkg_id, "foo:bar").is_err()); + assert!(validate_feature_name(pkg_id, "foo?").is_err()); + assert!(validate_feature_name(pkg_id, "?foo").is_err()); + assert!(validate_feature_name(pkg_id, "ⒶⒷⒸ").is_err()); + assert!(validate_feature_name(pkg_id, "a¼").is_err()); + assert!(validate_feature_name(pkg_id, "").is_err()); + } +} From b2afdccc475dec75300b65065aec1e639c2bcbc4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 12 Dec 2023 10:41:36 -0600 Subject: [PATCH 03/18] refactor: Use inline formatting --- src/cargo/util/restricted_names.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/restricted_names.rs b/src/cargo/util/restricted_names.rs index 2e2fa83e274..2ff5884a4e1 100644 --- a/src/cargo/util/restricted_names.rs +++ b/src/cargo/util/restricted_names.rs @@ -220,24 +220,18 @@ pub fn validate_feature_name(pkg_id: PackageId, name: &str) -> CargoResult<()> { if let Some(ch) = chars.next() { if !(unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_' || ch.is_digit(10)) { bail!( - "invalid character `{}` in feature `{}` in package {}, \ + "invalid character `{ch}` in feature `{name}` in package {pkg_id}, \ the first character must be a Unicode XID start character or digit \ (most letters or `_` or `0` to `9`)", - ch, - name, - pkg_id ); } } for ch in chars { if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-' || ch == '+' || ch == '.') { bail!( - "invalid character `{}` in feature `{}` in package {}, \ + "invalid character `{ch}` in feature `{name}` in package {pkg_id}, \ characters must be Unicode XID characters, '-', `+`, or `.` \ (numbers, `+`, `-`, `_`, `.`, or most letters)", - ch, - name, - pkg_id ); } } From a8a376cab1222c6df86a0c1ee3c6184e1775a967 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 12 Dec 2023 10:49:27 -0600 Subject: [PATCH 04/18] refactor: Allow more/less feature name locations --- src/cargo/core/summary.rs | 2 +- src/cargo/util/restricted_names.rs | 45 ++++++++++++------------------ 2 files changed, 19 insertions(+), 28 deletions(-) diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 3c6e869a802..344466ce1f9 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -192,7 +192,7 @@ fn build_feature_map( // Validate features are listed properly. for (feature, fvs) in &map { - validate_feature_name(pkg_id, feature)?; + validate_feature_name(feature, format_args!(" in package {pkg_id}"))?; for fv in fvs { // Find data for the referenced dependency... let dep_data = { diff --git a/src/cargo/util/restricted_names.rs b/src/cargo/util/restricted_names.rs index 2ff5884a4e1..ee561195a8d 100644 --- a/src/cargo/util/restricted_names.rs +++ b/src/cargo/util/restricted_names.rs @@ -1,6 +1,5 @@ //! Helpers for validating and checking names like package and crate names. -use crate::core::PackageId; use crate::util::CargoResult; use anyhow::bail; use std::path::Path; @@ -205,7 +204,7 @@ pub fn validate_profile_name(name: &str) -> CargoResult<()> { Ok(()) } -pub fn validate_feature_name(pkg_id: PackageId, name: &str) -> CargoResult<()> { +pub fn validate_feature_name(name: &str, loc: impl std::fmt::Display) -> CargoResult<()> { if name.is_empty() { bail!("feature name cannot be empty"); } @@ -220,7 +219,7 @@ pub fn validate_feature_name(pkg_id: PackageId, name: &str) -> CargoResult<()> { 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}` in package {pkg_id}, \ + "invalid character `{ch}` in feature `{name}`{loc}, \ the first character must be a Unicode XID start character or digit \ (most letters or `_` or `0` to `9`)", ); @@ -229,7 +228,7 @@ pub fn validate_feature_name(pkg_id: PackageId, name: &str) -> CargoResult<()> { for ch in chars { if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-' || ch == '+' || ch == '.') { bail!( - "invalid character `{ch}` in feature `{name}` in package {pkg_id}, \ + "invalid character `{ch}` in feature `{name}`{loc}, \ characters must be Unicode XID characters, '-', `+`, or `.` \ (numbers, `+`, `-`, `_`, `.`, or most letters)", ); @@ -241,32 +240,24 @@ pub fn validate_feature_name(pkg_id: PackageId, name: &str) -> CargoResult<()> { #[cfg(test)] mod tests { use super::*; - use crate::sources::CRATES_IO_INDEX; - use crate::util::into_url::IntoUrl; - - use crate::core::SourceId; #[test] fn valid_feature_names() { - let loc = CRATES_IO_INDEX.into_url().unwrap(); - let source_id = SourceId::for_registry(&loc).unwrap(); - let pkg_id = PackageId::try_new("foo", "1.0.0", source_id).unwrap(); - - assert!(validate_feature_name(pkg_id, "c++17").is_ok()); - assert!(validate_feature_name(pkg_id, "128bit").is_ok()); - assert!(validate_feature_name(pkg_id, "_foo").is_ok()); - assert!(validate_feature_name(pkg_id, "feat-name").is_ok()); - assert!(validate_feature_name(pkg_id, "feat_name").is_ok()); - assert!(validate_feature_name(pkg_id, "foo.bar").is_ok()); + assert!(validate_feature_name("c++17", "").is_ok()); + assert!(validate_feature_name("128bit", "").is_ok()); + assert!(validate_feature_name("_foo", "").is_ok()); + assert!(validate_feature_name("feat-name", "").is_ok()); + assert!(validate_feature_name("feat_name", "").is_ok()); + assert!(validate_feature_name("foo.bar", "").is_ok()); - assert!(validate_feature_name(pkg_id, "+foo").is_err()); - assert!(validate_feature_name(pkg_id, "-foo").is_err()); - assert!(validate_feature_name(pkg_id, ".foo").is_err()); - assert!(validate_feature_name(pkg_id, "foo:bar").is_err()); - assert!(validate_feature_name(pkg_id, "foo?").is_err()); - assert!(validate_feature_name(pkg_id, "?foo").is_err()); - assert!(validate_feature_name(pkg_id, "ⒶⒷⒸ").is_err()); - assert!(validate_feature_name(pkg_id, "a¼").is_err()); - assert!(validate_feature_name(pkg_id, "").is_err()); + assert!(validate_feature_name("+foo", "").is_err()); + assert!(validate_feature_name("-foo", "").is_err()); + assert!(validate_feature_name(".foo", "").is_err()); + assert!(validate_feature_name("foo:bar", "").is_err()); + assert!(validate_feature_name("foo?", "").is_err()); + assert!(validate_feature_name("?foo", "").is_err()); + assert!(validate_feature_name("ⒶⒷⒸ", "").is_err()); + assert!(validate_feature_name("a¼", "").is_err()); + assert!(validate_feature_name("", "").is_err()); } } From c1d9b764015403915cbe482652e41dd07c6984a9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 12 Dec 2023 11:16:00 -0600 Subject: [PATCH 05/18] fix: Improve feature name errors This moves feature name validation early enough in the process to get TOML errors. --- src/cargo/util_schemas/manifest.rs | 59 +++++++++++++++++++++++++- tests/testsuite/features.rs | 20 ++++++++- tests/testsuite/features_namespaced.rs | 4 ++ 3 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/cargo/util_schemas/manifest.rs b/src/cargo/util_schemas/manifest.rs index b0aa6393f6f..7bd88a64ea0 100644 --- a/src/cargo/util_schemas/manifest.rs +++ b/src/cargo/util_schemas/manifest.rs @@ -10,11 +10,13 @@ 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}; use serde_untagged::UntaggedEnumVisitor; +use crate::util::restricted_names; use crate::util_schemas::core::PackageIdSpec; use crate::util_semver::PartialVersion; @@ -38,7 +40,7 @@ pub struct TomlManifest { pub build_dependencies: Option>, #[serde(rename = "build_dependencies")] pub build_dependencies2: Option>, - pub features: Option>>, + pub features: Option>>, pub target: Option>, pub replace: Option>, pub patch: Option>>, @@ -69,7 +71,7 @@ impl TomlManifest { .or(self.build_dependencies2.as_ref()) } - pub fn features(&self) -> Option<&BTreeMap>> { + pub fn features(&self) -> Option<&BTreeMap>> { self.features.as_ref() } } @@ -1108,6 +1110,59 @@ impl TomlTarget { } } +#[derive(Serialize, Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[serde(transparent)] +pub struct FeatureName = String>(T); + +impl> FeatureName { + pub fn new(name: T) -> Result { + restricted_names::validate_feature_name(name.as_ref(), "")?; + Ok(Self(name)) + } + + pub fn into_inner(self) -> T { + self.0 + } +} + +impl> std::convert::AsRef for FeatureName { + fn as_ref(&self) -> &str { + self.0.as_ref() + } +} + +impl> std::ops::Deref for FeatureName { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl<'a> std::str::FromStr for FeatureName { + type Err = anyhow::Error; + + fn from_str(value: &str) -> Result { + Self::new(value.to_owned()) + } +} + +impl<'de, T: AsRef + serde::Deserialize<'de>> serde::Deserialize<'de> for FeatureName { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let inner = T::deserialize(deserializer)?; + FeatureName::new(inner).map_err(serde::de::Error::custom) + } +} + +impl> Display for FeatureName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.as_ref().fmt(f) + } +} + /// Corresponds to a `target` entry, but `TomlTarget` is already used. #[derive(Serialize, Deserialize, Debug, Clone)] #[serde(rename_all = "kebab-case")] diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs index 7086c3a037a..febdf52fed8 100644 --- a/tests/testsuite/features.rs +++ b/tests/testsuite/features.rs @@ -60,6 +60,10 @@ fn empty_feature_name() { [ERROR] failed to parse manifest at `[..]` Caused by: + TOML parse error at line 8, column 17 + | + 8 | \"\" = [] + | ^^ feature name cannot be empty ", ) @@ -2055,7 +2059,11 @@ fn invalid_feature_names_error() { error: failed to parse manifest at `[ROOT]/foo/Cargo.toml` Caused by: - invalid character `+` in feature `+foo` in package foo v0.1.0 ([ROOT]/foo), \ + TOML parse error at line 8, column 17 + | + 8 | \"+foo\" = [] + | ^^^^^^ + invalid character `+` in feature `+foo`, \ the first character must be a Unicode XID start character or digit \ (most letters or `_` or `0` to `9`) ", @@ -2082,7 +2090,11 @@ Caused by: error: failed to parse manifest at `[ROOT]/foo/Cargo.toml` Caused by: - invalid character `&` in feature `a&b` in package foo v0.1.0 ([ROOT]/foo), \ + TOML parse error at line 8, column 13 + | + 8 | \"a&b\" = [] + | ^^^^^ + invalid character `&` in feature `a&b`, \ characters must be Unicode XID characters, '-', `+`, or `.` \ (numbers, `+`, `-`, `_`, `.`, or most letters) ", @@ -2115,6 +2127,10 @@ fn invalid_feature_name_slash_error() { error: failed to parse manifest at `[CWD]/Cargo.toml` Caused by: + TOML parse error at line 7, column 17 + | + 7 | \"foo/bar\" = [] + | ^^^^^^^^^ feature named `foo/bar` is not allowed to contain slashes ", ) diff --git a/tests/testsuite/features_namespaced.rs b/tests/testsuite/features_namespaced.rs index f24186c1591..b79be55e83c 100644 --- a/tests/testsuite/features_namespaced.rs +++ b/tests/testsuite/features_namespaced.rs @@ -439,6 +439,10 @@ fn crate_syntax_bad_name() { [ERROR] failed to parse manifest at [..]/foo/Cargo.toml` Caused by: + TOML parse error at line 10, column 17 + | + 10 | \"dep:bar\" = [] + | ^^^^^^^^^ feature named `dep:bar` is not allowed to start with `dep:` ", ) From 4e1fac8ce5c05a46fec1879f611105d24e174c30 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 12 Dec 2023 15:35:16 -0600 Subject: [PATCH 06/18] refactor: Remove unused feature location parameter --- src/cargo/core/summary.rs | 5 ++--- src/cargo/util/restricted_names.rs | 36 +++++++++++++++--------------- src/cargo/util_schemas/manifest.rs | 2 +- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 344466ce1f9..27fa3be3711 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -50,7 +50,7 @@ impl Summary { ) } } - let feature_map = build_feature_map(pkg_id, features, &dependencies)?; + let feature_map = build_feature_map(features, &dependencies)?; Ok(Summary { inner: Rc::new(Inner { package_id: pkg_id, @@ -141,7 +141,6 @@ impl Hash for Summary { /// Checks features for errors, bailing out a CargoResult:Err if invalid, /// and creates FeatureValues for each feature. fn build_feature_map( - pkg_id: PackageId, features: &BTreeMap>, dependencies: &[Dependency], ) -> CargoResult { @@ -192,7 +191,7 @@ fn build_feature_map( // Validate features are listed properly. for (feature, fvs) in &map { - validate_feature_name(feature, format_args!(" in package {pkg_id}"))?; + validate_feature_name(feature)?; for fv in fvs { // Find data for the referenced dependency... let dep_data = { diff --git a/src/cargo/util/restricted_names.rs b/src/cargo/util/restricted_names.rs index ee561195a8d..8c2f2c69f65 100644 --- a/src/cargo/util/restricted_names.rs +++ b/src/cargo/util/restricted_names.rs @@ -204,7 +204,7 @@ pub fn validate_profile_name(name: &str) -> CargoResult<()> { Ok(()) } -pub fn validate_feature_name(name: &str, loc: impl std::fmt::Display) -> CargoResult<()> { +pub fn validate_feature_name(name: &str) -> CargoResult<()> { if name.is_empty() { bail!("feature name cannot be empty"); } @@ -219,7 +219,7 @@ pub fn validate_feature_name(name: &str, loc: impl std::fmt::Display) -> CargoRe 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}`{loc}, \ + "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`)", ); @@ -228,7 +228,7 @@ pub fn validate_feature_name(name: &str, loc: impl std::fmt::Display) -> CargoRe for ch in chars { if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-' || ch == '+' || ch == '.') { bail!( - "invalid character `{ch}` in feature `{name}`{loc}, \ + "invalid character `{ch}` in feature `{name}`, \ characters must be Unicode XID characters, '-', `+`, or `.` \ (numbers, `+`, `-`, `_`, `.`, or most letters)", ); @@ -243,21 +243,21 @@ mod tests { #[test] fn valid_feature_names() { - assert!(validate_feature_name("c++17", "").is_ok()); - assert!(validate_feature_name("128bit", "").is_ok()); - assert!(validate_feature_name("_foo", "").is_ok()); - assert!(validate_feature_name("feat-name", "").is_ok()); - assert!(validate_feature_name("feat_name", "").is_ok()); - assert!(validate_feature_name("foo.bar", "").is_ok()); + assert!(validate_feature_name("c++17").is_ok()); + assert!(validate_feature_name("128bit").is_ok()); + assert!(validate_feature_name("_foo").is_ok()); + assert!(validate_feature_name("feat-name").is_ok()); + assert!(validate_feature_name("feat_name").is_ok()); + assert!(validate_feature_name("foo.bar").is_ok()); - assert!(validate_feature_name("+foo", "").is_err()); - assert!(validate_feature_name("-foo", "").is_err()); - assert!(validate_feature_name(".foo", "").is_err()); - assert!(validate_feature_name("foo:bar", "").is_err()); - assert!(validate_feature_name("foo?", "").is_err()); - assert!(validate_feature_name("?foo", "").is_err()); - assert!(validate_feature_name("ⒶⒷⒸ", "").is_err()); - assert!(validate_feature_name("a¼", "").is_err()); - assert!(validate_feature_name("", "").is_err()); + assert!(validate_feature_name("+foo").is_err()); + assert!(validate_feature_name("-foo").is_err()); + assert!(validate_feature_name(".foo").is_err()); + assert!(validate_feature_name("foo:bar").is_err()); + assert!(validate_feature_name("foo?").is_err()); + assert!(validate_feature_name("?foo").is_err()); + assert!(validate_feature_name("ⒶⒷⒸ").is_err()); + assert!(validate_feature_name("a¼").is_err()); + assert!(validate_feature_name("").is_err()); } } diff --git a/src/cargo/util_schemas/manifest.rs b/src/cargo/util_schemas/manifest.rs index 7bd88a64ea0..1cccf369699 100644 --- a/src/cargo/util_schemas/manifest.rs +++ b/src/cargo/util_schemas/manifest.rs @@ -1116,7 +1116,7 @@ pub struct FeatureName = String>(T); impl> FeatureName { pub fn new(name: T) -> Result { - restricted_names::validate_feature_name(name.as_ref(), "")?; + restricted_names::validate_feature_name(name.as_ref())?; Ok(Self(name)) } From 586b0106634b6f6ddf3bcb07cde74fe150326b70 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 12 Dec 2023 15:37:18 -0600 Subject: [PATCH 07/18] refactor: Do all feature name validation through FeatureName --- src/cargo/core/summary.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 27fa3be3711..2137d633250 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -1,7 +1,7 @@ use crate::core::{Dependency, PackageId, SourceId}; use crate::util::interning::InternedString; -use crate::util::restricted_names::validate_feature_name; use crate::util::CargoResult; +use crate::util_schemas::manifest::FeatureName; use crate::util_schemas::manifest::RustVersion; use anyhow::bail; use semver::Version; @@ -191,7 +191,7 @@ fn build_feature_map( // Validate features are listed properly. for (feature, fvs) in &map { - validate_feature_name(feature)?; + FeatureName::new(feature)?; for fv in fvs { // Find data for the referenced dependency... let dep_data = { From a3976cd47cc7f8e640da1a1e9321b6e9fd778542 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 12 Dec 2023 15:55:09 -0600 Subject: [PATCH 08/18] fix: Improve profile name errors --- src/cargo/util/toml/mod.rs | 4 -- src/cargo/util_schemas/manifest.rs | 69 +++++++++++++++++++++++++++++- tests/testsuite/profile_custom.rs | 16 ++++++- 3 files changed, 81 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 9439ca9179e..25c7d3bd9c0 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -23,7 +23,6 @@ use crate::core::{GitReference, PackageIdSpec, SourceId, WorkspaceConfig, Worksp use crate::sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY}; use crate::util::errors::{CargoResult, ManifestError}; use crate::util::interning::InternedString; -use crate::util::restricted_names; use crate::util::{ self, config::ConfigRelativePath, validate_package_name, Config, IntoUrl, OptVersionReq, }; @@ -2020,9 +2019,6 @@ pub fn validate_profile( } } - // Profile name validation - restricted_names::validate_profile_name(name)?; - if let Some(dir_name) = &root.dir_name { // This is disabled for now, as we would like to stabilize named // profiles without this, and then decide in the future if it is diff --git a/src/cargo/util_schemas/manifest.rs b/src/cargo/util_schemas/manifest.rs index 1cccf369699..826be15809c 100644 --- a/src/cargo/util_schemas/manifest.rs +++ b/src/cargo/util_schemas/manifest.rs @@ -641,10 +641,10 @@ impl Default for TomlDetailedDependency

{ } #[derive(Deserialize, Serialize, Clone, Debug, Default)] -pub struct TomlProfiles(pub BTreeMap); +pub struct TomlProfiles(pub BTreeMap); impl TomlProfiles { - pub fn get_all(&self) -> &BTreeMap { + pub fn get_all(&self) -> &BTreeMap { &self.0 } @@ -1110,6 +1110,65 @@ impl TomlTarget { } } +#[derive(Serialize, Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[serde(transparent)] +pub struct ProfileName = String>(T); + +impl> ProfileName { + pub fn new(name: T) -> Result { + restricted_names::validate_profile_name(name.as_ref())?; + Ok(Self(name)) + } + + pub fn into_inner(self) -> T { + self.0 + } +} + +impl> std::convert::AsRef for ProfileName { + fn as_ref(&self) -> &str { + self.0.as_ref() + } +} + +impl> std::ops::Deref for ProfileName { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl> std::borrow::Borrow for ProfileName { + fn borrow(&self) -> &str { + self.0.as_ref() + } +} + +impl<'a> std::str::FromStr for ProfileName { + type Err = anyhow::Error; + + fn from_str(value: &str) -> Result { + Self::new(value.to_owned()) + } +} + +impl<'de, T: AsRef + serde::Deserialize<'de>> serde::Deserialize<'de> for ProfileName { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let inner = T::deserialize(deserializer)?; + ProfileName::new(inner).map_err(serde::de::Error::custom) + } +} + +impl> Display for ProfileName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.as_ref().fmt(f) + } +} + #[derive(Serialize, Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] #[serde(transparent)] pub struct FeatureName = String>(T); @@ -1139,6 +1198,12 @@ impl> std::ops::Deref for FeatureName { } } +impl> std::borrow::Borrow for FeatureName { + fn borrow(&self) -> &str { + self.0.as_ref() + } +} + impl<'a> std::str::FromStr for FeatureName { type Err = anyhow::Error; diff --git a/tests/testsuite/profile_custom.rs b/tests/testsuite/profile_custom.rs index f7139e55267..cf9828d3794 100644 --- a/tests/testsuite/profile_custom.rs +++ b/tests/testsuite/profile_custom.rs @@ -86,6 +86,10 @@ fn invalid_profile_name() { [ERROR] failed to parse manifest at [..] Caused by: + TOML parse error at line 7, column 26 + | + 7 | [profile.'.release-lto'] + | ^^^^^^^^^^^^^^ invalid character `.` in profile name `.release-lto` Allowed characters are letters, numbers, underscore, and hyphen. ", @@ -626,6 +630,7 @@ See https://doc.rust-lang.org/cargo/reference/profiles.html for more on configur ), ); + let highlight = "^".repeat(name.len()); p.cargo("build") .with_status(101) .with_stderr(&format!( @@ -633,11 +638,14 @@ See https://doc.rust-lang.org/cargo/reference/profiles.html for more on configur error: failed to parse manifest at `[ROOT]/foo/Cargo.toml` Caused by: - profile name `{}` is reserved + TOML parse error at line 6, column 30 + | + 6 | [profile.{name}] + | {highlight} + profile name `{name}` is reserved Please choose a different name. See https://doc.rust-lang.org/cargo/reference/profiles.html for more on configuring profiles. ", - name )) .run(); } @@ -663,6 +671,10 @@ Caused by: error: failed to parse manifest at `[ROOT]/foo/Cargo.toml` Caused by: + TOML parse error at line 7, column 25 + | + 7 | [profile.debug] + | ^^^^^ profile name `debug` is reserved To configure the default development profile, use the name `dev` as in [profile.dev] See https://doc.rust-lang.org/cargo/reference/profiles.html for more on configuring profiles. From 847804d566dd83eebf0188903c5faf141653fdc3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 12 Dec 2023 16:40:33 -0600 Subject: [PATCH 09/18] refactor(cli): Validate via ProfileName I was going to have clap use `ProfileName` but the `cargo rustc --profile` (yes that specific) accepts `check` as a profile name and we convert that to `dev` later in the process, making that not work. --- src/cargo/util/command_prelude.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cargo/util/command_prelude.rs b/src/cargo/util/command_prelude.rs index 3e236a6f735..8b76f436a9b 100644 --- a/src/cargo/util/command_prelude.rs +++ b/src/cargo/util/command_prelude.rs @@ -11,6 +11,7 @@ use crate::util::{ print_available_benches, print_available_binaries, print_available_examples, print_available_packages, print_available_tests, }; +use crate::util_schemas::manifest::ProfileName; use crate::util_schemas::manifest::StringOrVec; use crate::CargoResult; use anyhow::bail; @@ -605,7 +606,7 @@ Run `{cmd}` to see possible targets." bail!("profile `doc` is reserved and not allowed to be explicitly specified") } (_, _, Some(name)) => { - restricted_names::validate_profile_name(name)?; + ProfileName::new(name)?; name } }; From 00bde96c18888bfd72453cd81ae29b3241689823 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 13 Dec 2023 11:05:57 -0600 Subject: [PATCH 10/18] refactor(schema): Reduce str newtype duplication --- src/cargo/util_schemas/manifest.rs | 150 +++++++++++------------------ 1 file changed, 54 insertions(+), 96 deletions(-) diff --git a/src/cargo/util_schemas/manifest.rs b/src/cargo/util_schemas/manifest.rs index 826be15809c..e1f1584066c 100644 --- a/src/cargo/util_schemas/manifest.rs +++ b/src/cargo/util_schemas/manifest.rs @@ -1110,122 +1110,80 @@ impl TomlTarget { } } -#[derive(Serialize, Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -#[serde(transparent)] -pub struct ProfileName = String>(T); +macro_rules! str_newtype { + ($name:ident) => { + #[derive(Serialize, Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] + #[serde(transparent)] + pub struct $name = String>(T); + + impl> $name { + pub fn into_inner(self) -> T { + self.0 + } + } -impl> ProfileName { - pub fn new(name: T) -> Result { - restricted_names::validate_profile_name(name.as_ref())?; - Ok(Self(name)) - } + impl> std::convert::AsRef for $name { + fn as_ref(&self) -> &str { + self.0.as_ref() + } + } - pub fn into_inner(self) -> T { - self.0 - } -} + impl> std::ops::Deref for $name { + type Target = T; -impl> std::convert::AsRef for ProfileName { - fn as_ref(&self) -> &str { - self.0.as_ref() - } -} + fn deref(&self) -> &Self::Target { + &self.0 + } + } -impl> std::ops::Deref for ProfileName { - type Target = T; + impl> std::borrow::Borrow for $name { + fn borrow(&self) -> &str { + self.0.as_ref() + } + } - fn deref(&self) -> &Self::Target { - &self.0 - } -} + impl<'a> std::str::FromStr for $name { + type Err = anyhow::Error; -impl> std::borrow::Borrow for ProfileName { - fn borrow(&self) -> &str { - self.0.as_ref() - } -} + fn from_str(value: &str) -> Result { + Self::new(value.to_owned()) + } + } -impl<'a> std::str::FromStr for ProfileName { - type Err = anyhow::Error; + impl<'de, T: AsRef + serde::Deserialize<'de>> serde::Deserialize<'de> for $name { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let inner = T::deserialize(deserializer)?; + Self::new(inner).map_err(serde::de::Error::custom) + } + } - fn from_str(value: &str) -> Result { - Self::new(value.to_owned()) - } + impl> Display for $name { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.as_ref().fmt(f) + } + } + }; } -impl<'de, T: AsRef + serde::Deserialize<'de>> serde::Deserialize<'de> for ProfileName { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let inner = T::deserialize(deserializer)?; - ProfileName::new(inner).map_err(serde::de::Error::custom) - } -} +str_newtype!(ProfileName); -impl> Display for ProfileName { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.0.as_ref().fmt(f) +impl> ProfileName { + pub fn new(name: T) -> Result { + restricted_names::validate_profile_name(name.as_ref())?; + Ok(Self(name)) } } -#[derive(Serialize, Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -#[serde(transparent)] -pub struct FeatureName = String>(T); +str_newtype!(FeatureName); impl> FeatureName { pub fn new(name: T) -> Result { restricted_names::validate_feature_name(name.as_ref())?; Ok(Self(name)) } - - pub fn into_inner(self) -> T { - self.0 - } -} - -impl> std::convert::AsRef for FeatureName { - fn as_ref(&self) -> &str { - self.0.as_ref() - } -} - -impl> std::ops::Deref for FeatureName { - type Target = T; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl> std::borrow::Borrow for FeatureName { - fn borrow(&self) -> &str { - self.0.as_ref() - } -} - -impl<'a> std::str::FromStr for FeatureName { - type Err = anyhow::Error; - - fn from_str(value: &str) -> Result { - Self::new(value.to_owned()) - } -} - -impl<'de, T: AsRef + serde::Deserialize<'de>> serde::Deserialize<'de> for FeatureName { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let inner = T::deserialize(deserializer)?; - FeatureName::new(inner).map_err(serde::de::Error::custom) - } -} - -impl> Display for FeatureName { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.0.as_ref().fmt(f) - } } /// Corresponds to a `target` entry, but `TomlTarget` is already used. From 6b6eb067148aa8758e96c55e6d084aa12da78f5c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 13 Dec 2023 11:27:02 -0600 Subject: [PATCH 11/18] fix: Improve package name errors --- src/cargo/util/toml/mod.rs | 21 ++++++-------- src/cargo/util/toml/targets.rs | 2 +- src/cargo/util_schemas/manifest.rs | 45 ++++++++++++++++++------------ tests/testsuite/build.rs | 14 +++++++++- 4 files changed, 50 insertions(+), 32 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 25c7d3bd9c0..8affc69a44b 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -23,9 +23,7 @@ use crate::core::{GitReference, PackageIdSpec, SourceId, WorkspaceConfig, Worksp use crate::sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY}; use crate::util::errors::{CargoResult, ManifestError}; use crate::util::interning::InternedString; -use crate::util::{ - self, config::ConfigRelativePath, validate_package_name, Config, IntoUrl, OptVersionReq, -}; +use crate::util::{self, config::ConfigRelativePath, Config, IntoUrl, OptVersionReq}; use crate::util_schemas::manifest; use crate::util_schemas::manifest::RustVersion; @@ -309,9 +307,9 @@ pub fn prepare_for_publish( fn map_deps( config: &Config, - deps: Option<&BTreeMap>, + deps: Option<&BTreeMap>, filter: impl Fn(&manifest::TomlDependency) -> bool, - ) -> CargoResult>> { + ) -> CargoResult>> { let Some(deps) = deps else { return Ok(None) }; let deps = deps .iter() @@ -479,7 +477,6 @@ pub fn to_real_manifest( }; let package_name = package.name.trim(); - validate_package_name(package_name, "package name", "")?; let resolved_path = package_root.join("Cargo.toml"); @@ -627,11 +624,11 @@ pub fn to_real_manifest( fn process_dependencies( cx: &mut Context<'_, '_>, - new_deps: Option<&BTreeMap>, + new_deps: Option<&BTreeMap>, kind: Option, workspace_config: &WorkspaceConfig, inherit_cell: &LazyCell, - ) -> CargoResult>> { + ) -> CargoResult>> { let Some(dependencies) = new_deps else { return Ok(None); }; @@ -642,12 +639,12 @@ pub fn to_real_manifest( }) }; - let mut deps: BTreeMap = BTreeMap::new(); + let mut deps: BTreeMap = + BTreeMap::new(); for (n, v) in dependencies.iter() { let resolved = dependency_inherit_with(v.clone(), n, inheritable, cx)?; let dep = dep_to_dependency(&resolved, n, cx, kind)?; let name_in_toml = dep.name_in_toml().as_str(); - validate_package_name(name_in_toml, "dependency name", "")?; let kind_name = match kind { Some(k) => k.kind_table(), None => "dependencies", @@ -660,7 +657,7 @@ pub fn to_real_manifest( unused_dep_keys(name_in_toml, &table_in_toml, v.unused_keys(), cx.warnings); cx.deps.push(dep); deps.insert( - n.to_string(), + n.clone(), manifest::InheritableDependency::Value(resolved.clone()), ); } @@ -1466,7 +1463,7 @@ macro_rules! package_field_getter { #[derive(Clone, Debug, Default)] pub struct InheritableFields { package: Option, - dependencies: Option>, + dependencies: Option>, lints: Option, // Bookkeeping to help when resolving values from above diff --git a/src/cargo/util/toml/targets.rs b/src/cargo/util/toml/targets.rs index 3659fd74c37..7d0f1891e15 100644 --- a/src/cargo/util/toml/targets.rs +++ b/src/cargo/util/toml/targets.rs @@ -127,7 +127,7 @@ pub(super) fn targets( // Verify names match available build deps. let bdeps = manifest.build_dependencies.as_ref(); for name in &metabuild.0 { - if !bdeps.map_or(false, |bd| bd.contains_key(name)) { + if !bdeps.map_or(false, |bd| bd.contains_key(name.as_str())) { anyhow::bail!( "metabuild package `{}` must be specified in `build-dependencies`", name diff --git a/src/cargo/util_schemas/manifest.rs b/src/cargo/util_schemas/manifest.rs index e1f1584066c..dac50345ca1 100644 --- a/src/cargo/util_schemas/manifest.rs +++ b/src/cargo/util_schemas/manifest.rs @@ -33,17 +33,17 @@ pub struct TomlManifest { pub example: Option>, pub test: Option>, pub bench: Option>, - pub dependencies: Option>, - pub dev_dependencies: Option>, + pub dependencies: Option>, + pub dev_dependencies: Option>, #[serde(rename = "dev_dependencies")] - pub dev_dependencies2: Option>, - pub build_dependencies: Option>, + pub dev_dependencies2: Option>, + pub build_dependencies: Option>, #[serde(rename = "build_dependencies")] - pub build_dependencies2: Option>, + pub build_dependencies2: Option>, pub features: Option>>, pub target: Option>, pub replace: Option>, - pub patch: Option>>, + pub patch: Option>>, pub workspace: Option, pub badges: Option, pub lints: Option, @@ -59,13 +59,13 @@ impl TomlManifest { self.package.as_ref().or(self.project.as_ref()) } - pub fn dev_dependencies(&self) -> Option<&BTreeMap> { + pub fn dev_dependencies(&self) -> Option<&BTreeMap> { self.dev_dependencies .as_ref() .or(self.dev_dependencies2.as_ref()) } - pub fn build_dependencies(&self) -> Option<&BTreeMap> { + pub fn build_dependencies(&self) -> Option<&BTreeMap> { self.build_dependencies .as_ref() .or(self.build_dependencies2.as_ref()) @@ -87,7 +87,7 @@ pub struct TomlWorkspace { // Properties that can be inherited by members. pub package: Option, - pub dependencies: Option>, + pub dependencies: Option>, pub lints: Option, } @@ -125,7 +125,7 @@ pub struct InheritablePackage { pub struct TomlPackage { pub edition: Option, pub rust_version: Option, - pub name: String, + pub name: PackageName, pub version: Option, pub authors: Option, pub build: Option, @@ -592,7 +592,7 @@ pub struct TomlDetailedDependency { pub default_features: Option, #[serde(rename = "default_features")] pub default_features2: Option, - pub package: Option, + pub package: Option, pub public: Option, /// One or more of `bin`, `cdylib`, `staticlib`, `bin:`. @@ -1168,6 +1168,15 @@ macro_rules! str_newtype { }; } +str_newtype!(PackageName); + +impl> PackageName { + pub fn new(name: T) -> Result { + restricted_names::validate_package_name(name.as_ref(), "package name", "")?; + Ok(Self(name)) + } +} + str_newtype!(ProfileName); impl> ProfileName { @@ -1190,23 +1199,23 @@ impl> FeatureName { #[derive(Serialize, Deserialize, Debug, Clone)] #[serde(rename_all = "kebab-case")] pub struct TomlPlatform { - pub dependencies: Option>, - pub build_dependencies: Option>, + pub dependencies: Option>, + pub build_dependencies: Option>, #[serde(rename = "build_dependencies")] - pub build_dependencies2: Option>, - pub dev_dependencies: Option>, + pub build_dependencies2: Option>, + pub dev_dependencies: Option>, #[serde(rename = "dev_dependencies")] - pub dev_dependencies2: Option>, + pub dev_dependencies2: Option>, } impl TomlPlatform { - pub fn dev_dependencies(&self) -> Option<&BTreeMap> { + pub fn dev_dependencies(&self) -> Option<&BTreeMap> { self.dev_dependencies .as_ref() .or(self.dev_dependencies2.as_ref()) } - pub fn build_dependencies(&self) -> Option<&BTreeMap> { + pub fn build_dependencies(&self) -> Option<&BTreeMap> { self.build_dependencies .as_ref() .or(self.build_dependencies2.as_ref()) diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 13553671e3c..dd67161d6b1 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -460,6 +460,10 @@ fn cargo_compile_with_empty_package_name() { [ERROR] failed to parse manifest at `[..]` Caused by: + TOML parse error at line 3, column 16 + | + 3 | name = \"\" + | ^^ package name cannot be empty ", ) @@ -479,6 +483,10 @@ fn cargo_compile_with_invalid_package_name() { [ERROR] failed to parse manifest at `[..]` Caused by: + TOML parse error at line 3, column 16 + | + 3 | name = \"foo::bar\" + | ^^^^^^^^^^ invalid character `:` in package name: `foo::bar`, [..] ", ) @@ -1182,7 +1190,11 @@ fn cargo_compile_with_invalid_dep_rename() { error: failed to parse manifest at `[..]` Caused by: - invalid character ` ` in dependency name: `haha this isn't a valid name 🐛`, characters must be Unicode XID characters (numbers, `-`, `_`, or most letters) + TOML parse error at line 7, column 17 + | + 7 | \"haha this isn't a valid name 🐛\" = { package = \"libc\", version = \"0.1\" } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + invalid character ` ` in package name: `haha this isn't a valid name 🐛`, characters must be Unicode XID characters (numbers, `-`, `_`, or most letters) ", ) .run(); From db2e31407721bdc3d7c279f6c83b748ff5f09740 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 13 Dec 2023 11:37:08 -0600 Subject: [PATCH 12/18] fix: Improve registry name errors? Because of workspace inheritance, the errors aren't the greatest --- src/cargo/util_schemas/manifest.rs | 11 ++++++++++- tests/testsuite/alt_registry.rs | 18 ++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/cargo/util_schemas/manifest.rs b/src/cargo/util_schemas/manifest.rs index dac50345ca1..171c4136d5d 100644 --- a/src/cargo/util_schemas/manifest.rs +++ b/src/cargo/util_schemas/manifest.rs @@ -572,7 +572,7 @@ impl<'de, P: Deserialize<'de> + Clone> de::Deserialize<'de> for TomlDependency

{ pub version: Option, - pub registry: Option, + pub registry: Option, /// The URL of the `registry` field. /// This is an internal implementation detail. When Cargo creates a /// package, it replaces `registry` with `registry-index` so that the @@ -1177,6 +1177,15 @@ impl> PackageName { } } +str_newtype!(RegistryName); + +impl> RegistryName { + pub fn new(name: T) -> Result { + restricted_names::validate_package_name(name.as_ref(), "registry name", "")?; + Ok(Self(name)) + } +} + str_newtype!(ProfileName); impl> ProfileName { diff --git a/tests/testsuite/alt_registry.rs b/tests/testsuite/alt_registry.rs index 8982b1941a7..e347af1c708 100644 --- a/tests/testsuite/alt_registry.rs +++ b/tests/testsuite/alt_registry.rs @@ -715,7 +715,14 @@ fn bad_registry_name() { [ERROR] failed to parse manifest at `[CWD]/Cargo.toml` Caused by: - invalid character ` ` in registry name: `bad name`, [..]", + TOML parse error at line 7, column 17 + | + 7 | [dependencies.bar] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + invalid character ` ` in registry name: `bad name`, [..] + + +", ) .run(); @@ -1618,7 +1625,14 @@ fn empty_dependency_registry() { [ERROR] failed to parse manifest at `[CWD]/Cargo.toml` Caused by: - registry name cannot be empty", + TOML parse error at line 7, column 23 + | + 7 | bar = { version = \"0.1.0\", registry = \"\" } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + registry name cannot be empty + + +", ) .run(); } From 1da305301cbddbb9d4ec0579738c550df3a5dd28 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 13 Dec 2023 13:03:27 -0600 Subject: [PATCH 13/18] refactor: Validate via RegistryName/PackageName --- src/cargo/ops/cargo_add/crate_spec.rs | 4 ++-- src/cargo/ops/cargo_new.rs | 8 ++++++-- src/cargo/util/command_prelude.rs | 5 +++-- src/cargo/util/config/mod.rs | 5 +++-- src/cargo/util_schemas/core/package_id_spec.rs | 6 +++--- tests/testsuite/cargo_add/empty_dep_name/stderr.log | 2 +- 6 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/cargo/ops/cargo_add/crate_spec.rs b/src/cargo/ops/cargo_add/crate_spec.rs index 4cf6f484a32..65c58314f65 100644 --- a/src/cargo/ops/cargo_add/crate_spec.rs +++ b/src/cargo/ops/cargo_add/crate_spec.rs @@ -4,7 +4,7 @@ use anyhow::Context as _; use super::Dependency; use crate::util::toml_mut::dependency::RegistrySource; -use crate::util::validate_package_name; +use crate::util_schemas::manifest::PackageName; use crate::CargoResult; /// User-specified crate @@ -28,7 +28,7 @@ impl CrateSpec { .map(|(n, v)| (n, Some(v))) .unwrap_or((pkg_id, None)); - validate_package_name(name, "dependency name", "")?; + PackageName::new(name)?; if let Some(version) = version { semver::VersionReq::parse(version) diff --git a/src/cargo/ops/cargo_new.rs b/src/cargo/ops/cargo_new.rs index 1c06b5f8258..57c7e268e3a 100644 --- a/src/cargo/ops/cargo_new.rs +++ b/src/cargo/ops/cargo_new.rs @@ -4,6 +4,7 @@ use crate::util::important_paths::find_root_manifest_for_wd; use crate::util::toml_mut::is_sorted; use crate::util::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo}; use crate::util::{restricted_names, Config}; +use crate::util_schemas::manifest::PackageName; use anyhow::{anyhow, Context}; use cargo_util::paths::{self, write_atomic}; use serde::de; @@ -180,7 +181,7 @@ fn check_name( }; let bin_help = || { let mut help = String::from(name_help); - if has_bin { + if has_bin && !name.is_empty() { help.push_str(&format!( "\n\ If you need a binary with the name \"{name}\", use a valid package \ @@ -197,7 +198,10 @@ fn check_name( } help }; - restricted_names::validate_package_name(name, "package name", &bin_help())?; + PackageName::new(name).map_err(|err| { + let help = bin_help(); + anyhow::anyhow!("{err}{help}") + })?; if restricted_names::is_keyword(name) { anyhow::bail!( diff --git a/src/cargo/util/command_prelude.rs b/src/cargo/util/command_prelude.rs index 8b76f436a9b..24c8c098dde 100644 --- a/src/cargo/util/command_prelude.rs +++ b/src/cargo/util/command_prelude.rs @@ -12,6 +12,7 @@ use crate::util::{ print_available_packages, print_available_tests, }; use crate::util_schemas::manifest::ProfileName; +use crate::util_schemas::manifest::RegistryName; use crate::util_schemas::manifest::StringOrVec; use crate::CargoResult; use anyhow::bail; @@ -834,7 +835,7 @@ Run `{cmd}` to see possible targets." (None, None) => config.default_registry()?.map(RegistryOrIndex::Registry), (None, Some(i)) => Some(RegistryOrIndex::Index(i.into_url()?)), (Some(r), None) => { - restricted_names::validate_package_name(r, "registry name", "")?; + RegistryName::new(r)?; Some(RegistryOrIndex::Registry(r.to_string())) } (Some(_), Some(_)) => { @@ -849,7 +850,7 @@ Run `{cmd}` to see possible targets." match self._value_of("registry").map(|s| s.to_string()) { None => config.default_registry(), Some(registry) => { - restricted_names::validate_package_name(®istry, "registry name", "")?; + RegistryName::new(®istry)?; Ok(Some(registry)) } } diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index c0dd42d39b8..1c1b949a783 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -77,9 +77,10 @@ use crate::sources::CRATES_IO_REGISTRY; use crate::util::errors::CargoResult; use crate::util::network::http::configure_http_handle; use crate::util::network::http::http_handle; +use crate::util::try_canonicalize; use crate::util::{internal, CanonicalUrl}; -use crate::util::{try_canonicalize, validate_package_name}; use crate::util::{Filesystem, IntoUrl, IntoUrlWithBase, Rustc}; +use crate::util_schemas::manifest::RegistryName; use anyhow::{anyhow, bail, format_err, Context as _}; use cargo_credential::Secret; use cargo_util::paths; @@ -1551,7 +1552,7 @@ impl Config { /// Gets the index for a registry. pub fn get_registry_index(&self, registry: &str) -> CargoResult { - validate_package_name(registry, "registry name", "")?; + RegistryName::new(registry)?; if let Some(index) = self.get_string(&format!("registries.{}.index", registry))? { self.resolve_registry_index(&index).with_context(|| { format!( diff --git a/src/cargo/util_schemas/core/package_id_spec.rs b/src/cargo/util_schemas/core/package_id_spec.rs index 07d1fb5ee9b..015bfa928d7 100644 --- a/src/cargo/util_schemas/core/package_id_spec.rs +++ b/src/cargo/util_schemas/core/package_id_spec.rs @@ -6,9 +6,9 @@ use semver::Version; use serde::{de, ser}; use url::Url; -use crate::util::validate_package_name; use crate::util_schemas::core::GitReference; use crate::util_schemas::core::SourceKind; +use crate::util_schemas::manifest::PackageName; use crate::util_semver::PartialVersion; /// Some or all of the data required to identify a package: @@ -97,7 +97,7 @@ impl PackageIdSpec { Some(version) => Some(version.parse::()?), None => None, }; - validate_package_name(name, "pkgid", "")?; + PackageName::new(name)?; Ok(PackageIdSpec { name: String::from(name), version, @@ -182,7 +182,7 @@ impl PackageIdSpec { None => (String::from(path_name), None), } }; - validate_package_name(name.as_str(), "pkgid", "")?; + PackageName::new(&name)?; Ok(PackageIdSpec { name, version, diff --git a/tests/testsuite/cargo_add/empty_dep_name/stderr.log b/tests/testsuite/cargo_add/empty_dep_name/stderr.log index 1bb057cd14d..d9547a42acf 100644 --- a/tests/testsuite/cargo_add/empty_dep_name/stderr.log +++ b/tests/testsuite/cargo_add/empty_dep_name/stderr.log @@ -1 +1 @@ -error: dependency name cannot be empty +error: package name cannot be empty From 8e68a0a8ba6cb8bb4cb0b91e3294de71babc571a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 13 Dec 2023 13:19:49 -0600 Subject: [PATCH 14/18] refactor: Sanitize via PackageName --- src/cargo/util/toml/embedded.rs | 3 ++- src/cargo/util_schemas/manifest.rs | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/cargo/util/toml/embedded.rs b/src/cargo/util/toml/embedded.rs index 4c57195d4f4..cad7abc38fa 100644 --- a/src/cargo/util/toml/embedded.rs +++ b/src/cargo/util/toml/embedded.rs @@ -1,6 +1,7 @@ use anyhow::Context as _; use crate::util::restricted_names; +use crate::util_schemas::manifest::PackageName; use crate::CargoResult; use crate::Config; @@ -171,7 +172,7 @@ fn sanitize_name(name: &str) -> String { '-' }; - let mut name = restricted_names::sanitize_package_name(name, placeholder); + let mut name = PackageName::sanitize(name, placeholder).into_inner(); loop { if restricted_names::is_keyword(&name) { diff --git a/src/cargo/util_schemas/manifest.rs b/src/cargo/util_schemas/manifest.rs index 171c4136d5d..232448f11ae 100644 --- a/src/cargo/util_schemas/manifest.rs +++ b/src/cargo/util_schemas/manifest.rs @@ -1177,6 +1177,15 @@ impl> PackageName { } } +impl PackageName { + pub fn sanitize(name: impl AsRef, placeholder: char) -> Self { + PackageName(restricted_names::sanitize_package_name( + name.as_ref(), + placeholder, + )) + } +} + str_newtype!(RegistryName); impl> RegistryName { From d9a6043e8ab475e0ccfd78010f9d0ef2d922483b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 13 Dec 2023 13:26:06 -0600 Subject: [PATCH 15/18] refactor: Move validation into util_schemas --- src/cargo/util/mod.rs | 1 - src/cargo/util/restricted_names.rs | 213 -------------------- src/cargo/util_schemas/manifest.rs | 2 +- src/cargo/util_schemas/mod.rs | 2 + src/cargo/util_schemas/restricted_names.rs | 215 +++++++++++++++++++++ 5 files changed, 218 insertions(+), 215 deletions(-) create mode 100644 src/cargo/util_schemas/restricted_names.rs diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index 16c15770fe4..e5ecd077fd7 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -21,7 +21,6 @@ pub(crate) use self::io::LimitErrorReader; pub use self::lockserver::{LockServer, LockServerClient, LockServerStarted}; pub use self::progress::{Progress, ProgressStyle}; pub use self::queue::Queue; -pub use self::restricted_names::validate_package_name; pub use self::rustc::Rustc; pub use self::semver_ext::OptVersionReq; pub use self::vcs::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo}; diff --git a/src/cargo/util/restricted_names.rs b/src/cargo/util/restricted_names.rs index 8c2f2c69f65..f047b0d6656 100644 --- a/src/cargo/util/restricted_names.rs +++ b/src/cargo/util/restricted_names.rs @@ -1,7 +1,5 @@ //! Helpers for validating and checking names like package and crate names. -use crate::util::CargoResult; -use anyhow::bail; use std::path::Path; /// Returns `true` if the name contains non-ASCII characters. @@ -36,80 +34,6 @@ pub fn is_conflicting_artifact_name(name: &str) -> bool { ["deps", "examples", "build", "incremental"].contains(&name) } -/// Check the base requirements for a package name. -/// -/// This can be used for other things than package names, to enforce some -/// 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) -> CargoResult<()> { - if name.is_empty() { - bail!("{what} cannot be empty"); - } - - 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, - what, - help - ); - } - 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 `_`){}", - ch, - what, - name, - help - ); - } - } - 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){}", - ch, - what, - name, - help - ); - } - } - Ok(()) -} - -/// Ensure a package name is [valid][validate_package_name] -pub 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() { - if (unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_') && !ch.is_digit(10) { - slug.push(ch); - break; - } - } - while let Some(ch) = chars.next() { - if unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-' { - slug.push(ch); - } else { - slug.push(placeholder); - } - } - if slug.is_empty() { - slug.push_str("package"); - } - slug -} - /// Check the entire path for names reserved in Windows. pub fn is_windows_reserved_path(path: &Path) -> bool { path.iter() @@ -124,140 +48,3 @@ pub fn is_windows_reserved_path(path: &Path) -> bool { pub fn is_glob_pattern>(name: T) -> bool { name.as_ref().contains(&['*', '?', '[', ']'][..]) } - -/// Validate dir-names and profile names according to RFC 2678. -pub fn validate_profile_name(name: &str) -> CargoResult<()> { - if let Some(ch) = name - .chars() - .find(|ch| !ch.is_alphanumeric() && *ch != '_' && *ch != '-') - { - bail!( - "invalid character `{}` in profile name `{}`\n\ - Allowed characters are letters, numbers, underscore, and hyphen.", - ch, - name - ); - } - - 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 - ); - } - 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 - ); - } - - // These are some arbitrary reservations. We have no plans to use - // these, but it seems safer to reserve a few just in case we want to - // add more built-in profiles in the future. We can also uses special - // syntax like cargo:foo if needed. But it is unlikely these will ever - // be used. - if matches!( - lower_name.as_str(), - "build" - | "check" - | "clean" - | "config" - | "fetch" - | "fix" - | "install" - | "metadata" - | "package" - | "publish" - | "report" - | "root" - | "run" - | "rust" - | "rustc" - | "rustdoc" - | "target" - | "tmp" - | "uninstall" - ) || lower_name.starts_with("cargo") - { - bail!( - "profile name `{}` is reserved\n\ - Please choose a different name.\n\ - {}", - name, - SEE_DOCS - ); - } - - Ok(()) -} - -pub fn validate_feature_name(name: &str) -> CargoResult<()> { - if name.is_empty() { - bail!("feature name cannot be empty"); - } - - if name.starts_with("dep:") { - bail!("feature named `{name}` is not allowed to start with `dep:`",); - } - if name.contains('/') { - bail!("feature named `{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`)", - ); - } - } - 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)", - ); - } - } - Ok(()) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn valid_feature_names() { - assert!(validate_feature_name("c++17").is_ok()); - assert!(validate_feature_name("128bit").is_ok()); - assert!(validate_feature_name("_foo").is_ok()); - assert!(validate_feature_name("feat-name").is_ok()); - assert!(validate_feature_name("feat_name").is_ok()); - assert!(validate_feature_name("foo.bar").is_ok()); - - assert!(validate_feature_name("+foo").is_err()); - assert!(validate_feature_name("-foo").is_err()); - assert!(validate_feature_name(".foo").is_err()); - assert!(validate_feature_name("foo:bar").is_err()); - assert!(validate_feature_name("foo?").is_err()); - assert!(validate_feature_name("?foo").is_err()); - assert!(validate_feature_name("ⒶⒷⒸ").is_err()); - assert!(validate_feature_name("a¼").is_err()); - assert!(validate_feature_name("").is_err()); - } -} diff --git a/src/cargo/util_schemas/manifest.rs b/src/cargo/util_schemas/manifest.rs index 232448f11ae..074e8caf20e 100644 --- a/src/cargo/util_schemas/manifest.rs +++ b/src/cargo/util_schemas/manifest.rs @@ -16,8 +16,8 @@ use serde::ser; use serde::{Deserialize, Serialize}; use serde_untagged::UntaggedEnumVisitor; -use crate::util::restricted_names; use crate::util_schemas::core::PackageIdSpec; +use crate::util_schemas::restricted_names; use crate::util_semver::PartialVersion; /// This type is used to deserialize `Cargo.toml` files. diff --git a/src/cargo/util_schemas/mod.rs b/src/cargo/util_schemas/mod.rs index a2d0a0736a8..84b6c39a89b 100644 --- a/src/cargo/util_schemas/mod.rs +++ b/src/cargo/util_schemas/mod.rs @@ -7,3 +7,5 @@ pub mod core; pub mod manifest; + +mod restricted_names; diff --git a/src/cargo/util_schemas/restricted_names.rs b/src/cargo/util_schemas/restricted_names.rs new file mode 100644 index 00000000000..e1e2c71a7af --- /dev/null +++ b/src/cargo/util_schemas/restricted_names.rs @@ -0,0 +1,215 @@ +//! Helpers for validating and checking names like package and crate names. + +use anyhow::bail; +use anyhow::Result; + +/// Check the base requirements for a package name. +/// +/// This can be used for other things than package names, to enforce some +/// 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<()> { + if name.is_empty() { + bail!("{what} cannot be empty"); + } + + 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, + what, + help + ); + } + 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 `_`){}", + ch, + what, + name, + help + ); + } + } + 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){}", + ch, + what, + name, + help + ); + } + } + Ok(()) +} + +/// Ensure a package name is [valid][validate_package_name] +pub 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() { + if (unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_') && !ch.is_digit(10) { + slug.push(ch); + break; + } + } + while let Some(ch) = chars.next() { + if unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-' { + slug.push(ch); + } else { + slug.push(placeholder); + } + } + if slug.is_empty() { + slug.push_str("package"); + } + slug +} + +/// Validate dir-names and profile names according to RFC 2678. +pub fn validate_profile_name(name: &str) -> Result<()> { + if let Some(ch) = name + .chars() + .find(|ch| !ch.is_alphanumeric() && *ch != '_' && *ch != '-') + { + bail!( + "invalid character `{}` in profile name `{}`\n\ + Allowed characters are letters, numbers, underscore, and hyphen.", + ch, + name + ); + } + + 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 + ); + } + 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 + ); + } + + // These are some arbitrary reservations. We have no plans to use + // these, but it seems safer to reserve a few just in case we want to + // add more built-in profiles in the future. We can also uses special + // syntax like cargo:foo if needed. But it is unlikely these will ever + // be used. + if matches!( + lower_name.as_str(), + "build" + | "check" + | "clean" + | "config" + | "fetch" + | "fix" + | "install" + | "metadata" + | "package" + | "publish" + | "report" + | "root" + | "run" + | "rust" + | "rustc" + | "rustdoc" + | "target" + | "tmp" + | "uninstall" + ) || lower_name.starts_with("cargo") + { + bail!( + "profile name `{}` is reserved\n\ + Please choose a different name.\n\ + {}", + name, + SEE_DOCS + ); + } + + Ok(()) +} + +pub fn validate_feature_name(name: &str) -> Result<()> { + if name.is_empty() { + bail!("feature name cannot be empty"); + } + + if name.starts_with("dep:") { + bail!("feature named `{name}` is not allowed to start with `dep:`",); + } + if name.contains('/') { + bail!("feature named `{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`)", + ); + } + } + 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)", + ); + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn valid_feature_names() { + assert!(validate_feature_name("c++17").is_ok()); + assert!(validate_feature_name("128bit").is_ok()); + assert!(validate_feature_name("_foo").is_ok()); + assert!(validate_feature_name("feat-name").is_ok()); + assert!(validate_feature_name("feat_name").is_ok()); + assert!(validate_feature_name("foo.bar").is_ok()); + + assert!(validate_feature_name("+foo").is_err()); + assert!(validate_feature_name("-foo").is_err()); + assert!(validate_feature_name(".foo").is_err()); + assert!(validate_feature_name("foo:bar").is_err()); + assert!(validate_feature_name("foo?").is_err()); + assert!(validate_feature_name("?foo").is_err()); + assert!(validate_feature_name("ⒶⒷⒸ").is_err()); + assert!(validate_feature_name("a¼").is_err()); + assert!(validate_feature_name("").is_err()); + } +} From b9524d4cd6a819805d6217af3c9b9e717164cd63 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Dec 2023 11:27:47 -0600 Subject: [PATCH 16/18] test(schema): Unit test for more feature name cases --- src/cargo/util_schemas/restricted_names.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cargo/util_schemas/restricted_names.rs b/src/cargo/util_schemas/restricted_names.rs index e1e2c71a7af..2d22ce4f2f7 100644 --- a/src/cargo/util_schemas/restricted_names.rs +++ b/src/cargo/util_schemas/restricted_names.rs @@ -202,9 +202,12 @@ mod tests { assert!(validate_feature_name("feat_name").is_ok()); assert!(validate_feature_name("foo.bar").is_ok()); + assert!(validate_feature_name("").is_err()); assert!(validate_feature_name("+foo").is_err()); assert!(validate_feature_name("-foo").is_err()); assert!(validate_feature_name(".foo").is_err()); + assert!(validate_feature_name("dep:bar").is_err()); + assert!(validate_feature_name("foo/bar").is_err()); assert!(validate_feature_name("foo:bar").is_err()); assert!(validate_feature_name("foo?").is_err()); assert!(validate_feature_name("?foo").is_err()); From acef3c054e52dedae1fed87d74954b8e2feb9e65 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Dec 2023 11:30:09 -0600 Subject: [PATCH 17/18] refactor(schema): Remove redundant path --- src/cargo/util_schemas/manifest.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/util_schemas/manifest.rs b/src/cargo/util_schemas/manifest.rs index 074e8caf20e..ffaf47a2b98 100644 --- a/src/cargo/util_schemas/manifest.rs +++ b/src/cargo/util_schemas/manifest.rs @@ -1122,7 +1122,7 @@ macro_rules! str_newtype { } } - impl> std::convert::AsRef for $name { + impl> AsRef for $name { fn as_ref(&self) -> &str { self.0.as_ref() } From cfa9421b2ec807d6e8fabf7551b09eb752c0cbc2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Dec 2023 11:36:09 -0600 Subject: [PATCH 18/18] docs(schema): Document newtypes --- src/cargo/util_schemas/manifest.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/cargo/util_schemas/manifest.rs b/src/cargo/util_schemas/manifest.rs index ffaf47a2b98..390658b0e46 100644 --- a/src/cargo/util_schemas/manifest.rs +++ b/src/cargo/util_schemas/manifest.rs @@ -1112,6 +1112,7 @@ impl TomlTarget { macro_rules! str_newtype { ($name:ident) => { + /// Verified string newtype #[derive(Serialize, Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] #[serde(transparent)] pub struct $name = String>(T); @@ -1171,6 +1172,7 @@ macro_rules! str_newtype { str_newtype!(PackageName); impl> PackageName { + /// Validated package name pub fn new(name: T) -> Result { restricted_names::validate_package_name(name.as_ref(), "package name", "")?; Ok(Self(name)) @@ -1178,6 +1180,9 @@ impl> PackageName { } impl PackageName { + /// Coerce a value to be a validate package name + /// + /// Replaces invalid values with `placeholder` pub fn sanitize(name: impl AsRef, placeholder: char) -> Self { PackageName(restricted_names::sanitize_package_name( name.as_ref(), @@ -1189,6 +1194,7 @@ impl PackageName { str_newtype!(RegistryName); impl> RegistryName { + /// Validated registry name pub fn new(name: T) -> Result { restricted_names::validate_package_name(name.as_ref(), "registry name", "")?; Ok(Self(name)) @@ -1198,6 +1204,7 @@ impl> RegistryName { str_newtype!(ProfileName); impl> ProfileName { + /// Validated profile name pub fn new(name: T) -> Result { restricted_names::validate_profile_name(name.as_ref())?; Ok(Self(name)) @@ -1207,6 +1214,7 @@ impl> ProfileName { str_newtype!(FeatureName); impl> FeatureName { + /// Validated feature name pub fn new(name: T) -> Result { restricted_names::validate_feature_name(name.as_ref())?; Ok(Self(name))