Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add fixes in the sat resolver #14707

Merged
merged 2 commits into from
Oct 22, 2024
Merged

test: add fixes in the sat resolver #14707

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af9b2d7
test: Avoid creating duplicate variables in the sat resolver
x-hgg-x Oct 19, 2024
c85d832
test: Avoid needlessly cloning in the sat resolver
x-hgg-x Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 27 additions & 32 deletions crates/resolver-tests/src/sat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::collections::hash_map::Entry;
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
use std::fmt::Write;

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

let dep_feature_var_map = dep
.features()
.iter()
.map(|&f| (f, solver.new_var()))
.collect();

var_for_is_dependencies_features_used
let dep_feature_vars = var_for_is_dependencies_features_used
.entry(name)
.or_default()
.insert((kind, platform), dep_feature_var_map);
.entry((kind, platform))
.or_default();

for &feature_name in dep.features() {
if let Entry::Vacant(entry) = dep_feature_vars.entry(feature_name) {
entry.insert(solver.new_var());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change? Is this just to avoid allocating several new_var's? Do we really end up with dependencies that activate the same feature more than once?

Also would this be clearer as .entry(feature_name).or_insert_with(|| solver.new_var())?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change? Is this just to avoid allocating several new_var's?

Yes

Also would this be clearer as .entry(feature_name).or_insert_with(|| solver.new_var())?

I didn't use or_insert_with because I don't need the returned Entry.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that makes sense.

Do we really end up with dependencies that activate the same feature more than once?

For testing purposes I added a else {todo!()} to each of these, and all tests pass. So I don't think our test suite hits this case. What am I missing? Do you have plans that will add test code that hits this case? Are there significant downsides to a few calls to solver.new_var()? I'm not objecting, I'm just trying to understand.

Copy link
Contributor Author

@x-hgg-x x-hgg-x Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else {todo!()} branch will trigger if we have a dependency with a list of features containing duplicates, or if the feature "dep/feature" or "dep?/feature" is listed multiple times among all features declared in a summary.

I will add a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there significant downsides to a few calls to solver.new_var()?

Maybe not, but removing duplicates will improve the debugging if we want to manually inspect the clauses.

}
}
}

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

for dep_features_vars in var_for_is_dependencies_features_used
for dep_feature_vars in var_for_is_dependencies_features_used
.get_mut(&dep_name)
.expect("feature dep name exists")
.values_mut()
{
dep_features_vars.insert(dep_feature, solver.new_var());
if let Entry::Vacant(entry) = dep_feature_vars.entry(dep_feature) {
entry.insert(solver.new_var());
}
}
}
}
Expand Down Expand Up @@ -176,7 +180,6 @@ fn process_pkg_features(
pkg_feature_var_map: &HashMap<InternedString, varisat::Var>,
pkg_dependencies: &[Dependency],
pkg_features: &FeatureMap,
check_dev_dependencies: bool,
) {
let optional_dependencies = pkg_dependencies
.iter()
Expand All @@ -199,7 +202,7 @@ fn process_pkg_features(
FeatureValue::Dep { dep_name } => {
// Add a clause for each dependency with the provided name (normal/build/dev with target)
for (&(dep_kind, _), &dep_var) in &var_for_is_dependencies_used[&dep_name] {
if dep_kind == DepKind::Development && !check_dev_dependencies {
if dep_kind == DepKind::Development {
continue;
}
solver.add_clause(&[pkg_feature_var.negative(), dep_var.positive()]);
Expand All @@ -223,7 +226,7 @@ fn process_pkg_features(
// Add clauses for each dependency with the provided name (normal/build/dev with target)
let dep_var_map = &var_for_is_dependencies_used[&dep_name];
for (&(dep_kind, dep_platform), &dep_var) in dep_var_map {
if dep_kind == DepKind::Development && !check_dev_dependencies {
if dep_kind == DepKind::Development {
continue;
}

Expand Down Expand Up @@ -270,36 +273,32 @@ fn process_compatible_dep_summaries(
}

let (name, kind, platform) = (dep.name_in_toml(), dep.kind(), dep.platform());
let dep_var_map = &var_for_is_dependencies_used[&name];
let dep_var = dep_var_map[&(kind, platform)];

let dep_var = var_for_is_dependencies_used[&name][&(kind, platform)];
let dep_feature_var_map = &var_for_is_dependencies_features_used[&name][&(kind, platform)];

let compatible_summaries = by_name
let compatible_pkg_ids = by_name
.get(&dep.package_name())
.into_iter()
.flatten()
.filter(|s| dep.matches(s))
.filter(|s| dep.features().iter().all(|f| s.features().contains_key(f)))
.cloned()
.map(|s| s.package_id())
.collect::<Vec<_>>();

// At least one compatible package should be activated
let dep_clause = compatible_summaries
let dep_clause = compatible_pkg_ids
.iter()
.map(|s| var_for_is_packages_used[&s.package_id()].positive())
.map(|id| var_for_is_packages_used[&id].positive())
.chain([dep_var.negative()])
.collect::<Vec<_>>();

solver.add_clause(&dep_clause);

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

for s in &compatible_summaries {
let s_pkg_id = s.package_id();
let s_var = var_for_is_packages_used[&s_pkg_id];
let s_feature_var_map = &var_for_is_packages_features_used[&s_pkg_id];
for &id in &compatible_pkg_ids {
let s_var = var_for_is_packages_used[&id];
let s_feature_var_map = &var_for_is_packages_features_used[&id];

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

impl SatResolver {
pub fn new<'a>(registry: impl IntoIterator<Item = &'a Summary>) -> Self {
let check_dev_dependencies = false;

let mut by_name: HashMap<InternedString, Vec<Summary>> = HashMap::new();
for pkg in registry {
by_name.entry(pkg.name()).or_default().push(pkg.clone());
Expand Down Expand Up @@ -423,7 +419,6 @@ impl SatResolver {
&var_for_is_packages_features_used[&pkg_id],
pkg_dependencies,
pkg_features,
check_dev_dependencies,
);

process_compatible_dep_summaries(
Expand All @@ -434,7 +429,7 @@ impl SatResolver {
&var_for_is_packages_features_used,
&by_name,
pkg_dependencies,
check_dev_dependencies,
false,
);
}

Expand Down