Validate package and extra name (#290)

`PackageName` and `ExtraName` can now only be constructed from valid
names. They share the same rules, so i gave them the same
implementation. Constructors are split between `new` (owned) and
`from_str` (borrowed), with the owned version avoiding allocations.

Closes #279

---------

Co-authored-by: Zanie <contact@zanie.dev>

crates/distribution-filename/src/source_distribution.rs

@@ -4,7 +4,7 @@ use std::str::FromStr;
 use thiserror::Error;
 
 use pep440_rs::Version;
-use puffin_normalize::PackageName;
+use puffin_normalize::{InvalidNameError, PackageName};
 
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum SourceDistributionExtension {
@@ -71,8 +71,12 @@ impl SourceDistributionFilename {
             ));
         };
 
+        let actual_package_name = PackageName::from_str(&stem[..package_name.as_ref().len()])
+            .map_err(|err| {
+                SourceDistributionFilenameError::InvalidPackageName(filename.to_string(), err)
+            })?;
         if stem.len() <= package_name.as_ref().len() + "-".len()
-            || &PackageName::new(&stem[..package_name.as_ref().len()]) != package_name
+            || &actual_package_name != package_name
         {
             return Err(SourceDistributionFilenameError::InvalidFilename {
                 filename: filename.to_string(),
@@ -109,11 +113,14 @@ pub enum SourceDistributionFilenameError {
     InvalidExtension(String),
     #[error("Source distribution filename version section is invalid: {0}")]
     InvalidVersion(String),
+    #[error("Source distribution filename has an invalid package name: {0}")]
+    InvalidPackageName(String, #[source] InvalidNameError),
 }
 
 #[cfg(test)]
 mod tests {
     use puffin_normalize::PackageName;
+    use std::str::FromStr;
 
     use crate::SourceDistributionFilename;
 
@@ -126,9 +133,12 @@ mod tests {
             "foo-lib-1.2.3.tar.gz",
         ] {
             assert_eq!(
-                SourceDistributionFilename::parse(normalized, &PackageName::new("foo_lib"))
+                SourceDistributionFilename::parse(
-                    .unwrap()
+                    normalized,
-                    .to_string(),
+                    &PackageName::from_str("foo_lib").unwrap()
+                )
+                .unwrap()
+                .to_string(),
                 normalized
             );
         }
@@ -137,7 +147,11 @@ mod tests {
     #[test]
     fn errors() {
         for invalid in ["b-1.2.3.zip", "a-1.2.3-gamma.3.zip", "a-1.2.3.tar.zstd"] {
-            assert!(SourceDistributionFilename::parse(invalid, &PackageName::new("a")).is_err());
+            assert!(SourceDistributionFilename::parse(
+                invalid,
+                &PackageName::from_str("a").unwrap()
+            )
+            .is_err());
         }
     }
 }

crates/distribution-filename/src/wheel.rs

@@ -6,7 +6,7 @@ use url::Url;
 
 use pep440_rs::Version;
 use platform_tags::Tags;
-use puffin_normalize::PackageName;
+use puffin_normalize::{InvalidNameError, PackageName};
 
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct WheelFilename {
@@ -87,10 +87,12 @@ impl FromStr for WheelFilename {
                 )
             };
 
+        let distribution = PackageName::from_str(distribution)
+            .map_err(|err| WheelFilenameError::InvalidPackageName(filename.to_string(), err))?;
         let version = Version::from_str(version)
             .map_err(|err| WheelFilenameError::InvalidVersion(filename.to_string(), err))?;
         Ok(WheelFilename {
-            distribution: PackageName::new(distribution),
+            distribution,
             version,
             python_tag: python_tag.split('.').map(String::from).collect(),
             abi_tag: abi_tag.split('.').map(String::from).collect(),
@@ -165,4 +167,6 @@ pub enum WheelFilenameError {
     InvalidWheelFileName(String, String),
     #[error("The wheel filename \"{0}\" has an invalid version part: {1}")]
     InvalidVersion(String, String),
+    #[error("The wheel filename \"{0}\" has an invalid package name")]
+    InvalidPackageName(String, InvalidNameError),
 }

crates/pep508-rs/src/lib.rs

@@ -11,7 +11,7 @@
 //! let marker = r#"requests [security,tests] >= 2.8.1, == 2.8.* ; python_version > "3.8""#;
 //! let dependency_specification = Requirement::from_str(marker).unwrap();
 //! assert_eq!(dependency_specification.name.as_ref(), "requests");
-//! assert_eq!(dependency_specification.extras, Some(vec![ExtraName::new("security"), ExtraName::new("tests")]));
+//! assert_eq!(dependency_specification.extras, Some(vec![ExtraName::from_str("security").unwrap(), ExtraName::from_str("tests").unwrap()]));
 //! ```
 
 #![deny(missing_docs)]
@@ -63,21 +63,13 @@ pub struct Pep508Error {
 #[derive(Debug, Error, Clone, Eq, PartialEq)]
 pub enum Pep508ErrorSource {
     /// An error from our parser
+    #[error("{0}")]
     String(String),
     /// A url parsing error
     #[error(transparent)]
     UrlError(#[from] url::ParseError),
 }
 
-impl Display for Pep508ErrorSource {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        match self {
-            Pep508ErrorSource::String(string) => string.fmt(f),
-            Pep508ErrorSource::UrlError(parse_err) => parse_err.fmt(f),
-        }
-    }
-}
-
 impl Display for Pep508Error {
     /// Pretty formatting with underline
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
@@ -568,7 +560,10 @@ fn parse_name(chars: &mut CharIter) -> Result<PackageName, Pep508Error> {
                     });
                 }
             }
-            Some(_) | None => return Ok(PackageName::new(name)),
+            Some(_) | None => {
+                return Ok(PackageName::new(name)
+                    .expect("`PackageName` validation should match PEP 508 parsing"));
+            }
         }
     }
 }
@@ -641,10 +636,16 @@ fn parse_extras(chars: &mut CharIter) -> Result<Option<Vec<ExtraName>>, Pep508Er
         // end or next identifier?
         match chars.next() {
             Some((_, ',')) => {
-                extras.push(ExtraName::new(buffer));
+                extras.push(
+                    ExtraName::new(buffer)
+                        .expect("`ExtraName` validation should match PEP 508 parsing"),
+                );
             }
             Some((_, ']')) => {
-                extras.push(ExtraName::new(buffer));
+                extras.push(
+                    ExtraName::new(buffer)
+                        .expect("`ExtraName` validation should match PEP 508 parsing"),
+                );
                 break;
             }
             Some((pos, other)) => {
@@ -944,8 +945,11 @@ mod tests {
         let requests = Requirement::from_str(input).unwrap();
         assert_eq!(input, requests.to_string());
         let expected = Requirement {
-            name: PackageName::new("requests"),
+            name: PackageName::from_str("requests").unwrap(),
-            extras: Some(vec![ExtraName::new("security"), ExtraName::new("tests")]),
+            extras: Some(vec![
+                ExtraName::from_str("security").unwrap(),
+                ExtraName::from_str("tests").unwrap(),
+            ]),
             version_or_url: Some(VersionOrUrl::VersionSpecifier(
                 [
                     VersionSpecifier::new(
@@ -1086,7 +1090,7 @@ mod tests {
     #[test]
     fn error_extras1() {
         let numpy = Requirement::from_str("black[d]").unwrap();
-        assert_eq!(numpy.extras, Some(vec![ExtraName::new("d")]));
+        assert_eq!(numpy.extras, Some(vec![ExtraName::from_str("d").unwrap()]));
     }
 
     #[test]
@@ -1094,7 +1098,10 @@ mod tests {
         let numpy = Requirement::from_str("black[d,jupyter]").unwrap();
         assert_eq!(
             numpy.extras,
-            Some(vec![ExtraName::new("d"), ExtraName::new("jupyter")])
+            Some(vec![
+                ExtraName::from_str("d").unwrap(),
+                ExtraName::from_str("jupyter").unwrap()
+            ])
         );
     }
 
@@ -1141,7 +1148,7 @@ mod tests {
                 .unwrap();
         let url = "https://github.com/pypa/pip/archive/1.3.1.zip#sha1=da9234ee9982d4bbb3c72346a6de940a148ea686";
         let expected = Requirement {
-            name: PackageName::new("pip"),
+            name: PackageName::from_str("pip").unwrap(),
             extras: None,
             marker: None,
             version_or_url: Some(VersionOrUrl::Url(Url::parse(url).unwrap())),

crates/puffin-cli/src/commands/mod.rs

@@ -4,7 +4,7 @@ use std::time::Duration;
 pub(crate) use add::add;
 pub(crate) use clean::clean;
 pub(crate) use freeze::freeze;
-pub(crate) use pip_compile::pip_compile;
+pub(crate) use pip_compile::{extra_name_with_clap_error, pip_compile};
 pub(crate) use pip_sync::pip_sync;
 pub(crate) use pip_uninstall::pip_uninstall;
 pub(crate) use remove::remove;

crates/puffin-cli/src/commands/pip_compile.rs

@@ -16,7 +16,9 @@ use platform_tags::Tags;
 use puffin_client::RegistryClientBuilder;
 use puffin_dispatch::BuildDispatch;
 use puffin_interpreter::Virtualenv;
+use puffin_normalize::ExtraName;
 use puffin_resolver::{Manifest, PreReleaseMode, ResolutionFailureReporter, ResolutionMode};
+use std::str::FromStr;
 
 use crate::commands::reporters::ResolverReporter;
 use crate::commands::{elapsed, ExitStatus};
@@ -229,3 +231,12 @@ impl From<bool> for UpgradeMode {
         }
     }
 }
+
+pub(crate) fn extra_name_with_clap_error(arg: &str) -> Result<ExtraName> {
+    ExtraName::from_str(arg).map_err(|_err| {
+        anyhow!(
+            "Extra names must start and end with a letter or digit and may only \
+            contain -, _, ., and alphanumeric characters"
+        )
+    })
+}

crates/puffin-cli/src/commands/remove.rs

@@ -2,6 +2,7 @@ use std::path::PathBuf;
 
 use anyhow::Result;
 use miette::{Diagnostic, IntoDiagnostic};
+use puffin_normalize::PackageName;
 use thiserror::Error;
 use tracing::info;
 
@@ -12,7 +13,7 @@ use crate::printer::Printer;
 
 /// Remove a dependency from the workspace.
 #[allow(clippy::unnecessary_wraps)]
-pub(crate) fn remove(name: &str, _printer: Printer) -> Result<ExitStatus> {
+pub(crate) fn remove(name: &PackageName, _printer: Printer) -> Result<ExitStatus> {
     match remove_impl(name) {
         Ok(status) => Ok(status),
         Err(err) => {
@@ -46,7 +47,7 @@ enum RemoveError {
     RemovalError(String, #[source] WorkspaceError),
 }
 
-fn remove_impl(name: &str) -> miette::Result<ExitStatus> {
+fn remove_impl(name: &PackageName) -> miette::Result<ExitStatus> {
     // Locate the workspace.
     let cwd = std::env::current_dir().into_diagnostic()?;
     let Some(workspace_root) = puffin_workspace::find_pyproject_toml(cwd) else {

crates/puffin-cli/src/main.rs

@@ -9,11 +9,11 @@ use directories::ProjectDirs;
 use tempfile::tempdir;
 use url::Url;
 
-use puffin_normalize::ExtraName;
+use puffin_normalize::{ExtraName, PackageName};
 use puffin_resolver::{PreReleaseMode, ResolutionMode};
 use requirements::ExtrasSpecification;
 
-use crate::commands::ExitStatus;
+use crate::commands::{extra_name_with_clap_error, ExitStatus};
 use crate::index_urls::IndexUrls;
 use crate::requirements::RequirementsSource;
 
@@ -78,7 +78,7 @@ struct PipCompileArgs {
     constraint: Vec<PathBuf>,
 
     /// Include optional dependencies in the given extra group name; may be provided more than once.
-    #[clap(long, conflicts_with = "all_extras")]
+    #[clap(long, conflicts_with = "all_extras", value_parser = extra_name_with_clap_error)]
     extra: Vec<ExtraName>,
 
     /// Include all optional dependencies.
@@ -169,7 +169,7 @@ struct AddArgs {
 #[derive(Args)]
 struct RemoveArgs {
     /// The name of the package to remove (e.g., `Django`).
-    name: String,
+    name: PackageName,
 }
 
 async fn inner() -> Result<ExitStatus> {

crates/puffin-cli/src/requirements.rs

@@ -112,7 +112,9 @@ impl RequirementsSpecification {
                         for (name, optional_requirements) in
                             project.optional_dependencies.unwrap_or_default()
                         {
-                            let normalized_name = ExtraName::new(name);
+                            // TODO(konstin): It's not ideal that pyproject-toml doesn't use
+                            // `ExtraName`
+                            let normalized_name = ExtraName::new(name)?;
                             if extras.contains(&normalized_name) {
                                 used_extras.insert(normalized_name);
                                 requirements.extend(optional_requirements);
@@ -120,7 +122,9 @@ impl RequirementsSpecification {
                         }
                     }
                     // Parse the project name
-                    project_name = Some(PackageName::new(project.name));
+                    project_name = Some(PackageName::new(project.name).with_context(|| {
+                        format!("Invalid `project.name` in {}", path.display())
+                    })?);
                 }
 
                 Self {

crates/puffin-cli/tests/snapshots/pip_compile__invalid_extra_name.snap

@@ -8,9 +8,9 @@ info:
     - "--extra"
     - invalid name!
     - "--cache-dir"
-    - /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpXIuamZ
+    - /tmp/.tmpiOHlGv
   env:
-    VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmp0LGEEX/.venv
+    VIRTUAL_ENV: /tmp/.tmpA05wES/.venv
 ---
 success: false
 exit_code: 2

crates/puffin-distribution/src/lib.rs

@@ -172,7 +172,7 @@ impl CachedDistribution {
             return Ok(None);
         };
 
-        let name = PackageName::new(name);
+        let name = PackageName::from_str(name)?;
         let version = Version::from_str(version).map_err(|err| anyhow!(err))?;
         let path = path.to_path_buf();
 
@@ -251,7 +251,7 @@ impl InstalledDistribution {
                 return Ok(None);
             };
 
-            let name = PackageName::new(name);
+            let name = PackageName::from_str(name)?;
             let version = Version::from_str(version).map_err(|err| anyhow!(err))?;
             let path = path.to_path_buf();
 

crates/puffin-normalize/src/extra_name.rs

@@ -1,10 +1,9 @@
+use serde::{Deserialize, Deserializer, Serialize};
 use std::fmt;
 use std::fmt::{Display, Formatter};
 use std::str::FromStr;
 
-use anyhow::{anyhow, Error, Result};
+use crate::{validate_and_normalize_owned, validate_and_normalize_ref, InvalidNameError};
-use once_cell::sync::Lazy;
-use regex::Regex;
 
 /// The normalized name of an extra dependency group.
 ///
@@ -14,91 +13,42 @@ use regex::Regex;
 /// See:
 /// - <https://peps.python.org/pep-0685/#specification/>
 /// - <https://packaging.python.org/en/latest/specifications/name-normalization/>
-#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize)]
 pub struct ExtraName(String);
 
+impl ExtraName {
+    /// Create a validated, normalized extra name.
+    pub fn new(name: String) -> Result<Self, InvalidNameError> {
+        validate_and_normalize_owned(name).map(Self)
+    }
+}
+
+impl FromStr for ExtraName {
+    type Err = InvalidNameError;
+
+    fn from_str(name: &str) -> Result<Self, Self::Err> {
+        validate_and_normalize_ref(name).map(Self)
+    }
+}
+
+impl<'de> Deserialize<'de> for ExtraName {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        let s = String::deserialize(deserializer)?;
+        Self::from_str(&s).map_err(serde::de::Error::custom)
+    }
+}
+
 impl Display for ExtraName {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         self.0.fmt(f)
     }
 }
 
-static NAME_NORMALIZE: Lazy<Regex> = Lazy::new(|| Regex::new(r"[-_.]+").unwrap());
-static NAME_VALIDATE: Lazy<Regex> =
-    Lazy::new(|| Regex::new(r"(?i)^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9])$").unwrap());
-
-impl ExtraName {
-    pub fn new(name: impl AsRef<str>) -> Self {
-        // TODO(charlie): Avoid allocating in the common case (when no normalization is required).
-        let mut normalized = NAME_NORMALIZE.replace_all(name.as_ref(), "-").to_string();
-        normalized.make_ascii_lowercase();
-        Self(normalized)
-    }
-
-    /// Create a validated, normalized extra name.
-    pub fn validate(name: impl AsRef<str>) -> Result<Self> {
-        if NAME_VALIDATE.is_match(name.as_ref()) {
-            Ok(Self::new(name))
-        } else {
-            Err(anyhow!(
-                "Extra names must start and end with a letter or digit and may only contain -, _, ., and alphanumeric characters"
-            ))
-        }
-    }
-}
-
 impl AsRef<str> for ExtraName {
     fn as_ref(&self) -> &str {
-        self.0.as_ref()
+        &self.0
-    }
-}
-
-impl FromStr for ExtraName {
-    type Err = Error;
-
-    fn from_str(name: &str) -> Result<Self> {
-        Self::validate(name)
-    }
-}
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn normalize() {
-        assert_eq!(ExtraName::new("friendly-bard").as_ref(), "friendly-bard");
-        assert_eq!(ExtraName::new("Friendly-Bard").as_ref(), "friendly-bard");
-        assert_eq!(ExtraName::new("FRIENDLY-BARD").as_ref(), "friendly-bard");
-        assert_eq!(ExtraName::new("friendly.bard").as_ref(), "friendly-bard");
-        assert_eq!(ExtraName::new("friendly_bard").as_ref(), "friendly-bard");
-        assert_eq!(ExtraName::new("friendly--bard").as_ref(), "friendly-bard");
-        assert_eq!(
-            ExtraName::new("FrIeNdLy-._.-bArD").as_ref(),
-            "friendly-bard"
-        );
-    }
-
-    #[test]
-    fn validate() {
-        // Unchanged
-        assert_eq!(
-            ExtraName::validate("friendly-bard").unwrap().as_ref(),
-            "friendly-bard"
-        );
-        assert_eq!(ExtraName::validate("1okay").unwrap().as_ref(), "1okay");
-        assert_eq!(ExtraName::validate("okay2").unwrap().as_ref(), "okay2");
-        // Normalizes
-        assert_eq!(
-            ExtraName::validate("Friendly-Bard").unwrap().as_ref(),
-            "friendly-bard"
-        );
-        // Failures...
-        assert!(ExtraName::validate(" starts-with-space").is_err());
-        assert!(ExtraName::validate("-starts-with-dash").is_err());
-        assert!(ExtraName::validate("ends-with-dash-").is_err());
-        assert!(ExtraName::validate("ends-with-space ").is_err());
-        assert!(ExtraName::validate("includes!invalid-char").is_err());
-        assert!(ExtraName::validate("space in middle").is_err());
     }
 }

crates/puffin-normalize/src/lib.rs

@@ -1,5 +1,110 @@
+use std::error::Error;
+use std::fmt::{Display, Formatter};
+
+use once_cell::sync::Lazy;
+use regex::Regex;
+
 pub use extra_name::ExtraName;
 pub use package_name::PackageName;
 
 mod extra_name;
 mod package_name;
+
+pub(crate) static NAME_NORMALIZE: Lazy<Regex> = Lazy::new(|| Regex::new(r"[-_.]+").unwrap());
+pub(crate) static NAME_VALIDATE: Lazy<Regex> =
+    Lazy::new(|| Regex::new(r"(?i)^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9])$").unwrap());
+
+pub(crate) fn validate_and_normalize_ref(
+    name: impl AsRef<str>,
+) -> Result<String, InvalidNameError> {
+    if !NAME_VALIDATE.is_match(name.as_ref()) {
+        return Err(InvalidNameError(name.as_ref().to_string()));
+    }
+    let mut normalized = NAME_NORMALIZE.replace_all(name.as_ref(), "-").to_string();
+    normalized.make_ascii_lowercase();
+    Ok(normalized)
+}
+
+pub(crate) fn validate_and_normalize_owned(mut name: String) -> Result<String, InvalidNameError> {
+    if !NAME_VALIDATE.is_match(name.as_ref()) {
+        return Err(InvalidNameError(name));
+    }
+    let normalized = NAME_NORMALIZE.replace_all(&name, "-");
+    // fast path: Don't allocate if we don't need to. An inplace ascii char replace would be
+    // nicer but doesn't exist
+    if normalized != name {
+        name = normalized.to_string();
+    }
+    name.make_ascii_lowercase();
+    Ok(name)
+}
+
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct InvalidNameError(String);
+
+impl Display for InvalidNameError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "Not a valid package or extra name: \"{}\". Names must start and end with a letter or \
+            digit and may only contain -, _, ., and alphanumeric characters",
+            self.0
+        )
+    }
+}
+
+impl Error for InvalidNameError {}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn normalize() {
+        let inputs = [
+            "friendly-bard",
+            "Friendly-Bard",
+            "FRIENDLY-BARD",
+            "friendly.bard",
+            "friendly_bard",
+            "friendly--bard",
+            "FrIeNdLy-._.-bArD",
+        ];
+        for input in inputs {
+            assert_eq!(validate_and_normalize_ref(input).unwrap(), "friendly-bard");
+            assert_eq!(
+                validate_and_normalize_owned(input.to_string()).unwrap(),
+                "friendly-bard"
+            );
+        }
+    }
+
+    #[test]
+    fn unchanged() {
+        // Unchanged
+        let unchanged = ["friendly-bard", "1okay", "okay2"];
+        for input in unchanged {
+            assert_eq!(validate_and_normalize_ref(input).unwrap(), input);
+            assert_eq!(
+                validate_and_normalize_owned(input.to_string()).unwrap(),
+                input
+            );
+        }
+    }
+
+    #[test]
+    fn failures() {
+        let failures = [
+            " starts-with-space",
+            "-starts-with-dash",
+            "ends-with-dash-",
+            "ends-with-space ",
+            "includes!invalid-char",
+            "space in middle",
+        ];
+        for input in failures {
+            assert!(validate_and_normalize_ref(input).is_err());
+            assert!(validate_and_normalize_owned(input.to_string()).is_err(),);
+        }
+    }
+}

crates/puffin-normalize/src/package_name.rs

@@ -1,10 +1,11 @@
 use std::fmt;
 use std::fmt::{Display, Formatter};
+use std::str::FromStr;
 
-use once_cell::sync::Lazy;
-use regex::Regex;
 use serde::{Deserialize, Deserializer, Serialize};
 
+use crate::{validate_and_normalize_owned, validate_and_normalize_ref, InvalidNameError};
+
 /// The normalized name of a package.
 ///
 /// Converts the name to lowercase and collapses any run of the characters `-`, `_` and `.`
@@ -14,27 +15,10 @@ use serde::{Deserialize, Deserializer, Serialize};
 #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize)]
 pub struct PackageName(String);
 
-impl From<&PackageName> for PackageName {
-    /// Required for `WaitMap::wait`
-    fn from(package_name: &PackageName) -> Self {
-        package_name.clone()
-    }
-}
-
-impl Display for PackageName {
-    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        self.0.fmt(f)
-    }
-}
-
-static NAME_NORMALIZE: Lazy<Regex> = Lazy::new(|| Regex::new(r"[-_.]+").unwrap());
-
 impl PackageName {
-    pub fn new(name: impl AsRef<str>) -> Self {
+    /// Create a validated, normalized package name.
-        // TODO(charlie): Avoid allocating in the common case (when no normalization is required).
+    pub fn new(name: String) -> Result<Self, InvalidNameError> {
-        let mut normalized = NAME_NORMALIZE.replace_all(name.as_ref(), "-").to_string();
+        validate_and_normalize_owned(name).map(Self)
-        normalized.make_ascii_lowercase();
-        Self(normalized)
     }
 
     /// Escape this name with underscores (`_`) instead of dashes (`-`)
@@ -45,9 +29,18 @@ impl PackageName {
     }
 }
 
-impl AsRef<str> for PackageName {
+impl From<&PackageName> for PackageName {
-    fn as_ref(&self) -> &str {
+    /// Required for `WaitMap::wait`
-        self.0.as_ref()
+    fn from(package_name: &PackageName) -> Self {
+        package_name.clone()
+    }
+}
+
+impl FromStr for PackageName {
+    type Err = InvalidNameError;
+
+    fn from_str(name: &str) -> Result<Self, Self::Err> {
+        validate_and_normalize_ref(name).map(Self)
     }
 }
 
@@ -57,25 +50,18 @@ impl<'de> Deserialize<'de> for PackageName {
         D: Deserializer<'de>,
     {
         let s = String::deserialize(deserializer)?;
-        Ok(Self::new(s))
+        Self::from_str(&s).map_err(serde::de::Error::custom)
     }
 }
 
-#[cfg(test)]
+impl Display for PackageName {
-mod tests {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-    use super::*;
+        self.0.fmt(f)
-
+    }
-    #[test]
+}
-    fn normalize() {
+
-        assert_eq!(PackageName::new("friendly-bard").as_ref(), "friendly-bard");
+impl AsRef<str> for PackageName {
-        assert_eq!(PackageName::new("Friendly-Bard").as_ref(), "friendly-bard");
+    fn as_ref(&self) -> &str {
-        assert_eq!(PackageName::new("FRIENDLY-BARD").as_ref(), "friendly-bard");
+        &self.0
-        assert_eq!(PackageName::new("friendly.bard").as_ref(), "friendly-bard");
-        assert_eq!(PackageName::new("friendly_bard").as_ref(), "friendly-bard");
-        assert_eq!(PackageName::new("friendly--bard").as_ref(), "friendly-bard");
-        assert_eq!(
-            PackageName::new("FrIeNdLy-._.-bArD").as_ref(),
-            "friendly-bard"
-        );
     }
 }

crates/puffin-package/src/pypi_types/metadata.rs

@@ -13,7 +13,7 @@ use tracing::warn;
 
 use pep440_rs::{Pep440Error, Version, VersionSpecifiers};
 use pep508_rs::{Pep508Error, Requirement};
-use puffin_normalize::PackageName;
+use puffin_normalize::{ExtraName, InvalidNameError, PackageName};
 
 /// Python Package Metadata 2.1 as specified in
 /// <https://packaging.python.org/specifications/core-metadata/>
@@ -48,7 +48,7 @@ pub struct Metadata21 {
     pub requires_python: Option<VersionSpecifiers>,
     pub requires_external: Vec<String>,
     pub project_urls: HashMap<String, String>,
-    pub provides_extras: Vec<String>,
+    pub provides_extras: Vec<ExtraName>,
 }
 
 /// <https://github.com/PyO3/python-pkginfo-rs/blob/d719988323a0cfea86d4737116d7917f30e819e2/src/error.rs>
@@ -86,6 +86,8 @@ pub enum Error {
     /// Invalid Requirement
     #[error(transparent)]
     Pep508Error(#[from] Pep508Error),
+    #[error(transparent)]
+    InvalidName(#[from] InvalidNameError),
 }
 
 /// From <https://github.com/PyO3/python-pkginfo-rs/blob/d719988323a0cfea86d4737116d7917f30e819e2/src/metadata.rs#LL78C2-L91C26>
@@ -127,7 +129,7 @@ impl Metadata21 {
             headers
                 .get_first_value("Name")
                 .ok_or(Error::FieldNotFound("Name"))?,
-        );
+        )?;
         let version = Version::from_str(
             &headers
                 .get_first_value("Version")
@@ -155,9 +157,9 @@ impl Metadata21 {
             .map(|requires_dist| LenientRequirement::from_str(requires_dist).map(Requirement::from))
             .collect::<Result<Vec<_>, _>>()?;
         let provides_dist = get_all_values("Provides-Dist")
-            .iter()
+            .into_iter()
             .map(PackageName::new)
-            .collect();
+            .collect::<Result<Vec<_>, _>>()?;
         let obsoletes_dist = get_all_values("Obsoletes-Dist");
         let maintainer = get_first_value("Maintainer");
         let maintainer_email = get_first_value("Maintainer-email");
@@ -174,7 +176,10 @@ impl Metadata21 {
                 Some((name, value)) => Ok((name.to_string(), value.trim().to_string())),
             })
             .collect::<Result<_, _>>()?;
-        let provides_extras = get_all_values("Provides-Extra");
+        let provides_extras = get_all_values("Provides-Extra")
+            .into_iter()
+            .map(ExtraName::new)
+            .collect::<Result<Vec<_>, _>>()?;
         let description_content_type = get_first_value("Description-Content-Type");
         Ok(Metadata21 {
             metadata_version,

crates/puffin-resolver/src/pubgrub/dependencies.rs

@@ -53,7 +53,7 @@ impl PubGrubDependencies {
                     .clone()
                     .into_iter()
                     .flatten()
-                    .map(|extra| to_pubgrub(requirement, Some(ExtraName::new(extra)))),
+                    .map(|extra| to_pubgrub(requirement, Some(extra))),
             ) {
                 let (package, version) = result?;
 
@@ -101,7 +101,7 @@ impl PubGrubDependencies {
                     .clone()
                     .into_iter()
                     .flatten()
-                    .map(|extra| to_pubgrub(constraint, Some(ExtraName::new(extra)))),
+                    .map(|extra| to_pubgrub(constraint, Some(extra))),
             ) {
                 let (package, version) = result?;
 

crates/puffin-resolver/src/resolver.rs

@@ -518,7 +518,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
                     if !metadata
                         .provides_extras
                         .iter()
-                        .any(|provided_extra| ExtraName::new(provided_extra) == *extra)
+                        .any(|provided_extra| provided_extra == extra)
                     {
                         return Ok(Dependencies::Unknown);
                     }

crates/puffin-workspace/src/workspace.rs

@@ -98,7 +98,7 @@ impl Workspace {
     }
 
     /// Remove a dependency from the workspace.
-    pub fn remove_dependency(&mut self, name: &str) -> Result<(), WorkspaceError> {
+    pub fn remove_dependency(&mut self, name: &PackageName) -> Result<(), WorkspaceError> {
         let Some(project) = self
             .document
             .get_mut("project")
@@ -123,7 +123,7 @@ impl Workspace {
                 return false;
             };
 
-            PackageName::new(name) == existing.name
+            name == &existing.name
         });
 
         let Some(index) = index else {