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,
	)?;

[cassaundra]

Thanks for taking care of this! Much appreciated for all your hard work.

[weihanglo]

@epage Compiler team would like to run some edition 2024 tests, but this Cargo bug is blocking them. Anything that you can think of might not be appropriate for us to beta-backport #14295?

They need it in beta because rustc bootstrap is using beta compiler for stage0.

cc rust-lang/rust#127672

[compiler-errors]

Howdy. I'm the one who asked if we could backport this.

I'd really like to start dogfooding edition 2024 in the compiler, but since the last beta bump contained #14221, I can't use any crate that has a feature of a non-optional dependency. This basically makes edition 2024 unusable for me until the next beta bump, which is some time from now.

I know that backports are not typically warranted for fixes that affect unstable-only codepaths, since we expect users just to update their nightly compiler in that case, but I feel like the compiler might be an exception here.

[epage]

Is dogfooding the compiler really a big enough concern to move that we bypass our standard testing procedures to cut out a 1 month delay?

I'm also not in a good place for analyzing this for signing off on rushing this to beta. Edition 2024 has already accumulated months of a backlog I need to get through and I have more pressing personal matters to attend to.

[traviscross]

I'm not in a position to weigh how this affects the Cargo team and what the other priorities might be.¹ With that caveat, in terms of how this affects the edition team and our plans and timeline, I can say that one month does matter to us. We're only two months out from when we need to go/no go all items. So not losing a month of testing at this point is meaningful and would be helpful.

Footnotes

1. With respect to pressing personal matters, of course, Rust can wait. No pressure there. ↩

[joshtriplett]

I'm looking at #14295 and it seems like a simple revert, with the bulk of it being removing an if Edition::Edition2024 <= edition block and a testsuite change. It should have approximately zero effect when not using the 2024 edition.

Given that, and given that it's only a beta backport (not a stable backport), and that we can specifically retest this once backported, I personally (not speaking for the Cargo team) think it'd be safe to backport this to unblock edition 2024 testing.

[epage]

If other people have time to do the analysis and are ok with the backport, go for it. In putting time towards this, we need to keep in mind that, at least to me, the stable-to-stable regression is a much higher prioruty.

Before we do anything, is the compiler switch-over to Edition 2024 or just for testing purposes? If its going to be merged, does the fact that the fate of this feature is uncertain? We are in the midst of deciding whether to cut it, hack up fixes, or change this feature.

I also hope we are taking lessons learned from this as another area where what should not become an emergency becomes one.

[joshtriplett]

@epage

the stable-to-stable regression is a much higher prioruty

Which issue is the stable-to-stable regression?

[weihanglo]

@joshtriplett #14348.