test: fix dep feature behavior in the sat resolver

Behavior of the feature:
* if dependency `dep_name` is not optional, its feature `"bar"` is activated.
* if dependency `dep_name` is optional:
    - if this is a weak dependency feature:
        - feature `"bar"` of dependency `dep_name` is activated if `dep_name` has been activated.
    - if this is not a weak dependency feature:
        - feature `dep_name` is activated if it exists.
        - dependency `dep_name` is activated.
        - feature `"bar"` of dependency `dep_name` is activated.

Author: x-hgg-x

crates/resolver-tests/src/sat.rs

@@ -98,6 +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,
+    optional_dependencies: &HashSet<(DepKind, InternedString)>,
     compatible_dep_summaries_map: &HashMap<InternedString, HashMap<DepKind, Vec<Summary>>>,
 ) {
     // add clauses for package features
@@ -133,39 +134,76 @@ fn process_pkg_features(
                     dep_feature: dep_feature_name,
                     weak,
                 } => {
+                    // Behavior of the feature:
+                    // * if dependency `dep_name` is not optional, its feature `"bar"` is activated.
+                    // * if dependency `dep_name` is optional:
+                    //     - if this is a weak dependency feature:
+                    //         - feature `"bar"` of dependency `dep_name` is activated if `dep_name` has been activated.
+                    //     - if this is not a weak dependency feature:
+                    //         - feature `dep_name` is activated if it exists.
+                    //         - dependency `dep_name` is activated.
+                    //         - feature `"bar"` of dependency `dep_name` is activated.
+
                     // add clauses for each dependency with the provided name (normal/build/dev)
-                    for compatible_dep_summaries in compatible_dep_summaries_map
+                    for (&dep_kind, compatible_dep_summaries) in compatible_dep_summaries_map
                         .get(&dep_name)
                         .into_iter()
-                        .flat_map(|map| map.values())
+                        .flatten()
                     {
-                        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 compatible_vars = compatible_dep_summaries
+                            .iter()
+                            .filter_map(|dep| {
+                                let dep_feature_var_map =
+                                    &var_for_is_packages_features_used[&dep.package_id()];
+
+                                match dep_feature_var_map.get(&dep_feature_name) {
+                                    None => None,
+                                    Some(&dep_feature_var) => {
+                                        let dep_var = var_for_is_packages_used[&dep.package_id()];
+                                        Some((dep_var, dep_feature_var))
+                                    }
+                                }
+                            })
+                            .collect::<Vec<_>>();
 
-                            let dep_var = var_for_is_packages_used[&dep.package_id()];
+                        if compatible_vars.is_empty() {
+                            if !weak {
+                                solver.add_clause(&[pkg_feature_var.negative()]);
+                            } else {
+                                for dep in compatible_dep_summaries {
+                                    let dep_var = var_for_is_packages_used[&dep.package_id()];
+                                    solver.add_clause(&[
+                                        pkg_feature_var.negative(),
+                                        dep_var.negative(),
+                                    ]);
+                                }
+                            }
+                            continue;
+                        }
 
+                        for &(dep_var, dep_feature_var) in &compatible_vars {
                             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(pkg_feature_var.negative());
+                        if !weak && optional_dependencies.contains(&(dep_kind, dep_name)) {
+                            let dep_clause = compatible_vars
+                                .iter()
+                                .map(|(dep_var, _)| dep_var.positive())
+                                .chain([pkg_feature_var.negative()])
+                                .collect::<Vec<_>>();
+
                             solver.add_clause(&dep_clause);
+
+                            if let Some(other_feature_var) = pkg_feature_var_map.get(&dep_name) {
+                                solver.add_clause(&[
+                                    pkg_feature_var.negative(),
+                                    other_feature_var.positive(),
+                                ]);
+                            }
                         }
                     }
                 }
@@ -298,6 +336,12 @@ impl SatResolver {
             let pkg_dependencies = pkg.dependencies();
             let pkg_features = pkg.features();
 
+            let optional_dependencies = pkg_dependencies
+                .iter()
+                .filter(|dep| dep.is_optional())
+                .map(|dep| (dep.kind(), dep.name_in_toml()))
+                .collect();
+
             let compatible_dep_summaries_map =
                 find_all_compatible_dep_summaries(pkg_dependencies, &by_name, false);
 
@@ -307,6 +351,7 @@ impl SatResolver {
                 &var_for_is_packages_features_used,
                 &var_for_is_packages_features_used[&pkg_id],
                 pkg_features,
+                &optional_dependencies,
                 &compatible_dep_summaries_map,
             );
 

crates/resolver-tests/tests/validated.rs

@@ -177,6 +177,47 @@ fn missing_dep_feature() {
     assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
 }
 
+#[test]
+fn missing_weak_dep_feature() {
+    let reg = registry(vec![
+        pkg("a"),
+        pkg_dep_with("dep1", vec![dep("a")], &[("f", &["a/a"])]),
+        pkg_dep_with("dep2", vec!["a".opt()], &[("f", &["a/a"])]),
+        pkg_dep_with("dep3", vec!["a".opt()], &[("f", &["a?/a"])]),
+        pkg_dep_with("dep4", vec!["x".opt()], &[("f", &["x?/a"])]),
+    ]);
+
+    let deps = vec![dep("dep1").with(&["f"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+
+    let deps = vec![dep("dep2").with(&["f"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+
+    let deps = vec![dep("dep2").with(&["a", "f"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+
+    // Weak dependencies are not supported yet in the dependency resolver
+    let deps = vec![dep("dep3").with(&["f"])];
+    assert!(resolve(deps.clone(), &reg).is_err());
+    assert!(SatResolver::new(&reg).sat_resolve(&deps));
+
+    let deps = vec![dep("dep3").with(&["a", "f"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+
+    // Weak dependencies are not supported yet in the dependency resolver
+    let deps = vec![dep("dep4").with(&["f"])];
+    assert!(resolve(deps.clone(), &reg).is_err());
+    assert!(SatResolver::new(&reg).sat_resolve(&deps));
+
+    let deps = vec![dep("dep4").with(&["x", "f"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+}
+
 #[test]
 fn conflict_feature_and_sys() {
     let reg = registry(vec![
@@ -299,3 +340,71 @@ fn multiple_dep_kinds_with_different_packages() {
     let mut sat_resolver = SatResolver::new(&reg);
     assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
 }
+
+#[test]
+fn dep_feature_with_shadowing_feature() {
+    let reg = registry(vec![
+        pkg_dep_with("a", vec![], &[("b", &[])]),
+        pkg_dep_with(
+            "dep",
+            vec!["a".opt().rename("aa"), "c".opt()],
+            &[("default", &["aa/b"]), ("aa", &["c"])],
+        ),
+    ]);
+
+    let deps = vec!["dep".with(&["default"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+}
+
+#[test]
+fn dep_feature_not_optional_with_shadowing_feature() {
+    let reg = registry(vec![
+        pkg_dep_with("a", vec![], &[("b", &[])]),
+        pkg_dep_with(
+            "dep",
+            vec!["a".rename("aa"), "c".opt()],
+            &[("default", &["aa/b"]), ("aa", &["c"])],
+        ),
+    ]);
+
+    let deps = vec!["dep".with(&["default"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_ok());
+}
+
+#[test]
+fn dep_feature_weak_with_shadowing_feature() {
+    let reg = registry(vec![
+        pkg_dep_with("a", vec![], &[("b", &[])]),
+        pkg_dep_with(
+            "dep",
+            vec!["a".opt().rename("aa"), "c".opt()],
+            &[("default", &["aa?/b"]), ("aa", &["c"])],
+        ),
+    ]);
+
+    let deps = vec!["dep".with(&["default"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_ok());
+}
+
+#[test]
+fn dep_feature_duplicate_with_shadowing_feature() {
+    let reg = registry(vec![
+        pkg_dep_with("a", vec![], &[("b", &[])]),
+        pkg_dep_with(
+            "dep",
+            vec![
+                "a".opt().rename("aa"),
+                dep_kind("a", DepKind::Build).rename("aa"),
+                "c".opt(),
+            ],
+            &[("default", &["aa/b"]), ("aa", &["c"])],
+        ),
+    ]);
+
+    let deps = vec!["dep".with(&["default"])];
+    let mut sat_resolver = SatResolver::new(&reg);
+    assert!(resolve_and_validated(deps, &reg, &mut sat_resolver).is_err());
+}