artifact deps should work when target field specified coexists with optional = true

[weihanglo]

What does this PR try to resolve?

fixes #10526

Previously, is_dep_activated depends on activated_dependencies,
which is a map of PackageFeaturesKey to its own activated DepFeatures.
PackageFeaturesKey in feature resolver is always comprised of

• A PackageId of a given dependency, and
• A enum value that helps decoupling activated features for that dependency.
  Currently depending on the type of given dependency.

Looking into issue 10526, it has an activated_dependencies of

{
    (mycrate, NormalOrDevOrArtifactTarget(None)) -> [dep:mybindep]
}

However, this line accidentally use a parent's pkgid
and a dependency's FeaturesFor to compose a key:

(mycrate, NormalOrDevOrArtifactTarget("x86_64-unknown-linux-gnu"))

That is never a valid key to query features for artifacts dependency.
A valid key should be comprised of components both from the parent:

(mycrate, NormalOrDevOrArtifactTarget(None))

Or both from the dependency itself:

(mybindep, NormalOrDevOrArtifactTarget("x86_64-unknown-linux-gnu"))

As aforementioned activated_dependencies only stores parent packages
and their activated features. Those informations are included in
activated_features as well, so this commit goes with the route that
removes activated_dependencies and uses only dependency's infomation
to query if itself is activated.

How should we test and review this PR?

This first commit logs the current "incorrect" behaviour.
The second commit contain the fix.
The last commit update some docs.

Additional information

There could be some other refactors to make the interaction between -Zbindeps and feature resolver a bit better. I'll try to send more follow-ups if life don't bite me again.

[rust-highfive]

r? @epage

(rust-highfive has picked a reviewer for you, use r? to override)

[ehuss]

If my understanding of this is correct, the removal of activated_dependencies is depending on this in order to know whether or not a dependency is activated. That is, the presence of the set determines whether or not something is activated. Is that correct?

If so, I'm slightly uncomfortable with relying on that. The idea was that in the future there would not be an empty set used for things that don't have any features set. Then, the call to expect in activated_features could be removed, and instead be something like unwrap_or_default().

However, there isn't really a strong need for that (I just kind wanted to get rid of the expect() call). I think it should be fine, but can you update the comment linked above? Essentially just remove it and write something to explain the importance of inserting an empty set (that the presence of set also means the dependency is activated). How does that sound to you?

[weihanglo]

Thank you for the review! Of course I will update that comment! And maybe explain more on the purpose of activated_features and activated_dependencies as well.

Just curious, besides removing expect, is there any concern/extension/ambiguity I am not aware of when we start depending on activated_features for checking activation of a dependency? Maybe I miss some contexts, but it seems that we can switch to unwrap_or_default nowadays even if we stop inserting empty set from now on.

[weihanglo]

Rebased and added comments per discussion.

[ehuss]

Looks like these lines are duplicated.

[weihanglo]

Just tweak the comment a bit further.

[ehuss]

Just curious, besides removing expect, is there any concern/extension/ambiguity I am not aware of when we start depending on activated_features for checking activation of a dependency?

I'm not sure. I recall having separate maps seemed important at the time, but I don't remember.

Maybe I miss some contexts, but it seems that we can switch to unwrap_or_default nowadays even if we stop inserting empty set from now on.

I'm not sure I follow. With this change, it must insert an empty set, so skipping that step is no longer an option?

[weihanglo]

I feel like we are not on the same page, @ehuss 😆

Maybe I miss some contexts, but it seems that we can switch to unwrap_or_default nowadays even if we stop inserting empty set from now on.

I'm not sure I follow. With this change, it must insert an empty set, so skipping that step is no longer an option?

activated_features_int returns a Result so unwrap_or_default can easily recover from it with a default. There is no need of the presence of an empty set, no?

cargo/src/cargo/core/resolver/features.rs

Lines 352 to 363 in 1945b44

	fn activated_features_int(
	&self,
	pkg_id: PackageId,
	features_for: FeaturesFor,
	) -> CargoResult<Vec<InternedString>> {
	let fk = features_for.apply_opts(&self.opts);
	if let Some(fs) = self.activated_features.get(&(pkg_id, fk)) {
	Ok(fs.iter().cloned().collect())
	} else {
	bail!("features did not find {:?} {:?}", pkg_id, fk)
	}
	}

[ehuss]

Ah, I was conflating "switch to unwrap_or_default" and "remove inserting the empty set", as I was thinking of them as a single action that would be done together. I suppose it could switch to unwrap_or_default, but I like the validation that expect has been giving.

[ehuss]

Thanks!

@bors r+

[bors]

📌 Commit 2b913b6 has been approved by ehuss

It is now in the queue for this repository.

[bors]

⌛ Testing commit 2b913b6 with merge d4c38af...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing d4c38af to master...