test: allow multiple dependencies with the same name but a different kind in the sat resolver

Author: x-hgg-x

crates/resolver-tests/src/sat.rs

@@ -1,6 +1,7 @@
 use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
 use std::fmt::Write;
 
+use cargo::core::dependency::DepKind;
 use cargo::core::{Dependency, FeatureMap, FeatureValue, PackageId, Summary};
 use cargo::util::interning::{InternedString, INTERNED_DEFAULT};
 use varisat::ExtendFormula;
@@ -59,29 +60,36 @@ fn sat_at_most_one_by_key<K: std::hash::Hash + Eq>(
     by_keys
 }
 
-fn find_compatible_dep_summaries_by_name_in_toml(
+fn find_all_compatible_dep_summaries(
     pkg_dependencies: &[Dependency],
     by_name: &HashMap<InternedString, Vec<Summary>>,
-) -> HashMap<InternedString, Vec<Summary>> {
+    check_dev_dependencies: bool,
+) -> HashMap<InternedString, HashMap<DepKind, Vec<Summary>>> {
     let empty_vec = vec![];
 
-    pkg_dependencies
-        .iter()
-        .map(|dep| {
-            let name_in_toml = dep.name_in_toml();
+    let mut compatible_dep_summaries_map = HashMap::<_, HashMap<_, _>>::new();
 
-            let compatible_summaries = by_name
-                .get(&dep.package_name())
-                .unwrap_or(&empty_vec)
-                .iter()
-                .filter(|s| dep.matches_id(s.package_id()))
-                .filter(|s| dep.features().iter().all(|f| s.features().contains_key(f)))
-                .cloned()
-                .collect::<Vec<_>>();
+    for dep in pkg_dependencies {
+        if dep.kind() == DepKind::Development && !check_dev_dependencies {
+            continue;
+        }
 
-            (name_in_toml, compatible_summaries)
-        })
-        .collect()
+        let compatible_summaries = by_name
+            .get(&dep.package_name())
+            .unwrap_or(&empty_vec)
+            .iter()
+            .filter(|s| dep.matches_id(s.package_id()))
+            .filter(|s| dep.features().iter().all(|f| s.features().contains_key(f)))
+            .cloned()
+            .collect::<Vec<_>>();
+
+        compatible_dep_summaries_map
+            .entry(dep.name_in_toml())
+            .or_default()
+            .insert(dep.kind(), compatible_summaries);
+    }
+
+    compatible_dep_summaries_map
 }
 
 fn process_pkg_features(
@@ -90,7 +98,7 @@ fn process_pkg_features(
     var_for_is_packages_features_used: &HashMap<PackageId, HashMap<InternedString, varisat::Var>>,
     pkg_feature_var_map: &HashMap<InternedString, varisat::Var>,
     pkg_features: &FeatureMap,
-    compatible_dep_summaries_by_name_in_toml: &HashMap<InternedString, Vec<Summary>>,
+    compatible_dep_summaries_map: &HashMap<InternedString, HashMap<DepKind, Vec<Summary>>>,
 ) {
     // add clauses for package features
     for (&feature_name, feature_values) in pkg_features {
@@ -105,47 +113,61 @@ fn process_pkg_features(
                     ]);
                 }
                 FeatureValue::Dep { dep_name } => {
-                    let dep_clause = compatible_dep_summaries_by_name_in_toml[&dep_name]
-                        .iter()
-                        .map(|dep| var_for_is_packages_used[&dep.package_id()].positive())
-                        .chain([pkg_feature_var.negative()])
-                        .collect::<Vec<_>>();
+                    // add a clause for each dependency with the provided name (normal/build/dev)
+                    for compatible_dep_summaries in compatible_dep_summaries_map
+                        .get(&dep_name)
+                        .into_iter()
+                        .flat_map(|map| map.values())
+                    {
+                        let dep_clause = compatible_dep_summaries
+                            .iter()
+                            .map(|dep| var_for_is_packages_used[&dep.package_id()].positive())
+                            .chain([pkg_feature_var.negative()])
+                            .collect::<Vec<_>>();
 
-                    solver.add_clause(&dep_clause);
+                        solver.add_clause(&dep_clause);
+                    }
                 }
                 FeatureValue::DepFeature {
                     dep_name,
                     dep_feature: dep_feature_name,
                     weak,
                 } => {
-                    let mut dep_clause = Vec::new();
-
-                    for dep in &compatible_dep_summaries_by_name_in_toml[&dep_name] {
-                        let dep_feature_var_map =
-                            &var_for_is_packages_features_used[&dep.package_id()];
-
-                        let Some(dep_feature_var) = dep_feature_var_map.get(&dep_feature_name)
-                        else {
-                            continue;
-                        };
-
-                        let dep_var = var_for_is_packages_used[&dep.package_id()];
-
-                        solver.add_clause(&[
-                            pkg_feature_var.negative(),
-                            dep_var.negative(),
-                            dep_feature_var.positive(),
-                        ]);
+                    // add clauses for each dependency with the provided name (normal/build/dev)
+                    for compatible_dep_summaries in compatible_dep_summaries_map
+                        .get(&dep_name)
+                        .into_iter()
+                        .flat_map(|map| map.values())
+                    {
+                        let mut dep_clause = Vec::new();
+
+                        for dep in compatible_dep_summaries {
+                            let dep_feature_var_map =
+                                &var_for_is_packages_features_used[&dep.package_id()];
+
+                            let Some(dep_feature_var) = dep_feature_var_map.get(&dep_feature_name)
+                            else {
+                                continue;
+                            };
+
+                            let dep_var = var_for_is_packages_used[&dep.package_id()];
+
+                            solver.add_clause(&[
+                                pkg_feature_var.negative(),
+                                dep_var.negative(),
+                                dep_feature_var.positive(),
+                            ]);
+
+                            if !weak {
+                                dep_clause.push(dep_var.positive());
+                            }
+                        }
 
                         if !weak {
-                            dep_clause.push(dep_var.positive());
+                            dep_clause.push(pkg_feature_var.negative());
+                            solver.add_clause(&dep_clause);
                         }
                     }
-
-                    if !weak {
-                        dep_clause.push(pkg_feature_var.negative());
-                        solver.add_clause(&dep_clause);
-                    }
                 }
             }
         }
@@ -158,11 +180,15 @@ fn process_pkg_dependencies(
     var_for_is_packages_features_used: &HashMap<PackageId, HashMap<InternedString, varisat::Var>>,
     pkg_var: varisat::Var,
     pkg_dependencies: &[Dependency],
-    compatible_dep_summaries_by_name_in_toml: &HashMap<InternedString, Vec<Summary>>,
+    compatible_dep_summaries_map: &HashMap<InternedString, HashMap<DepKind, Vec<Summary>>>,
 ) {
     for dep in pkg_dependencies {
-        let compatible_dep_summaries =
-            &compatible_dep_summaries_by_name_in_toml[&dep.name_in_toml()];
+        let Some(compatible_dep_summaries) = compatible_dep_summaries_map
+            .get(&dep.name_in_toml())
+            .and_then(|map| map.get(&dep.kind()))
+        else {
+            continue;
+        };
 
         // add clauses for package dependency features
         for dep_summary in compatible_dep_summaries {
@@ -272,16 +298,16 @@ impl SatResolver {
             let pkg_dependencies = pkg.dependencies();
             let pkg_features = pkg.features();
 
-            let compatible_dep_summaries_by_name_in_toml =
-                find_compatible_dep_summaries_by_name_in_toml(pkg_dependencies, &by_name);
+            let compatible_dep_summaries_map =
+                find_all_compatible_dep_summaries(pkg_dependencies, &by_name, false);
 
             process_pkg_features(
                 &mut solver,
                 &var_for_is_packages_used,
                 &var_for_is_packages_features_used,
                 &var_for_is_packages_features_used[&pkg_id],
                 pkg_features,
-                &compatible_dep_summaries_by_name_in_toml,
+                &compatible_dep_summaries_map,
             );
 
             process_pkg_dependencies(
@@ -290,7 +316,7 @@ impl SatResolver {
                 &var_for_is_packages_features_used,
                 var_for_is_packages_used[&pkg_id],
                 pkg_dependencies,
-                &compatible_dep_summaries_by_name_in_toml,
+                &compatible_dep_summaries_map,
             );
         }
 
@@ -330,16 +356,16 @@ impl SatResolver {
 
         old_root_vars.push(root_var);
 
-        let compatible_dep_summaries_by_name_in_toml =
-            find_compatible_dep_summaries_by_name_in_toml(root_dependencies, &by_name);
+        let compatible_dep_summaries_map =
+            find_all_compatible_dep_summaries(root_dependencies, &by_name, true);
 
         process_pkg_dependencies(
             solver,
             var_for_is_packages_used,
             var_for_is_packages_features_used,
             root_var,
             root_dependencies,
-            &compatible_dep_summaries_by_name_in_toml,
+            &compatible_dep_summaries_map,
         );
 
         solver.assume(&assumption);

crates/resolver-tests/tests/validated.rs

@@ -1,7 +1,7 @@
-use cargo::core::Dependency;
+use cargo::core::{dependency::DepKind, Dependency};
 
 use resolver_tests::{
-    helpers::{dep, dep_req, pkg, pkg_dep, pkg_dep_with, registry, ToDep},
+    helpers::{dep, dep_kind, dep_req, dep_req_kind, pkg, pkg_dep, pkg_dep_with, registry, ToDep},
     pkg, resolve, resolve_and_validated,
     sat::SatResolver,
 };
@@ -216,8 +216,86 @@ fn conflict_weak_features() {
 
     let deps = vec![dep("dep").with(&["a1", "a2"])];
 
-    // The following asserts should be updated when support for weak dependencies
-    // is added to the dependency resolver.
+    // Weak dependencies are not supported yet in the dependency resolver
     assert!(resolve(deps.clone(), &reg).is_err());
     assert!(SatResolver::new(&reg).sat_resolve(&deps));
 }
+
+#[test]
+fn multiple_dep_kinds() {
+    let reg = registry(vec![
+        pkg(("a-sys", "1.0.0")),
+        pkg(("a-sys", "2.0.0")),
+        pkg_dep_with(
+            "dep1",
+            vec![
+                dep_req("a-sys", "1.0.0").opt(),
+                dep_req_kind("a-sys", "2.0.0", DepKind::Build).opt(),
+            ],
+            &[("default", &["dep:a-sys"])],
+        ),
+        pkg_dep_with(
+            "dep2",
+            vec![
+                dep_req("a-sys", "1.0.0").opt(),
+                dep_req_kind("a-sys", "2.0.0", DepKind::Development).rename("a-sys-dev"),
+            ],
+            &[("default", &["dep:a-sys", "a-sys-dev/bad"])],
+        ),
+    ]);
+
+    let deps = vec![dep("dep1")];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+
+    let deps = vec![dep("dep2")];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_ok());
+
+    let deps = vec![
+        dep_req("a-sys", "1.0.0"),
+        dep_req_kind("a-sys", "2.0.0", DepKind::Build).rename("a2"),
+    ];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+
+    let deps = vec![
+        dep_req("a-sys", "1.0.0"),
+        dep_req_kind("a-sys", "2.0.0", DepKind::Development).rename("a2"),
+    ];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+}
+
+#[test]
+fn multiple_dep_kinds_with_different_packages() {
+    let reg = registry(vec![
+        pkg_dep_with("a", vec![], &[("f", &[])]),
+        pkg_dep_with("b", vec![], &[("f", &[])]),
+        pkg_dep_with("c", vec![], &[("g", &[])]),
+        pkg_dep_with(
+            "dep1",
+            vec![
+                "a".opt().rename("x").with(&["f"]),
+                dep_kind("b", DepKind::Build).opt().rename("x").with(&["f"]),
+            ],
+            &[("default", &["x"])],
+        ),
+        pkg_dep_with(
+            "dep2",
+            vec![
+                "a".opt().rename("x").with(&["f"]),
+                dep_kind("c", DepKind::Build).opt().rename("x").with(&["f"]),
+            ],
+            &[("default", &["x"])],
+        ),
+    ]);
+
+    let deps = vec!["dep1".with(&["default"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_ok());
+
+    let deps = vec!["dep2".with(&["default"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+}