Use deserialize to simplify manifest construction

Author: jneem

core/stdlib/std.ncl

@@ -2582,18 +2582,42 @@
       = fun x n => %pow% x n,
   },
 
-  package = {
+  package =
+  let
+    # https://semver.org is kind enough to supply this "official" semver regex.
+    semver_re =  m%"^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$"%
+  in
+  let
+    # An expression representing a range of semver versions, like `<1.2`.
+    # Unlike semver itself, semver range expressions don't seem to have an official standard.
+    semver_main_range_re =  m%"(~|=|\^|<|>|<=|>=)?(0|[1-9]\d*)(\.(0|[1-9]\d*))?(\.(0|[1-9]\d*))?"%
+  in
+  let
+    # For requirements with non-empty prereleases, we support exactly two flavors: 0.1.2-pre3 or =0.1.2-pre3.
+    # We could also support inequalities, but we should avoid ^ and ~ because prereleases have no guaranteed
+    # compatibility semantics.
+    semver_prerelease_range_re = m%"=?(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)-((0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(\.(0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*)"%
+  in
+  let semver_range_re = "(%{semver_main_range_re})|(%{semver_prerelease_range_re})" in
+  let semver_req_re = m%"^(%{semver_range_re}(,\s*%{semver_range_re})*)$"% in
+  {
+    is_semver_req
+      : String -> Bool
+      | doc m%"
+        Returns true if a string is a valid semantic version requirement.
+      "%
+      = std.string.is_match semver_req_re
+      ,
     is_semver
       : String -> Bool
       | doc m%"
         Returns true if a string is a valid semantic version identifier.
       "%
-      # This lovely expression is the officially recommended semver regex from https://semver.org
-      = std.string.is_match m%"^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$"%
+      = std.string.is_match semver_re
       ,
     Semver 
       | doc m%"
-        A contract that checks whether a string represents a valid semantic version ("semver") identifier.
+        A contract for semantic version ("semver") identifiers.
 
         # Examples
 
@@ -2606,6 +2630,20 @@
         ```
       "%
       = std.contract.from_predicate is_semver,
+    SemverReq
+      | doc m%"
+        A contract for semantic version ("semver") requirements.
+
+        # Examples
+
+        ```nickel
+        "^1.2" | std.package.SemverReq
+        => "^1.2"
+
+        ">=1.2, <1.4" | std.package.SemverReq
+        => ">=1.2, <1.4"
+      "%
+      = std.contract.from_predicate is_semver_req,
     Manifest = {
       name
         | String
@@ -2614,12 +2652,14 @@
         "%,
 
       version
+        | String
         | Semver
         | doc m%"
           The version of this package.
         "%,
 
-      nickel-version
+      nickel_version
+        | String
         | Semver
         | doc m%"
           The minimal nickel version required for this package.
@@ -2630,7 +2670,7 @@
           [|
             'Path String,
             'Git { url | String, },
-            'Index { name | String, version | String | Semver },
+            'Index { name | String, version | String | SemverReq },
           |]
           }
         | doc m%"

package/src/lib.rs

@@ -16,10 +16,11 @@ use util::cache_dir;
 /// A source includes the place to fetch a package from (e.g. git or a registry),
 /// along with possibly some narrowing-down of the allowed versions (e.g. a range
 /// of versions, or a git commit id).
-#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize)]
 pub enum Dependency {
     // TODO: allow targeting branches or revisions, and allow supplying a relative path
     Git {
+        #[serde(with = "serde_url")]
         url: gix::Url,
         //tree: Option<git2::Oid>,
     },

package/src/manifest.rs

@@ -3,7 +3,6 @@ use std::{
     path::{Path, PathBuf},
 };
 
-use gix::Url;
 use nickel_lang_core::{
     cache::normalize_rel_path,
     eval::cache::CacheImpl,
@@ -24,7 +23,15 @@ use crate::{
     Dependency, Precise,
 };
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Deserialize)]
+struct ManifestFileFormat {
+    pub name: Ident,
+    pub version: semver::Version,
+    pub nickel_version: semver::Version,
+    pub dependencies: HashMap<Ident, Dependency>,
+}
+
+#[derive(Clone, Debug, PartialEq)]
 pub struct ManifestFile {
     // The directory containing the manifest file. Path deps are resolved relative to this.
     // If `None`, path deps aren't allowed.
@@ -148,142 +155,23 @@ impl ManifestFile {
     fn from_term(rt: &RichTerm) -> Result<Self, Error> {
         // This is only ever called with terms that have passed the `std.package.Manifest`
         // contract, so we can assume that they have the right fields.
-        fn err(s: &str) -> Error {
-            Error::InternalManifestError { msg: s.to_owned() }
-        }
-
-        let Term::Record(data) = rt.as_ref() else {
-            return Err(err("manifest not a record"));
-        };
-
-        // FIXME: yuck
-        let name = data
-            .fields
-            .get(&Ident::new("name"))
-            .ok_or_else(|| err("no name"))?
-            .value
-            .as_ref()
-            .ok_or_else(|| err("name has no value"))?;
-        let Term::Str(name) = name.as_ref() else {
-            return Err(err("name not a string"));
-        };
-
-        let version = data
-            .fields
-            .get(&Ident::new("version"))
-            .ok_or_else(|| err("no version"))?
-            .value
-            .as_ref()
-            .ok_or_else(|| err("version has no value"))?;
-        let Term::Str(version) = version.as_ref() else {
-            return Err(err("version not a string"));
-        };
-
-        let nickel_version = data
-            .fields
-            .get(&Ident::new("nickel-version"))
-            .ok_or_else(|| err("no nickel-version"))?
-            .value
-            .as_ref()
-            .ok_or_else(|| err("nickel-version has no value"))?;
-        let Term::Str(nickel_version) = nickel_version.as_ref() else {
-            return Err(err("nickel-version not a string"));
-        };
-
-        let deps = data
-            .fields
-            .get(&Ident::new("dependencies"))
-            .ok_or_else(|| err("no dependencies"))?
-            .value
-            .as_ref()
-            .ok_or_else(|| err("dependencies has no value"))?;
-        let Term::Record(deps) = deps.as_ref() else {
-            return Err(err("dependencies not a record"));
-        };
-
-        let mut ret = Self {
-            dependencies: HashMap::new(),
+        let ManifestFileFormat {
+            name,
+            version,
+            nickel_version,
+            dependencies,
+        } = ManifestFileFormat::deserialize(rt.clone())
+            .map_err(|e| Error::InternalManifestError { msg: e.to_string() })?;
+        Ok(Self {
             parent_dir: None,
-            version: version.parse().map_err(|_| err("invalid version"))?,
-            nickel_version: nickel_version
-                .parse()
-                .map_err(|_| err("invalid nickel version"))?,
-            name: Ident::new(name),
-        };
-
-        for (name, dep) in &deps.fields {
-            let Term::EnumVariant { tag, arg, .. } = dep
-                .value
-                .as_ref()
-                .ok_or_else(|| err("dependency has no value"))?
-                .as_ref()
-            else {
-                return Err(err("dependency not an enum"));
-            };
-
-            match tag.ident().label() {
-                "Git" => {
-                    let Term::Record(data) = arg.as_ref() else {
-                        return Err(err("payload wasn't a record"));
-                    };
-
-                    let url = data
-                        .fields
-                        .get(&Ident::new("url"))
-                        .ok_or_else(|| err("no url"))?
-                        .value
-                        .as_ref()
-                        .ok_or_else(|| err("url has no value"))?;
-                    let Term::Str(url) = url.as_ref() else {
-                        return Err(err("url wasn't a string"));
-                    };
-
-                    ret.dependencies.insert(
-                        name.ident(),
-                        Dependency::Git {
-                            url: Url::try_from(url.to_string()).map_err(|e| Error::InvalidUrl {
-                                url: url.to_string(),
-                                msg: e.to_string(),
-                            })?,
-                        },
-                    );
-                }
-                "Path" => {
-                    let Term::Str(path) = arg.as_ref() else {
-                        return Err(err("payload wasn't a string"));
-                    };
-
-                    ret.dependencies.insert(
-                        name.ident(),
-                        Dependency::Path {
-                            path: PathBuf::from(path.to_string()),
-                        },
-                    );
-                }
-                "Index" => {
-                    let payload: IndexPayload = serde_json::from_value(
-                        serde_json::to_value(arg.as_ref()).map_err(|_| err("bad payload"))?,
-                    )
-                    .map_err(|_| err("bad payload"))?;
-
-                    let id: crate::index::Id = payload.name.parse().unwrap();
-                    let version: semver::VersionReq = payload.version.parse().unwrap();
-                    ret.dependencies
-                        .insert(name.ident(), Dependency::Index { id, version });
-                }
-                _ => return Err(err("bad tag")),
-            }
-        }
-        Ok(ret)
+            name,
+            version,
+            nickel_version,
+            dependencies,
+        })
     }
 }
 
-#[derive(Deserialize)]
-struct IndexPayload {
-    name: String,
-    version: String,
-}
-
 #[derive(Clone, Debug)]
 pub struct RealizedDependency {
     /// Either `Git` or `Path`.
@@ -408,3 +296,26 @@ impl Realization {
         Ok(id)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn manifest() {
+        let manifest = ManifestFile::from_contents(
+            r#"{name = "foo", version = "1.0.0", nickel_version = "1.8.0"}"#.as_bytes(),
+        )
+        .unwrap();
+        assert_eq!(
+            manifest,
+            ManifestFile {
+                parent_dir: None,
+                name: "foo".into(),
+                version: semver::Version::new(1, 0, 0),
+                nickel_version: semver::Version::new(1, 8, 0),
+                dependencies: HashMap::default()
+            }
+        )
+    }
+}