Activation of features of non-optional dependencies breaks manifest for 2024 edition

[cassaundra]

Problem

Activation of an foo/bar feature dependency where foo is not an optional dependency results causes a manifest error in edition 2024:

[dependencies]
foo = "..."
bar = { version = "...", optional = true }

[features]
bar = ["dep:bar", "foo/bar"] # -> ["dep:bar", "foo/bar", "dep:foo"]

...results in...

feature `bar` includes `dep:foo`, but `foo` is not an optional dependency

Explanation

#14221 introduces implicit activation of dep: for non-weak dependencies when parsing/updating the manifest in edition 2024. This issue appears to be to due to a missing check for whether or not the dependency of the activated feature is marked as optional.

This issue was also brought up in a comment post-merge, and in a Zulip discussion.

Reproducible example

cargo-features = ["edition2024"]

[package]
name = "foobar"
edition = "2024"
version = "0.1.0"

[dependencies]
chrono = "0.4.38"
serde = { version = "1.0.204", optional = true }

[features]
serde = ["dep:serde", "chrono/serde"] # -> ["dep:serde", "chrono/serde", "dep:chrono"]

Version

cargo --version:

cargo 1.82.0-nightly (5f6b9a922 2024-07-19)
release: 1.82.0-nightly
commit-hash: 5f6b9a92201d78af75dc24f14662c3e2dacbbbe1
commit-date: 2024-07-19

[epage]

This check happens before dependence resolution/workspace inheritance, but since optional can't be set in workspace deps, we should be able to determine its optionality from the raw InheritableDependency without needing to check the workspace manifest.

I'd prefer not to have assumptions like this buried in the code

Speaking of assumptions, though it felt dirty, I tried to go the opposite direction and make normalizing of features dependent on normalization of dependencies. However, normalization of dependencies is already dependent on normalization of features.

Alternatively, we could revert the change and try and introduce it later in the process, perhaps in resolution instead?

We need to insert it in a place that knows of and can handle editions. I also prefer it to be recorded in cargo publish which was one of our care abouts when we decided on this direction, see #14016 (comment)

[epage]

To re-summarize the problem

package.edition = "2021"
[features]
feature = ["dep/feature"]
[dependencies]
dep = { version = "1", optional = true }

will cause a dep implicit feature to be created

As we want to suppress implicit features in 2024, we strip all optional dependencies like that. This led to a new problem:

package.edition = "2024"
[features]
feature = ["dep/feature"]
[dependencies]
dep = { version = "1", optional = true }

As dep is stripped, this fails because dep/feature can't be resolved. The user can workaround this by adding dep:dep (discoverability is poor).

We solved this by implicitly adding dep:dep to feature. However, I forgot to check dependencies.dep.optional, so if it isn't optional, it also fails but in a way the user can't workaround.

Logically, checking dependencies.dep.optional gets circular because

• resolve_features needs to know about resolved dependencies to add dep:dep
• resolve_dependencies needs to know about resolved features to know what dep:dep are present

We could break this cycle by making a lot of assumptions about the unresolved forms.

[epage]

Relevant sections of code:

cargo/src/cargo/util/toml/mod.rs

Line 327 in 7f20798

resolved_toml.features = resolve_features(original_toml.features.as_ref(), edition)?;

cargo/src/cargo/util/toml/mod.rs

Lines 371 to 383 in 7f20798

	let activated_opt_deps = resolved_toml
	.features
	.as_ref()
	.map(|map| {
	map.values()
	.flatten()
	.filter_map(|f| match FeatureValue::new(InternedString::new(f)) {
	Dep { dep_name } => Some(dep_name.as_str()),
	_ => None,
	})
	.collect::<HashSet<_>>()
	})
	.unwrap_or_default();

cargo/src/cargo/util/toml/mod.rs

Lines 385 to 395 in 7f20798

	resolved_toml.dependencies = resolve_dependencies(
	gctx,
	edition,
	&features,
	original_toml.dependencies.as_ref(),
	&activated_opt_deps,
	None,
	&inherit,
	package_root,
	warnings,
	)?;