2024 implicit feature migration deletes dependency features

[ehuss]

Problem

Migrating to 2024 causes foo/feature_name features to get removed for optional dependencies. Those need to be preserved.

Steps

Example manifest:

cargo-features = ["edition2024"]
[package]
name = "foo"
version = "0.1.0"
edition = "2021"

[dependencies]
serde = { version = "1.0.203", optional = true }

[features]
serde = ["serde/derive"]

Running cargo fix --edition on this will result in:

serde = ["dep:serde"]

Note that the /derive feature got removed.

Possible Solution(s)

When there is a ["foo/feature_name"] syntax in the [features] table, and dep:foo isn't found anywhere in the [features] table, it should be rewritten to be ["dep:foo", "foo/feature_name"]. Some things to be careful of:

• Adjust all places that foo/xxx is found (such as x = ["foo/a"] and y = ["foo/b"], both x and y need a "dep:foo")
• Should not disturb any other features (like ["foo/feature_name", "bar"] would be rewritten to ["dep:foo", "foo/feature_name", "bar"])
• Should avoid duplicates (["foo/a", "foo/b"] should not have two dep:foo entries added).

Notes

No response

Version

cargo 1.80.0-nightly (7a6fad098 2024-05-31)
release: 1.80.0-nightly
commit-hash: 7a6fad0984d28c8330974636972aa296b67c4513
commit-date: 2024-05-31
host: aarch64-apple-darwin
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0 (sys:0.4.72+curl-8.6.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Mac OS 14.5.0 [64-bit]

[epage]

When there is a ["foo/feature_name"] syntax in the [features] table, and dep:foo isn't found anywhere in the [features] table, it should be rewritten to be ["dep:foo", "foo/feature_name"]

Why does it need to be re-written, rather than ignored? I thought "foo/feature_name" was "dep:foo", "foo?/feature_name".

[ehuss]

I think it is the same in spirit, but just serde/derive fails in the new edition with:

error: failed to parse manifest at `/Users/eric/Temp/foo/Cargo.toml`

Caused by:
  feature `x` includes `serde/derive`, but `serde` is not a dependency

To be honest, I don't understand that error "is not a dependency". I'm guessing since the implicit feature is missing, perhaps it just fails to look it up in the features hash map? I have not looked.

[epage]

The error is because the implementation strips unused optional dependencies.

I'll look into this to make sure we correctly detect what should be unused dependencies and update things accordingly.

[epage]

Huh, this is surprising

Baseline:

[package]
name = "cargo-14010"
version = "0.1.0"
edition = "2021"

[dependencies]
serde = { version = "1.0.203", optional = true }

[features]

cargo +1.64 metadata | jq '.packages[0].features'

{
  "serde": [
    "dep:serde"
  ]
}

w/

foo = ["dep:serde"]

{
  "foo": [
    "dep:serde"
  ]
}

w/

foo = ["serde/derive"]

{
  "foo": [
    "serde/derive"
  ],
  "serde": [
    "dep:serde"
  ]
}

w/

foo = ["dep:serde", "serde?/derive"]

{
  "foo": [
    "dep:serde",
    "serde?/derive"
  ]
}

So "dep_name/feature_name" is different from "dep:dep_name", "dep_name?/feature_name" in that the implicit feature doesn't get suppressed.

I'm guessing we had looked at the code when implementing the lint and stuff but I partially copied it but didn't think it through but went off of my incorrect intuition.

[epage]

This makes me worried about dep_name/feature_name syntax on Edition 2024. It now requires also adding dep:dep_name at which point it isn't much different than dep_name?/feature_name.

I had missing this part in past discussions (e.g. this zulip topic). It seems like we should have either changed it in Edition 2024 to not need dep:dep_name or transition out the syntax according to #10556. I worry that changing the meaning of dep_name/feature_name would be too subtle. Maybe we should at least go forward with the deprecation of dep_name/feature_name as mentioned in the comments of #10556?

[epage]

Also, another reason why deprecating implicit features doesn't play well with dep_name/feature_name is that

[features]
foo = ["dep:dep_name"]
bar = ["dep_name/feature_name"]

and

[features]
bar = ["dep:dep_name", "dep_name/feature_name"]

are valid while the following is not

[features]
bar = ["dep_name/feature_name"]

That inconsistency feels weird.

[linyihai]

So "dep_name/feature_name" is different from "dep:dep_name", "dep_name?/feature_name" in that the implicit feature doesn't get suppressed.
Yes, I suppose so.

And then the most important step is:
Then If the dep_name/dep_name_feature or dep_name?/dep_name_feature exists, then we should keep them when applying Cargo fix ?