fix(allocator): remove Vec::bump method
#13039
Merged
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
Member
Author
This was referenced Aug 13, 2025
CodSpeed Instrumentation Performance ReportMerging #13039 will not alter performanceComparing Summary
Footnotes |
Dunqing
approved these changes
Aug 13, 2025
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
branch
from
6b33f35 to
68b7ce3
Compare
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
branch
from
68b7ce3 to
b76e7e6
Compare
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_vec_bump_method
branch
from
dda09b3 to
7eed95f
Compare
aapoalas
reviewed
Aug 14, 2025
overlookmotel
changed the base branch from
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
to
graphite-base/13039
overlookmotel
changed the base branch from
graphite-base/13039
to
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
overlookmotel
changed the base branch from
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
to
graphite-base/13039
overlookmotel
force-pushed
the
graphite-base/13039
branch
from
b76e7e6 to
72247e6
Compare
overlookmotel
force-pushed
the
08-12-fix_allocator_remove_vec_bump_method
branch
from
7eed95f to
4a35895
Compare
overlookmotel
changed the base branch from
graphite-base/13039
to
08-12-fix_allocator_remove_clone_impl_from_vec_
camc314
approved these changes
Aug 19, 2025
Contributor
Merge activity
|
`Vec::bump` method is unsound. * `Vec` is `Sync`, which means that you can have 2 `&Vec` references on different threads. * `Vec::bump` allows converting a `&Vec` into a `&Bump`, which means you can obtain 2 `&Bump`s on different threads. * `Bump` uses interior mutability, so you only need a `&Bump` (not `&mut Bump`) to allocate into that arena. * `Bump` is not thread-safe (hence it's not `Sync`) and allocating into the arena from 2 different threads simultaneously is UB. We don't use this method anyway, and I can't see any prospect of us doing so in future, so remove it. This will enable us to keep `Vec` being `Sync` without unsoundness.
graphite-app bot
pushed a commit
that referenced
this pull request
Aug 19, 2025
`Vec::clone` method is unsound, for the same reasons as `Vec::bump` is (#13039). `clone` only takes a `&self` and then uses it to access the `&Bump` contained in `Vec`, and allocates into that arena. Because `Vec` is `Sync`, that can result in 2 threads allocating into same `Bump` simultaneously, which is UB. `Clone` is of limited use, because usually usually the contents of the `Vec` aren't `Clone`. We have `CloneIn` trait for this purpose. We don't currently use `Clone` anywhere, so we can remove it.
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_clone_impl_from_vec_
branch
from
72247e6 to
9ac418d
Compare
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_vec_bump_method
branch
from
4a35895 to
af3b98e
Compare
graphite-app bot
pushed a commit
that referenced
this pull request
Aug 19, 2025
…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.
Base automatically changed from
08-12-fix_allocator_remove_clone_impl_from_vec_
to
main
August 19, 2025 21:42
graphite-app
bot
deleted the
08-12-fix_allocator_remove_vec_bump_method
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.
Vec::bumpmethod is unsound.VecisSync, which means that you can have 2&Vecreferences on different threads.Vec::bumpallows converting a&Vecinto a&Bump, which means you can obtain 2&Bumps on different threads.Bumpuses interior mutability, so you only need a&Bump(not&mut Bump) to allocate into that arena.Bumpis not thread-safe (hence it's notSync) and allocating into the arena from 2 different threads simultaneously is UB.We don't use this method anyway, and I can't see any prospect of us doing so in future, so remove it. This will enable us to keep
VecbeingSyncwithout unsoundness.