Concerns about making `resolver = "2"` the default in 2021 edition

[weiznich]

The announcement post of the 2021 edition includes a section about making resolver = "2" default as part of this edition. I want to use this post to raise my concerns about changing the default resolver as part of the next edition, I do not want to discuss the general need of this feature or a specific design nor want I question that this feature unblocks certain use cases.

First of all the blog post states the following:

Editions do not split the ecosystem

The most important rule for editions is that crates in one edition can interoperate seamlessly with crates compiled in other editions. This ensures that the decision to migrate to a newer edition is a "private one" that the crate can make without affecting others.

The requirement for crate interoperability implies some limits on the kinds of changes that we can make in an edition. In general, changes that occur in an edition tend to be "skin deep". All Rust code, regardless of edition, is ultimately compiled to the same internal representation within the compiler.

I cannot see how this statement is true for the resolver = "2", as using the new resolver is nothing that is contained to a specific crate. It applies to the whole dependency tree and therefore forces the behaviour down to all dependencies. In my opinion this violates that the decision of migrating to a newer edition as a "private one" that a crate can make without affection others.

Crates may have been written before the new resolver were even a thing and could relay on the behaviour of the old resolver. Especially for popular crates this will likely create a large pressure to support the new behaviour even if this would imply a breaking change or results in decreased usability due to users have to specify matching feature flags for different crates now. Essentially this places the cost of doing a breaking change to the resolver algorithm onto the ecosystem instead.

See this cargo issue for a specific issue encountered due to this.

To finish this with something that is actually actionable: I can see that resolver = "2" is a feature that is needed to unblock specific use cases. I don't think it should be the default in the next edition, for the reasons outlined above. On top of that I would like to see improvements about the documentation/messaging around this feature. Specifically I believe that if cargo encounters an error building a specific dependency with the new resolver it should emit a bold warning that this is likely caused by opting into the new resolver behaviour and is not a bug in the corresponding crate. It would be great to also get a suggestion from cargo how to fix the specific problem by either suggesting adding a specific dependency with corresponding feature flags based on a diff between the dependency tree build by the different resolver methods or by just suggesting to try the old resolver instead.

[bjorn3]

@weiznich

I cannot see how this statement is true for the resolver = "2" , as using the new resolver is nothing that is contained to a specific crate. It applies to the whole dependency tree and therefore forces the behaviour down to all dependencies. In my opinion this violates that the decision of migrating to a newer edition as a "private one" that a crate can make without affection others.

A crate doesn't force it down to all of its dependencies. It applies to a workspace. If one of your dependencies overrides the resolver, it doesn't affect you or the dependencies of said dependency when you don't opt into it. This is the same for profile settings or .cargo/config.toml.

[weiznich]

@bjorn3

A crate doesn't force it down to all of its dependencies. It applies to a workspace. If one of your dependencies overrides the resolver, it doesn't affect you or the dependencies of said dependency when you don't opt into it. This is the same for profile settings or .cargo/config.toml .

My statement was not written from the perspective of some user depending on some other crate, but rather the other way around. Let's use a specific example. I'm one of the diesel maintainers, which is likely a quite popular crate. Now let's assume that some user that depends on diesel as port of their dependency tree decided to use resolver = "2". For this specific example this will currently cause diesel not to compile (for reasons that are basically caused by the design of resolver = "2", but that's unrelated to the argument). Now is diesel neither part of their local workspace, nor can they simply change diesel in any way to make the code compile again (as it is a third party crate for them). Essentially this feature applies to the whole dependency tree, without given crates in that tree any option to do anything about it. My point here is that neither does diesel has a way to signal "resolver ="2"` is not supported/tested (as it was released before the new resolver was even a thing) nor is there any easy way for crates depended on diesel to fix that problem on their own (+ cargo does not even indicate that this problem may be caused by the crate depended on diesel).

Again: It's likely fine to have resolver = "2" as separate feature (with hopefully improved wording about potential issues), but I'm opposed to make it default in the 2021 edition for this reasons.

[matklad]

@weiznich

for reasons that are basically caused by the design of resolver = "2" , but that's unrelated to the argument). Now is diesel neither part of their local workspace, nor can they simply change diesel in any way to make the code compile again

I think it would be helpful to see a concrete example here. My current understanding is that new resolver activates fewer features by default. The leaf crate can forcibly enable more features by adding an explicit dependency to Cargo.toml. Ie, if diesel breaks because diesel-dep doesn't get the f feature with the new resolver, the end-user can add

[dependencies]
diesel-dep = {version="1", features=["f"]}

to their Cargo.toml, no?

[CAD97]

I see what you mean here, about it not being locally contained. A small example:

//! resolver = "1"
//! [package]
//! name = "a"
//! [features]
//! foo = []

#[cfg(foo)]
fn foo() {}

//! resolver = "1"
//! [package]
//! name = "b"
//! [dependencies]
//! a = "1"
//! [test-dependencies]
//! a = { version = "1", features = ["foo"] }

fn bar() { a::foo() }

//! resolver = "2"
//! [package]
//! name = "c"
//! [dependencies]
//! b = "1"

and whoops! b compiles fine on its own, but as a dependency of c, it doesn't compile. This is because b is (likely accidentally) relying on the resolver=1 unification of test features for its non-test functionality. When compiled under resolver=1, it compiles fine; when compiled under resolver=2, it fails to compile.

The resolver could be adjusted to be nontrasnsitive, in that features are unified for any packages using resolver=1, even if the root crate has resolver=2. However, I think this defeats the purpose of resolver=2; most crates work fine and have specified their dependency features correctly.

I disagree that fixing this puts a large burden on crate authors, though. It is a bug in their metadata, in that they said they could build with some set of dependencies, but couldn't. The fix for the impacted crates is just too add whichever feature flags were implied to their direct dependencies explicitly. Plus, the end user does have a way around it, in that they can declare a dependency on the dependency that's missing a feature flag and enable it. Or just switch to resolver=1.

Switching to resolver=2 for crates that specify edition=2021 is I think a fair move (it's morally equivalent to adding resolver=2 to cargo new'd manifests, which is clearly acceptable), but I think it needs a warning period where even on resolver=1, cargo/rustc will track the resolver=2 feature set and warn if you use any item that wouldn't be on without the "leaked" features. (Noting what items are missing is good enough, the developer can easily figure it out from there.)

Also, just to be clear to OP: the feature isolation in resolver=2 is just between runtime dependencies and nonruntime dependencies. If your runtime dependency graph transitively turns on some feature, it'll still be on. Plus, while it's technically breaking, it's generally considered not a semver major change to turn off features in your own dependencies, which is roughly equivalent to what resolver=2 is doing.

(TL;DR: relying on dependency features you didn't turn on in [dependencies] is not guaranteed to continue working by the community's definition of stability, unless a specific crate provides a stronger guarantee.)

All of this is assuming it does work as I've described here and OP alleges; I didn't check. I do believe it's true, though.

[weiznich]

Ok, let's show the complete real world problem from diesel here. The point is that this does not only happen with dev-dependencies, but also with normal build dependencies like that one used by proc macros. Hopefully that makes it much clearer why I believe that this feature should not be enabled by default in the 2021 edition.

First of all this does not happen with diesel itself, but requires additional crates from the diesel ecosystem. Let's assume the following minimal example. (The third party crate does not need to contain any code for this)

[package]
name = "my-fancy-crate-depending-on-diesel"
version = "0.1.0"
edition = "2018"
resolver = "2"

[dependencies]
diesel = { version = "1.4.6", features = ["postgres"] }
diesel_migrations = { version = "1.4.0" }

This results in the following dependency tree:

my-fancy-crate-depending-on-diesel v0.1.0 (/tmp/my-fancy-crate-depending-on-diesel)
├── diesel v1.4.6
│   ├── bitflags v1.2.1
│   ├── byteorder v1.4.3
│   ├── diesel_derives v1.4.1 (proc-macro)
│   │   ├── proc-macro2 v1.0.26
│   │   │   └── unicode-xid v0.2.2
│   │   ├── quote v1.0.9
│   │   │   └── proc-macro2 v1.0.26 (*)
│   │   └── syn v1.0.72
│   │       ├── proc-macro2 v1.0.26 (*)
│   │       ├── quote v1.0.9 (*)
│   │       └── unicode-xid v0.2.2
│   └── pq-sys v0.4.6
└── 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 (*)
        ├── proc-macro2 v1.0.26 (*)
        ├── quote v1.0.9 (*)
        └── syn v1.0.72 (*)

So what's the problem with this? Diesel itself depends on the diesel_derives crate. It reexports proc macros from this crate to simplify the setup for our users and relays internally on the same proc macros to implement specific features. As diesel provides support for different databases in a single crate, there are feature flags to enable support for a specific database system. Those also affect the diesel_derives crate, as some of the generated code is database specific. By itself this is totally fine with both resolver variants. Now there is the diesel_migrations crate which provides the functionality to apply database migrations to a existing database using diesels connections. In practice is diesel_migrations a facade crate, which combines a proc macro (embed_migration!, which embeds the migrations into the final binary) and some run time implementation behind the same crate name. The actual runtime implementation is wrapped into the migrations_internals crate, which depends on diesel as it needs to use some traits defined there (like the Connection trait to actual have something to apply the migrations to). Also this is fine on itself. The problem now arises from how the migrations_macros crate implements the embed_migration! macro. This implementation relays on code in migrations_internals and therefore on diesel. As a consequences this means diesel is build as dependency of a proc macro. The new resolver considers this to be a different target then the actual compilation target for good reasons, but the problem is that the new resolver considers the diesel_derives crate to be the same for both target. As consequence the derives crate will generate code that relays on specific features in the diesel crate enabled, while those features are not enabled for the proc macro target build diesel crate. (They are not even needed there, just some general definitions in diesel are required there...)

I'm aware of several ways to fix this, but each of them comes with in my opinion a quite large cost:

• We could change the diesel_migrations crate to accept a feature matching those passed to the diesel crate to pass down the corresponding feature flag. This change would require a breaking major version bump of the diesel_migrations crate as we cannot simply add new features that are required for specific configurations. Additionally users now would have to pass matching features to two crates instead of one, where the second crate (diesel_migrations) does not really need the features at all.
• We could request our users to put also a [dev-dependency] diesel = { version = "1.4.6", features = ["postgres"] }. I don't think this would be practical as this likely results in a large amount of bug reports about an apparently broken diesel version. This would be likely different if cargo itself would suggest that as fix.
• We could try to share less code between those crates, which makes it much harder to maintain (as we now have a large amount of code to maintain) + that does not fix any old version.

[Nemo157]

@weiznich

We could request our users to put also a [dev-dependency] diesel = { version = "1.4.6", features = ["postgres"] } . I don't think this would be practical as this likely results in a large amount of bug reports about an apparently broken diesel version. This would be likely different if cargo itself would suggest that as fix.

This should be a [build-dependencies], not a [dev-dependencies].

[Nemo157]

The issue in this specific case seems to be non-additive features in diesel_derives. I can provoke the same build failure with resolver = "1" using:

[package]
name = "my-fancy-crate-depending-on-diesel"
version = "0.1.0"
edition = "2018"

[dependencies]
diesel = { version = "1.4.6" }
diesel_derives = { version = "1.4.1", features = ["postgres"] }

This probably comes about as a consequence of there being no good way to do shared-features between a proc-macro and its runtime crate, but fundamentally the resolver = "2" build failure is correct with how features is defined today.

[weiznich]

That's correct and that's one of the reasons why diesel_derives is reexported from diesel. As a side note I'm not sure if "non-additive" is the right word here, as it's totally fine to pass all combinations of possible features to diesel_derives as long as they match the feature flags passed to the diesel main crate.

I believe this shows quite good that there are legitimate reasons to use the old resolver to work around issues around conditional code generation in proc macros. As already written I've no doubt that the new resolver has is own justification and unblocks new use cases. I'm just asking for a way to communicate to potential users that this feature does not play well with certain design choices and that they need to do certain steps to make things working. (At least as long as there is no better way to pass features to a proc macro crate.) As already written, I would likely be fine with this if cargo would output a help/note style message that the user should try to add the corresponding [build-dependencies] to match the resolver = "1" behaviour.

[bascule]

@weiznich

As already written I've no doubt that the new resolver has is own justification and unblocks new use cases.

As a data point here: the old feature unification behavior was a nightmare for Rust embedded developers, because if your embedded project shared any crate dependencies with your build-dependencies or dev-dependencies, and the latter activated the std feature of a shared crate dependency with your product, it would activate the std feature project-wide and your project code would no longer link due to a std dependency.

IMO that was effectively a bug in resolver = "1" which makes life very hard for anyone working on Rust projects where the host and target differ.

[mjbshaw]

@CAD97

I think it needs a warning period where even on resolver=1, cargo/rustc will track the resolver=2 feature set and warn if you use any item that wouldn't be on without the "leaked" features.

I like this idea, and I just wanted to highlight it to see what others think.

[CAD97]

So to be clear, the problem is actually that resolver=2 is still doing some unification, just inconsistently!

When depending on diesel, diesel_derives gets the same feature set, and needs to match the feature set of diesel (as they're logically one unit).

The issue, then, is depending on diesel as a build-dependency. Diesel's feature set is reset, but desel_derives' is still unified with the feature set enabled through runtime diesel.

To me, this is a bug in resolver=2. The point of resolver=2 is that the build dependency configuration and runtime dependency configuration are fully separated. This isn't a case of accidentally relying on leaked features, this is a case of runtime features (mixed) leaking into the build dependencies, rather than build dependency features leaking into runtime dependencies.

So there is actually a simple solution that will let diesel compile: go back to resolver=1 unification for build-dependencies, so long as this doesn't leak into runtime dependencies. We don't really care about over enabling features at build time.

A slightly more nuanced approach is to isolate build dependencies' features from build dependencies' build dependencies' features. This would fix the diesel issue by bringing down diesel_derive's feature level rather than bringing up diesel's. However, I don't know if this would then lead to fixpoint iteration building $(build dependencies)of+, so the simpler approach of just unifying more aggressively again for build dependencies seems better.

Plus, this will lead to fewer builds: the diesel example has to build diesel twice, once at the runtime feature level and once at a lower level for build dependencies. Upping the feature level at build time would remove a necessary crate compilation.

If I've misunderstood the situation, @weiznich, please correct me, but this seems like a resolvable issue that just wasn't well understood.

(I still think we should have a warning period for those who have the simple reliance on a build-to-runtime leaked feature.)

[josh]

I think completely partitioning build dependencies and not unifying features even among their dependencies makes sense. I don't think that would lead to excessive duplication, and crates reduce such duplication by enabling additional features on their build dependencies.

[Nemo157]

@weiznich

As a side note I'm not sure if "non-additive" is the right word here, as it's totally fine to pass all combinations of possible features to diesel_derives as long as they match the feature flags passed to the diesel main crate.

The postgres feature of diesel_derives is non-additive because it changes the behaviour of an existing API the crate exports (diesel_derives::SqlType). An alternative API change that would be additive is if enabling it simply enabled an additional option on that API, and then users of that API such as diesel could guard enabling that option on whether they have had that feature enabled (and when doing so arrange to guarantee that diesel/postgres was also active), e.g. reusing the existing attribute to configure the generated postgres code as a flag to also enable the postgres support could look like:

#[derive(SqlType)]
#[cfg_attr(feature = "postges", postgres(oid = "16", array_oid = "1000"))]
pub struct Bool;

---

@CAD97

A slightly more nuanced approach is to isolate build dependencies' features from build dependencies' build dependencies' features.

This is unnecessary with additive features. The issue from build-features leaking into runtime-features is that this is going across a target-boundary. Activating a feature may require additional dependencies, which may not be supported on the runtime-target. But build-dependencies and build-dependencies-of-build-dependencies are both being built for the host-target, if it would be possible to build the crate twice with the independent build-dependency-features and build-dependency-of-build-dependency-features¹, then it would be possible to build with these features combined.

¹which is actually a tree, not just another layer, since each build-dependency has a theoretically independent set of build-dependencies

[CAD97]

The problem inherent is that the feature set of diesel and diesel_macros are expected to match. This is typically considered fine in the ecosystem; diesel_macros is an implementation detail of diesel, and should not be used directly. While diesel_macros could be written in such a way that this isn't true, by just enabling support which is then asked for, this is in fact extra effort that wouldn't be necessary if they were one crate, as they're intended to be presented.

The additive nature of features is why I'm in favor of solving the build failure by more aggressively "leaking" runtime features into build time features, rather than separating them more. I suppose there's a potential case where you're targeting a triple with support for feature X but your build host doesn't have support, but that seems much less likely, and defaulting to "leaking" runtime to buildtime will reduce the number of compilations required when target triple == build triple.

To me it makes sense that:

• resolver=1, legacy, should warn if the code breaks under resolver=2;
• resolver=2, the eventual new default, should prevent build dependency specification from influencing runtime dependency features, but runtime dependency specification should "leak" to buildtime dependency features, to reduce crate duplication and avoid fixpoint iteration; and potentially
• resolver=3, the optional airgapped solution, should avoid runtime dependency specification from influencing buildtime dependency features, recursively, through fixpoint iteration for build dependencies of build dependencies of build dependencies ..., for those cases where this is requires due to a less capable build host or just desirable for testing a library is correct at specifying its minimal requirements.

And it should be considered a library bug if a library fails to compile under resolver=2, and a library specification bug on par with --minimal-versions if it fails to compile under resolver=3.

I'm not in charge so this is just a suggestion, but I think this is a reasonable design. The private macro implementation crate pattern (ab?)used by diesel described here would work fine on any of the three resolvers I've specified.

And because I like names more than numbers:

• resolver=1 => resolver=unify or resolver=legacy
• resolver=2 => resolver=default or resolver=fast
• resolver=3 => resolver=strict

(The ideas in this post are freely available released into the public domain, or, failing that, are available under the terms of the UNLICENSE, WTFPL, MIT, CC0, or any other license as desired.)