Cargo claims to ignore binary dependencies, but does unify features anyway

[weiznich]

Problem

Given the following Cargo.toml file:

[package]
name = "rocket_web_server"
version = "0.1.0"
edition = "2021"

[dependencies]
diesel = { version = "=2.2.2", features = ["postgres"] }

[build-dependencies]
diesel_cli = { version = "=2.2.1", features = ["postgres"] }

Cargo emits the following output:

$ cargo check
warning: rocket_web_server v0.1.0 (/tmp/test_diesel) ignoring invalid dependency `diesel_cli` which is missing a lib target
   Compiling proc-macro2 v1.0.86
   Compiling unicode-ident v1.0.12
   Compiling ident_case v1.0.1
   Compiling strsim v0.11.1
   Compiling fnv v1.0.7
   Compiling either v1.13.0
   Compiling heck v0.5.0
   Compiling pq-sys v0.6.1
    Checking byteorder v1.5.0
    Checking bitflags v2.6.0
    Checking itoa v1.0.11
   Compiling quote v1.0.36
   Compiling syn v2.0.74
   Compiling darling_core v0.20.10
   Compiling diesel_table_macro_syntax v0.2.0
   Compiling darling_macro v0.20.10
   Compiling darling v0.20.10
   Compiling dsl_auto_type v0.1.2
   Compiling diesel_derives v2.2.2
    Checking diesel v2.2.2
error[E0432]: unresolved import `diesel`
   --> /home/weiznich/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.2.2/src/expression/functions/date_and_time.rs:30:1
    |
30  | / define_sql_function! {
31  | |     /// Represents the SQL `DATE` function. The argument should be a Timestamp
32  | |     /// expression, and the return value will be an expression of type Date.
33  | |     ///
...   |
50  | |     fn date(expr: Timestamp) -> Date;
51  | | }
    | |_^ could not find `sqlite` in the crate root
// many more errors indicating that the sqlite feature was somewhat enabled by the `diesel_cli` build dependency

Relevant default features at the time of writing:
Diesel: no backends enabled
Diesel-CLI: postgres sqlite mysql

Steps

1. Create a new crate
2. Past the Cargo.toml content from above
3. Run cargo check

Possible Solution(s)

Cargo should ignore features coming from invalid dependencies or it shouldn't claim that it ignored these dependencies.

Notes

As additional note: There is also some problematic behavior how features are unified here. The proc macro crate diesel-derives is build once for both the backend dependency and the build-dependency. Given that this must be a separate crate and we must generate feature dependent code there and cargo chooses to compile the crate once for both sets of features there is no way for us as diesel authors to generate the correct set of code. As far as I remember that is expected behavior but it is incredibility frustrating to not being able to fix that correctly on our side. It's also something our users sometimes running into and getting confusing error messages. Funnily it's something that worked with the old resolver. I remember that I raised this concern already back then while choosing to go with the v2 resolver by default for the 2021 edition but that concern was dismissed back then.

Version

cargo version --verbose            
cargo 1.80.0 (376290515 2024-07-16)
release: 1.80.0
commit-hash: 37629051518c3df9ac2c1744589362a02ecafa99
commit-date: 2024-07-16
host: x86_64-unknown-linux-gnu
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Fedora 40.0.0 [64-bit]

(The same happens with a recent nightly)

[epage]

When it says the dependency is ignored, I believe it is referring to build impact. We resolve features before we know whether a package has a [lib] or not, so we cannot ignore it for feature unification.

Could you help me understand the impact of this? If its "invalid" to have that dependency in the first place, wouldn't the correction be to remove it? At that point, feature unification wouldn't matter.

[weiznich]

When it says the dependency is ignored, I believe it is referring to build impact. We resolve features before we know whether a package has a [lib] or not, so we cannot ignore it for feature unification.

If that's the case, then I find the message emitted highly misleading. It shouldn't state that this crate is ignored, but instead something like "skipping to build crate diesel-cli as it doesn't contain a [lib]section. Consider removing it from your[*dependencies]`"

Could you help me understand the impact of this? If its "invalid" to have that dependency in the first place, wouldn't the correction be to remove it? At that point, feature unification wouldn't matter.

Ok let me try that. As you observed and as mentioned by cargo it's not meaningful to have a [[bin]] crate in any of your dependency sections. Projects like diesel provide a custom CLI tool for certain tasks (in that case managing your database). It seems like people coming from other ecosystems (npm?) somewhat expect to put that tool also in their dependency list as that seems to be the way that it is handled in these ecosystems?. That results in broken builds for them, with confusing error messages from an upstream crate, which is turn is a really bad first users experience. I also expect that this does mostly happen for users that are really new to the rust ecosystem at all. That's for the this specific issue.

Now there is a deeper underlying issue in how the feature unification works here. The examples uses a [[bin]] crate, but you could easily use diesel as library dependency with different features flags there. Say to copy something from postgres to sqlite as part of a build script to populate your tests or something like that to give a somewhat realistic example. You would hit the same issue there as well. Given that there is no real good solution to that problem I might consider to advice in the diesel documentation to prefer using the 2018 edition for these cases, as the feature unification is really not helpful here. Although I'm not sure if the cargo team prefers that people go down that way, especially as it's advised by a somewhat large crate like diesel.

(Also I disagree with adding the features label here. The diagnostic is clearly a bug (it's not ignored as demonstrated) and the unification is arguably at least not helpful.)

[epage]

If that's the case, then I find the message emitted highly misleading. It shouldn't state that this crate is ignored, but instead something like "skipping to build crate diesel-cli as it doesn't contain a [lib]section. Consider removing it from your[*dependencies]`"

At this time, I can't enumerate all of the ways it is ignored and the distinction of how much its ignored doesn't seem relevant to the actual problem you are concerned about.

It seems like people coming from other ecosystems (npm?) somewhat expect to put that tool also in their dependency list as that seems to be the way that it is handled in these ecosystems?.

So the root problem is that end-users are trying to transfer knowledge from other ecosystems which is breaking with diesel due to how features are being resolved.

Is that right? Seems like our conversation then should focus on that and we should close this issue. For the knowledge-transfer part, if we had #2267, then your docs could reference it and that would bypass the problem

Now there is a deeper underlying issue in how the feature unification works here. The examples uses a [[bin]] crate, but you could easily use diesel as library dependency with different features flags there. Say to copy something from postgres to sqlite as part of a build script to populate your tests or something like that to give a somewhat realistic example. You would hit the same issue there as well. Given that there is no real good solution to that problem I might consider to advice in the diesel documentation to prefer using the 2018 edition for these cases, as the feature unification is really not helpful here. Although I'm not sure if the cargo team prefers that people go down that way, especially as it's advised by a somewhat large crate like diesel.

As this is off topic for this issue, I've not dug into this. Might be good to have a dedicated space to talk about it. Real quick though, you don't need to tell people to use an old edition but you can tell them to set resolver = "1". However, that can break other workflows, so if diesel is only expected to work with that then it is making itself incompatible with other parts of the ecosystem.

(Also I disagree with adding the features label here. The diagnostic is clearly a bug (it's not ignored as demonstrated) and the unification is arguably at least not helpful.)

I added the A-features label which is labeled as "Area: features - conditional compilation" as this is related to the features resolver and feature unification. I did not change C-bug to C-enhancement.