Can't generate feature gated code from a proc macro in a reliable way when used by both normal/build dependencies with resolver="2"

[weiznich]

Problem

Consider the following example:

• 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!).

Steps

For a real word example try to build the following crate:

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

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

[build-dependencies]
diesel = { version = "=2.2.1", features = ["sqlite"] }

This breaks diesel as we cannot encapsulate the proc-macros and the main crate as "one" unit.

Possible Solution(s)

• Do not unify features for proc macros between build-dependencies and normal dependencies
• Do unify not only the proc macro dependencies but also the normal crates between build dependencies and normal dependencies (i.e resolver="1" behavior.)

Notes

See the offtopic part of #14406 for more discussion on this topic

Version

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]

[epage]

Originally reported at https://internals.rust-lang.org/t/concerns-about-making-resolver-2-the-default-in-2021-edition/14684

[epage]

proc-macros are in an unfortunate situation where the main crate depends on them but they generate code for the main crate, requiring them to know what is in the main crate. Another problem that crops up due to this cyclic relationship is that the proc-macro can generate code that is "too new" for the main crate if work isn't done by the end-user or the maintainer to keep them in lockstep (e.g. using = version requirements).

[epage]

See also some past discussion at rust-lang/rust#88903

[Diggsey]

Would this be addressed by providing some way for proc macro crates to get information about the fully resolved crate graph at runtime?

That way the proc macro crate could generate code based on the features actually enabled for the consuming crate, and might not even need features itself (potentially improving compile times...) It would also allow the proc macro crate to error if the consuming crate has an incompatible version.

[epage]

Another idea I had brought up at one point was that we expose what features are enabled for dependencies, see #9094 (comment)

This would allow a proc-macro to generate code with cfg's like #[cfg(feature = "diesel/sqlite")] so that it adapted to the features of the production code.

This assumes that the user is directly depending on diesel much like proc-macros assume there is an extern for diesel. Solving $crate for proc-macros wouldn't help here and we'd need another solution. One might be that we extend the exposing of cfgs to not just direct dependencies but their public dependencies. If the generated code is expected to eventually reach code from the facade crate, then it will have to be through public dependencies.

This then gets into package name vs dependency name. If its package name, we can't predict it (true also for code without $crate). If its dependency name, then there can be ambiguity.

[epage]

We currently tell build scripts about what of your own features are enabled, see CARGO_FEATURE_<name>. One limitation is we don't provide a list of all of the features.

We could use this or a similar mechanism to tell proc macros and build scripts about dependency features like my idea for cfg. We likely don't want to report everything as then that leaks out dependency details. We also can't just cache cargo metadata as that is just the results of the dependency resolver and not the feature resolver adapting to the current build.

[0x53A]

I feel resolver=2 is a poor default, because it is often unexpected behavior.

It's probably gonna make me unpopular, but is there a chance to default back to resolver=1 and leave resolver=2 for use cases that need it, like the mentioned no_std builds?

Otherwise, another workaround I'd like to propose is to enable patching the feature list of dependencies. That's not going to help with diametrically opposed features like std vs no_std, but it would help in cases where the generated code is incompatible because a feature is missing.

Example:

there's a proc macro crate P, which depends on crate A

[dependencies]
A = { features = [] }

---

There's the main app which depends on both the proc macro crate P and also the crate A but with a different feature set, which causes incorrect macro expansion:

[dependencies]
P = {  }
A = { features = [ "additional_feature" ] }

# what I'd like to to is force unify A between main crate (this toml) and P
[patch.P.dependencies.A]
features = [ "additional_feature" ]

That's still a clunky hack, and really non-obvious, but would at least offer a path to solve this situation for some cases.

[epage]

Changing the default for resolver requires an Edition. The window for Edition 2024 is past and so we'd be talking about Edition 2027 (presumably).

I doubt we'd want to roll back to v1's unification behavior because it also affects target dependency unification and normal/dev dependency unification which I've not seen concern raised over, see https://doc.rust-lang.org/nightly/cargo/reference/resolver.html#feature-resolver-version-2

As for reverting host/target unification in a v4 resolver (edition 2024 includes a v3 resolver), we'd need to weigh these proc-macro use cases against the other use cases affected and the other potential solutions we'd have for proc-macros.

Looking at the history we've pieced together for this issue,

1. May 2021: Reported on Internals without much discussion
2. Sept 2021: Reported to the Rust project (Update to the 2021 edition via cargo fix breaks code rust#88903)
3. Sept 2021: Update to the 2021 edition via cargo fix breaks code rust#88903's author reported they fixed it on their side (Ensure compatibility with 2021 edition by working around cargo featur… diesel-rs/diesel#2902)
4. Sept 2021: Update to the 2021 edition via cargo fix breaks code rust#88903's author closed it in Change diesel compatibility messages #9927
5. Aug 2024: Can't generate feature gated code from a proc macro in a reliable way when used by both normal/build dependencies with resolver="2" #14415 (this issue) is opened by Update to the 2021 edition via cargo fix breaks code rust#88903's author
6. Oct 2024: [maybe a bug] common dependencies are not unified between main crate and proc-macro crate, if features differ #14740 is opened as a duplicate of this issue

In going 3 years before the problem is reported as affecting people, its hard to see this as a pressing concern that would outweigh the users who benefit from host/target unification. We are also still exploring other solutions that might benefit both proc-macro authors and people needing v2 resolver..

btw we will be exposing more knobs for the resolver with rust-lang/rfcs#3692 and there is a future possibility of giving more control over the parts of resolver v2. There are a lot more questions about doing that than there was for rust-lang/rfcs#3692 though.

[weiznich]

I would like again to firmly disagree that changing the default feature resolver is something that's allowed at an edition boundary. The cargo team shouldn't even consider that. See here for the extended reasoning

---

In going 3 years before the problem is reported as affecting people, its hard to see this as a pressing concern that would outweigh the users who benefit from host/target unification. We are also still exploring other solutions that might benefit both proc-macro authors and people needing v2 resolver..

I need to strongly disagree here that this is not a pressing concern. Given that this still is a behavior regression caused by the resolver 2 switch this alone should make this a high priority issue for the cargo team. I already pointed that out several times back then. I cannot force the team to take this kind of issues seriously, but just stating that it doesn't seem to be pressing misrepresents the facts.

btw we will be exposing more knobs for the resolver with rust-lang/rfcs#3692 and there is a future possibility of giving more control over the parts of resolver v2. There are a lot more questions about doing that than there was for rust-lang/rfcs#3692 though.

You explicitly stated in RFC-3692 that this won't have any feature to control host-target feature unification, so it cannot help for this issue. As pointed out there several times it only makes this problem worse, without providing a solution. So please do not bring it up as solution here.

---

As for the actual issue: At least for the diesel usecase any solution that would just expose the feature flags of the consuming crate (diesel in that case) to the proc-macro crate (diesel_derives in that case) would be sufficient. If that's via an explicit API or via side channels like environment variables doesn't really matter much for the consuming crate, as long as these information are tracked by the cargo and sufficient rebuilds are triggered in case these values change. I think that might be a better solution than tweaking feature unification as it completely sidesteps the problem. After all my main point of frustration here is not that the unification might change, but that I cannot do anything to migrate the issues that this can cause for my users.

[epage]

I would like again to firmly disagree that changing the default feature resolver is something that's allowed at an edition boundary. The cargo team shouldn't even consider that. See #14754 (comment)

Maybe put a different way, this isn't worth talking about for a couple of years, including whether we are allowed to make that change or not.

I need to strongly disagree here that this is not a pressing concern. Given that this still is a behavior regression caused by the resolver 2 switch this alone should make this a high priority issue for the cargo team. I already pointed that out several times back then. I cannot force the team to take this kind of issues seriously, but just stating that it doesn't seem to be pressing misrepresents the facts.

I understand you feel this is a high severity issue. You've explained the issue in the past. What is missing is helping us understand why you feel it is high severity. Being a regression is not enough. We have other regressions and we will continue to have them. There are also many "features" that are blocking people from what they are trying to accomplish and the delineation between feature and regression is just semantics to them.

In helping us understand why this is important, it would help to illustrate the impact and likelihood.

Impact is fairly well understood, the build fails. How severe that is is dependent on available mitigations. It would be good for us to explore in this issue what workarounds could exist and what their limitations are. For example, what is the negative impact of the user enabling extra features on normal dependencies?

As for likelihood, its hard to feel like hitting this is a likely occurrence based on the information we've collected so far. For example, can you help point to places where users have hit this? I've actually was wanting to look at what these user reports are like and tried to find them in diesel's repo but I only found the initial issue without any discussion since then of users running into this or backlinks from duplicate issues being closed.

btw we will be exposing more knobs for the resolver with rust-lang/rfcs#3692 and there is a future possibility of giving more control over the parts of resolver v2. There are a lot more questions about doing that than there was for rust-lang/rfcs#3692 though.

You explicitly stated in RFC-3692 that this won't have any feature to control host-target feature unification, so it cannot help for this issue. As pointed out there several times it only makes this problem worse, without providing a solution. So please do not bring it up as solution here.

I didn't say RFC 3692 would help with this issue. I was pointing out a future possibility from it.

[weiznich]

I understand you feel this is a high severity issue. You've explained the issue in the past. What is missing is helping us understand why you feel it is high severity. Being a regression is not enough. We have other regressions and we will continue to have them. There are also many "features" that are blocking people from what they are trying to accomplish and the delineation between feature and regression is just semantics to them.

I consider this as high severity because this:

• regresses behaviour for a popular crate
• has no known workaround
• certain other members of various rust team already agreed on that being something that should be addressed properly via warnings/suggested workarounds, etc. (Just check the old discussions for this, you will find these comments from other people there)

The later never happened for whatever reason, but now you as a team instead try to make that situation even worse.

In helping us understand why this is important, it would help to illustrate the impact and likelihood.

Honestly from my point of view this all does not really matter, as you as a cargo team decided to break that old behaviour. That's your bug and therefore your responsibility to fix that.

That written: I'm aware of at least two occurrences of this (or a really similar issue) in the last week:

• See the comment from @0x53A above
• This post on users.rust-lang.org

Given that frequency I wouldn't call that a very rare issue.

Impact is fairly well understood, the build fails. How severe that is is dependent on available mitigations. It would be good for us to explore in this issue what workarounds could exist and what their limitations are. For example, what is the negative impact of the user enabling extra features on normal dependencies?

As written already several times before: Enabling additional features might be not an option as that can pull in undesirable dependencies, like native libraries.
Other than that I'm not aware of any mitigation, which again makes this in my eyes an high severity issue.

As for likelihood, its hard to feel like hitting this is a likely occurrence based on the information we've collected so far. For example, can you help point to places where users have hit this? I've actually was wanting to look at what these user reports are like and tried to find them in diesel's repo but I only found the initial issue without any discussion since then of users running into this or backlinks from duplicate issues being closed.

See the examples given above. That are only reports from the last seven days that I'm aware of without a larger search. I would like at least to claim that you significantly underestimate the impact of that if you only concentrate on diesel here. Also there is always a significant number of unreported occurrences of such behaviour as people might dismiss it as strange interaction or whatever.