Avoid using owned `String` for package name constructors (#11768)

## Summary

Since we use `SmallString` internally, there's no benefit to passing an
owned string to the `PackageName` constructor (same goes for
`ExtraName`, etc.). I've kept them for now (maybe that will change in
the future, so it's useful to have clients passed own values if they
_can_), but removed a bunch of usages where we were casting from `&str`
to `String` needlessly to use the constructor.
This commit is contained in:
Charlie Marsh 2025-02-24 21:06:15 -10:00 committed by GitHub
parent 6080dcbf7b
commit 76c3caf24f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 54 additions and 60 deletions

View File

@ -95,7 +95,7 @@ mod tests {
Vec::new() Vec::new()
); );
($($x:expr),+ $(,)?) => ( ($($x:expr),+ $(,)?) => (
vec![$(ExtraName::new($x.into()).unwrap()),+] vec![$(ExtraName::from_owned($x.into()).unwrap()),+]
) )
} }

View File

@ -6,7 +6,7 @@ use serde::{Deserialize, Deserializer, Serialize};
use uv_small_str::SmallString; use uv_small_str::SmallString;
use crate::{validate_and_normalize_owned, validate_and_normalize_ref, InvalidNameError}; use crate::{validate_and_normalize_ref, InvalidNameError};
/// The normalized name of an extra dependency. /// The normalized name of an extra dependency.
/// ///
@ -22,8 +22,11 @@ pub struct ExtraName(SmallString);
impl ExtraName { impl ExtraName {
/// Create a validated, normalized extra name. /// Create a validated, normalized extra name.
pub fn new(name: String) -> Result<Self, InvalidNameError> { ///
validate_and_normalize_owned(name).map(Self) /// At present, this is no more efficient than calling [`ExtraName::from_str`].
#[allow(clippy::needless_pass_by_value)]
pub fn from_owned(name: String) -> Result<Self, InvalidNameError> {
validate_and_normalize_ref(&name).map(Self)
} }
/// Return the underlying extra name as a string. /// Return the underlying extra name as a string.

View File

@ -7,7 +7,7 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer};
use uv_small_str::SmallString; use uv_small_str::SmallString;
use crate::{validate_and_normalize_owned, validate_and_normalize_ref, InvalidNameError}; use crate::{validate_and_normalize_ref, InvalidNameError};
/// The normalized name of a dependency group. /// The normalized name of a dependency group.
/// ///
@ -20,8 +20,11 @@ pub struct GroupName(SmallString);
impl GroupName { impl GroupName {
/// Create a validated, normalized group name. /// Create a validated, normalized group name.
pub fn new(name: String) -> Result<Self, InvalidNameError> { ///
validate_and_normalize_owned(name).map(Self) /// At present, this is no more efficient than calling [`GroupName::from_str`].
#[allow(clippy::needless_pass_by_value)]
pub fn from_owned(name: String) -> Result<Self, InvalidNameError> {
validate_and_normalize_ref(&name).map(Self)
} }
} }
@ -69,4 +72,4 @@ impl AsRef<str> for GroupName {
/// Internally, we model dependency groups as a generic concept; but externally, we only expose the /// Internally, we model dependency groups as a generic concept; but externally, we only expose the
/// `dev-dependencies` group. /// `dev-dependencies` group.
pub static DEV_DEPENDENCIES: LazyLock<GroupName> = pub static DEV_DEPENDENCIES: LazyLock<GroupName> =
LazyLock::new(|| GroupName::new("dev".to_string()).unwrap()); LazyLock::new(|| GroupName::from_str("dev").unwrap());

View File

@ -13,15 +13,6 @@ mod extra_name;
mod group_name; mod group_name;
mod package_name; mod package_name;
/// Validate and normalize an owned package or extra name.
pub(crate) fn validate_and_normalize_owned(name: String) -> Result<SmallString, InvalidNameError> {
if is_normalized(&name)? {
Ok(SmallString::from(name))
} else {
Ok(SmallString::from(normalize(&name)?))
}
}
/// Validate and normalize an unowned package or extra name. /// Validate and normalize an unowned package or extra name.
pub(crate) fn validate_and_normalize_ref( pub(crate) fn validate_and_normalize_ref(
name: impl AsRef<str>, name: impl AsRef<str>,
@ -151,12 +142,6 @@ mod tests {
validate_and_normalize_ref(input).unwrap().as_ref(), validate_and_normalize_ref(input).unwrap().as_ref(),
"friendly-bard" "friendly-bard"
); );
assert_eq!(
validate_and_normalize_owned(input.to_string())
.unwrap()
.as_ref(),
"friendly-bard"
);
} }
} }
@ -186,12 +171,6 @@ mod tests {
let unchanged = ["friendly-bard", "1okay", "okay2"]; let unchanged = ["friendly-bard", "1okay", "okay2"];
for input in unchanged { for input in unchanged {
assert_eq!(validate_and_normalize_ref(input).unwrap().as_ref(), input); assert_eq!(validate_and_normalize_ref(input).unwrap().as_ref(), input);
assert_eq!(
validate_and_normalize_owned(input.to_string())
.unwrap()
.as_ref(),
input
);
assert!(is_normalized(input).unwrap()); assert!(is_normalized(input).unwrap());
} }
} }
@ -209,7 +188,6 @@ mod tests {
]; ];
for input in failures { for input in failures {
assert!(validate_and_normalize_ref(input).is_err()); assert!(validate_and_normalize_ref(input).is_err());
assert!(validate_and_normalize_owned(input.to_string()).is_err());
assert!(is_normalized(input).is_err()); assert!(is_normalized(input).is_err());
} }
} }

View File

@ -6,7 +6,7 @@ use serde::{Deserialize, Deserializer, Serialize};
use uv_small_str::SmallString; use uv_small_str::SmallString;
use crate::{validate_and_normalize_owned, validate_and_normalize_ref, InvalidNameError}; use crate::{validate_and_normalize_ref, InvalidNameError};
/// The normalized name of a package. /// The normalized name of a package.
/// ///
@ -33,8 +33,11 @@ pub struct PackageName(SmallString);
impl PackageName { impl PackageName {
/// Create a validated, normalized package name. /// Create a validated, normalized package name.
pub fn new(name: String) -> Result<Self, InvalidNameError> { ///
validate_and_normalize_owned(name).map(Self) /// At present, this is no more efficient than calling [`PackageName::from_str`].
#[allow(clippy::needless_pass_by_value)]
pub fn from_owned(name: String) -> Result<Self, InvalidNameError> {
validate_and_normalize_ref(&name).map(Self)
} }
/// Escape this name with underscores (`_`) instead of dashes (`-`) /// Escape this name with underscores (`_`) instead of dashes (`-`)

View File

@ -625,7 +625,8 @@ fn parse_extras_cursor<T: Pep508Url>(
// Add the parsed extra // Add the parsed extra
extras.push( extras.push(
ExtraName::new(buffer).expect("`ExtraName` validation should match PEP 508 parsing"), ExtraName::from_str(&buffer)
.expect("`ExtraName` validation should match PEP 508 parsing"),
); );
is_first_iteration = false; is_first_iteration = false;
} }

View File

@ -20,7 +20,7 @@ impl Metadata10 {
/// Parse the [`Metadata10`] from a `PKG-INFO` file, as included in a source distribution. /// Parse the [`Metadata10`] from a `PKG-INFO` file, as included in a source distribution.
pub fn parse_pkg_info(content: &[u8]) -> Result<Self, MetadataError> { pub fn parse_pkg_info(content: &[u8]) -> Result<Self, MetadataError> {
let headers = Headers::parse(content)?; let headers = Headers::parse(content)?;
let name = PackageName::new( let name = PackageName::from_owned(
headers headers
.get_first_value("Name") .get_first_value("Name")
.ok_or(MetadataError::FieldNotFound("Name"))?, .ok_or(MetadataError::FieldNotFound("Name"))?,

View File

@ -40,7 +40,7 @@ impl ResolutionMetadata {
pub fn parse_metadata(content: &[u8]) -> Result<Self, MetadataError> { pub fn parse_metadata(content: &[u8]) -> Result<Self, MetadataError> {
let headers = Headers::parse(content)?; let headers = Headers::parse(content)?;
let name = PackageName::new( let name = PackageName::from_owned(
headers headers
.get_first_value("Name") .get_first_value("Name")
.ok_or(MetadataError::FieldNotFound("Name"))?, .ok_or(MetadataError::FieldNotFound("Name"))?,
@ -63,13 +63,15 @@ impl ResolutionMetadata {
.map(VersionSpecifiers::from); .map(VersionSpecifiers::from);
let provides_extras = headers let provides_extras = headers
.get_all_values("Provides-Extra") .get_all_values("Provides-Extra")
.filter_map(|provides_extra| match ExtraName::new(provides_extra) { .filter_map(
|provides_extra| match ExtraName::from_owned(provides_extra) {
Ok(extra_name) => Some(extra_name), Ok(extra_name) => Some(extra_name),
Err(err) => { Err(err) => {
warn!("Ignoring invalid extra: {err}"); warn!("Ignoring invalid extra: {err}");
None None
} }
}) },
)
.collect::<Vec<_>>(); .collect::<Vec<_>>();
let dynamic = headers let dynamic = headers
.get_all_values("Dynamic") .get_all_values("Dynamic")
@ -116,7 +118,7 @@ impl ResolutionMetadata {
} }
// The `Name` and `Version` fields are required, and can't be dynamic. // The `Name` and `Version` fields are required, and can't be dynamic.
let name = PackageName::new( let name = PackageName::from_owned(
headers headers
.get_first_value("Name") .get_first_value("Name")
.ok_or(MetadataError::FieldNotFound("Name"))?, .ok_or(MetadataError::FieldNotFound("Name"))?,
@ -141,13 +143,15 @@ impl ResolutionMetadata {
.map(VersionSpecifiers::from); .map(VersionSpecifiers::from);
let provides_extras = headers let provides_extras = headers
.get_all_values("Provides-Extra") .get_all_values("Provides-Extra")
.filter_map(|provides_extra| match ExtraName::new(provides_extra) { .filter_map(
|provides_extra| match ExtraName::from_owned(provides_extra) {
Ok(extra_name) => Some(extra_name), Ok(extra_name) => Some(extra_name),
Err(err) => { Err(err) => {
warn!("Ignoring invalid extra: {err}"); warn!("Ignoring invalid extra: {err}");
None None
} }
}) },
)
.collect::<Vec<_>>(); .collect::<Vec<_>>();
Ok(Self { Ok(Self {

View File

@ -1,7 +1,9 @@
use std::borrow::Borrow; use std::borrow::Borrow;
use std::str::FromStr;
use itertools::Itertools; use itertools::Itertools;
use rustc_hash::FxHashMap; use rustc_hash::FxHashMap;
use uv_normalize::{ExtraName, GroupName, PackageName}; use uv_normalize::{ExtraName, GroupName, PackageName};
use uv_pep508::{ use uv_pep508::{
ExtraOperator, MarkerEnvironment, MarkerEnvironmentBuilder, MarkerExpression, MarkerOperator, ExtraOperator, MarkerEnvironment, MarkerEnvironmentBuilder, MarkerExpression, MarkerOperator,
@ -506,7 +508,7 @@ fn encode_package_extra(package: &PackageName, extra: &ExtraName) -> ExtraName {
// character in `package` or `extra` values. But if we know the length of // character in `package` or `extra` values. But if we know the length of
// the package name, we can always parse each field unambiguously. // the package name, we can always parse each field unambiguously.
let package_len = package.as_str().len(); let package_len = package.as_str().len();
ExtraName::new(format!("extra-{package_len}-{package}-{extra}")).unwrap() ExtraName::from_owned(format!("extra-{package_len}-{package}-{extra}")).unwrap()
} }
/// Encodes the given package name and its corresponding group into a valid /// Encodes the given package name and its corresponding group into a valid
@ -514,7 +516,7 @@ fn encode_package_extra(package: &PackageName, extra: &ExtraName) -> ExtraName {
fn encode_package_group(package: &PackageName, group: &GroupName) -> ExtraName { fn encode_package_group(package: &PackageName, group: &GroupName) -> ExtraName {
// See `encode_package_extra`, the same considerations apply here. // See `encode_package_extra`, the same considerations apply here.
let package_len = package.as_str().len(); let package_len = package.as_str().len();
ExtraName::new(format!("group-{package_len}-{package}-{group}")).unwrap() ExtraName::from_owned(format!("group-{package_len}-{package}-{group}")).unwrap()
} }
#[derive(Debug)] #[derive(Debug)]
@ -583,7 +585,7 @@ impl<'a> ParsedRawExtra<'a> {
} }
fn to_conflict_item(&self) -> Result<ConflictItem, ResolveError> { fn to_conflict_item(&self) -> Result<ConflictItem, ResolveError> {
let package = PackageName::new(self.package().to_string()).map_err(|name_error| { let package = PackageName::from_str(self.package()).map_err(|name_error| {
ResolveError::InvalidValueInConflictMarker { ResolveError::InvalidValueInConflictMarker {
kind: "package", kind: "package",
name_error, name_error,
@ -591,7 +593,7 @@ impl<'a> ParsedRawExtra<'a> {
})?; })?;
match *self { match *self {
ParsedRawExtra::Extra { extra, .. } => { ParsedRawExtra::Extra { extra, .. } => {
let extra = ExtraName::new(extra.to_string()).map_err(|name_error| { let extra = ExtraName::from_str(extra).map_err(|name_error| {
ResolveError::InvalidValueInConflictMarker { ResolveError::InvalidValueInConflictMarker {
kind: "extra", kind: "extra",
name_error, name_error,
@ -600,7 +602,7 @@ impl<'a> ParsedRawExtra<'a> {
Ok(ConflictItem::from((package, extra))) Ok(ConflictItem::from((package, extra)))
} }
ParsedRawExtra::Group { group, .. } => { ParsedRawExtra::Group { group, .. } => {
let group = GroupName::new(group.to_string()).map_err(|name_error| { let group = GroupName::from_str(group).map_err(|name_error| {
ResolveError::InvalidValueInConflictMarker { ResolveError::InvalidValueInConflictMarker {
kind: "group", kind: "group",
name_error, name_error,
@ -760,7 +762,7 @@ mod tests {
/// Shortcut for creating an extra name. /// Shortcut for creating an extra name.
fn create_extra(name: &str) -> ExtraName { fn create_extra(name: &str) -> ExtraName {
ExtraName::new(name.to_string()).unwrap() ExtraName::from_str(name).unwrap()
} }
/// Shortcut for creating a conflict marker from an extra name. /// Shortcut for creating a conflict marker from an extra name.

View File

@ -861,7 +861,7 @@ impl PyProjectTomlMut {
let Some(dependencies) = dependencies.as_array() else { let Some(dependencies) = dependencies.as_array() else {
continue; continue;
}; };
let Ok(extra) = ExtraName::new(extra.to_string()) else { let Ok(extra) = ExtraName::from_str(extra) else {
continue; continue;
}; };
@ -878,7 +878,7 @@ impl PyProjectTomlMut {
let Some(dependencies) = dependencies.as_array() else { let Some(dependencies) = dependencies.as_array() else {
continue; continue;
}; };
let Ok(group) = GroupName::new(group.to_string()) else { let Ok(group) = GroupName::from_str(group) else {
continue; continue;
}; };

View File

@ -126,7 +126,7 @@ pub(crate) async fn init(
// Pre-normalize the package name by removing any leading or trailing // Pre-normalize the package name by removing any leading or trailing
// whitespace, and replacing any internal whitespace with hyphens. // whitespace, and replacing any internal whitespace with hyphens.
let name = name.trim().replace(' ', "-"); let name = name.trim().replace(' ', "-");
PackageName::new(name)? PackageName::from_owned(name)?
} }
}; };