Skip to content

Commit d0c80ec

Browse files
committed
address nits
1 parent 8cd9b0c commit d0c80ec

File tree

4 files changed

+43
-19
lines changed

4 files changed

+43
-19
lines changed

src/cargo/core/resolver/context.rs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use log::debug;
99

1010
use crate::core::interning::InternedString;
1111
use crate::core::{Dependency, FeatureValue, PackageId, SourceId, Summary};
12+
use crate::util::config::Config;
1213
use crate::util::CargoResult;
1314
use crate::util::Graph;
1415

@@ -217,7 +218,7 @@ impl Context {
217218
parent: Option<&Summary>,
218219
s: &'b Summary,
219220
method: &'b Method<'_>,
220-
config: Option<&crate::util::config::Config>,
221+
config: Option<&Config>,
221222
) -> ActivateResult<Vec<(Dependency, Vec<InternedString>)>> {
222223
let dev_deps = match *method {
223224
Method::Everything => true,
@@ -329,14 +330,16 @@ impl Context {
329330
}
330331

331332
pub fn graph(&self) -> Graph<PackageId, Vec<Dependency>> {
332-
let mut graph = Graph::new();
333+
let mut graph: Graph<PackageId, Vec<Dependency>> = Graph::new();
333334
self.activations
334335
.values()
335336
.for_each(|(r, _)| graph.add(r.package_id()));
336337
for i in self.parents.iter() {
337338
graph.add(*i);
338339
for (o, e) in self.parents.edges(i) {
339-
*graph.link(*o, *i) = e.to_vec();
340+
let old_link = graph.link(*o, *i);
341+
debug_assert!(old_link.is_empty());
342+
*old_link = e.to_vec();
340343
}
341344
}
342345
graph

src/cargo/core/resolver/mod.rs

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1088,7 +1088,11 @@ fn check_duplicate_pkgs_in_lockfile(resolve: &Resolve) -> CargoResult<()> {
10881088
Ok(())
10891089
}
10901090

1091-
/// re-run all used resolve_features so it can print warnings
1091+
/// Re-run all used `resolve_features` so it can print warnings.
1092+
/// Within the resolution loop we do not pass a `config` to `resolve_features`
1093+
/// this tells it not to print warnings. Hear we do pass `config` triggering it to print them.
1094+
/// This is done as the resolution is NP-Hard so the loop can potentially call `resolve features`
1095+
/// an exponential number of times, but this pass is linear in the number of dependencies.
10921096
fn emit_warnings(
10931097
cx: &Context,
10941098
resolve: &Resolve,
@@ -1098,22 +1102,26 @@ fn emit_warnings(
10981102
let mut new_cx = cx.clone();
10991103
new_cx.resolve_features = im_rc::HashMap::new();
11001104
let mut features_from_dep = HashMap::new();
1101-
for (summery, method) in summaries {
1105+
for (summary, method) in summaries {
11021106
for (dep, features) in new_cx
1103-
.resolve_features(None, summery, &method, config)
1104-
.expect("can not resolve_features for a required summery")
1107+
.resolve_features(None, summary, &method, config)
1108+
.expect("can not resolve_features for a required summary")
11051109
{
1106-
features_from_dep.insert((summery.package_id(), dep), features);
1110+
features_from_dep.insert((summary.package_id(), dep), features);
11071111
}
11081112
}
1109-
for summery in resolve.sort().iter().rev().map(|id| {
1113+
// crates enable features for their dependencies.
1114+
// So by iterating reverse topologically we process all of the parents before each child.
1115+
// Then we know all the needed features for each crate.
1116+
let topological_sort = resolve.sort();
1117+
for summary in topological_sort.iter().rev().map(|id| {
11101118
cx.activations
11111119
.get(&id.as_activations_key())
11121120
.expect("id in dependency graph but not in activations")
11131121
.0
11141122
.clone()
11151123
}) {
1116-
for (parent, deps) in cx.parents.edges(&summery.package_id()) {
1124+
for (parent, deps) in cx.parents.edges(&summary.package_id()) {
11171125
for dep in deps.iter() {
11181126
let features = features_from_dep
11191127
.remove(&(*parent, dep.clone()))
@@ -1125,10 +1133,10 @@ fn emit_warnings(
11251133
uses_default_features: dep.uses_default_features(),
11261134
};
11271135
for (dep, features) in new_cx
1128-
.resolve_features(None, &summery, &method, config)
1136+
.resolve_features(None, &summary, &method, config)
11291137
.expect("can not resolve_features for a used dep")
11301138
{
1131-
features_from_dep.insert((summery.package_id(), dep), features);
1139+
features_from_dep.insert((summary.package_id(), dep), features);
11321140
}
11331141
}
11341142
}

src/cargo/core/resolver/types.rs

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,8 +219,21 @@ impl<'a> RegistryQueryer<'a> {
219219
debug!("\t{} => {}", dep.package_name(), dep.version_req());
220220
}
221221
if let Some(r) = &replace {
222-
self.used_replacements
223-
.insert(summary.package_id(), r.package_id());
222+
if let Some(old) = self
223+
.used_replacements
224+
.insert(summary.package_id(), r.package_id())
225+
{
226+
debug_assert_eq!(
227+
r.package_id(),
228+
old,
229+
"we are inconsistent about witch replacement is used for {:?}, \
230+
one time we used {:?}, \
231+
now we are adding {:?}.",
232+
summary.package_id(),
233+
old,
234+
r.package_id()
235+
)
236+
}
224237
}
225238

226239
candidate.replace = replace;

tests/testsuite/resolve.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,13 +111,13 @@ proptest! {
111111
) {
112112
let reg = registry(input.clone());
113113
let mut removed_input = input.clone();
114-
for (summery_idx, dep_idx) in indexes_to_remove {
114+
for (summary_idx, dep_idx) in indexes_to_remove {
115115
if !removed_input.is_empty() {
116-
let summery_idx = summery_idx.index(removed_input.len());
117-
let deps = removed_input[summery_idx].dependencies();
116+
let summary_idx = summary_idx.index(removed_input.len());
117+
let deps = removed_input[summary_idx].dependencies();
118118
if !deps.is_empty() {
119-
let new = remove_dep(&removed_input[summery_idx], dep_idx.index(deps.len()));
120-
removed_input[summery_idx] = new;
119+
let new = remove_dep(&removed_input[summary_idx], dep_idx.index(deps.len()));
120+
removed_input[summary_idx] = new;
121121
}
122122
}
123123
}

0 commit comments

Comments
 (0)