Clean up most warnings

jneem · jneem · commit 0b984adccd11 · 2024-11-05T11:47:45.000+07:00
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/cli/src/input.rs b/cli/src/input.rs
@@ -57,10 +57,7 @@ impl<C: clap::Args + Customize> Prepare for InputOptions<C> {
             let resolution = manifest.resolve()?;
             for (pkg_id, versions) in &resolution.package_map {
                 for v in versions {
-                    resolution
-                        .index
-                        .ensure_downloaded(pkg_id, v.clone())
-                        .unwrap();
+                    resolution.index.ensure_downloaded(pkg_id, *v).unwrap();
                 }
             }
             let package_map = resolution.package_map(&manifest)?;
diff --git a/git/Cargo.toml b/git/Cargo.toml
@@ -7,6 +7,7 @@ edition = "2021"
 [dependencies]
 anyhow.workspace = true
 gix = { workspace = true, features = ["blocking-network-client"] }
+serde.workspace = true
 tempfile.workspace = true
 thiserror.workspace = true
 
diff --git a/git/src/lib.rs b/git/src/lib.rs
@@ -49,7 +49,7 @@ pub struct Spec {
 }
 
 /// The different kinds of git "thing" that we can target.
-#[derive(Clone, Debug, PartialEq, Eq, Hash, Default)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash, Default, serde::Serialize, serde::Deserialize)]
 pub enum Target {
     /// By default, we target the remote HEAD.
     #[default]
diff --git a/package/src/error.rs b/package/src/error.rs
@@ -1,6 +1,8 @@
 use std::path::{Path, PathBuf};
 
-use nickel_lang_core::{eval::cache::CacheImpl, identifier::Ident, program::Program};
+use nickel_lang_core::{
+    eval::cache::CacheImpl, identifier::Ident, package::ObjectId, program::Program,
+};
 
 pub enum Error {
     Io {
@@ -16,7 +18,12 @@ pub enum Error {
         path: PathBuf,
     },
     RestrictedPath {
-        package: Ident,
+        /// The url of the git package that tried the bad import.
+        package_url: Box<gix::Url>,
+        /// The git id of the bad package.
+        package_commit: ObjectId,
+        /// The relative path of the bad package within its git repo.
+        package_path: PathBuf,
         attempted: PathBuf,
         restriction: PathBuf,
     },
@@ -63,13 +70,16 @@ impl std::fmt::Display for Error {
                 }
             }
             Error::RestrictedPath {
-                package,
                 attempted,
                 restriction,
+                package_url,
+                package_commit,
+                package_path,
             } => {
                 write!(
                     f,
-                    "package {package} tried to import path {}, but can only import from {}",
+                    "git package {package_url}@{package_commit}/{} tried to import path {}, but can only import from {}",
+                    package_path.display(),
                     attempted.display(),
                     restriction.display()
                 )
diff --git a/package/src/index/mod.rs b/package/src/index/mod.rs
@@ -226,7 +226,7 @@ impl PackageIndex {
 
     pub fn ensure_downloaded(&self, id: &Id, v: SemanticVersion) -> anyhow::Result<()> {
         let package = self
-            .package(id, v.clone())
+            .package(id, v)
             .ok_or(anyhow!("tried to download an unknown package"))?;
         let precise = Precise::Index {
             id: id.clone(),
diff --git a/package/src/lib.rs b/package/src/lib.rs
@@ -10,16 +10,13 @@ pub mod util;
 pub use manifest::ManifestFile;
 use nickel_lang_core::{cache::normalize_abs_path, package::ObjectId};
 use pubgrub::version::SemanticVersion;
-use semver::Version;
 use serde::{Deserialize, Serialize};
 use util::cache_dir;
 
-// TODO: allow targeting branches or revisions, and allow supplying a relative path
 #[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize)]
 pub struct GitDependency {
     #[serde(with = "serde_url")]
     url: gix::Url,
-    #[serde(with = "git_target")]
     spec: nickel_lang_git::Target,
 }
 
@@ -87,7 +84,7 @@ impl Dependency {
                     version: dep_version,
                 },
                 Precise::Index { id, version },
-            ) => id == dep_id && dep_version.matches(&version),
+            ) => id == dep_id && dep_version.matches(version),
             _ => false,
         }
     }
@@ -110,19 +107,6 @@ mod serde_url {
     }
 }
 
-mod git_target {
-    use nickel_lang_git::Target;
-    use serde::{de::Error, Deserialize, Serialize as _};
-
-    pub fn serialize<S: serde::Serializer>(url: &Target, ser: S) -> Result<S::Ok, S::Error> {
-        todo!()
-    }
-
-    pub fn deserialize<'de, D: serde::Deserializer<'de>>(de: D) -> Result<Target, D::Error> {
-        todo!()
-    }
-}
-
 #[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize, PartialOrd, Ord)]
 pub struct IndexPrecise {
     id: index::Id,
@@ -198,7 +182,7 @@ impl Precise {
 
     pub fn version(&self) -> Option<SemanticVersion> {
         match self {
-            Precise::Index { version, .. } => Some(version.clone()),
+            Precise::Index { version, .. } => Some(*version),
             _ => None,
         }
     }
diff --git a/package/src/lock.rs b/package/src/lock.rs
@@ -61,7 +61,6 @@ pub struct LockFile {
 }
 
 impl LockFile {
-    // TODO: move the implementation here
     pub fn new(manifest: &ManifestFile, resolution: &Resolution) -> Result<Self, Error> {
         // We don't put all packages in the lock file: we ignore dependencies (and therefore also
         // transitive dependencies) of path deps. In order to figure out what to include, we
diff --git a/package/src/manifest.rs b/package/src/manifest.rs
@@ -195,7 +195,7 @@ impl Realization {
                 return Ok(());
             }
             (Dependency::Git(git), _) => {
-                let id = self.realize_one(&git)?;
+                let id = self.realize_one(git)?;
                 UnversionedPrecise::Git {
                     id,
                     url: git.url.clone(),
@@ -206,12 +206,18 @@ impl Realization {
             (Dependency::Path { path }, Some(relative_to)) => {
                 let p = normalize_rel_path(&relative_to.local_path().join(path));
                 match relative_to {
-                    Precise::Git { id, url: repo, .. } => {
+                    Precise::Git {
+                        id,
+                        url: repo,
+                        path,
+                    } => {
                         let repo_path = repo_root(id);
                         let p = p
                             .strip_prefix(&repo_path)
                             .map_err(|_| Error::RestrictedPath {
-                                package: "TODO".into(),
+                                package_url: Box::new(repo.clone()),
+                                package_commit: *id,
+                                package_path: path.clone(),
                                 attempted: p.clone(),
                                 restriction: repo_path.to_owned(),
                             })?;
diff --git a/package/src/resolve.rs b/package/src/resolve.rs
@@ -10,28 +10,21 @@
 //! with version `2.0`. Since we present them to pubgrub
 //! as different packages, they can both appear in the final resolution.
 
-use std::{
-    borrow::Borrow,
-    collections::HashMap,
-    path::{Path, PathBuf},
-};
+use std::{borrow::Borrow, collections::HashMap, path::PathBuf};
 
 use nickel_lang_core::{cache::normalize_path, identifier::Ident, package::PackageMap};
 use pubgrub::{
     report::{DefaultStringReporter, Reporter as _},
     solver::DependencyProvider,
-    version::{SemanticVersion, Version},
+    version::SemanticVersion,
 };
-use semver::Comparator;
 
 use crate::{
     error::{Error, IoResultExt as _},
     index::{Id, IndexDependency, PackageIndex},
     lock::LockFile,
     manifest::Realization,
-    util::semver_to_pg,
-    Dependency, GitDependency, IndexPrecise, ManifestFile, ObjectId, Precise, UnversionedPackage,
-    VersionReq,
+    Dependency, IndexPrecise, ManifestFile, ObjectId, Precise, VersionReq,
 };
 
 type VersionRange = pubgrub::range::Range<SemanticVersion>;
@@ -104,18 +97,6 @@ impl PackageRegistry {
     }
 }
 
-fn bucket_versions(
-    vs: impl Iterator<Item = SemanticVersion>,
-) -> impl Iterator<Item = SemanticVersion> {
-    let mut vs: Vec<_> = vs
-        .map(BucketVersion::from)
-        .map(SemanticVersion::from)
-        .collect();
-    vs.sort();
-    vs.dedup();
-    vs.into_iter()
-}
-
 /// A bucket version represents a collection of compatible semver versions.
 #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
 pub enum BucketVersion {
@@ -298,32 +279,25 @@ impl DependencyProvider<Package, SemanticVersion> for PackageRegistry {
                 let deps = self
                     .unversioned_deps(p)
                     .into_iter()
-                    .map(|dep| {
-                        match dep {
-                            Dependency::Git(_) | Dependency::Path { .. } => {
-                                let dep_precise = self.realized_unversioned.dependency
-                                    [&(precise.clone(), dep.clone())]
-                                    .clone();
-                                (
-                                    Package::Unversioned(dep_precise.into()),
-                                    VersionRange::any(),
-                                )
-                            }
-                            // We're making a proxy for every dependency on a repo package, but we could skip
-                            // the proxy if the dependency range stays within a single semver range.
-                            Dependency::Index { id, version } => {
-                                let pkg = Package::Bucket(Bucket {
-                                    id,
-                                    version: version.clone().into(),
-                                });
-
-                                let range = match version {
-                                    VersionReq::Compatible(v) => VersionRange::higher_than(v),
-                                    VersionReq::Exact(v) => VersionRange::exact(v),
-                                };
-
-                                (pkg, range)
-                            }
+                    .map(|dep| match dep {
+                        Dependency::Git(_) | Dependency::Path { .. } => {
+                            let dep_precise = self.realized_unversioned.dependency
+                                [&(precise.clone(), dep.clone())]
+                                .clone();
+                            (Package::Unversioned(dep_precise), VersionRange::any())
+                        }
+                        Dependency::Index { id, version } => {
+                            let pkg = Package::Bucket(Bucket {
+                                id,
+                                version: version.clone().into(),
+                            });
+
+                            let range = match version {
+                                VersionReq::Compatible(v) => VersionRange::higher_than(v),
+                                VersionReq::Exact(v) => VersionRange::exact(v),
+                            };
+
+                            (pkg, range)
                         }
                     })
                     .collect();
@@ -358,13 +332,6 @@ impl DependencyProvider<Package, SemanticVersion> for PackageRegistry {
     }
 }
 
-fn version_req_to_range(req: &VersionReq) -> pubgrub::range::Range<SemanticVersion> {
-    match req {
-        VersionReq::Compatible(v) => VersionRange::higher_than(v.clone()),
-        VersionReq::Exact(v) => VersionRange::exact(v.clone()),
-    }
-}
-
 pub struct Resolution {
     pub realization: Realization,
     pub package_map: HashMap<Id, Vec<SemanticVersion>>,
@@ -375,12 +342,12 @@ pub fn resolve(manifest: &ManifestFile) -> Result<Resolution, Error> {
     resolve_with_lock(manifest, &LockFile::default())
 }
 
-fn previously_locked(top_level: &Package, lock: &LockFile) -> HashMap<Package, SemanticVersion> {
+fn previously_locked(_top_level: &Package, lock: &LockFile) -> HashMap<Package, SemanticVersion> {
     fn precise_to_index(p: &Precise) -> Option<IndexPrecise> {
         match p {
             Precise::Index { id, version } => Some(IndexPrecise {
                 id: id.clone(),
-                version: version.clone(),
+                version: *version,
             }),
             _ => None,
         }
@@ -414,11 +381,10 @@ fn previously_locked(top_level: &Package, lock: &LockFile) -> HashMap<Package, S
 pub fn resolve_with_lock(manifest: &ManifestFile, lock: &LockFile) -> Result<Resolution, Error> {
     let mut realization = Realization::default();
 
-    // FIXME: figure out how to represent the top-level package, recalling that `realization` assumes
-    // that every time we need to look up a dependency we need a parent package.
+    // TODO: this assumes that the top-level package has a path. Is there a a use-case for resolving
+    // packages without a top-level path?
     let root_path = manifest.parent_dir.as_deref();
     for dep in manifest.dependencies.values() {
-        // FIXME: unwrap
         realization.realize_all(root_path.unwrap(), dep, None)?;
     }
     let top_level = UnversionedPrecise::Path {
@@ -449,7 +415,7 @@ pub fn resolve_with_lock(manifest: &ManifestFile, lock: &LockFile) -> Result<Res
     let mut selected = HashMap::<Id, Vec<SemanticVersion>>::new();
     for (pkg, vers) in resolution.iter() {
         if let Package::Bucket(Bucket { id, .. }) = pkg {
-            selected.entry(id.clone()).or_default().push(vers.clone());
+            selected.entry(id.clone()).or_default().push(*vers);
         }
     }
     Ok(Resolution {
@@ -478,12 +444,11 @@ impl Resolution {
             },
             Dependency::Index { id, version } => Precise::Index {
                 id: id.clone(),
-                version: self.package_map[id]
+                version: *self.package_map[id]
                     .iter()
                     .filter(|v| version.matches(v))
                     .max()
-                    .unwrap()
-                    .clone(),
+                    .unwrap(),
             },
         }
     }
@@ -500,7 +465,7 @@ impl Resolution {
                     .collect()
             }
             Precise::Index { id, version } => {
-                let pkg = self.index.package(id, version.clone()).unwrap();
+                let pkg = self.index.package(id, *version).unwrap();
                 pkg.deps
                     .into_iter()
                     .map(move |(dep_name, dep)| {
@@ -529,7 +494,7 @@ impl Resolution {
         let index_precises = self.package_map.iter().flat_map(|(id, vs)| {
             vs.iter().map(|v| Precise::Index {
                 id: id.clone(),
-                version: v.clone(),
+                version: *v,
             })
         });
         ret.extend(index_precises);

Original file line number	Diff line number	Diff line change
`@@ -57,10 +57,7 @@ impl<C: clap::Args + Customize> Prepare for InputOptions<C> {`
`57`	`57`	`let resolution = manifest.resolve()?;`
`58`	`58`	`for (pkg_id, versions) in &resolution.package_map {`
`59`	`59`	`for v in versions {`
`60`		`- resolution`
`61`		`- .index`
`62`		`- .ensure_downloaded(pkg_id, v.clone())`
`63`		`- .unwrap();`
	`60`	`+ resolution.index.ensure_downloaded(pkg_id, *v).unwrap();`
`64`	`61`	`}`
`65`	`62`	`}`
`66`	`63`	`let package_map = resolution.package_map(&manifest)?;`
Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ pub struct Spec {`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`/// The different kinds of git "thing" that we can target.`
`52`		`-#[derive(Clone, Debug, PartialEq, Eq, Hash, Default)]`
	`52`	`+#[derive(Clone, Debug, PartialEq, Eq, Hash, Default, serde::Serialize, serde::Deserialize)]`
`53`	`53`	`pub enum Target {`
`54`	`54`	`/// By default, we target the remote HEAD.`
`55`	`55`	`#[default]`
Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,6 @@ pub struct LockFile {`
`61`	`61`	`}`
`62`	`62`
`63`	`63`	`impl LockFile {`
`64`		`- // TODO: move the implementation here`
`65`	`64`	`pub fn new(manifest: &ManifestFile, resolution: &Resolution) -> Result<Self, Error> {`
`66`	`65`	`// We don't put all packages in the lock file: we ignore dependencies (and therefore also`
`67`	`66`	`// transitive dependencies) of path deps. In order to figure out what to include, we`