Add some documentation to the resolver #5195

alexcrichton · 2018-03-16T14:33:52Z

I spent a few hours yesterday re-teaching myself the resolver and wanted to add some comments along the way!

rust-highfive · 2018-03-16T14:33:56Z

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2018-03-16T14:40:26Z

cc @Eh2406 I'm sure you'll be interested in this! This'll surely conflict with #5187 so I'd like to hold off on merging this until that goes through

alexcrichton · 2018-03-16T14:41:30Z

src/cargo/core/resolver/mod.rs

                    let mut has_past_conflicting_dep = just_here_for_the_error_messages;
                    if !has_past_conflicting_dep {
                        if let Some(conflicting) = frame
                            .remaining_siblings
                            .clone()
-                            .filter_map(|(_, (deb, _, _))| {
-                                past_conflicting_activations.get(&deb).and_then(|past_bad| {


This part I was a little confused about given that we also modify the past_conflicting_activations above, and before merging I'd like to incorporate #5168 (comment) into the comment on this if statement

matklad

❤️

Looks great to me, modulo a couple of typos. However, as I don't really understand deeply how resolve works, it will take me large amount of time to actually verify that comments accurately describe the thing the code is doing, so

@bors r=Eh2406

matklad · 2018-03-16T14:48:26Z

src/cargo/core/resolver/mod.rs

    MissingFeatures(String),
 }

 enum ActivateError {
-    Error(::failure::Error),
+    Fatal(::failure::Error),


Maybe change ::failure::Error to CargoError as well? They are one:

pub use failure::Error as CargoError;

matklad · 2018-03-16T14:59:55Z

src/cargo/core/resolver/mod.rs

+    // `past_conflicting_activations`is a cache of the reasons for each time we
+    // backtrack. For example after several backtracks we may have:
+    //
+    //  past_conflicting_activations[`foo = "^1.0.2"`] = vac![


matklad · 2018-03-16T15:01:40Z

src/cargo/core/resolver/mod.rs

+
+        // `conflicting_activations` stores all the reasons we were unable to
+        // activate candidates. One of these reasons will have to go away for
+        // backtracking to find a place to restart  It is also the list of


missing . before It.

matklad · 2018-03-16T15:03:24Z

src/cargo/core/resolver/mod.rs

            })?;

+            // TODO: explain this
            if just_here_for_the_error_messages && !backtracked && has_another {


We know we are going to fail all the way to the user: if just_here_for_the_error_messages and
yet we have another thing to try: has_another so if we try this it won't make it all the way to the user. Insted it will fail and bactrak to us to try the next candidate. So let's just skip to the next candidate.

Aha that definitely makes sense!

Put another way, if just_here_for_the_error_messages is true, we basically want to exhaust the iterator immediately to collect the HashMap it spits out as to why we filtered out candidates?

Sort of. If just_here_for_the_error_messages then we ether want to

Exhaust the iterator immediately (as you said) so this dep can bactrack to the users.

Activate the last candidate so one of our dependencies can bactrack to the users.

Oh hm I'm not sure I understand then. For that second condition where we want to activate the last candidate so we can backtrack, how come the last candidate is special? Is that because it's the only one which'll force a backtrack if it fails?

Yes? It is the only one that does not add a backtrack frame for our children to come back to. And we could just pick the first one and not add a backtrack frame, but then the error message will be different.

Befor: ... which is depended on by `bar v0.1.0`
After: ... which is depended on by `bar v0.1.99`

Eh2406 · 2018-03-16T15:36:40Z

I have been reading the comments and they look great to me so far! I got up to This is used during backtracking to ensure we can skip over as much work as possible., but notest that I was no longer actually focusing well. I will take a break and come back to this. The part I read looks really good!

Eh2406

Overall looks like a lot of improvement.

Eh2406 · 2018-03-16T17:29:29Z

src/cargo/core/resolver/mod.rs

            })?;

+            // TODO: explain this
            if just_here_for_the_error_messages && !backtracked && has_another {


We know we are going to fail all the way to the user: if just_here_for_the_error_messages and
yet we have another thing to try: has_another so if we try this it won't make it all the way to the user. Insted it will fail and bactrak to us to try the next candidate. So let's just skip to the next candidate.

Eh2406 · 2018-03-16T17:31:53Z

src/cargo/core/resolver/mod.rs

+                // global list of past conflicting activations, effectively
+                // globally poisoning `dep` if `conflicting_activations` ever
+                // shows up again. This is used during backtracking to ensure we
+                // can skip over as much work as possible.


I miss read this as meaning past_conflicting_activations instead of conflicting_activations

Eh2406 · 2018-03-16T17:46:30Z

src/cargo/core/resolver/mod.rs

+                    // If a dependency is known to fail we may want to skip this
+                    // frame as this is surely a dead end. If there are more
+                    // candidates we wait until the last one to generate an
+                    // error message in this case.


This is better than what I had, by a lot, but I wonder if it could be clearer. Semse like a lot of interacting clauses in the last sentence. What is in this case modifying?
Maybe:

If a dependency is known to fail we want to skip this frame as this is surely a dead end. If there are more candidates then we can skip this frame without affecting error messages. However, if this is last candidate we activate to get its error message.

Eh2406 · 2018-03-16T17:48:12Z

src/cargo/core/resolver/mod.rs

+                // backtrack from errors like these
+                Err(ActivateError::Fatal(e)) => return Err(e),
+
+                // We failed to do a bland conflict, bah! Record this in our


Don't quite follow this sentence.

Oops, "to do" was meant to be "due to"

Eh2406 · 2018-03-16T17:49:31Z

src/cargo/core/resolver/mod.rs

-                    // `activate` changed `cx` and then failed so put things back.
-                    cx = b.context_backup;
-                } // else we are just using the cx for the error message so we can live with a corrupted one
+                backtrack_stack.extend(backtrack);


extend brilliant!

bors · 2018-03-17T18:21:53Z

☔ The latest upstream changes (presumably #5187) made this pull request unmergeable. Please resolve the merge conflicts.

Eh2406 · 2018-03-18T16:52:18Z

To be clear I am +1 when you feel this is ready and it is mergeable. :-)

Then I think working together, we are in a good place to solve #4810 (comment); the last reproducible and in the wild exponentially slow resolution (that I have found).

alexcrichton · 2018-03-19T15:41:17Z

Ok I've rebased and updated with the recent merge conflict, @Eh2406 mind giving it a once over to make sure it still makes sense?

Eh2406

Lots of good in this PR! Just some small nits, and ci is red.

Eh2406 · 2018-03-19T16:14:31Z

src/cargo/core/resolver/mod.rs

+/// is defined within a `Context`.
+///
+/// Candidates passed to `new` may not be returned from `next` as they could be
+/// filtered out, and if iteration stops a map of all failed candidates will be


Nit: I think it returns a map of all conflicting already activated PackageId not a map of the filtered (not yet activated) candidates.

Aha yes indeed!

Eh2406 · 2018-03-19T16:18:25Z

src/cargo/core/resolver/mod.rs

+    ///
+    /// If we've reached the end of the iterator here then `Err` will be
+    /// returned. The error will contain a map of package id to conflict reason,
+    /// where each package id was filtered out from the original list for the


I think this has the structure backwards, where each package id that has already been activated that causes something to be filtered out to the reason it caused a candidate to be skipped.

Eh2406 · 2018-03-19T16:36:58Z

src/cargo/core/resolver/mod.rs

+        // things to explain in the error message if we fail to resolve.
+        //
+        // This is a map of package id to a reason why that packaged caused a
+        // conflict for us.


This is a very well explained description of what is happening. Perhaps it can be used for my nits.

Eh2406 · 2018-03-19T16:41:51Z

src/cargo/core/resolver/mod.rs

+                    // If we're only here for the error messages then we know
+                    // one of our candidate deps will fail, meaning we will
+                    // fail and that none of the backtrack frames will find a
+                    // candidate that will help. Consequently le'ts clean up the


le'ts -> let's

This is currently my best-effort attempt to document various portions of the resolver with the logic that's been added recently. It at least helped me understand a bit what was going on so I hope it can help others as well!

alexcrichton · 2018-03-19T20:55:31Z

@bors: r=Eh2406

Ok cool, thanks for double checking me!

bors · 2018-03-19T20:55:32Z

📌 Commit 5695119 has been approved by Eh2406

bors · 2018-03-19T20:55:39Z

⌛ Testing commit 5695119 with merge 6ececacf83586369c86be162153c9e45d75f6920...

alexcrichton · 2018-03-19T21:04:27Z

@bors: retry

bors · 2018-03-19T21:42:04Z

💔 Test failed - status-travis

alexcrichton · 2018-03-19T21:42:25Z

@bors: retry

alexcrichton · 2018-03-19T21:50:48Z

@bors: retry

bors · 2018-03-19T22:23:40Z

⌛ Testing commit 5695119 with merge a9b1369...

Add some documentation to the resolver I spent a few hours yesterday re-teaching myself the resolver and wanted to add some comments along the way!

bors · 2018-03-19T23:05:20Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Eh2406
Pushing a9b1369 to master...

rust-highfive assigned matklad Mar 16, 2018

alexcrichton mentioned this pull request Mar 16, 2018

Faster resolver: Cache past conflicting_activations, prevent doing the same work repeatedly. #5168

Merged

alexcrichton commented Mar 16, 2018

View reviewed changes

matklad reviewed Mar 16, 2018

View reviewed changes

Eh2406 reviewed Mar 16, 2018

View reviewed changes

alexcrichton force-pushed the resolver-docs branch from b574800 to 20239d2 Compare March 19, 2018 15:40

alexcrichton changed the title ~~[WIP] Some documentation for the resolver~~ Add some documentation to the resolver Mar 19, 2018

alexcrichton force-pushed the resolver-docs branch from 20239d2 to 92fb7d0 Compare March 19, 2018 15:43

Eh2406 reviewed Mar 19, 2018

View reviewed changes

Add some documentation to the resolver

5695119

This is currently my best-effort attempt to document various portions of the resolver with the logic that's been added recently. It at least helped me understand a bit what was going on so I hope it can help others as well!

alexcrichton force-pushed the resolver-docs branch from 92fb7d0 to 5695119 Compare March 19, 2018 20:55

bors added a commit that referenced this pull request Mar 19, 2018

Auto merge of #5195 - alexcrichton:resolver-docs, r=Eh2406

a9b1369

Add some documentation to the resolver I spent a few hours yesterday re-teaching myself the resolver and wanted to add some comments along the way!

bors merged commit 5695119 into rust-lang:master Mar 19, 2018

alexcrichton deleted the resolver-docs branch April 25, 2018 19:28

ehuss added this to the 1.26.0 milestone Feb 6, 2022

Add some documentation to the resolver #5195

Add some documentation to the resolver #5195

Conversation

alexcrichton commented Mar 16, 2018

rust-highfive commented Mar 16, 2018

alexcrichton commented Mar 16, 2018

Choose a reason for hiding this comment

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eh2406 commented Mar 16, 2018

Eh2406 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 17, 2018

Eh2406 commented Mar 18, 2018

alexcrichton commented Mar 19, 2018

Eh2406 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Mar 19, 2018

bors commented Mar 19, 2018

bors commented Mar 19, 2018

alexcrichton commented Mar 19, 2018

bors commented Mar 19, 2018

alexcrichton commented Mar 19, 2018

alexcrichton commented Mar 19, 2018

bors commented Mar 19, 2018

bors commented Mar 19, 2018