fix(allocator): Add `allocator_api` feature to allow usage of `core::alloc::` #10676

TimDiekmann · 2025-04-28T15:15:50Z

Hi,

The crate allocator-api2 has a feature flag "nightly" to either use the #![feature(allocator_api)]-based types from core::alloc or use their own. If allocator-api2 is used by another crate at the same time which enables the allocator-api2/nightly feature, a compile error will happen due to feature flag unification of Cargo as oxc_-_allocator will then use the re-exported types from core::alloc through allocator_api2 which will result in the error that the allocator_api feature is not enabled.

There is a quite easy fix: Simply add #![feature(allocator_api)] to the top of oxc-allocator's lib.rs. However, as this requires a nightly compiler, the possibly easiest way to stay compatible is to add a nightly feature.

I don't know the exact procedure to contribute to this repository, but as it's effectively a 4-line change I thought I'd just open a PR.

Relevant issue:

Recommended usage is impossible to satisfy zakarumych/allocator-api2#19

graphite-app · 2025-04-28T15:15:54Z

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

0-merge - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

Boshen · 2025-04-28T15:34:41Z

Can you provide some context on how you are using this crate which requires nightly?

TimDiekmann · 2025-04-28T15:45:48Z

It's not specifically that this crate requires a nightly compiler, but as allocator_api2 does not follow Cargo guidelines about feature unification, it has to be handled by downstream crates. I'll try to summarize it in a quick way:

allocator_api2 effectively has this

// When the `nightly` feature is enabled, use `core::alloc` ...
#[cfg(feature = "nightly")]
pub use core::alloc::*

// ... otherwise use own definitions to allow usage on stable toolchain
#[cfg(not(feature = "nightly"))]
pub use self::alloc::*

As soon one crate is using allocator_api2 with the nightly feature enabled (e.g. hashbrown has the option), every other crate depending on allocator_api2 will have the same set of features enabled
This means, that oxc_allocator effectively will see core::alloc::Allocator instead of allocator_api2::Allocator
core::alloc::Allocator, however, requires the nightly feature allocator_api

Generally, this means that any crate depending on allocator_api2, directly or indirectly, needs to handle this case. To handle it it's as easy as simply enabling the feature flag depending on a feature flag.

TimDiekmann · 2025-04-28T16:08:10Z

And a bit more concrete: We utilize the allocator A in Box<T, A> and use bumpalo as an allocator. This means, we require the feature flag allocator_api on bumpalo. Similarly, we require the Allocator trait from the std-library in hashbrown. This implies, that hashbrown and bumpalo both use allocator-api2/nightly, which then implies that oxc_allocator fails to compile because that crate is not allowed to use core::alloc::Allocator.

Dunqing · 2025-04-29T00:47:06Z

I see the reason. We only have a few places that rely on allocator-api2, and those can be replaced later, but not quickly. Is this your blocker? If so, I think we can merge first. We may remove it if we get rid of allocator-api2 later.

TimDiekmann · 2025-04-29T05:35:23Z

I currently use my branch directly, so I would not see updates from your side. I would prefer to not use a fork, so if we could merge it and if not required anymore removing it would be great!

codspeed-hq · 2025-04-29T08:57:49Z

CodSpeed Instrumentation Performance Report

Merging #10676 will not alter performance

_{Comparing TimDiekmann:main (3c04062) with main (20a4a8c)}

Summary

✅ 38 untouched benchmarks

overlookmotel · 2025-04-29T10:59:30Z

We'll need to keep allocator_api2 for as long as we use hashbrown for HashMaps which store their data in an arena (which we do in Semantic). Getting rid of that would require replacing hashbrown with our own HashMap impl - which could have some advantages, but is obviously no small undertaking!

In short: I expect we'll be using allocator_api2 for a long time.

This PR is a good way to make that workable for downstream users who are on nightly. My only comments are:

Can we call the feature allocator_api instead of nightly? "nightly" is a bit broad.
We should document what this feature does, and in what circumstances you'd want to use it. Those docs would explain the problem it's designed to solve, much as @TimDiekmann has in this issue. Those docs should be in crates/oxc_allocator/lib.rs. I think it'd be better for those docs to err on side of being lengthy. The difference between allocator-api and allocator-api2 may be a bit confusing for those not familiar with this area.

overlookmotel · 2025-04-29T11:12:45Z

The CI failures are because we run cargo check, cargo test, and cargo clippy with --all-features. We'd need to change that.

Maybe add a feature tests to each crate which enables all features except ones which rely on nightly? (we'd do that in a separate PR which we'd merge before this one)

TimDiekmann · 2025-04-29T11:18:39Z

The CI failures are because we run cargo check, cargo test, and cargo clippy with --all-features. We'd need to change that.

Maybe add a feature tests to each crate which enables all features except ones which rely on nightly? (we'd do that in a separate PR which we'd merge before this one)

I was about to ask about what we do about the CI, it just took me a bit to figure out where cargo lint is defined 😄
Do you want me to open the PR or would you do that?

overlookmotel · 2025-04-29T11:31:09Z

Actually... (and sorry I didn't notice this earlier)... zakarumych/allocator-api2#30 has now been merged and it removes the nightly feature from allocator-api2. Before we go ahead with this, would it be worthwhile asking allocator-api2 maintainers if they can cut a release?

As far as I understand, that would also solve the problem, and would avoid these unpleasant workarounds with features in Oxc.

TimDiekmann · 2025-04-29T11:38:42Z

It was already asked if a new release could be done in zakarumych/allocator-api2#30 (comment). But even if the crate would be released, hashbrown and bumpalo also would need an update, which implies that they need to add logic to determine the nightly toolchain and conditionally enable the allocator_api. For the same reason, a [patch.crates-io] would not work because this also would apply to hashbrown and bumpalo and it would result in an error that the allocator-api2/nightly feature does not exist.
I think the feature flag would be the easiest way forward actually.

TimDiekmann · 2025-04-29T11:48:10Z

If allocator_api2, hashbrown, and bumpalo got an update, I'm happy to contribute to oxc to update the usage as well. The least I could say is that I have quite some experience with the (std) allocator API 😉

overlookmotel · 2025-04-29T12:12:15Z

I think the feature flag would be the easiest way forward actually.

OK. I see your point.

Thanks for making the changes I requested. Some of the docs for other features wasn't quite correct (our fault - they should have been documented already). I've added docs for them in #10695. Please could you rebase on top of it?

Concerning the CI failures: I'd like to consult with @Boshen on this, as it's not my area. My suggestion is:

Add a feature to each crate which enables all features except ones which rely on nightly? (we'd do that in a separate PR which we'd merge before this one)

Maybe the best name for that feature is all_stable. Then we'd run tests with cargo test --features all_stable instead of cargo test --all-features.

Unfortunately, Boshen is largely away from desk for next few days, so it may take a little time for him to come back on this.

TimDiekmann · 2025-04-29T13:00:11Z

Thanks, much appreciated!

Quick question on process: do you squash-merge PRs on your end, or would you prefer contributors to squash commits themselves before merging?

On the feature name — full does seem to be the established convention. Building on that, full-stable seems like a reasonable extension. Depending on how you envision expanding CI and how much emphasis you place on nightly toolchains, you might also consider adding a matrix entry for the nightly channel at some point. That said, I assume it’s not a high priority at the moment.

No rush on merging this — I’m happy to work off my own branch for now using [patch.crates-io] to simplify things. But mid-term, it would be ideal to get this upstreamed so we can more easily keep our oxc dependency up to date.

TimDiekmann · 2025-05-13T10:28:27Z

Any updates on the CI issue?

NeuralModder · 2025-05-18T19:23:48Z

@overlookmotel @TimDiekmann A few updates:

The PR for allocator-api2 (Remove nightly feature zakarumych/allocator-api2#30) has been merged and a new release has been made
Hashbrown didn't actually need the nightly feature for allocator-api2 so it was taken care of on their end a while back (Don't enable allocator-api2/nightly feature rust-lang/hashbrown#606), though they should probably still update the allocator-api2 version
For bumpalo, there's a PR open to update allocator-api2, in case that's needed

overlookmotel · 2025-05-18T23:35:57Z

Thanks very much @NeuralModder. We're in the process of replacing bumpalo with our own allocator - that should be done within a few days.

So, if I'm understanding right, if we bump allocator-api2 to latest, and we get rid of bumpalo, the need for a nightly feature goes away. @TimDiekmann Please can you confirm if I have this right?

TimDiekmann · 2025-05-19T08:21:43Z

Yes, as far as I can see, we would not need this anymore then. Instead, we'd need a small build-script to detect the nightly toolchain and conditionally enable the allocator-api feature. I'm happy to adjust this PR to account for this. I'd do something like I did for error-stack:
https://github.com/hashintel/hash/blob/9cdaaeca48e921a0e03b87c54c6afdc70b07db4d/libs/error-stack/build.rs#L6-L9

overlookmotel · 2025-05-19T08:54:51Z

Thanks for coming back.

We are using rustversion crate for this: https://github.com/oxc-project/oxc/blob/25c6266fdba423372523e403a369ab986b25cfce/crates/oxc_data_structures/src/pointer_ext.rs

oxc_allocator crate will have a dependency on oxc_data_structures crate with the feature that uses rustversion enabled. So could we use rustversion for the "is it nightly compiler?" check, rather than introducing another build script?

You clearly understand this better than me, so please say if that's not an ideal solution.

TimDiekmann · 2025-05-19T10:58:29Z

Sure, that works as well, that prevents an additional dependency as well 🙂

TimDiekmann · 2025-05-19T12:18:20Z

Actually, it seems that rustversion does not support enabling feature-flags such as #![rustversion::attr(nightly, feature(allocator_api))]

warning: crate-level attribute should be an inner attribute: add an exclamation mark: `#![foo]`
  --> crates/oxc_allocator/src/lib.rs:23:31
   |
23 | #![rustversion::attr(nightly, feature(allocator_api))]
   |                               ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_attributes)]` on by default

A workaround would be to at least use rustversion over rustc_version (to avoid additional dependencies), and add a super simple build.rs (that's the full file):

fn main() {
    println!("cargo:rustc-check-cfg=cfg(nightly)");
    if rustversion::cfg!(nightly) {
        println!("cargo:rustc-cfg=nightly");
    }
}

which would allow checking nightly in cfg_attr directly:

#![cfg_attr(nightly, feature(allocator_api))]

But let's wait for bumpalo to be removed (or updated).

I prepared the changes for the new allocator_api2 version (the CI will fail until bumpalo is changed/removed).

Cargo.toml

TimDiekmann · 2025-05-19T12:25:56Z

crates/oxc_allocator/build.rs

@@ -0,0 +1,6 @@
+fn main() {
+    println!("cargo:rustc-check-cfg=cfg(nightly)");


This is required to prevent a warning on #[cfg(nightly)]. This effectively adds nightly to the list of allowed cfg-attributes for the crate.

TimDiekmann · 2025-05-19T12:26:35Z

crates/oxc_allocator/src/lib.rs

@@ -20,6 +20,7 @@
 //!   Usage of this feature is not advisable, and it will be removed as soon as we're able to.

 #![warn(missing_docs)]
+#![cfg_attr(nightly, feature(allocator_api))]


Now, we simply use allocator_api when we're on nightly. This is the correct usage of allocator_api2 since version 0.3.0. This keeps the allocator-api isolated in this crates. Downstream crates do not need to care anymore (unless they use allocator_api2 as a direct dependency as well).

overlookmotel · 2025-05-23T09:59:24Z

Just to update you on where we're at...

We have a prototype of our new allocator which is going to replace bumpalo. However, we've hit a snag. We also use bumpalo::collections::String in various places, and it doesn't have a generic A: Allocator bound - it depends directly on bumpalo::Bump.

So before we can replace bumpalo::Bump with our own allocator, we need our own String type which is a wrapper around oxc_allocator::Vec instead of bumpalo::collections::Vec (#11246).

So, in short, we are progressing towards replacing bumpalo, but it's taking longer than expected.

TimDiekmann · 2025-06-11T18:22:07Z

@overlookmotel could you take another look at this PR? I updated the changes to use std::alloc over allocator_api2 on nightly with is now properly working on the nightly channel. The stable channel is unaffected and CI should just pass. I tested that locally with my code to make sure everything is fine.

This reverts commit 5adc2e3.

TimDiekmann · 2025-06-11T18:38:32Z

Actually, it seems that bumpalo still causing issues for us, so nevermind probably 😅

fix: Add nightly feature to enable allocator_api feature

c2b689b

TimDiekmann changed the title ~~fix: Add nightly feature to enable allocator_api feature~~ fix(allocator): Add nightly feature to enable allocator_api feature Apr 28, 2025

github-actions bot added the C-bug Category - Bug label Apr 28, 2025

fix: Add feature flags for hashbrown and bumpalo as well

e92db28

TimDiekmann added 2 commits April 29, 2025 13:42

chore: Add docs and rename feature flag to allocator_api

d10d806

fix: Update feature flag condition

0db8a62

overlookmotel requested review from overlookmotel and Boshen April 29, 2025 12:12

Merge branch 'main' into main

333f583

TimDiekmann changed the title ~~fix(allocator): Add nightly feature to enable allocator_api feature~~ fix(allocator): Add allocator_api feature to allow usage of core::alloc:: Apr 29, 2025

TimDiekmann mentioned this pull request Apr 29, 2025

H-4438: Implement initial codegen capabilities hashintel/hash#7027

Merged

3 tasks

Prepare for allocator_api@0.3.0

3c04062

< A3E2 strong> TimDiekmann commented May 19, 2025

View reviewed changes

TimDiekmann and others added 3 commits June 11, 2025 18:53

Merge branch 'main' into main

8eae6d8

Update Cargo.toml

f14ac4e

Use std::alloc on nightly

5adc2e3

Revert "Use std::alloc on nightly"

c68ad49

This reverts commit 5adc2e3.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(allocator): Add `allocator_api` feature to allow usage of `core::alloc::` #10676

fix(allocator): Add `allocator_api` feature to allow usage of `core::alloc::` #10676

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -0,0 +1,6 @@
		fn main() {
		println!("cargo:rustc-check-cfg=cfg(nightly)");

Uh oh!

fix(allocator): Add allocator_api feature to allow usage of core::alloc:: #10676

Are you sure you want to change the base?

fix(allocator): Add allocator_api feature to allow usage of core::alloc:: #10676

Conversation

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CodSpeed Instrumentation Performance Report

Merging #10676 will not alter performance

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix(allocator): Add `allocator_api` feature to allow usage of `core::alloc::` #10676

fix(allocator): Add `allocator_api` feature to allow usage of `core::alloc::` #10676