Skip to content

Commit 42f4143

Browse files
committed
Auto merge of #14707 - x-hgg-x:sat-fixes, r=Eh2406
test: add fixes in the sat resolver ### What does this PR try to resolve? This is a follow-up of #14614. ### How should we test and review this PR? Commit 1 removes duplicate variables in the sat resolver. Commit 2 removes useless clones in the sat resolver. r? Eh2406
2 parents edd36eb + c85d832 commit 42f4143

File tree

1 file changed

+27
-32
lines changed
  • crates/resolver-tests/src

1 file changed

+27
-32
lines changed

crates/resolver-tests/src/sat.rs

Lines changed: 27 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
use std::collections::hash_map::Entry;
12
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
23
use std::fmt::Write;
34

@@ -86,16 +87,17 @@ fn create_dependencies_vars<'a>(
8687
.or_default()
8788
.insert((kind, platform), solver.new_var());
8889

89-
let dep_feature_var_map = dep
90-
.features()
91-
.iter()
92-
.map(|&f| (f, solver.new_var()))
93-
.collect();
94-
95-
var_for_is_dependencies_features_used
90+
let dep_feature_vars = var_for_is_dependencies_features_used
9691
.entry(name)
9792
.or_default()
98-
.insert((kind, platform), dep_feature_var_map);
93+
.entry((kind, platform))
94+
.or_default();
95+
96+
for &feature_name in dep.features() {
97+
if let Entry::Vacant(entry) = dep_feature_vars.entry(feature_name) {
98+
entry.insert(solver.new_var());
99+
}
100+
}
99101
}
100102

101103
for feature_values in pkg_features.values() {
@@ -109,12 +111,14 @@ fn create_dependencies_vars<'a>(
109111
continue;
110112
};
111113

112-
for dep_features_vars in var_for_is_dependencies_features_used
114+
for dep_feature_vars in var_for_is_dependencies_features_used
113115
.get_mut(&dep_name)
114116
.expect("feature dep name exists")
115117
.values_mut()
116118
{
117-
dep_features_vars.insert(dep_feature, solver.new_var());
119+
if let Entry::Vacant(entry) = dep_feature_vars.entry(dep_feature) {
120+
entry.insert(solver.new_var());
121+
}
118122
}
119123
}
120124
}
@@ -176,7 +180,6 @@ fn process_pkg_features(
176180
pkg_feature_var_map: &HashMap<InternedString, varisat::Var>,
177181
pkg_dependencies: &[Dependency],
178182
pkg_features: &FeatureMap,
179-
check_dev_dependencies: bool,
180183
) {
181184
let optional_dependencies = pkg_dependencies
182185
.iter()
@@ -199,7 +202,7 @@ fn process_pkg_features(
199202
FeatureValue::Dep { dep_name } => {
200203
// Add a clause for each dependency with the provided name (normal/build/dev with target)
201204
for (&(dep_kind, _), &dep_var) in &var_for_is_dependencies_used[&dep_name] {
202-
if dep_kind == DepKind::Development && !check_dev_dependencies {
205+
if dep_kind == DepKind::Development {
203206
continue;
204207
}
205208
solver.add_clause(&[pkg_feature_var.negative(), dep_var.positive()]);
@@ -223,7 +226,7 @@ fn process_pkg_features(
223226
// Add clauses for each dependency with the provided name (normal/build/dev with target)
224227
let dep_var_map = &var_for_is_dependencies_used[&dep_name];
225228
for (&(dep_kind, dep_platform), &dep_var) in dep_var_map {
226-
if dep_kind == DepKind::Development && !check_dev_dependencies {
229+
if dep_kind == DepKind::Development {
227230
continue;
228231
}
229232

@@ -270,36 +273,32 @@ fn process_compatible_dep_summaries(
270273
}
271274

272275
let (name, kind, platform) = (dep.name_in_toml(), dep.kind(), dep.platform());
273-
let dep_var_map = &var_for_is_dependencies_used[&name];
274-
let dep_var = dep_var_map[&(kind, platform)];
275-
276+
let dep_var = var_for_is_dependencies_used[&name][&(kind, platform)];
276277
let dep_feature_var_map = &var_for_is_dependencies_features_used[&name][&(kind, platform)];
277278

278-
let compatible_summaries = by_name
279+
let compatible_pkg_ids = by_name
279280
.get(&dep.package_name())
280281
.into_iter()
281282
.flatten()
282283
.filter(|s| dep.matches(s))
283284
.filter(|s| dep.features().iter().all(|f| s.features().contains_key(f)))
284-
.cloned()
285+
.map(|s| s.package_id())
285286
.collect::<Vec<_>>();
286287

287288
// At least one compatible package should be activated
288-
let dep_clause = compatible_summaries
289+
let dep_clause = compatible_pkg_ids
289290
.iter()
290-
.map(|s| var_for_is_packages_used[&s.package_id()].positive())
291+
.map(|id| var_for_is_packages_used[&id].positive())
291292
.chain([dep_var.negative()])
292293
.collect::<Vec<_>>();
293294

294295
solver.add_clause(&dep_clause);
295296

296297
for (&feature_name, &dep_feature_var) in dep_feature_var_map {
297298
// At least one compatible package with the additional feature should be activated
298-
let dep_feature_clause = compatible_summaries
299+
let dep_feature_clause = compatible_pkg_ids
299300
.iter()
300-
.filter_map(|s| {
301-
var_for_is_packages_features_used[&s.package_id()].get(&feature_name)
302-
})
301+
.filter_map(|id| var_for_is_packages_features_used[&id].get(&feature_name))
303302
.map(|var| var.positive())
304303
.chain([dep_feature_var.negative()])
305304
.collect::<Vec<_>>();
@@ -311,10 +310,9 @@ fn process_compatible_dep_summaries(
311310
// For the selected package for this dependency, the `"default"` feature should be activated if it exists
312311
let mut dep_default_clause = vec![dep_var.negative()];
313312

314-
for s in &compatible_summaries {
315-
let s_pkg_id = s.package_id();
316-
let s_var = var_for_is_packages_used[&s_pkg_id];
317-
let s_feature_var_map = &var_for_is_packages_features_used[&s_pkg_id];
313+
for &id in &compatible_pkg_ids {
314+
let s_var = var_for_is_packages_used[&id];
315+
let s_feature_var_map = &var_for_is_packages_features_used[&id];
318316

319317
if let Some(s_default_feature_var) = s_feature_var_map.get(&INTERNED_DEFAULT) {
320318
dep_default_clause
@@ -348,8 +346,6 @@ pub struct SatResolver {
348346

349347
impl SatResolver {
350348
pub fn new<'a>(registry: impl IntoIterator<Item = &'a Summary>) -> Self {
351-
let check_dev_dependencies = false;
352-
353349
let mut by_name: HashMap<InternedString, Vec<Summary>> = HashMap::new();
354350
for pkg in registry {
355351
by_name.entry(pkg.name()).or_default().push(pkg.clone());
@@ -423,7 +419,6 @@ impl SatResolver {
423419
&var_for_is_packages_features_used[&pkg_id],
424420
pkg_dependencies,
425421
pkg_features,
426-
check_dev_dependencies,
427422
);
428423

429424
process_compatible_dep_summaries(
@@ -434,7 +429,7 @@ impl SatResolver {
434429
&var_for_is_packages_features_used,
435430
&by_name,
436431
pkg_dependencies,
437-
check_dev_dependencies,
432+
false,
438433
);
439434
}
440435

0 commit comments

Comments
 (0)