Conflict tracking #5037
Conflict tracking #5037
Conversation
… easier to extend.
…ions that are in conflict
|
(rust_highfive has picked a reviewer for you, use r? to override) |
src/cargo/core/resolver/mod.rs
Outdated
| @@ -681,11 +695,13 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, | |||
| parent: Summary::clone(&parent), | |||
| dep: Dependency::clone(&dep), | |||
| features: Rc::clone(&features), | |||
| conflicting_activations: conflicting_activations.clone(), | |||
There was a problem hiding this comment.
I'm 90% sure this is always empty - the only place we add to conflicting_activations is when we've run out of candidates, in which case we're about to backtrack...which means conflicting_activations immediately gets overwritten with an empty set again.
This makes sense, because when backtracking you only care about the conflicts for the current dep rather than for the whole resolve history. It also simplifies the code significantly because we don't need to keep it in backtrack frames.
There was a problem hiding this comment.
Indeed, conflicting_activations is at this time mostly unused. It is used to move conflicting to find_candidate and then use to shuttle the info to activation_error. The intent is to allow anything to add to the conflicting_activations at any point in the proses. (specifically for #5000). It would be reasonable to remove it in this PR and add it back in in that PR iif it turns out to be useful there.
There was a problem hiding this comment.
@Eh2406 that sounds like a good strategy to me!
If we need it later we can always add it back in.
|
@bors: r+ |
|
📌 Commit 8899093 has been approved by |
Conflict tracking This is an alternative implementation of #4834. This is slower but hopefully more flexible and clearer. The idea is to keep a list of `PackageId`'s that have caused us to skip a `Candidate`. Then we can use the list when we are backtracking if any items in our list have not been activated then we will have new `Candidate`'s to try so we should stop backtracking. Or to say that another way; We can continue backtracking as long as all the items in our list is still activated. Next this new framework was used to make the error messages more focused. We only need to list the versions that conflict, as opposed to all previously activated versions. Why is this more flexible? 1. It is not limited to conflicts within the same package. If `RemainingCandidates.next` skips something because of a links attribute, that is easy to record, just add the `PackageId` to the set `conflicting_prev_active`. 2. Arbitrary things can add conflicts to the backtracking. If we fail to activate because some dependency needs a feature that does not exist, that is easy to record, just add the `PackageId` to the set `conflicting_activations`. 3. All things that could cause use to fail will be in the error messages, as the error messages loop over the set. 4. With a simple extension, replacing the `HashSet` with a `HashMap<_, Reason>`, we can customize the error messages to show the nature of the conflict. @alexcrichton, @aidanhs, Does the logic look right? Does this seem clearer to you?
|
☀️ Test successful - status-appveyor, status-travis |
This is an alternative implementation of #4834. This is slower but hopefully more flexible and clearer. The idea is to keep a list of
PackageId's that have caused us to skip aCandidate. Then we can use the list when we are backtracking if any items in our list have not been activated then we will have newCandidate's to try so we should stop backtracking. Or to say that another way; We can continue backtracking as long as all the items in our list is still activated.Next this new framework was used to make the error messages more focused. We only need to list the versions that conflict, as opposed to all previously activated versions.
Why is this more flexible?
RemainingCandidates.nextskips something because of a links attribute, that is easy to record, just add thePackageIdto the setconflicting_prev_active.PackageIdto the setconflicting_activations.HashSetwith aHashMap<_, Reason>, we can customize the error messages to show the nature of the conflict.@alexcrichton, @aidanhs, Does the logic look right? Does this seem clearer to you?