Automatically upgrade locked versions on conflicts (#2218)

maciektr · web-flow · commit 4fdeb78106ac · 2025-05-08T08:51:32.000Z
fix #2031
diff --git a/scarb/src/core/lockfile.rs b/scarb/src/core/lockfile.rs
@@ -1,10 +1,12 @@
-use std::collections::BTreeSet;
+use std::collections::{BTreeMap, BTreeSet};
 use std::str::FromStr;
 
 use anyhow::{Context, Result, anyhow};
+use itertools::Itertools;
 use semver::Version;
 use serde::{Deserialize, Serialize};
 use serde_repr::{Deserialize_repr, Serialize_repr};
+use smallvec::SmallVec;
 use toml_edit::DocumentMut;
 use typed_builder::TypedBuilder;
 
@@ -23,12 +25,44 @@ pub enum LockVersion {
 
 #[derive(Debug, Clone, Eq, PartialEq, Default, Serialize, Deserialize)]
 #[serde(rename_all = "kebab-case")]
+#[serde(from = "serdex::Lockfile", into = "serdex::Lockfile")]
 pub struct Lockfile {
-    pub version: LockVersion,
-    #[serde(rename = "package")]
-    #[serde(default = "BTreeSet::new")]
-    #[serde(skip_serializing_if = "BTreeSet::is_empty")]
-    pub packages: BTreeSet<PackageLock>,
+    version: LockVersion,
+    // Note that the current compilation model will not allow Scarb to emit lock files with multiple
+    // entries for the same package name. This structure allows this for the sake of future-proofing
+    // and being more defensive against lock files tinkered with by hand. Thus, we use `SmallVec`
+    // to optimise for the common case of a single entry only.
+    packages: BTreeMap<PackageName, SmallVec<[PackageLock; 1]>>,
+}
+
+mod serdex {
+    use crate::core::lockfile::{LockVersion, PackageLock};
+    use serde::{Deserialize, Serialize};
+    use std::collections::BTreeSet;
+
+    #[derive(Serialize, Deserialize)]
+    pub struct Lockfile {
+        version: LockVersion,
+        #[serde(rename = "package")]
+        #[serde(default = "BTreeSet::new")]
+        #[serde(skip_serializing_if = "BTreeSet::is_empty")]
+        packages: BTreeSet<PackageLock>,
+    }
+
+    impl From<Lockfile> for super::Lockfile {
+        fn from(value: Lockfile) -> Self {
+            Self::new(value.packages).with_version(value.version)
+        }
+    }
+
+    impl From<super::Lockfile> for Lockfile {
+        fn from(value: super::Lockfile) -> Self {
+            Self {
+                version: value.version,
+                packages: value.packages.into_values().flatten().collect(),
+            }
+        }
+    }
 }
 
 #[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Serialize, Deserialize, TypedBuilder)]
@@ -57,12 +91,24 @@ fn skip_path_source_id(sid: &Option<SourceId>) -> bool {
 
 impl Lockfile {
     pub fn new(packages: impl IntoIterator<Item = PackageLock>) -> Self {
+        let packages = packages
+            .into_iter()
+            .map(|lock| (lock.name.clone(), lock))
+            .chunk_by(|(name, _)| name.clone());
+        let packages = packages
+            .into_iter()
+            .map(|(name, locks)| (name, locks.into_iter().map(|(_, lock)| lock).collect()))
+            .collect();
         Self {
             version: Default::default(),
-            packages: packages.into_iter().collect(),
+            packages,
         }
     }
 
+    fn with_version(self, version: LockVersion) -> Self {
+        Self { version, ..self }
+    }
+
     pub fn from_resolve(resolve: &Resolve) -> Self {
         let include_package = |package_id: &PackageId| !package_id.source_id.is_std();
         let packages = resolve
@@ -87,13 +133,24 @@ impl Lockfile {
     }
 
     pub fn packages(&self) -> impl Iterator<Item = &PackageLock> {
-        self.packages.iter()
+        self.packages.values().flatten()
+    }
+
+    pub fn packages_by_name<'a>(
+        &'a self,
+        name: &PackageName,
+    ) -> Box<dyn Iterator<Item = &'a PackageLock> + 'a> {
+        if let Some(locks) = self.packages.get(name) {
+            Box::new(locks.iter())
+        } else {
+            Box::new(std::iter::empty())
+        }
     }
 
-    pub fn packages_matching(&self, dependency: ManifestDependency) -> Option<Result<PackageId>> {
-        self.packages()
-            .filter(|p| dependency.matches_name_and_version(&p.name, &p.version))
-            .find(|p| {
+    pub fn package_matching(&self, dependency: ManifestDependency) -> Option<Result<PackageId>> {
+        self.packages_by_name(&dependency.name)
+            .find(|p| dependency.matches_name_and_version(&p.name, &p.version))
+            .filter(|p| {
                 p.source
                     .map(|sid| sid.can_lock_source_id(dependency.source_id))
                     // No locking occurs on path sources.
diff --git a/scarb/src/core/resolver.rs b/scarb/src/core/resolver.rs
@@ -196,7 +196,7 @@ impl Resolve {
     /// In all of these cases, we want to report an error to indicate that something is awry.
     /// Normal execution (esp. just using the default registry) should never run into this.
     pub fn check_checksums(&self, lockfile: &Lockfile) -> Result<()> {
-        for package_lock in &lockfile.packages {
+        for package_lock in lockfile.packages() {
             let (locked, source_id) = match (package_lock.checksum.as_ref(), package_lock.source) {
                 (None, None) => continue,
                 (Some(_), None) => {
diff --git a/scarb/src/resolver/algorithm/mod.rs b/scarb/src/resolver/algorithm/mod.rs
@@ -3,7 +3,7 @@ use crate::core::registry::Registry;
 use crate::core::registry::patch_map::PatchMap;
 use crate::core::{PackageId, Resolve, Summary};
 use crate::resolver::algorithm::provider::{
-    DependencyProviderError, PubGrubDependencyProvider, PubGrubPackage, rewrite_locked_dependency,
+    DependencyProviderError, PubGrubDependencyProvider, PubGrubPackage, lock_dependency,
 };
 use crate::resolver::algorithm::solution::{build_resolve, validate_solution};
 use crate::resolver::algorithm::state::{Request, ResolverState};
@@ -86,12 +86,7 @@ pub async fn resolve(
     for summary in summaries {
         for dep in summary.full_dependencies() {
             let dep = patch_map.lookup(dep);
-            let locked_package_id = lockfile.packages_matching(dep.clone());
-            let dep = if let Some(locked_package_id) = locked_package_id {
-                rewrite_locked_dependency(dep.clone(), locked_package_id?)
-            } else {
-                dep.clone()
-            };
+            let dep = lock_dependency(&lockfile, dep.clone())?;
             if state.index.packages().register(dep.clone()) {
                 request_sink.send(Request::Package(dep.clone())).await?;
             }
diff --git a/scarb/src/resolver/algorithm/provider.rs b/scarb/src/resolver/algorithm/provider.rs
@@ -249,7 +249,7 @@ impl DependencyProvider for PubGrubDependencyProvider {
                 .unwrap();
         }
 
-        // Prioritize by ordering from root.
+        // Prioritize by ordering from the root.
         let priority = self.priority.read().unwrap().get(package).copied();
         if let Some(priority) = priority {
             return Some(PubGrubPriority::Unspecified(Reverse(priority)));
@@ -268,13 +268,38 @@ impl DependencyProvider for PubGrubDependencyProvider {
         // Query available versions.
         let dependency: ManifestDependency = package.to_dependency(range.clone());
         let summaries = self.wait_for_summaries(dependency)?;
+        let summaries = summaries
+            .into_iter()
+            .filter(|summary| range.contains(&summary.package_id.version))
+            .sorted_by_key(|summary| summary.package_id.version.clone())
+            .collect_vec();
 
         // Choose version.
-        let summary = summaries
-            .into_iter()
-            .find(|summary| range.contains(&summary.package_id.version));
+        let locked = self.lockfile.packages_by_name(&package.name).find(|p| {
+            p.name == package.name
+                && range.contains(&p.version)
+                && p.source
+                    .map(|source| {
+                        // Git sources are rewritten to the locked source before fetching summaries.
+                        source.is_registry() && source.can_lock_source_id(package.source_id)
+                    })
+                    .unwrap_or_default()
+        });
+        let summary = locked
+            .and_then(|locked| {
+                summaries
+                    .iter()
+                    .find(|summary| {
+                        summary.package_id.name == locked.name
+                            && summary.package_id.version == locked.version
+                            && summary.package_id.source_id == locked.source.expect("source set to `None` is filtered out when searching the lockfile")
+                    })
+                    .cloned()
+            })
+            // No version locked - using the highest matching summary.
+            .or_else(|| summaries.last().cloned());
 
-        // Store retrieved summary for selected version.
+        // Store retrieved summary for the selected version.
         if let Some(summary) = summary.as_ref() {
             self.packages
                 .write()
@@ -364,12 +389,17 @@ impl From<DependencyVersionReq> for SemverPubgrub {
 
 /// Check lockfile for a matching package.
 /// Rewrite the dependency if a matching package is found.
-fn lock_dependency(
+pub fn lock_dependency(
     lockfile: &Lockfile,
     dep: ManifestDependency,
 ) -> Result<ManifestDependency, DependencyProviderError> {
+    if dep.source_id.is_registry() {
+        // We do not rewrite to the locked version for registry dependencies
+        // because they will be locked in the `choose_version` step of pubgrub.
+        return Ok(dep);
+    }
     lockfile
-        .packages_matching(dep.clone())
+        .package_matching(dep.clone())
         .map(|locked_package_id| Ok(rewrite_locked_dependency(dep.clone(), locked_package_id?)))
         .unwrap_or(Ok(dep))
 }
diff --git a/scarb/src/resolver/mod.rs b/scarb/src/resolver/mod.rs
@@ -548,7 +548,6 @@ mod tests {
     }
 
     #[test]
-    #[ignore = "TODO(#2031): upgrade lock that matches only part of the deps"]
     fn lock_matching_only_some_deps_is_upgraded() {
         check_with_lock(
             registry![("foo v0.1.0", []), ("foo v0.1.1", [])],
@@ -567,12 +566,6 @@ mod tests {
             // with the following error.
             // Ideally, the lock should be upgraded instead.
             Ok(pkgs!["foo v0.1.1"]),
-            // Current error message:
-            // Err(indoc! { r#"
-            //     version solving failed:
-            //     Because root_2 1.0.0 depends on foo >=0.1.1, <0.2.0 and root_1 1.0.0 depends on foo >=0.1.0, <0.1.1, root_2 1.0.0, root_1 1.0.0 are incompatible.
-            //     And because we are solving dependencies of root_2 1.0.0, root_1 1.0.0 is forbidden.
-            // "#}),
         );
     }
 
diff --git a/scarb/src/resolver/primitive.rs b/scarb/src/resolver/primitive.rs
@@ -71,7 +71,7 @@ pub async fn resolve(
             for dep in summary.filtered_full_dependencies(dep_filter) {
                 let dep = rewrite_dependency_source_id(registry, &package_id, dep).await?;
 
-                let locked_package_id = lockfile.packages_matching(dep.clone());
+                let locked_package_id = lockfile.package_matching(dep.clone());
                 let dep = if let Some(locked_package_id) = locked_package_id {
                     rewrite_locked_dependency(dep.clone(), locked_package_id?)
                 } else {
