Auto merge of #13166 - epage:validate, r=weihanglo

refactor: Pull name validation into `util_schemas` ### What does this PR try to resolve? In preparation for #12801, this moves the last dependency on the rest of the `cargo` crate into the future `util_schemas` crate. It does this by refocusing the code from being validation functions to being newtypes. I did not try to thread the newtypes everywhere, that can be an exercise for the future. There are places I didn't put newtypes because it didn't seem worth it (e.g. places needing `StringOrVec`, `ProfileName` not being used in CLI because of the `check` psuedo-profile, etc) The main risk with this is when validation changes according to a nightly feature, like packages-as-namespaces. My thought is that the validation code would be updated for the nightly behavior and then the caller would check if it isn't nightly and fail in that case. This has a side effect of improving the error messages for manifest parsing because we will show more context ### How should we test and review this PR? #13164 should be reviewed first ### Additional information

Author: bors

src/cargo/core/summary.rs

@@ -1,6 +1,7 @@
 use crate::core::{Dependency, PackageId, SourceId};
 use crate::util::interning::InternedString;
 use crate::util::CargoResult;
+use crate::util_schemas::manifest::FeatureName;
 use crate::util_schemas::manifest::RustVersion;
 use anyhow::bail;
 use semver::Version;
@@ -49,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,
@@ -140,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<InternedString, Vec<InternedString>>,
  dependencies: &[Dependency],
 ) -> CargoResult<FeatureMap> {
@@ -191,19 +191,7 @@ 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)?;
+ FeatureName::new(feature)?;
  for fv in fvs {
  // Find data for the referenced dependency...
  let dep_data = {
@@ -429,68 +417,3 @@ impl fmt::Display for FeatureValue {
 }
 
 pub type FeatureMap = BTreeMap<InternedString, Vec<FeatureValue>>;
-
-fn validate_feature_name(pkg_id: PackageId, name: &str) -> CargoResult<()> {
- if name.is_empty() {
- bail!("feature name cannot be empty");
- }
- 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());
- }
-}

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)

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!(

src/cargo/util/command_prelude.rs

@@ -11,6 +11,8 @@ 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::RegistryName;
 use crate::util_schemas::manifest::StringOrVec;
 use crate::CargoResult;
 use anyhow::bail;
@@ -605,7 +607,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
  }
  };
@@ -833,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(_)) => {
@@ -848,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(&registry, "registry name", "")?;
+ RegistryName::new(&registry)?;
  Ok(Some(registry))
  }
  }

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<Url> {
- 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!(

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};

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,82 +48,3 @@ pub fn is_windows_reserved_path(path: &Path) -> bool {
 pub fn is_glob_pattern<T: AsRef<str>>(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(())
-}