Small resolver cleanups (#15040)

This is just three small resolver cleanups I found while doing some
other work.
This commit is contained in:
Weihang Lo 2025-02-05 22:03:38 +00:00 committed by GitHub
commit 8eab10a348
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 31 additions and 32 deletions

View File

@ -3,10 +3,10 @@ use super::errors::ActivateResult;
use super::types::{ActivationsKey, ConflictMap, ConflictReason, FeaturesSet, ResolveOpts}; use super::types::{ActivationsKey, ConflictMap, ConflictReason, FeaturesSet, ResolveOpts};
use super::RequestedFeatures; use super::RequestedFeatures;
use crate::core::{Dependency, PackageId, Summary}; use crate::core::{Dependency, PackageId, Summary};
use crate::util::interning::InternedString; use crate::util::interning::{InternedString, INTERNED_DEFAULT};
use crate::util::Graph; use crate::util::Graph;
use anyhow::format_err; use anyhow::format_err;
use std::collections::HashMap; use std::collections::{BTreeSet, HashMap};
use tracing::debug; use tracing::debug;
// A `Context` is basically a bunch of local resolution information which is // A `Context` is basically a bunch of local resolution information which is
@ -121,6 +121,7 @@ impl ResolverContext {
} }
} }
debug!("checking if {} is already activated", summary.package_id()); debug!("checking if {} is already activated", summary.package_id());
let empty_features = BTreeSet::new();
match &opts.features { match &opts.features {
// This returns `false` for CliFeatures just for simplicity. It // This returns `false` for CliFeatures just for simplicity. It
// would take a bit of work to compare since they are not in the // would take a bit of work to compare since they are not in the
@ -135,16 +136,16 @@ impl ResolverContext {
features, features,
uses_default_features, uses_default_features,
} => { } => {
let has_default_feature = summary.features().contains_key("default"); let has_default_feature = summary.features().contains_key(&INTERNED_DEFAULT);
Ok(match self.resolve_features.get(&id) { let prev = self
Some(prev) => { .resolve_features
features.is_subset(prev) .get(&id)
&& (!uses_default_features .map(|f| &**f)
|| prev.contains("default") .unwrap_or(&empty_features);
|| !has_default_feature) Ok(features.is_subset(prev)
} && (!uses_default_features
None => features.is_empty() && (!uses_default_features || !has_default_feature), || prev.contains(&INTERNED_DEFAULT)
}) || !has_default_feature))
} }
} }
} }

View File

@ -59,7 +59,6 @@
//! over the place. //! over the place.
use std::collections::{BTreeMap, HashMap, HashSet}; use std::collections::{BTreeMap, HashMap, HashSet};
use std::mem;
use std::rc::Rc; use std::rc::Rc;
use std::time::{Duration, Instant}; use std::time::{Duration, Instant};
@ -142,9 +141,7 @@ pub fn resolve(
let mut past_conflicting_activations = conflict_cache::ConflictCache::new(); let mut past_conflicting_activations = conflict_cache::ConflictCache::new();
let resolver_ctx = loop { let resolver_ctx = loop {
let resolver_ctx = ResolverContext::new();
let resolver_ctx = activate_deps_loop( let resolver_ctx = activate_deps_loop(
resolver_ctx,
&mut registry, &mut registry,
summaries, summaries,
first_version, first_version,
@ -199,13 +196,13 @@ pub fn resolve(
/// If all dependencies can be activated and resolved to a version in the /// If all dependencies can be activated and resolved to a version in the
/// dependency graph, `cx` is returned. /// dependency graph, `cx` is returned.
fn activate_deps_loop( fn activate_deps_loop(
mut resolver_ctx: ResolverContext,
registry: &mut RegistryQueryer<'_>, registry: &mut RegistryQueryer<'_>,
summaries: &[(Summary, ResolveOpts)], summaries: &[(Summary, ResolveOpts)],
first_version: Option<VersionOrdering>, first_version: Option<VersionOrdering>,
gctx: Option<&GlobalContext>, gctx: Option<&GlobalContext>,
past_conflicting_activations: &mut conflict_cache::ConflictCache, past_conflicting_activations: &mut conflict_cache::ConflictCache,
) -> CargoResult<ResolverContext> { ) -> CargoResult<ResolverContext> {
let mut resolver_ctx = ResolverContext::new();
let mut backtrack_stack = Vec::new(); let mut backtrack_stack = Vec::new();
let mut remaining_deps = RemainingDeps::new(); let mut remaining_deps = RemainingDeps::new();
@ -759,22 +756,8 @@ impl RemainingCandidates {
) -> Option<(Summary, bool)> { ) -> Option<(Summary, bool)> {
for b in self.remaining.iter() { for b in self.remaining.iter() {
let b_id = b.package_id(); let b_id = b.package_id();
// The `links` key in the manifest dictates that there's only one
// package in a dependency graph, globally, with that particular
// `links` key. If this candidate links to something that's already
// linked to by a different package then we've gotta skip this.
if let Some(link) = b.links() {
if let Some(&a) = cx.links.get(&link) {
if a != b_id {
conflicting_prev_active
.entry(a)
.or_insert_with(|| ConflictReason::Links(link));
continue;
}
}
}
// Otherwise the condition for being a valid candidate relies on // The condition for being a valid candidate relies on
// semver. Cargo dictates that you can't duplicate multiple // semver. Cargo dictates that you can't duplicate multiple
// semver-compatible versions of a crate. For example we can't // semver-compatible versions of a crate. For example we can't
// simultaneously activate `foo 1.0.2` and `foo 1.2.0`. We can, // simultaneously activate `foo 1.0.2` and `foo 1.2.0`. We can,
@ -791,12 +774,27 @@ impl RemainingCandidates {
} }
} }
// Otherwise the `links` key in the manifest dictates that there's only one
// package in a dependency graph, globally, with that particular
// `links` key. If this candidate links to something that's already
// linked to by a different package then we've gotta skip this.
if let Some(link) = b.links() {
if let Some(&a) = cx.links.get(&link) {
if a != b_id {
conflicting_prev_active
.entry(a)
.or_insert_with(|| ConflictReason::Links(link));
continue;
}
}
}
// Well if we made it this far then we've got a valid dependency. We // Well if we made it this far then we've got a valid dependency. We
// want this iterator to be inherently "peekable" so we don't // want this iterator to be inherently "peekable" so we don't
// necessarily return the item just yet. Instead we stash it away to // necessarily return the item just yet. Instead we stash it away to
// get returned later, and if we replaced something then that was // get returned later, and if we replaced something then that was
// actually the candidate to try first so we return that. // actually the candidate to try first so we return that.
if let Some(r) = mem::replace(&mut self.has_another, Some(b.clone())) { if let Some(r) = self.has_another.replace(b.clone()) {
return Some((r, true)); return Some((r, true));
} }
} }