Fix overriding mixing with default features

[alexcrichton]

Previously Cargo had a bug where if a crate without a default feature was
overridden with one that did indeed have a default feature then the default may
not end up getting activated by accident. The fix was to avoid returning too
quickly hen activating dependencies until after we've inspected and learned
about replacements.

Closes #3812

[rust-highfive]

r? @brson

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

[alexcrichton]

r? @matklad

[jethrogb]

Whew this must have been a pain to figure out! Thanks a lot.

If you run the test in this PR on the old code, will it sometimes pass non-deterministically?

I think this works... sort-of. Since the failure mode of this bug was that most of the time, the default feature would not be selected, this might be considered a breaking change since the default feature will now always be selected. Looks like I'm running into #1796 with this fix.

[alexcrichton]

Yeah the test would fail nondeterministically, that's how I figured this fixed it :). A thousand or so runs with no failures makes me think this is confirmed as the right fix.

Yeah features across build deps and across actual deps are kinda hairy unfortunately :(

[matklad]

I am not sure about this && activated. Can it cause the same replacement to be activated twice? Is it bad?

If I remove && activated, the tests pass just as well. So we either don't need it, or don't have some test?

[matklad]

That is, we can move perhaps all ifs after replacement:

let candidate = match candidate.replace {
    Some(replace) => {
        cx.resolve_replacements.insert(candidate.summary.package_id().clone(),
                                       replace.package_id().clone());
        trace!("replacing {} with {}", candidate.summary.package_id(),
                replace.package_id());
        replace
    }
    None => candidate.summary,
};

if cx.flag_activated(&candidate, method) {
    return Ok(None)
}
trace!("activating {}", candidate.package_id());

[matklad]

Ah, looks like moving if forward won't work because during replacement we need to activate both the original and the replacement.

[matklad]

@bors: r+

Okey, I am still not sure if && activated bit is right, but this PR definitely should fix the issue.

[bors]

📌 Commit 2d4cb3e has been approved by matklad

[bors]

⌛ Testing commit 2d4cb3e with merge bb1472e...

[bors]

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

[alexcrichton]

Ah yeah so we definitely need to activate both the original and the replacement, but to answer your original question:

In general Cargo's resolution algorithm is just a brute force search with some heuristics thrown in. We then pick the first successful resolution reached, or we try every possible resolution and determine they're all errors and return an error. (I don't actually know which error...)

In that sense what this flag_activated function is doing is activating a specific version for a crate. This involves registering it in the dependency graph. The check here (return Ok(None)) is purely an optimization saying that if we've already activated this component there's no need to keep activating and traversing the graph here (reducing duplicate work). In that sense I think the && activated is ok because it's not harmful to re-activate something it'll just end up in extra work being done which would otherwise be avoided.

Note that the crux of this bug was that we were returning too soon! When activating a crate you have to follow through with dependency traversal if any new features were activated, but if you've already activated all relevant features then you don't have to go further. The bug here was that we thought that we had already activated a crate when in fact we had forgotten to activate the replacement's default feature.