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.

[weiznich]

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.

Nevertheless the error message can and likely should be changed in such a way that it does not imply that a crate is completely ignored when it isn't. Maybe just drop the ignoring from the message if you want a simple fix?

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 https://github.com/rust-lang/cargo/issues/2267, then your docs could reference it and that would bypass the problem

That doesn't really address my problem. I think fine with having a separate tool that can be installed via cargo install. Again I have two issues with the current behavior:

• Minor issue: The error message is misleading, it should either ignore everything (including features) or not claim to ignore things. That's something that is a problem for me as rust developer with some experience, because cargo claims something here that is not done. To be clear here: I'm fine with just changing the error message.
• The more fundamental issue is the feature unification behavior. I do not think it is currently possible to model what diesel does with resolver = "2" without potentially running into problems there if you have different flags for build dependencies and real dependencies. That's a fundamental limitation and it's not solved by something like #2267. It's especially frustrating that this behavior changed at an edition boundary and as already pointed out back than: That was strictly speaking a change that is not allowed at the edition boundary as it breaks non-local crates.

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.

Do you want me to fill a new issue for that? Or what is the right way forward here. I mean I raised that issue already back than as the cargo team introduced resolver = "2" so it's not exactly new or unknown.

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.

Sorry, I missed that.

[epage]

Nevertheless the error message can and likely should be changed in such a way that it does not imply that a crate is completely ignored when it isn't. Maybe just drop the ignoring from the message if you want a simple fix?

I don't think dropping "ignoring" would improve the error message but make it worse.

That doesn't really address my problem. I think fine with having a separate tool that can be installed via cargo install. Again I have two issues with the current behavior:

Sorry if I wasn't clear enough. I wasn't trying to say that #2267 is exclusively needed to solve this problem. I scoped it specifically as a way to help users with knowledge transfer. There is still room to discuss feature resolution side to this.

Do you want me to fill a new issue for that? Or what is the right way forward here. I mean I raised that issue already back than as the cargo team introduced resolver = "2" so it's not exactly new or unknown.

It might depend on what was said at that time. In my mind, if you are operating within the bounds of how you are expected to use cargo and it isn't work, then that is legitimate for us track, even if we can't immediately fix it.

[weiznich]

I don't think dropping "ignoring" would improve the error message but make it worse.

Just to confirm this: You agree that this message is misleading?

It might depend on what was said at that time. In my mind, if you are operating within the bounds of how you are expected to use cargo and it isn't work, then that is legitimate for us track, even if we can't immediately fix it.

I do not feel that you care at all about that problem. Given that this is essentially a stable-stable regression: How do you expect me as a crate author to solve the following situation:

• There is a crate "a" and "a_derives". These are separate as proc macros are required to live in a separate crate.
• Crate "a" uses "a_derives" internally and offers a set of additive features (lets call them "x", "y", "z").
• Crate "a_derives" has the same feature set and crate "a" forwards the features to "a_derives".
• Crate "a_derives" generates code based on this features assuming that crate "a" contains the relevant helper code
• Now a user of crate "a" adds it as dependency and as build-dependency to their own crate "c". The for the normal dependency they enable feature "x" and for the build dependency feature "y".

Assuming that host and target match:

• Cargo with resolver="2" now builds crate "a_derive" once with the features "x" and "y". It will build crate "a" twice once with the features "a" and once with "b", which will both fail as both variants of crate "a" do not contain all the relevant code
• Cargo with resolver="1" will build "a_derive" and "a" once with both features "x" and "y" enabled.

Given this situation: How do I resolve that while supporting both resolver versions. If the answer is: You cannot, then resolver = "2" is a breaking change and should have been marked as such (== shipping rust 2.0). I fully understand that it is far to late to roll back that change but I would like to see the cargo team to at least acknowledge that they broke something and that they should prioritizing working on a fix (and maybe more than working on implementing new features!).

I still consider filling that a separate stable-to-stable regression issue, especially as I feel that this concern now and also back then when it was introduced is ignored by the cargo team.

[epage]

I do not feel that you care at all about that problem. Given that this is essentially a stable-stable regression: How do you expect me as a crate author to solve the following situation:

I do care. I only held back from saying "create an issue" and added nuance because you had said it was discussed before and I wasn't a part of those discussions and didn't fully have a grasp of your problem. I gave my reply with very specific wording to mitigate for what I didn't know. From what I understand of what you said, if we don't have an issue for that (which I don't recall any) we should. It would be great if you can find the past conversations and link to them so we have historical context. As you were a participant in those, I assume it would be easier for you to find it, leveraging the context you remember.

I do expect there being some nuance to the discussion. On the surface, its easy to say "well, if your proc macro is generating code for a feature, then you should also mention the feature in your regular dependencies" but feature unification means that it isn't necessarily the user enabling the feature but some other dependency may be doing it and for them to track the features of their other dependencies can be difficult.

I do think referring to this as a breaking change and a stable-to-stable regression might be a bit strong.

 Just to confirm this: You agree that this message is misleading?

For the majority of users, it is telling them exactly what they need to know without information that could confuse them or lead them in the wrong direction for fixing it. There is likely a small set of users who would get the wrong impression and go down the wrong path due to the simplified model the message is used.

[weiznich]

https://internals.rust-lang.org/t/concerns-about-making-resolver-2-the-default-in-2021-edition/14684 for one of the places where previous discussion on the feature unification took place.

On the surface, its easy to say "well, if your proc macro is generating code for a feature, then you should also mention the feature in your regular dependencies" but feature unification means that it isn't necessarily the user enabling the feature but some other dependency may be doing it and for them to track the features of their other dependencies can be difficult.

Does that mean the macro should generate code that feature gates everything that's generated? So adding #[cfg(feature = "x")] in the generated code? How would that be supposed to work in users code? That would require that any downstream user adds a feature x to their crate. I don't think that's practical. If you mean something else by that, could you elaborate that with an example?

I do think referring to this as a breaking change and a stable-to-stable regression might be a bit strong.

How would you call it then? It is something that worked before and is now broken. That's a regression. And I can point to two stable releases for working/not working, which makes it a stable-to-stable regression.

For the majority of users, it is telling them exactly what they need to know without information that could confuse them or lead them in the wrong direction for fixing it. There is likely a small set of users who would get the wrong impression and go down the wrong path due to the simplified model the message is used.

Well it's always easy to claim that something is "right" for a majority of users, but do you have numbers for that? Otherwise it's just a guess, right?

I'm specifically not arguing that the message should go away or be radically changed. I just suggest that it shouldn't use an absolute "is ignored" if it is in fact not completely ignored. What is about "is not build" or something along that line.

[epage]

How would you call it then? It is something that worked before and is now broken. That's a regression. And I can point to two stable releases for working/not working, which makes it a stable-to-stable regression.

A stable-to-stable regression is when the same code base does not work on both versions. This is different as you have to change the project's Edition or Resolver field. The point of Editions is you are opting in to new behavior. The gray area wrt Editions is that generally editions are scoped to a package but here diesel's edition doesn't make a difference in what is happening to it. However, that does not affect whether this is a breaking change / stable-to-stable regression. Even if the Edition didn't control Resolver version (which is the case for any virtual workspace) then its still an opt-in.

Does that mean the macro should generate code that feature gates everything that's generated? So adding #[cfg(feature = "x")] in the generated code? How would that be supposed to work in users code? That would require that any downstream user adds a feature x to their crate. I don't think that's practical. If you mean something else by that, could you elaborate that with an example?

That workaround doesn't require generating #[cfg(feature = "x")] code but that the user does

[dependencies]
foo = { version = "1", features = ["one", "two"] }
[build-dependencies]
foo = { version = "1", features = ["one", "two"] }

Sometimes I've wished for a way to say "is dep/feature enabled?" cfg. If we had that, then could generate code with #[cfg(feature = "diesel/x")] (of course with all of the other problems of proc-macros not having a $crate).

[epage]

I should have re-evaluated this before my reply. We've gotten very far off topic for this issue. Feel free to create an issue if you want for the v2 resolver behavior (note that I didn't see any "cargo team says X" in that thread and I only recognized two cargo team members on that thread and neither really said much definitely about your case)