Remove the half-backed lockfile resolution

jneem · jneem · commit 28077fd976fe · 2024-05-06T17:50:42.000-05:00
diff --git a/core/src/cache.rs b/core/src/cache.rs
@@ -5,7 +5,7 @@ use crate::eval::cache::Cache as EvalCache;
 use crate::eval::Closure;
 #[cfg(feature = "nix-experimental")]
 use crate::nix_ffi;
-use crate::package::{self, LockFile, LockedPackageSource};
+use crate::package::{self, LockedPackageSource, ResolvedLockFile};
 use crate::parser::{lexer::Lexer, ErrorTolerantParser};
 use crate::position::TermPos;
 use crate::program::FieldPath;
@@ -100,7 +100,7 @@ pub struct Cache {
     error_tolerance: ErrorTolerance,
     import_paths: Vec<PathBuf>,
 
-    lock_file: LockFile,
+    lock_file: ResolvedLockFile,
 
     #[cfg(debug_assertions)]
     /// Skip loading the stdlib, used for debugging purpose
@@ -350,7 +350,7 @@ impl Cache {
             stdlib_ids: None,
             error_tolerance,
             import_paths: Vec::new(),
-            lock_file: LockFile::default(),
+            lock_file: ResolvedLockFile::default(),
 
             #[cfg(debug_assertions)]
             skip_stdlib: false,
@@ -364,7 +364,7 @@ impl Cache {
         self.import_paths.extend(paths.map(PathBuf::from));
     }
 
-    pub fn set_lock_file(&mut self, lock_file: LockFile) {
+    pub fn set_lock_file(&mut self, lock_file: ResolvedLockFile) {
         self.lock_file = lock_file;
     }
 
@@ -1346,39 +1346,10 @@ impl ImportResolver for Cache {
         pos: &TermPos,
     ) -> Result<(ResolvedTerm, FileId), ImportError> {
         let (possible_parents, pkg_id) = if let Some(pkg) = pkg {
-            let pkg_id = if let Some(parent_pkg) = parent.and_then(|p| self.package.get(&p)) {
-                // The parent package should have come from the lock file.
-                let parent = self
-                    .lock_file
-                    .data
-                    .packages
-                    .get(parent_pkg)
-                    .ok_or_else(|| ImportError::InternalError {
-                        msg: format!("unknown parent package {parent_pkg:?}"),
-                        pos: *pos,
-                    })?;
-                parent
-                    .dependencies
-                    .get(pkg)
-                    .ok_or_else(|| ImportError::MissingDependency {
-                        parent: Some(Box::new((parent.name.clone(), parent_pkg.clone()))),
-                        missing: pkg.clone(),
-                        pos: *pos,
-                    })?
-                    .clone()
-            } else {
-                self.lock_file
-                    .data
-                    .dependencies
-                    .get(pkg)
-                    .ok_or_else(|| ImportError::MissingDependency {
-                        parent: None,
-                        missing: pkg.clone(),
-                        pos: *pos,
-                    })?
-                    .clone()
-            };
-            (vec![pkg_id.local_path()], Some(pkg_id))
+            let pkg_id = self
+                .lock_file
+                .get(parent.and_then(|p| self.package.get(&p)), pkg, pos)?;
+            (vec![pkg_id.local_path()], Some(pkg_id.clone()))
         } else {
             // `parent` is the file that did the import. We first look in its containing directory, followed by
             // the directories in the import path.
diff --git a/core/src/error/mod.rs b/core/src/error/mod.rs
@@ -2,7 +2,6 @@
 //!
 //! Define error types for different phases of the execution, together with functions to generate a
 //! [codespan](https://crates.io/crates/codespan-reporting) diagnostic from them.
-use std::path::PathBuf;
 
 pub use codespan::{FileId, Files};
 pub use codespan_reporting::diagnostic::{Diagnostic, Label, LabelStyle};
@@ -583,14 +582,6 @@ pub enum ImportError {
         missing: Name,
         pos: TermPos,
     },
-    /// A package tried to import a file from a path it is not permitted to access.
-    /// (For example, a git dependency tried to import a path from a parent directory.)
-    InvalidPath {
-        /// The path that they tried to import.
-        attempted: PathBuf,
-        /// The places they're allowed to import from.
-        restriction: PathBuf,
-    },
 }
 
 /// An error occurred during serialization.
@@ -2587,12 +2578,6 @@ impl IntoDiagnostics<FileId> for ImportError {
 
                 vec![Diagnostic::error().with_message(msg).with_labels(labels)]
             }
-            ImportError::InvalidPath {
-                attempted,
-                restriction,
-            } => {
-                todo!()
-            }
         }
     }
 }
diff --git a/core/src/package.rs b/core/src/package.rs
@@ -1,3 +1,9 @@
+//! This module contains the part of package management that's needed by the evaluator.
+//!
+//! In particular, it doesn't contain any support for version resolution or
+//! dependency fetching, but defines lockfiles and allows them to be used to
+//! resolve package dependencies to paths.
+
 use std::{
     collections::HashMap,
     path::{Path, PathBuf},
@@ -7,10 +13,7 @@ use std::{
 use directories::ProjectDirs;
 use serde::{Deserialize, Serialize};
 
-use crate::{
-    cache::{normalize_abs_path, normalize_path},
-    error::ImportError,
-};
+use crate::{error::ImportError, position::TermPos};
 
 const ID_LEN: usize = 20;
 
@@ -134,8 +137,16 @@ impl From<Name> for String {
 /// A locked package source uniquely identifies the source of the package (with a specific version).
 #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)]
 pub enum LockedPackageSource {
-    Git { repo: String, tree: ObjectId },
-    Path { path: PathBuf },
+    Git {
+        repo: String,
+        tree: ObjectId,
+        /// Path of the package relative to the git repo root.
+        #[serde(default)]
+        path: PathBuf,
+    },
+    Path {
+        path: PathBuf,
+    },
 }
 
 impl LockedPackageSource {
@@ -158,31 +169,6 @@ impl LockedPackageSource {
         matches!(self, LockedPackageSource::Path { .. })
     }
 
-    fn canonicalize(
-        self,
-        parent_path: &Path,
-        restriction: Option<&Path>,
-    ) -> Result<Self, ImportError> {
-        assert!(parent_path.is_absolute());
-        assert!(restriction.map_or(true, Path::is_absolute));
-
-        match self {
-            LockedPackageSource::Git { .. } => Ok(self),
-            LockedPackageSource::Path { path } => {
-                let path = normalize_abs_path(&parent_path.join(path));
-                if let Some(restriction) = restriction {
-                    if !path.starts_with(restriction) {
-                        return Err(ImportError::InvalidPath {
-                            attempted: path.to_owned(),
-                            restriction: restriction.to_owned(),
-                        });
-                    }
-                }
-                Ok(LockedPackageSource::Path { path })
-            }
-        }
-    }
-
     /// Is this locked package available offline? If not, it needs to be fetched.
     pub fn is_available_offline(&self) -> bool {
         // We consider path-dependencies to be always available offline, even if they don't exist.
@@ -197,109 +183,70 @@ impl LockedPackageSource {
     }
 }
 
+/// A lock file that's been fully resolved, including path dependencies.
 #[derive(Clone, Debug, Default)]
-pub struct LockFile {
+pub struct ResolvedLockFile {
     /// Absolute path to the lock file's parent directory.
     ///
-    /// Path dependencies at the top-level are resolved relative to this. (Path dependencies
-    /// of dependencies are resolved relative to the dependency that imported them.)
+    /// Path dependencies at the top-level are resolved relative to this.
     pub path: PathBuf,
-    pub data: LockFileData,
+    /// The inner lockfile, which is now guaranteed to have closed dependencies.
+    pub inner: LockFile,
 }
 
 /// A lock file, specifying versions and names for all recursive dependencies.
+///
+/// This defines the on-disk format for lock files.
 #[derive(Clone, Debug, Default, Serialize, Deserialize)]
-pub struct LockFileData {
+pub struct LockFile {
     /// The dependencies of the current (top-level) package.
     pub dependencies: HashMap<Name, LockedPackageSource>,
     /// All packages that we know about, and the dependencies of each one.
     ///
-    /// We guarantee that this package list is closed, in the sense that everything that is
-    /// a dependency of some package here has its own entry in the map.
+    /// Note that the package list is not guaranteed to be closed: path dependencies
+    /// cannot have their dependencies resolved in the on-disk lockfile because they
+    /// can change at any time. *Some* path dependencies (for example, path dependencies
+    /// that are local to a git depencency repo) may have resolved dependencies.
     pub packages: HashMap<LockedPackageSource, LockFileEntry>,
 }
 
-impl LockFileData {
+impl ResolvedLockFile {
     /// Are all the packages mentioned in this lockfile available offline?
     pub fn is_available_offline(&self) -> bool {
-        self.packages
+        self.inner
+            .packages
             .keys()
             .all(LockedPackageSource::is_available_offline)
     }
-}
 
-impl LockFile {
-    pub fn canonicalize_paths(&mut self) -> Result<(), ImportError> {
-        let mut new = LockFile {
-            path: self.path.clone(),
-            data: LockFileData::default(),
+    pub fn get(
+        &self,
+        parent: Option<&LockedPackageSource>,
+        pkg: &Name,
+        pos: &TermPos,
+    ) -> Result<&LockedPackageSource, ImportError> {
+        // The parent package should have come from the lock file.
+        let (deps, parent_name) = if let Some(parent) = parent {
+            let parent_pkg =
+                self.inner
+                    .packages
+                    .get(parent)
+                    .ok_or_else(|| ImportError::InternalError {
+                        msg: format!("unknown parent package {parent:?}"),
+                        pos: *pos,
+                    })?;
+
+            (&parent_pkg.dependencies, Some(&parent_pkg.name))
+        } else {
+            (&self.inner.dependencies, None)
         };
-
-        struct StackEntry<'a> {
-            abs_parent: LockedPackageSource,
-            restriction: Option<PathBuf>,
-            entry: &'a LockFileEntry,
-        }
-        let mut stack = Vec::new();
-
-        fn add_dep<'stack, 'a: 'stack>(
-            stack: &'stack mut Vec<StackEntry<'a>>,
-            source: &LockedPackageSource,
-            parent: &Path,
-            restriction: Option<&Path>,
-            orig_packages: &'a HashMap<LockedPackageSource, LockFileEntry>,
-        ) -> Result<LockedPackageSource, ImportError> {
-            let abs_source = source.clone().canonicalize(parent, restriction)?;
-
-            if let Some(entry) = orig_packages.get(source) {
-                let parent_path = abs_source.local_path();
-                stack.push(StackEntry {
-                    restriction: if abs_source.is_path() {
-                        restriction.map(Path::to_owned)
-                    } else {
-                        Some(parent_path.clone())
-                    },
-                    abs_parent: abs_source.clone(),
-                    entry,
-                });
-            }
-            Ok(abs_source)
-        }
-
-        for (name, source) in &self.data.dependencies {
-            let abs_source = add_dep(&mut stack, source, &self.path, None, &self.data.packages)?;
-            new.data.dependencies.insert(name.clone(), abs_source);
-        }
-
-        while let Some(StackEntry {
-            abs_parent,
-            restriction,
-            entry,
-        }) = stack.pop()
-        {
-            let mut abs_dependencies = HashMap::new();
-            for (name, source) in &entry.dependencies {
-                let abs_source = add_dep(
-                    &mut stack,
-                    source,
-                    &abs_parent.local_path(),
-                    restriction.as_deref(),
-                    &self.data.packages,
-                )?;
-
-                // FIXME: insist that if there's overlap then everything agrees
-                abs_dependencies.insert(name.clone(), abs_source);
-            }
-            new.data.packages.insert(
-                abs_parent,
-                LockFileEntry {
-                    name: entry.name.clone(),
-                    dependencies: abs_dependencies,
-                },
-            );
-        }
-
-        Ok(())
+        deps.get(pkg).ok_or_else(|| ImportError::MissingDependency {
+            parent: parent
+                .zip(parent_name)
+                .map(|(p, n)| Box::new((n.clone(), p.clone()))),
+            missing: pkg.clone(),
+            pos: *pos,
+        })
     }
 }
 
diff --git a/core/src/program.rs b/core/src/program.rs
@@ -30,7 +30,7 @@ use crate::{
     identifier::LocIdent,
     label::Label,
     metrics::increment,
-    package::LockFile,
+    package::ResolvedLockFile,
     term::{
         make as mk_term, make::builder, record::Field, BinaryOp, MergePriority, RichTerm, Term,
     },
@@ -309,7 +309,7 @@ impl<EC: EvalCache> Program<EC> {
         self.vm.import_resolver_mut().add_import_paths(paths);
     }
 
-    pub fn set_lock_file(&mut self, lock: LockFile) {
+    pub fn set_lock_file(&mut self, lock: ResolvedLockFile) {
         self.vm.import_resolver_mut().set_lock_file(lock)
     }
 
