allocator-api2 icon indicating copy to clipboard operation
allocator-api2 copied to clipboard

Recommended usage is impossible to satisfy

Open orlp opened this issue 1 year ago • 7 comments

The recommended usage section in the README states the following:

Your library MAY provide a feature that will enable "allocator-api2/nightly". When this feature is enabled, your library MUST enable unstable #![feature(allocator_api)] or it may not compile.

This is impossible because of feature unification. Consider there are two crates a and b that depend on allocator-api2. a does not enable the nightly feature, b does. Now a project that contains both a and b will fail to compile, as b turning on the feature flag will also turn it on for a, but a does not contain #![feature(allocator_api)].

This does actually happen in the real world in a large complicated project which ends up depending on hashbrown twice: https://github.com/rust-lang/hashbrown/issues/564. But as more and more crates start using allocator-api2 you don't even need such a complicated scenario.

orlp avatar Oct 03 '24 12:10 orlp

It is easy to satisfy really. Just provide nightly feature if you depend on allocator-api2.

Then in case of large project stumbled on that library being added as dependency without enabling "nightly", large project can add that library as dependency and enable "nightly".

I guess better phrasing for the README would be

Your library MUST provide a feature that will enable "allocator-api2/nightly"

zakarumych avatar Oct 03 '24 16:10 zakarumych

Your library MUST provide a feature that will enable "allocator-api2/nightly"

That's also not satisfactory because this would need to infect the entire dependency chain to work. If not the exact same example is problematic again, just with one extra dependency:

root
   ---> a ---> allocator-api2
   ---> b ---> c ---> allocator-api2

Here b does not know anything about allocator-api2 nor does it want to know, it's just a user of library c, so it does not expose the nightly feature from c. Now if the root crate turns on the nightly feature on a things are broken again.

orlp avatar Oct 03 '24 16:10 orlp

Yes, but can be fixed in root crate, by turning on nightly in c

zakarumych avatar Oct 03 '24 18:10 zakarumych

The root crate does not depend on c, and should not need to care about it or whatever feature flags it uses.

It's just not a reasonable system. Suppose allocator-api2 takes off in popularity and all data structure crates start using it. A root binary crate now might have to add dozens of nightly feature flag propagations for all kinds of dependencies of dependencies (or even further down the dependency chain) that it truly does not care about. And that is after encountering mysterious "enable feature(allocator_api)" errors that in no way point to the issue being a non-propagated nightly flag to a dependency-of-dependency.

orlp avatar Oct 03 '24 19:10 orlp

Yes, but only if they want to build on nightly and enable "nightly" features in at least some of their dependencies that reaches allocator-api2.

I would love to be able to enable lang features based on features enabled in dependency alone. Without special feature enabled for the crate. Unfortunately it doesn't work like that.

So enabling "nightly" in transitive dependencies root crate doesn't care about is the lesser evil. Other solutions I found are worse.

Please share if you know how to make it work without such friction.

zakarumych avatar Oct 03 '24 20:10 zakarumych

@zakarumych Since the README says "Your library MAY provide a feature [...]" instead of "Your library MUST provide a feature [...]", a project I'm a part of has run into an issue that cannot be solved in-tree without downgrading dependencies as described here: https://github.com/rust-lang/hashbrown/issues/564#issuecomment-2611100458

A fix for that specific crate is in the works, thankfully. But the bigger issue we encountered was that our project was building perfectly fine, then one day, when we ran cargo update it broke (in a manner fundamentally the same as the original issue linked). I wasn't able to figure out which crate caused it, exactly, but it appears that something used to enable the nightly feature for [email protected] that no longer did after the update, and since we use [email protected] in our project with the nightly feature, we stumbled into the issue.

Now, I don't think a project randomly and cryptically breaking upon cargo update (or truly just randomly if Cargo.lock isn't tracked by the VCS) is good UX. Also, even if the README were updated in the way you said in your first comment here that I've repeated, it'd still be easy to not provide such a feature by mistake.

So here is the solution: Remove the nightly feature. For an example of how this should be handled instead: hashbrown actually already has all the pieces in place to use allocator_api2 without the nightly feature of allocator_api2, but still providing the option to use the unstable allocator-api, and it's all contained in this small file (and much of it is dedicated to providing the option of neither!): https://github.com/rust-lang/hashbrown/blob/master/src/raw/alloc.rs

I believe all crates that wish to use allocator_api2 when building on stable but also be able to use the real allocator-api when building on nightly should handle it similarly. It's simple and easy to do so, and if a project uses allocator_api2 without doing that, the worst thing that would happen is that they would continue to use the replacement API provided by allocator_api2 even if they could enable the real allocator-api when building on nightly.

I wouldn't mind making a PR removing the nightly feature and updating the README (and moving the contents of the stable module into the crate root since there would no longer be a need for such indirection) if you find yourself short on time. A minor version bump (0.x) would probably be in order after merge, since this would be a breaking change. And the other open PRs will need rebasing.

NeuralModder avatar Jan 25 '25 00:01 NeuralModder

@zakarumych I made the pull request: https://github.com/zakarumych/allocator-api2/pull/30

NeuralModder avatar Jan 29 '25 01:01 NeuralModder

I believe this issue can now be closed since #30 is merged and in production?

NeuralModder avatar May 18 '25 19:05 NeuralModder

You're right

zakarumych avatar May 19 '25 09:05 zakarumych