Building diesel fails using `resolver = "2"`

[weiznich]

Problem
Building diesel with resolver = "2" enabled fails, while builds using the old resolver succeed. It's unclear from the documentation if something in diesel needs to be changed or if this is a bug in the new resolver. See diesel-rs/diesel#2753 for the initial report.

I believe that this is an issue in cargo rather than in diesel for the following reason:

Building with resolver = "2" issues the following rustc invocation:

rustc --crate-name diesel ~/.cargo/registry/github.com-1ecc6299db9ec823/diesel-1.4.6/src/lib.rs 
--error-format=json 
--json=diagnostic-rendered-ansi,artifacts 
--crate-type lib 
--emit=dep-info,metadata,link 
-C embed-bitcode=no 
-C debuginfo=2 
-C metadata=8e83eb228db88a74 
-C extra-filename=-8e83eb228db88a74 
--out-dir /tmp/diesel-issue/target/debug/deps 
-L dependency=/tmp/diesel-issue/target/debug/deps 
--extern byteorder=/tmp/diesel-issue/target/debug/deps/libbyteorder-3ba0d986ba1f8d3d.rmeta 
--extern diesel_derives=/tmp/diesel-issue/target/debug/deps/libdiesel_derives-d5f5028a84dbe804.so 
--cap-lints allow 

which misses all specified and default features for diesel.

Building without resolver = "2" issues the following rustc invocation:

rustc --crate-name diesel ~/.cargo/registry/src/github.com-1ecc6299db9ec823/diesel-1.4.6/src/lib.rs 
--error-format=json 
--json=diagnostic-rendered-ansi,artifacts 
--crate-type lib 
--emit=dep-info,metadata,link 
-C embed-bitcode=no 
-C debuginfo=2 
--cfg 'feature="32-column-tables"' 
--cfg 'feature="bitflags"' 
--cfg 'feature="default"' 
--cfg 'feature="postgres"' 
--cfg 'feature="pq-sys"' 
--cfg 'feature="with-deprecated"' 
-C metadata=e4c51cf46b9b40ed 
-C extra-filename=-e4c51cf46b9b40ed 
--out-dir /tmp/diesel-issue/target/debug/deps 
-L dependency=/tmp/diesel-issue/target/debug/deps 
--extern bitflags=/tmp/diesel-issue/target/debug/deps/libbitflags-2a943fc4d3e795d7.rmeta 
--extern byteorder=/tmp/diesel-issue/target/debug/deps/libbyteorder-3ba0d986ba1f8d3d.rmeta 
--extern diesel_derives=/tmp/diesel-issue/target/debug/deps/libdiesel_derives-d5f5028a84dbe804.so 
--extern pq_sys=/tmp/diesel-issue/target/debug/deps/libpq_sys-2f8097271b957b37.rmeta 
--cap-lints allow 
-L native=/usr/lib/x86_64-linux-gnu

which contains the specified feature flags.

Steps

1. Clone this repository. (Or create a new project depending on diesel, enabling the "postgres" feature for diesel)
2. Change into the project directory
3. Issue cargo check

Notes

Output of cargo version:

cargo 1.51.0 (43b129a20 2021-03-16)

[toxeus]

I'm using diesel from the master branch and have experienced the same problem. After bumping to commit diesel-rs/diesel@bada4ad the issue got resolved.

[weiznich]

@toxeus Can you add some information which commit you used before the bump. The linked commit cannot be related to this, as it does not change any code that is related to this issue.

[toxeus]

@weiznich sure! It was diesel-rs/diesel@426b813. So there are about 300 commits to bisect through 😅

[weiznich]

Git bisect points to diesel-rs/diesel@35d68ba.

As another observation: This problem does only happen if diesel_migrations is part of the project. Now I should probably explain a bit the dependencies between the involved crates:

First a truncated dependency tree, to show the original dependencies for the 1.4.x release series:

diesel-issue v0.1.0 (/tmp/diesel-issue)
├── diesel v1.4.6
│   ├── diesel_derives v1.4.1 (proc-macro)
└── diesel_migrations v1.4.0
    ├── migrations_internals v1.4.1
    │   └── diesel v1.4.6 (*)
    └── migrations_macros v1.4.2 (proc-macro)
        ├── migrations_internals v1.4.1 (*)

As you can see diesel_migrations depends on two internal crates, migrations_internals and migrations_macros, where the first one is a normal rust crate, but the second one is a proc macro crate. migrations_internal internally depend on diesel and therefor on diesel_derives. migrations_macros uses the migrations_internal crate internally. Now does the diesel main crate itself pass down the backend feature flags to diesel_derives to enable backend specific code generation used in the main crate. migrations_internals depends on a plain diesel crate, without any feature enabled. Now as far as I understand the motivation behind resolver = "2", this thing should build separate dependency trees for build-dependencies (like proc-macros) and target dependencies. diesel is now both. Therefore it appears twice in the dependency graph built by resolver = "2", once with the "postgres" feature enabled, as part of the target dependency graph and once without the "postgres" feature enabled as part of the host/build-dependency graph. diesel_derives appears only once in both graphs, as part build-dependency graph so it takes the unified feature flags from both crates, which result in this compilation error for the host side version of diesel. Now this makes me wonder: Is that an issue in diesel? Is that an issue with the new solver?

[Eh2406]

As you guessed I don't understand the details of the dependencies of the involved crates, but if diesel needs some feature enabled for the host to build then a host crate needs to add that feature. Yes, this is different behavior then you get without resolver = "2". That is why it needed to be opt in.

[weiznich]

@Eh2406 That would all be fine if diesel actually had the option to opt into this feature (which we cannot simply do because we want to support a minimal rust version that is older than this feature). Our problem here is that any downstream crate can request to use the new resolver = "2" flag to opt into this. This requires us to implicitly support this behaviour without giving us any other option here. There is no way to opt out this behaviour, for crates that existed long before the creation of this flag. I believe a non existent resolver entry in a Cargo.toml should be treated like resolver = "1", which should be interpreted as opt out of the new resolver, regardless from what down stream crates request. In essence this would mean, down stream crates cannot use resolver = "2" as long as not all of their dependencies have opted into this feature. (And even that I feel that this feature is quite toxic, as it places the work to support this feature on the crate authors.)

If I understand #8088 correctly it is even worse by making this the default in the 2021 edition. This essentially breaks the promise bound to editions: You cannot use the edition of your choice anymore as a newer edition requires you to update code in a old edition.

That all written: I totally understand the motivation behind this feature, but the current implementation is just not up to the standards described in RFC-1122

Other than that: As the old behaviour work, but the new one doesn't: What's the canonical way to make code generation in a proc macro depended on some feature flag and use the crate containing the generated code, both on host and target side?

[ehuss]

I'm sorry you're having a frustrating experience with this. Unfortunately it is not possible for individual dependencies to use different resolver behavior, as the unification process is global across the crate graph.

For diesel, it looks like something like the following should allow it to work the same on both 1 and 2 resolvers:

diff --git a/diesel_migrations/Cargo.toml b/diesel_migrations/Cargo.toml
index 3c08a4c23..f026a258f 100644
--- a/diesel_migrations/Cargo.toml
+++ b/diesel_migrations/Cargo.toml
@@ -19,6 +19,6 @@ cfg-if = "0.1.0"

 [features]
 default = []
-sqlite = []
-postgres = []
-mysql = []
+sqlite = ["migrations_macros/sqlite"]
+postgres = ["migrations_macros/postgres"]
+mysql = ["migrations_macros/mysql"]
diff --git a/diesel_migrations/migrations_internals/Cargo.toml b/diesel_migrations/migrations_internals/Cargo.toml
index b85aeda0f..5342017a1 100644
--- a/diesel_migrations/migrations_internals/Cargo.toml
+++ b/diesel_migrations/migrations_internals/Cargo.toml
@@ -15,3 +15,6 @@ tempdir = "0.3.4"

 [features]
 default = []
+sqlite = ["diesel/sqlite"]
+postgres = ["diesel/postgres"]
+mysql = ["diesel/mysql"]
diff --git a/diesel_migrations/migrations_macros/Cargo.toml b/diesel_migrations/migrations_macros/Cargo.toml
index 8141bf665..9cb94aa8c 100644
--- a/diesel_migrations/migrations_macros/Cargo.toml
+++ b/diesel_migrations/migrations_macros/Cargo.toml
@@ -20,3 +20,6 @@ proc-macro = true

 [features]
 default = []
+sqlite = ["migrations_internals/sqlite"]
+postgres = ["migrations_internals/postgres"]
+mysql = ["migrations_internals/mysql"]

Essentially, diesel_migrations needs to forward the features to the proc-macro side of the graph. Users depending on diesel_migrations will also need to set the appropriate feature.

I realize this is difficult to diagnose and fix, and is more work than before. I'll think about what kind of tooling would be possible to help the process better. During this, I did recognize that there is a bug in cargo tree that is preventing it from displaying the correct information (the deduplication logic isn't working as intended).

[weiznich]

@ehuss Thanks for the suggestion how to fix this, it's nice to see that is possible to fix this issue on our side, but I believe that this should not have happened at all in that way.

Essentially, diesel_migrations needs to forward the features to the proc-macro side of the graph. Users depending on diesel_migrations will also need to set the appropriate feature.

Just to clarify that: Adding a new feature which needs to be correctly set to make a crate build does require a breaking change in the corresponding crate. This may not be possible/wanted due to multiple reasons. Essentially this is just moving the burden for this breaking change/major version bump from cargo to the ecosystem. (Thought I can understand this this likely affects only a minority of crates)

On top of that using the suggested fix will result in a degraded usability of our crate, as user now need to pass a matching set of feature flags to two different crates. (For diesel_migrations the real fix is to remove the built time dependency on diesel, as this essentially was not needed, but that will likely not be possible for other's with similar problems)

Unfortunately it is not possible for individual dependencies to use different resolver behavior, as the unification process is global across the crate graph.

That's exactly the point why I believe that this is essentially a breaking change. For some reasons (like usability, backward compatibility, …) it may not possible to support both the behaviour of the new and the old resolver in the same crate. All other features I'm aware of that where introduced via a new key in a Cargo.toml file where crate local, which means they did not affect any third party crates. This feature affects the whole dependency tree.

I realize this is difficult to diagnose and fix, and is more work than before. I'll think about what kind of tooling would be possible to help the process better. During this, I did recognize that there is a bug in cargo tree that is preventing it from displaying the correct information (the deduplication logic isn't working as intended).

Having better tooling here is definitively not wrong, but this does not solve the non-locality problem of this feature. I also totally understand why this feature was implemented and how it solves the targeted problem. I just want to raise my concerns that this feature, as it is currently implemented breaks, at least as far as I can tell, the rules specified by RFC-1122, by breaking unrelated third party code. If you don't think that's the case I would be curios to hear the reasoning here.

It would likely already help if cargo would emit bold warning if it hits an error in some dependency crate, that does not explicitly opt into this feature but that is built with resolver = "2" enabled. This warning should make it clear that the corresponding crate may not have been designed to work with the corresponding feature and that the user should try building it without resolver = "2" enabled (or cargo could even try that as a second step). Even that will create a huge pressure on popular crates to support this feature, so maybe it shouldn't be the default in the 2021 edition, but in the one after that to give the ecosystem a reasonable large amount of time to adjust to this change. Additionally there should likely be a way to explicitly opt out of this feature to force the old behaviour.

[Eh2406]

The time line for this feature is still being discussed, leaving it a separate option in 2021 is definitely being considered. How pane full it is for libraries to more accurately describe there features is definitely a concerned. So thank you for the feedback.

There are lots of users that have no way to build there projects with the existing behavior. Furthermore if a root crate wants to set resolver = "2" and use an older diesel, the root crate can add a dev-dependency on diesel with the needed features. So there are workarounds, admittedly unpleasant ones.

[weiznich]

Thanks for that response 👍

I believe something actionable here would be to improve the messages emitted by cargo/rustc on potential errors. At least for the diesel case it would already really help if cargo could point out that there is a difference between using the old and the new resolver. Maybe additionally a hint about setting a dev-dependency with the required feature flags could be edited? To prevent this from showing up as false positive this work could be done only if there was an error in some dependency crate.

[weiznich]

As this feature is now included in the official blog post about the 2021 edition I decided to open a thread in the internals forum about this issue.

I must say that I'm a bit confused as the statements made by @Eh2406 are quite contrary to that written in the blog post. I mean in the end this boils down to "let's break that use case in favor of another use case". I hoped that we had left that point of language design behind us with the release of rust 1.0, but it seems like this is not the case. I believe it's fine to break things, but that would imply that rust's stability guarantee is not valid anymore. I cannot see that this was decided by the rust core team via any RFC anywhere. Generally I'm disappointed on the communication around this feature.

To address a few other comments made here in that thread:

There are lots of users that have no way to build there projects with the existing behavior.

I cannot see how this is a justification for this change. I mean there are many things that are not possible due to language restrictions and at least for some of them fixing them would require a breaking change. There are quite a few examples in rust already where it is not possible to improve a specific feature due to language stability constraints. (See for example the discussion around unwinding through ffi boundaries or the design of std::sync::mpsc channels). Again in the end this boils down to: "We break existing users, to allow currently unsupported things". This may be fine as this is actually opt in for all involved parties, but that's not the case for this feature, especially not if it's made default in the next edition.

Furthermore if a root crate wants to set resolver = "2" and use an older diesel, the root crate can add a dev-dependency on diesel with the needed features. So there are workarounds, admittedly unpleasant ones.

I likely would react differently to this if cargo would emit a notice that adding diesel with specific features as dev-dependency would fix that problem, as this would make it clear that this is caused by setting resolver = "2" in a down stream crate. Emitting such a suggestion is a bit of work, but as far as I can tell has cargo access to all information required to emit such a message. As it currently stands I feel that the feature was released in rushed way, without providing an adequate level of diagnostics for eventual occurring issues

[toxeus]

I hoped that we had left that point of language design behind us with the release of rust 1.0, but it seems like this is not the case. I believe it's fine to break things, but that would imply that rust's stability guarantee is not valid anymore.

To be fair, it's not a change in the language but to the build-system. I'm unaware that the build-system is also covered by rust's stability guarantee.

I likely would react differently to this if cargo would emit a notice that adding diesel with specific features as dev-dependency would fix that problem

I agree that this is highly desirable but I also can understand that it's plausible that lack of such a diagnostic, for a complex setup as diesel has, can be just a simple oversight because build-systems are complex beasts. When downstream users encounter problems, they can file an issue which raises awareness on the quality of the diagnostics. Meanwhile, the user can opt-out of resolver = 2 until they receive help in the issue or a cargo version with better diagnostics is released.

[joshtriplett]

@toxeus Cargo is covered by Rust's stability guarantee. This is absolutely a breaking change, but that's why it's 1) opt-in, and 2) not being done by default until a new edition. That's how we normally handle changes that would otherwise be breaking.

We absolutely appreciate that because the new resolver applies to entire dependency trees, the migration story isn't as simple as that. That's part of what makes things like this even more difficult for Cargo than for Rust. Even changes to Cargo.toml and similar can typically be confined to the boundaries of one crate, but the resolver is one thing that can't; it inherently applies to the entire crate graph.