refactor!: remove Drop for Lockfile

.gitignore

@@ -14,3 +14,5 @@
 /*.rockspec
 *.snap.new
 /.rocks
+
+.pre-commit-config.yaml

rocks-lib/src/build/variables.rs

@@ -45,9 +45,9 @@ mod tests {
 
     #[test]
     fn substitute_helper() {
-        assert_eq!(substitute(get_var, "$(TEST_VAR)".into()), "foo".to_string());
+        assert_eq!(substitute(get_var, "$(TEST_VAR)"), "foo".to_string());
         assert_eq!(
-            substitute(get_var, "$(UNRECOGNISED)".into()),
+            substitute(get_var, "$(UNRECOGNISED)"),
             "$(UNRECOGNISED)".to_string()
         );
     }

rocks-lib/src/lockfile/mod.rs

@@ -1,5 +1,8 @@
+use std::error::Error;
 use std::fmt::Display;
 use std::io::{self, Write};
+use std::marker::PhantomData;
+use std::ops::{Deref, DerefMut};
 use std::{collections::HashMap, fs::File, io::ErrorKind, path::PathBuf};
 
 use itertools::Itertools;
@@ -412,19 +415,85 @@ impl TryFrom<&Option<String>> for LockConstraint {
     }
 }
 
+pub trait LockfilePermissions {}
+#[derive(Clone)]
+pub struct ReadOnly;
+#[derive(Clone)]
+pub struct ReadWrite;
+
+impl LockfilePermissions for ReadOnly {}
+impl LockfilePermissions for ReadWrite {}
+
 #[derive(Clone, Debug, Serialize, Deserialize)]
-pub struct Lockfile {
+pub struct Lockfile<P: LockfilePermissions> {
     #[serde(skip)]
     filepath: PathBuf,
+    #[serde(skip)]
+    _marker: PhantomData<P>,
     // TODO: Serialize this directly into a `Version`
     version: String,
     // NOTE: We cannot directly serialize to a `Sha256` object as they don't implement serde traits.
     rocks: HashMap<LocalPackageId, LocalPackage>,
     entrypoints: Vec<LocalPackageId>,
 }
 
-impl Lockfile {
-    pub fn new(filepath: PathBuf) -> io::Result<Self> {
+impl<P: LockfilePermissions> Lockfile<P> {
+    pub fn version(&self) -> &String {
+        &self.version
+    }
+
+    pub fn rocks(&self) -> &HashMap<LocalPackageId, LocalPackage> {
+        &self.rocks
+    }
+
+    pub fn get(&self, id: &LocalPackageId) -> Option<&LocalPackage> {
+        self.rocks.get(id)
+    }
+
+    pub(crate) fn list(&self) -> HashMap<PackageName, Vec<LocalPackage>> {
+        self.rocks()
+            .values()
+            .cloned()
+            .map(|locked_rock| (locked_rock.name().clone(), locked_rock))
+            .into_group_map()
+    }
+
+    pub(crate) fn has_rock(&self, req: &PackageReq) -> Option<LocalPackage> {
+        self.list()
+            .get(req.name())
+            .map(|packages| {
+                packages
+                    .iter()
+                    .rev()
+                    .find(|package| req.version_req().matches(package.version()))
+            })?
+            .cloned()
+    }
+
+    fn flush(&mut self) -> io::Result<()> {
+        let dependencies = self
+            .rocks
+            .iter()
+            .flat_map(|(_, rock)| rock.dependencies())
+            .collect_vec();
+
+        self.entrypoints = self
+            .rocks
+            .keys()
+            .filter(|id| !dependencies.iter().contains(&id))
+            .cloned()
+            .collect();
+
+        let content = serde_json::to_string_pretty(&self)?;
+
+        std::fs::write(&self.filepath, content)?;
+
+        Ok(())
+    }
+}
+
+impl Lockfile<ReadOnly> {
+    pub(crate) fn new(filepath: PathBuf) -> io::Result<Lockfile<ReadOnly>> {
         // Ensure that the lockfile exists
         match File::options().create_new(true).write(true).open(&filepath) {
             Ok(mut file) => {
@@ -443,13 +512,68 @@ impl Lockfile {
             Err(err) => return Err(err),
         }
 
-        let mut new: Lockfile = serde_json::from_str(&std::fs::read_to_string(&filepath)?)?;
+        let mut new: Lockfile<ReadOnly> =
+            serde_json::from_str(&std::fs::read_to_string(&filepath)?)?;
 
         new.filepath = filepath;
 
         Ok(new)
     }
 
+    /// Creates a temporary, writeable lockfile which can never flush.
+    pub fn into_temporary(self) -> Lockfile<ReadWrite> {
+        Lockfile::<ReadWrite> {
+            _marker: PhantomData,
+            filepath: self.filepath,
+            version: self.version,
+            rocks: self.rocks,
+            entrypoints: self.entrypoints,
+        }
+    }
+
+    /// Creates a lockfile guard, flushing the lockfile automatically
+    /// once the guard goes out of scope.
+    pub fn write_guard(self) -> LockfileGuard {
+        LockfileGuard(self.into_temporary())
+    }
+
+    /// Converts the current lockfile into a writeable one, executes `cb` and flushes
+    /// the lockfile.
+    pub fn map_then_flush<T, F, E>(self, cb: F) -> Result<T, E>
+    where
+        F: FnOnce(&mut Lockfile<ReadWrite>) -> Result<T, E>,
+        E: Error,
+        E: From<io::Error>,
+    {
+        let mut writeable_lockfile = self.into_temporary();
+
+        let result = cb(&mut writeable_lockfile)?;
+
+        writeable_lockfile.flush()?;
+
+        Ok(result)
+    }
+
+    // TODO: Add this once async closures are stabilized
+    // Converts the current lockfile into a writeable one, executes `cb` asynchronously and flushes
+    // the lockfile.
+    //pub async fn map_then_flush_async<T, F, E, Fut>(self, cb: F) -> Result<T, E>
+    //where
+    //    F: AsyncFnOnce(&mut Lockfile<ReadWrite>) -> Result<T, E>,
+    //    E: Error,
+    //    E: From<io::Error>,
+    //{
+    //    let mut writeable_lockfile = self.into_temporary();
+    //
+    //    let result = cb(&mut writeable_lockfile).await?;
+    //
+    //    writeable_lockfile.flush()?;
+    //
+    //    Ok(result)
+    //}
+}
+
+impl Lockfile<ReadWrite> {
     pub fn add(&mut self, rock: &LocalPackage) {
         self.rocks.insert(rock.id(), rock.clone());
     }
@@ -477,74 +601,53 @@ impl Lockfile {
         self.rocks.remove(target);
     }
 
-    pub fn version(&self) -> &String {
-        &self.version
-    }
+    // TODO: `fn entrypoints() -> Vec<LockedRock>`
+}
 
-    pub fn rocks(&self) -> &HashMap<LocalPackageId, LocalPackage> {
-        &self.rocks
-    }
+pub struct LockfileGuard(Lockfile<ReadWrite>);
 
-    pub fn get(&self, id: &LocalPackageId) -> Option<&LocalPackage> {
-        self.rocks.get(id)
+impl Serialize for LockfileGuard {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        self.0.serialize(serializer)
     }
+}
 
-    pub fn get_mut(&mut self, id: &LocalPackageId) -> Option<&mut LocalPackage> {
-        self.rocks.get_mut(id)
+impl<'de> Deserialize<'de> for LockfileGuard {
+    fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        Ok(LockfileGuard(Lockfile::<ReadWrite>::deserialize(
+            deserializer,
+        )?))
     }
+}
 
-    // TODO: `fn entrypoints() -> Vec<LockedRock>`
-
-    pub fn flush(&mut self) -> io::Result<()> {
-        let dependencies = self
-            .rocks
-            .iter()
-            .flat_map(|(_, rock)| rock.dependencies())
-            .collect_vec();
-
-        self.entrypoints = self
-            .rocks
-            .keys()
-            .filter(|id| !dependencies.iter().contains(&id))
-            .cloned()
-            .collect();
-
-        let content = serde_json::to_string_pretty(self)?;
-
-        std::fs::write(&self.filepath, content)?;
+impl Deref for LockfileGuard {
+    type Target = Lockfile<ReadWrite>;
 
-        Ok(())
-    }
-
-    pub(crate) fn list(&self) -> HashMap<PackageName, Vec<LocalPackage>> {
-        self.rocks()
-            .values()
-            .cloned()
-            .map(|locked_rock| (locked_rock.name().clone(), locked_rock))
-            .into_group_map()
+    fn deref(&self) -> &Self::Target {
+        &self.0
     }
+}
 
-    pub(crate) fn has_rock(&self, req: &PackageReq) -> Option<LocalPackage> {
-        self.list()
-            .get(req.name())
-            .map(|packages| {
-                packages
-                    .iter()
-                    .rev()
-                    .find(|package| req.version_req().matches(package.version()))
-            })?
-            .cloned()
+impl DerefMut for LockfileGuard {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
     }
 }
 
-impl Drop for Lockfile {
+impl Drop for LockfileGuard {
     fn drop(&mut self) {
         let _ = self.flush();
     }
 }
 
 #[cfg(feature = "lua")]
-impl mlua::UserData for Lockfile {
+impl mlua::UserData for Lockfile<ReadWrite> {
     fn add_methods<M: mlua::UserDataMethods<Self>>(methods: &mut M) {
         methods.add_method_mut("add", |_, this, package: LocalPackage| {
             this.add(&package);
@@ -622,7 +725,7 @@ mod tests {
         };
 
         let tree = Tree::new(temp.to_path_buf(), Lua51).unwrap();
-        let mut lockfile = tree.lockfile().unwrap();
+        let mut lockfile = tree.lockfile().unwrap().write_guard();
 
         let test_package = PackageSpec::parse("test1".to_string(), "0.1.0".to_string()).unwrap();
         let test_local_package = LocalPackage::from(

rocks-lib/src/luarocks/luarocks_installation.rs

@@ -99,9 +99,11 @@ impl LuaRocksInstallation {
         &self,
         progress: &Progress<ProgressBar>,
     ) -> Result<(), LuaRocksInstallError> {
-        let mut lockfile = self.tree.lockfile()?;
+        let mut lockfile = self.tree.lockfile()?.write_guard();
+
         let luarocks_req =
             PackageReq::new("luarocks".into(), Some(LUAROCKS_VERSION.into())).unwrap();
+
         if !self.tree.match_rocks(&luarocks_req)?.is_found() {
             let rockspec = Rockspec::new(LUAROCKS_ROCKSPEC).unwrap();
             let pkg = Build::new(&rockspec, &self.config, progress)
@@ -112,7 +114,7 @@ impl LuaRocksInstallation {
                 .await?;
             lockfile.add(&pkg);
         }
-        lockfile.flush()?;
+
         Ok(())
     }
 
@@ -123,7 +125,7 @@ impl LuaRocksInstallation {
         progress_arc: Arc<Progress<MultiProgress>>,
     ) -> Result<(), InstallBuildDependenciesError> {
         let progress = Arc::clone(&progress_arc);
-        let mut lockfile = self.tree.lockfile()?;
+        let mut lockfile = self.tree.lockfile()?.write_guard();
         let package_db = RemotePackageDB::from_config(&self.config).await?;
         let build_dependencies = match rockspec.rockspec_format {
             Some(RockspecFormat::_1_0 | RockspecFormat::_2_0) => {
@@ -204,7 +206,7 @@ impl LuaRocksInstallation {
                     );
                 });
         });
-        lockfile.flush()?;
+
         Ok(())
     }
 

rocks-lib/src/operations/install.rs

@@ -3,7 +3,7 @@ use std::{collections::HashMap, io, sync::Arc};
 use crate::{
     build::{Build, BuildBehaviour, BuildError},
     config::{Config, LuaVersion, LuaVersionUnset},
-    lockfile::{LocalPackage, LocalPackageId, LockConstraint, Lockfile, PinnedState},
+    lockfile::{LocalPackage, LocalPackageId, LockConstraint, Lockfile, PinnedState, ReadWrite},
     luarocks::{
         install_binary_rock::{BinaryRockInstall, InstallBinaryRockError},
         luarocks_installation::{
@@ -137,18 +137,18 @@ where
 {
     let lua_version = LuaVersion::from(config)?;
     let tree = Tree::new(config.tree().clone(), lua_version)?;
-    let mut lockfile = tree.lockfile()?;
-    let result = install_impl(packages, pin, package_db, config, &mut lockfile, progress).await;
-    lockfile.flush()?;
-    result
+
+    let mut lockfile = tree.lockfile()?.write_guard();
+
+    install_impl(packages, pin, package_db, config, &mut lockfile, progress).await
 }
 
 async fn install_impl(
     packages: Vec<(BuildBehaviour, PackageReq)>,
     pin: PinnedState,
     package_db: RemotePackageDB,
     config: &Config,
-    lockfile: &mut Lockfile,
+    lockfile: &mut Lockfile<ReadWrite>,
     progress_arc: Arc<Progress<MultiProgress>>,
 ) -> Result<Vec<LocalPackage>, InstallError> {
     let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel();

rocks-lib/src/operations/pin.rs

@@ -37,7 +37,7 @@ pub fn set_pinned_state(
     tree: &Tree,
     pin: PinnedState,
 ) -> Result<(), PinError> {
-    let mut lockfile = tree.lockfile()?;
+    let lockfile = tree.lockfile()?;
     let mut package = lockfile
         .get(package_id)
         .ok_or_else(|| PinError::PackageNotFound(package_id.clone()))?
@@ -71,9 +71,12 @@ pub fn set_pinned_state(
 
     fs_extra::move_items(&items, new_root, &CopyOptions::new())?;
 
-    lockfile.remove(&old_package);
-    lockfile.add(&package);
-    lockfile.flush()?;
+    lockfile.map_then_flush(|lockfile| {
+        lockfile.remove(&old_package);
+        lockfile.add(&package);
+
+        Ok::<_, io::Error>(())
+    })?;
 
     Ok(())
 }