Skip to content

Commit dca0b11

Browse files
ehussehuss
committed
Slightly rewrite how conflicting activation errors are processed.
I'm not sure why the original code partitioned the errors in the way that it did. I think it is better to exhaustively match all the reasons, so that when new reasons are added, it will be clear that this code needs to be updated. I also think it simplifies the code a little.
1 parent 36c69a1 commit dca0b11

File tree

1 file changed

+66
-66
lines changed

1 file changed

+66
-66
lines changed

‎src/cargo/core/resolver/errors.rs

Lines changed: 66 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -108,76 +108,76 @@ pub(super) fn activation_error(
108108

109109
let mut conflicting_activations: Vec<_> = conflicting_activations.iter().collect();
110110
conflicting_activations.sort_unstable();
111-
let (links_errors, mut other_errors): (Vec<_>, Vec<_>) = conflicting_activations
112-
.drain(..)
113-
.rev()
114-
.partition(|&(_, r)| r.is_links());
115-
116-
for &(p, r) in links_errors.iter() {
117-
if let ConflictReason::Links(ref link) = *r {
118-
msg.push_str("\n\nthe package `");
119-
msg.push_str(&*dep.package_name());
120-
msg.push_str("` links to the native library `");
121-
msg.push_str(link);
122-
msg.push_str("`, but it conflicts with a previous package which links to `");
123-
msg.push_str(link);
124-
msg.push_str("` as well:\n");
125-
}
126-
msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
127-
}
128-
129-
let (features_errors, mut other_errors): (Vec<_>, Vec<_>) = other_errors
130-
.drain(..)
131-
.partition(|&(_, r)| r.is_missing_features());
132-
133-
for &(p, r) in features_errors.iter() {
134-
if let ConflictReason::MissingFeatures(ref features) = *r {
135-
msg.push_str("\n\nthe package `");
136-
msg.push_str(&*p.name());
137-
msg.push_str("` depends on `");
138-
msg.push_str(&*dep.package_name());
139-
msg.push_str("`, with features: `");
140-
msg.push_str(features);
141-
msg.push_str("` but `");
142-
msg.push_str(&*dep.package_name());
143-
msg.push_str("` does not have these features.\n");
111+
// This is reversed to show the newest versions first. I don't know if there is
112+
// a strong reason to do this, but that is how the code previously worked
113+
// (see https://github.com/rust-lang/cargo/pull/5037) and I don't feel like changing it.
114+
conflicting_activations.reverse();
115+
// Flag used for grouping all semver errors together.
116+
let mut has_semver = false;
117+
118+
for (p, r) in &conflicting_activations {
119+
match r {
120+
ConflictReason::Semver => {
121+
has_semver = true;
122+
}
123+
ConflictReason::Links(link) => {
124+
msg.push_str("\n\nthe package `");
125+
msg.push_str(&*dep.package_name());
126+
msg.push_str("` links to the native library `");
127+
msg.push_str(&link);
128+
msg.push_str("`, but it conflicts with a previous package which links to `");
129+
msg.push_str(&link);
130+
msg.push_str("` as well:\n");
131+
msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
132+
}
133+
ConflictReason::MissingFeatures(features) => {
134+
msg.push_str("\n\nthe package `");
135+
msg.push_str(&*p.name());
136+
msg.push_str("` depends on `");
137+
msg.push_str(&*dep.package_name());
138+
msg.push_str("`, with features: `");
139+
msg.push_str(features);
140+
msg.push_str("` but `");
141+
msg.push_str(&*dep.package_name());
142+
msg.push_str("` does not have these features.\n");
143+
// p == parent so the full path is redundant.
144+
}
145+
ConflictReason::RequiredDependencyAsFeatures(features) => {
146+
msg.push_str("\n\nthe package `");
147+
msg.push_str(&*p.name());
148+
msg.push_str("` depends on `");
149+
msg.push_str(&*dep.package_name());
150+
msg.push_str("`, with features: `");
151+
msg.push_str(features);
152+
msg.push_str("` but `");
153+
msg.push_str(&*dep.package_name());
154+
msg.push_str("` does not have these features.\n");
155+
msg.push_str(
156+
" It has a required dependency with that name, \
157+
but only optional dependencies can be used as features.\n",
158+
);
159+
// p == parent so the full path is redundant.
160+
}
161+
ConflictReason::PublicDependency(pkg_id) => {
162+
// TODO: This needs to be implemented.
163+
unimplemented!("pub dep {:?}", pkg_id);
164+
}
165+
ConflictReason::PubliclyExports(pkg_id) => {
166+
// TODO: This needs to be implemented.
167+
unimplemented!("pub exp {:?}", pkg_id);
168+
}
144169
}
145-
// p == parent so the full path is redundant.
146170
}
147171

148-
let (required_dependency_as_features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors
149-
.drain(..)
150-
.partition(|&(_, r)| r.is_required_dependency_as_features());
151-
152-
for &(p, r) in required_dependency_as_features_errors.iter() {
153-
if let ConflictReason::RequiredDependencyAsFeatures(ref features) = *r {
154-
msg.push_str("\n\nthe package `");
155-
msg.push_str(&*p.name());
156-
msg.push_str("` depends on `");
157-
msg.push_str(&*dep.package_name());
158-
msg.push_str("`, with features: `");
159-
msg.push_str(features);
160-
msg.push_str("` but `");
161-
msg.push_str(&*dep.package_name());
162-
msg.push_str("` does not have these features.\n");
163-
msg.push_str(
164-
" It has a required dependency with that name, \
165-
but only optional dependencies can be used as features.\n",
166-
);
172+
if has_semver {
173+
// Group these errors together.
174+
msg.push_str("\n\nall possible versions conflict with previously selected packages.");
175+
for (p, r) in &conflicting_activations {
176+
if let ConflictReason::Semver = r {
177+
msg.push_str("\n\n previously selected ");
178+
msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
179+
}
167180
}
168-
// p == parent so the full path is redundant.
169-
}
170-
171-
if !other_errors.is_empty() {
172-
msg.push_str(
173-
"\n\nall possible versions conflict with \
174-
previously selected packages.",
175-
);
176-
}
177-
178-
for &(p, _) in other_errors.iter() {
179-
msg.push_str("\n\n previously selected ");
180-
msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
181181
}
182182

183183
msg.push_str("\n\nfailed to select a version for `");

0 commit comments

Comments
 (0)