fix(allocator): remove unsound Send impl and tighten Sync requirements for Vec
#13041
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Aug 13, 2025
Member
Author
This was referenced Aug 13, 2025
CodSpeed Instrumentation Performance ReportMerging #13041 will not alter performanceComparing Summary
|
Member
|
|
Dunqing
approved these changes
Aug 13, 2025
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_clone_impl_from_vec_
branch
from
82da014 to
32f9901
Compare
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_unsound_send_impl_and_tighten_sync_requirements_for_vec_
branch
from
1529968 to
49299c7
Compare
aapoalas
reviewed
Aug 14, 2025
overlookmotel
changed the base branch from
08-12-fix_allocator_remove_clone_impl_from_vec_
to
graphite-base/13041
overlookmotel
changed the base branch from
graphite-base/13041
to
08-12-fix_allocator_remove_clone_impl_from_vec_
overlookmotel
changed the base branch from
08-12-fix_allocator_remove_clone_impl_from_vec_
to
graphite-base/13041
overlookmotel
force-pushed
the
graphite-base/13041
branch
from
32f9901 to
4a35895
Compare
overlookmotel
force-pushed
the
08-12-fix_allocator_remove_unsound_send_impl_and_tighten_sync_requirements_for_vec_
branch
from
49299c7 to
bb6abbc
Compare
overlookmotel
changed the base branch from
graphite-base/13041
to
08-12-fix_allocator_remove_vec_bump_method
camc314
approved these changes
Aug 19, 2025
Contributor
Merge activity
|
…ments for `Vec` (#13041) It's unsound for `Vec` to be `Send` because if you can move a `Vec` to another thread, you can call `push` on it and it may allocate in the arena. This is unsound because another thread may be simultaneously be doing the same with another `Vec`, and `Bump` is not thread-safe. Remove the `Send` impl on `Vec`. However, we do want `Vec` to be `Sync`. There's no reason why you shouldn't have 2 `&Vec` references on different threads, as `&Vec` doesn't allow mutating the `Vec` in any way. The only hole we had previously which made this unsound was the `Vec::bump` method, which was removed in #13039. Just tighten up the trait bounds so that `Vec<T>` is `Sync` only if `T` is. This mirrors the standard library in relation to `T`. We're being more liberal than standard library in not requiring the allocator to be `Sync` too (`Bump` isn't), but that's OK because we don't have an equivalent of `std::vec::Vec`'s [`allocator` method](https://doc.rust-lang.org/std/vec/struct.Vec.html#method.allocator) (that was `Vec::bump`, and we've removed it). The reason these 2 impls were added originally was as a quick fix to make `oxc_semantic::Scoping` `Send` and `Sync`. That is addressed in a later PR in this stack.
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_vec_bump_method
branch
from
4a35895 to
af3b98e
Compare
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_unsound_send_impl_and_tighten_sync_requirements_for_vec_
branch
from
bb6abbc to
0815a89
Compare
graphite-app bot
pushed a commit
that referenced
this pull request
Aug 19, 2025
…ment for `HashMap` (#13203) Same as #13041. It's unsound for `oxc_allocator::HashMap` to be `Send` because if you can move a `HashMap` to another thread, you can call `insert` on it and it may allocate in the arena. This is unsound because another thread may be simultaneously be doing the same with another `HashMap`, and `Bump` is not thread-safe. Remove the `Send` impl on `HashMap`. However, we do want `HashMap` to be `Sync`. There's no reason why you shouldn't have 2 `&HashMap` references on different threads, as `&HashMap` doesn't allow mutating the `HashMap` in any way. Tighten up the trait bounds so `HashMap<K, V>` is `Sync` only if `K` and `V` both are. This is not yet completely sound. There are a couple of holes I'm aware of: 1. `allocator` method which allows obtaining a `&Bump` from a `&HashMap`. 2. `clone` which allocates into arena, taking only an immutable `&self` reference. This PR closes those holes as much as possible without a major refactor. It's not *completely* sound, but it's at least now quite difficult to commit UB - it'd be unlikely to happen by accident. So, not perfect, but at least there's now only a crack in the window, rather than the front door swinging wide open. --- The reason these 2 impls were added originally was as a quick fix to make `oxc_semantic::Scoping` `Send` and `Sync`. That is addressed in the next PR in this stack.
graphite-app bot
pushed a commit
that referenced
this pull request
Aug 19, 2025
…3042) Previous PRs removed unsound `Sync` impl for `Allocator` (#13033) and `Send` impl for `Vec` (#13041). Previously `ScopingCell` was `Send` and `Sync` (and therefore `Scoping` was too). The recent PRs mean that `ScopingCell` lost those traits too. This PR implements both traits again for `ScopingCell` by restricting its API slightly, so now it is `Send` and `Sync` again, but this time in a manner which maintains soundness. The comments in the code outline the logic of why I believe it to be sound. Rolldown relies on `ScopingCell` being `Send` and `Sync`. After this PR, this stack does not require any changes in Rolldown. I've checked that `cargo ck` in Rolldown passes when using the version of Oxc from this branch.
Base automatically changed from
08-12-fix_allocator_remove_vec_bump_method
to
main
August 19, 2025 21:44
graphite-app
bot
deleted the
08-12-fix_allocator_remove_unsound_send_impl_and_tighten_sync_requirements_for_vec_
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's unsound for
Vecto beSendbecause if you can move aVecto another thread, you can callpushon it and it may allocate in the arena. This is unsound because another thread may be simultaneously be doing the same with anotherVec, andBumpis not thread-safe.Remove the
Sendimpl onVec.However, we do want
Vecto beSync. There's no reason why you shouldn't have 2&Vecreferences on different threads, as&Vecdoesn't allow mutating theVecin any way. The only hole we had previously which made this unsound was theVec::bumpmethod, which was removed in #13039.Just tighten up the trait bounds so that
Vec<T>isSynconly ifTis. This mirrors the standard library in relation toT. We're being more liberal than standard library in not requiring the allocator to beSynctoo (Bumpisn't), but that's OK because we don't have an equivalent ofstd::vec::Vec'sallocatormethod (that wasVec::bump, and we've removed it).The reason these 2 impls were added originally was as a quick fix to make
oxc_semantic::ScopingSendandSync. That is addressed in a later PR in this stack.