Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad error message when a strong dep features's depedency is an unused optional dependency #14016

Open
epage opened this issue Jun 5, 2024 · 3 comments
Labels
A-editions Area: edition-specific issues A-features Area: features — conditional compilation C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@epage
Copy link
Contributor

epage commented Jun 5, 2024

Problem

If nothing can activate an optional dependency, cargo acts as if the dependency doesn't exist which creates poor error messages.

Also, with how things are arranged, the unused optional dependency lint doesn't get reported which could at least reduce the burden on the error message.

Steps

Baseline Cargo.toml:

cargo-features = ["edition2024"]
[package]
name = "cargo-14010"
version = "0.1.0"
edition = "2024"

[dependencies]
serde = { version = "1.0.203", optional = true }
$ cargo +nightly check -Zcargo-lints
warning: unused optional dependency
 --> Cargo.toml:8:1
  |
8 | serde = { version = "1.0.203", optional = true }
  | -----
  |
  = note: `cargo::unused_optional_dependency` is set to `warn` by default
  = help: remove the dependency or activate it in a feature with `dep:serde`
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.00s

Add the following to Cargo.toml:

[features]
serde = ["serde/derive"]
$ cargo +nightly check -Zcargo-lints
error: failed to parse manifest at `/home/epage/src/personal/dump/cargo-14010-1/Cargo.toml`

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

Possible Solution(s)

Notes

This was split out of #14015 assuming we'll go with a different solution than it

Version

No response

@epage epage added C-bug Category: bug A-features Area: features — conditional compilation A-editions Area: edition-specific issues S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Jun 5, 2024
@epage
Copy link
Contributor Author

epage commented Jun 5, 2024

#14010 (comment)

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
Copy link
Contributor Author

epage commented Jun 5, 2024

#14010 (comment)

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.

@epage
Copy link
Contributor Author

epage commented Jun 11, 2024

We discussed this in today's team meeting and lean towards "dep_name/feature_name" implying "dep:dep_name" in Edition 2024 with it being normalized in the published Cargo.toml file.

bors added a commit that referenced this issue Jun 17, 2024
fix(fix): Address problems with implicit -> explicit feature migration

### What does this PR try to resolve?

Within the scope of `cargo fix` there  are two problems
- We wipe out existing feature activations if it has the same name as an optional dependency
- The `Cargo.toml` isn't parseable because the unused optional dependency won't "exist" if just `dep_name/feature_name` is used

Fixes #14010

### How should we test and review this PR?

As for the unused optional dependency not "existing" error,
- #14015 is for improving the message for weak dep features
- #14016 is for re-evaluating how we handle this for strong dep features

Depending on what solution we go with for #14016, we might want to revisit the second migration within this PR.  This is one reason I made the commit separate (in addition to just making it clearer whats happening as this gets into some finer details of features).

### Additional information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-editions Area: edition-specific issues A-features Area: features — conditional compilation C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

1 participant