Remove a redundant heuristic

[Eh2406]

This adds a test for #5529, then removes the heuristic that caused that odd behavior.
I believe this heuristic is redundant with what the resolver already does.

If you believe this heuristic is needed please suggest a test case and I will add it to this PR.

This tests and closes #5529 and should close #5702.

[rust-highfive]

r? @matklad

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

[Eh2406]

Looks like some of these test cases are already in. let me investegate.

[alexcrichton]

Ah yeah I believe the failing tests are why this logic was here, but it should definitely be fine to move around the logic to keep the tests passing!

[Eh2406]

As usual when code looks redundant there's a corner case that has not been considered. In this case the fact that was not in my mental model was that querying some dependencies can have observable side effects. Specifically, git dependencies will be pulled too the latest commit.

[Eh2406]

This passes the tests, but I am not sure it is 100% correct. All the test cases have to do with git dependencies. All the externalities appear to have todo with pulling git changes. So I limited the huristick to just git dependencies.

[Eh2406]

Ok so there is an externality for querying a Crates.io dependency. It triggers a registry update #2895. (Although If I wanted to do an update but not update the registry, I'd think --offline is a better suggestion now that we have it.)

I seem to be out of ideas.

[alexcrichton]

Hm in general I've tried to minimize in Cargo the number of locations where the type of source is specialized, ideally all of them should be total black boxes to the resolver and Cargo. (that doesn't actually manifest currently but it's somewhat close!)

I'm not 100% sure that #5529 is a bug per se in the sense that both crate graphs are "correct" and Cargo doesn't have much ability to pick between the two to say which is "more correct". It may be the case though that we could add a new heuristic for sorting dependencies on whether the dependency will cause a slow update? (something like fetching the index or updating a git repo)

One thing we could also try is adding dummy entries from the lockfile into the "can be locked to" set in the registry. That way we may end up throwing them away if nothing uses it, but it's at least there as a candidate to be used?

[Eh2406]

I'm not 100% sure that #5529 is a bug per se

I'd think that a quickchek like invariant is that if a lockfile was newly generated and nun of the inputs have changed then all cargo update ... commands will not change the lock file. But given that the heuristics in the resolver are order dependant, I am not sure that this invariant is true.

One thing we could also try is adding dummy entries from the lockfile into the "can be locked to" set in the registry. That way we may end up throwing them away if nothing uses it, but it's at least there as a candidate to be used?

Rufly this heuristic should be looking at union(things locked, things preferred by conservative update) instead of just things locked?

[alexcrichton]

@Eh2406 I believe so yeah!

Awhile back I made some tweaks which were targeted at fixing errors during an update which otherwise didn't need to happen, and along the way what I ended up doing was to basically sort version candidates to the front if they were already in the lock file (preferring those) over other candidates. The upside of this is that most of the time the candidates in the lock file always work, but in the rare case they cause conflicts or something like that we still have the other candidates to try.

I think is similar in the sense that we should first try what was already in the lock file (in theory) or whatever candidates are available in the lock file. If that all fails though we should fall back to normal version resolution and update the registry and such. In theory this is sort of like an iterator where each call to next() may return a long time, and it's sorted by lockfile-first which don't need to update the registry.

I'm not sure how much of that's easy to implement though :(

[Eh2406]

I very agree! I really like your move things in lockfile to the front heuristic! The thrust of this PR is that we should rely on it more, and reduce the use of ="lockfile ver" heuristics.

Naively removing this heuristic did not work, as it indirectly signals some externalities not to trigger. So what we want is for the resolver to backtrack from querying with out externalities to querying with externalities. I have been thinking on that and have not figured out how to teach the resolver to do that. :-(

But I do think we can add a new layer of backtracking, If we add an argument to querying for whether to trigger externalities then we can do a conservative update by calling the resolver with that argument set to no externalities if it succeeds we are good, if not we can rerun the resolver argument set to use externalities.

I think this will work but have not tried it yet. I don't know when I will so I left this as notes for futcher me.

[alexcrichton]

So what we want is for the resolver to backtrack from querying with out externalities to querying with externalities

I think this is exactly the problem, yeah!

I'm down for a solution to this so long as it keeps test passing, so feel free to go wild!

[alexcrichton]

I'm gonna close this for now to help clear out the queue, but I'm all for merging w/ the last point figured out!