Add separate newtypes

Created using spr 1.3.6-beta.1

Cargo.toml

@@ -33,3 +33,7 @@ tokio = { version = "1.26", features = [ "full" ] }
 toml = "0.7.3"
 topological-sort = "0.2.2"
 walkdir = "2.3"
+
+[dev-dependencies]
+proptest = "1.6.0"
+test-strategy = "0.4.0"

src/config/identifier.rs

@@ -7,19 +7,100 @@ use std::{borrow::Cow, fmt, str::FromStr};
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
 
-/// A unique identifier for a configuration parameter.
+macro_rules! ident_newtype {
+    ($id:ident) => {
+        impl $id {
+            /// Creates a new identifier at runtime.
+            pub fn new<S: Into<String>>(s: S) -> Result<Self, InvalidConfigIdent> {
+                ConfigIdent::new(s).map(Self)
+            }
+
+            /// Creates a new identifier from a static string.
+            pub fn new_static(s: &'static str) -> Result<Self, InvalidConfigIdent> {
+                ConfigIdent::new_static(s).map(Self)
+            }
+
+            /// Creates a new identifier at compile time, panicking if it is
+            /// invalid.
+            pub const fn new_const(s: &'static str) -> Self {
+                Self(ConfigIdent::new_const(s))
+            }
+
+            /// Returns the identifier as a string.
+            #[inline]
+            pub fn as_str(&self) -> &str {
+                self.0.as_str()
+            }
+        }
+
+        impl AsRef<str> for $id {
+            #[inline]
+            fn as_ref(&self) -> &str {
+                self.0.as_ref()
+            }
+        }
+
+        impl std::fmt::Display for $id {
+            #[inline]
+            fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+                self.0.fmt(f)
+            }
+        }
+
+        impl FromStr for $id {
+            type Err = InvalidConfigIdent;
+
+            fn from_str(s: &str) -> Result<Self, Self::Err> {
+                ConfigIdent::new(s).map(Self)
+            }
+        }
+    };
+}
+
+/// A unique identifier for a package name.
 ///
-/// Config identifiers must be:
+/// Package names must be:
 ///
 /// * non-empty
 /// * ASCII printable
 /// * first character must be a letter
 /// * contain only letters, numbers, underscores, and hyphens
 ///
-/// In general, config identifiers represent Rust package and Oxide service names.
+/// These generally match the rules of Rust package names.
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize)]
+#[serde(transparent)]
+pub struct PackageName(ConfigIdent);
+ident_newtype!(PackageName);
+
+/// A unique identifier for a service name.
+///
+/// Package names must be:
+///
+/// * non-empty
+/// * ASCII printable
+/// * first character must be a letter
+/// * contain only letters, numbers, underscores, and hyphens
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize)]
+#[serde(transparent)]
+pub struct ServiceName(ConfigIdent);
+ident_newtype!(ServiceName);
+
+/// A unique identifier for a target preset.
+///
+/// Package names must be:
+///
+/// * non-empty
+/// * ASCII printable
+/// * first character must be a letter
+/// * contain only letters, numbers, underscores, and hyphens
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize)]
+#[serde(transparent)]
+pub struct PresetName(ConfigIdent);
+ident_newtype!(PresetName);
+
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize)]
 #[serde(transparent)]
-pub struct ConfigIdent(Cow<'static, str>);
+pub(crate) struct ConfigIdent(Cow<'static, str>);
 
 impl ConfigIdent {
     /// Creates a new config identifier at runtime.
@@ -143,6 +224,10 @@ impl fmt::Display for InvalidConfigIdent {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use serde_json::json;
+    use test_strategy::proptest;
+
+    static IDENT_REGEX: &str = r"[a-zA-Z][a-zA-Z0-9_-]*";
 
     #[test]
     fn valid_identifiers() {
@@ -151,7 +236,28 @@ mod tests {
         ];
         for &id in &valid {
             ConfigIdent::new(id).unwrap_or_else(|error| {
-                panic!("{} should have succeeded, but failed with: {:?}", id, error);
+                panic!(
+                    "ConfigIdent::new for {} should have succeeded, but failed with: {:?}",
+                    id, error
+                );
+            });
+            PackageName::new(id).unwrap_or_else(|error| {
+                panic!(
+                    "PackageName::new for {} should have succeeded, but failed with: {:?}",
+                    id, error
+                );
+            });
+            ServiceName::new(id).unwrap_or_else(|error| {
+                panic!(
+                    "ServiceName::new for {} should have succeeded, but failed with: {:?}",
+                    id, error
+                );
+            });
+            PresetName::new(id).unwrap_or_else(|error| {
+                panic!(
+                    "PresetName::new for {} should have succeeded, but failed with: {:?}",
+                    id, error
+                );
             });
         }
     }
@@ -162,7 +268,83 @@ mod tests {
             "", "1", "_", "-", "1_", "-a", "_a", "a!", "a ", "a\n", "a\t", "a\r", "a\x7F", "aɑ",
         ];
         for &id in &invalid {
-            ConfigIdent::new(id).expect_err(&format!("{} should have failed", id));
+            ConfigIdent::new(id)
+                .expect_err(&format!("ConfigIdent::new for {} should have failed", id));
+            PackageName::new(id)
+                .expect_err(&format!("PackageName::new for {} should have failed", id));
+            ServiceName::new(id)
+                .expect_err(&format!("ServiceName::new for {} should have failed", id));
+            PresetName::new(id)
+                .expect_err(&format!("PresetName::new for {} should have failed", id));
+
+            // Also ensure that deserialization fails.
+            let json = json!(id);
+            serde_json::from_value::<ConfigIdent>(json.clone()).expect_err(&format!(
+                "ConfigIdent deserialization for {} should have failed",
+                id
+            ));
+            serde_json::from_value::<PackageName>(json.clone()).expect_err(&format!(
+                "PackageName deserialization for {} should have failed",
+                id
+            ));
+            serde_json::from_value::<ServiceName>(json.clone()).expect_err(&format!(
+                "ServiceName deserialization for {} should have failed",
+                id
+            ));
+            serde_json::from_value::<PresetName>(json.clone()).expect_err(&format!(
+                "PresetName deserialization for {} should have failed",
+                id
+            ));
         }
     }
+
+    #[proptest]
+    fn valid_identifiers_proptest(#[strategy(IDENT_REGEX)] id: String) {
+        ConfigIdent::new(&id).unwrap_or_else(|error| {
+            panic!(
+                "ConfigIdent::new for {} should have succeeded, but failed with: {:?}",
+                id, error
+            );
+        });
+        PackageName::new(&id).unwrap_or_else(|error| {
+            panic!(
+                "PackageName::new for {} should have succeeded, but failed with: {:?}",
+                id, error
+            );
+        });
+        ServiceName::new(&id).unwrap_or_else(|error| {
+            panic!(
+                "ServiceName::new for {} should have succeeded, but failed with: {:?}",
+                id, error
+            );
+        });
+        PresetName::new(&id).unwrap_or_else(|error| {
+            panic!(
+                "PresetName::new for {} should have succeeded, but failed with: {:?}",
+                id, error
+            );
+        });
+    }
+
+    #[derive(Debug, Deserialize, Serialize, PartialEq, Eq)]
+    struct AllIdentifiers {
+        config: ConfigIdent,
+        package: PackageName,
+        service: ServiceName,
+        preset: PresetName,
+    }
+
+    #[proptest]
+    fn valid_identifiers_proptest_serde(#[strategy(IDENT_REGEX)] id: String) {
+        let all = AllIdentifiers {
+            config: ConfigIdent::new(&id).unwrap(),
+            package: PackageName::new(&id).unwrap(),
+            service: ServiceName::new(&id).unwrap(),
+            preset: PresetName::new(&id).unwrap(),
+        };
+
+        let json = serde_json::to_value(&all).unwrap();
+        let deserialized: AllIdentifiers = serde_json::from_value(json).unwrap();
+        assert_eq!(all, deserialized);
+    }
 }

src/config/imp.rs

@@ -12,12 +12,12 @@ use std::path::Path;
 use thiserror::Error;
 use topological_sort::TopologicalSort;
 
-use super::ConfigIdent;
+use super::PackageName;
 
 /// Describes a set of packages to act upon.
 ///
 /// This structure maps "package name" to "package"
-pub struct PackageMap<'a>(pub BTreeMap<&'a ConfigIdent, &'a Package>);
+pub struct PackageMap<'a>(pub BTreeMap<&'a PackageName, &'a Package>);
 
 // The name of a file which should be created by building a package.
 #[derive(Clone, Eq, Hash, Ord, PartialEq, PartialOrd)]
@@ -70,12 +70,12 @@ impl<'a> PackageMap<'a> {
 ///
 /// Returns packages in batches that may be built concurrently.
 pub struct PackageDependencyIter<'a> {
-    lookup_by_output: BTreeMap<OutputFile, (&'a ConfigIdent, &'a Package)>,
+    lookup_by_output: BTreeMap<OutputFile, (&'a PackageName, &'a Package)>,
     outputs: TopologicalSort<OutputFile>,
 }
 
 impl<'a> Iterator for PackageDependencyIter<'a> {
-    type Item = Vec<(&'a ConfigIdent, &'a Package)>;
+    type Item = Vec<(&'a PackageName, &'a Package)>;
 
     fn next(&mut self) -> Option<Self::Item> {
         if self.outputs.is_empty() {
@@ -105,7 +105,7 @@ impl<'a> Iterator for PackageDependencyIter<'a> {
 pub struct Config {
     /// Packages to be built and installed.
     #[serde(default, rename = "package")]
-    pub packages: BTreeMap<ConfigIdent, Package>,
+    pub packages: BTreeMap<PackageName, Package>,
 }
 
 impl Config {
@@ -156,22 +156,24 @@ pub fn parse<P: AsRef<Path>>(path: P) -> Result<Config, ParseError> {
 
 #[cfg(test)]
 mod test {
+    use crate::config::ServiceName;
+
     use super::*;
 
     #[test]
     fn test_order() {
-        let pkg_a_name = ConfigIdent::new_const("pkg-a");
+        let pkg_a_name = PackageName::new_const("pkg-a");
         let pkg_a = Package {
-            service_name: ConfigIdent::new_const("a"),
+            service_name: ServiceName::new_const("a"),
             source: PackageSource::Manual,
             output: PackageOutput::Tarball,
             only_for_targets: None,
             setup_hint: None,
         };
 
-        let pkg_b_name = ConfigIdent::new_const("pkg-b");
+        let pkg_b_name = PackageName::new_const("pkg-b");
         let pkg_b = Package {
-            service_name: ConfigIdent::new_const("b"),
+            service_name: ServiceName::new_const("b"),
             source: PackageSource::Composite {
                 packages: vec![pkg_a.get_output_file(&pkg_a_name)],
             },
@@ -200,10 +202,10 @@ mod test {
     #[test]
     #[should_panic(expected = "cyclic dependency in package manifest")]
     fn test_cyclic_dependency() {
-        let pkg_a_name = ConfigIdent::new_const("pkg-a");
-        let pkg_b_name = ConfigIdent::new_const("pkg-b");
+        let pkg_a_name = PackageName::new_const("pkg-a");
+        let pkg_b_name = PackageName::new_const("pkg-b");
         let pkg_a = Package {
-            service_name: ConfigIdent::new_const("a"),
+            service_name: ServiceName::new_const("a"),
             source: PackageSource::Composite {
                 packages: vec![String::from("pkg-b.tar")],
             },
@@ -212,7 +214,7 @@ mod test {
             setup_hint: None,
         };
         let pkg_b = Package {
-            service_name: ConfigIdent::new_const("b"),
+            service_name: ServiceName::new_const("b"),
             source: PackageSource::Composite {
                 packages: vec![String::from("pkg-a.tar")],
             },
@@ -238,9 +240,9 @@ mod test {
     #[test]
     #[should_panic(expected = "Could not find a package which creates 'pkg-b.tar'")]
     fn test_missing_dependency() {
-        let pkg_a_name = ConfigIdent::new_const("pkg-a");
+        let pkg_a_name = PackageName::new_const("pkg-a");
         let pkg_a = Package {
-            service_name: ConfigIdent::new_const("a"),
+            service_name: ServiceName::new_const("a"),
             source: PackageSource::Composite {
                 packages: vec![String::from("pkg-b.tar")],
             },