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(())
-}