fix(allocator): remove unsafe hacks from from_raw_parts methods#21283
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
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. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Merging this PR will not alter performance
Comparing Footnotes
|
6b49410 to
562c8d0
Compare
fc4541c to
7e805d4
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes the previous layout/offset “hacks” in Allocator::from_raw_parts by moving the raw-parts construction logic into the in-crate Bump implementation (now that bumpalo internals are locally available), reducing unsafe reach-into-private-struct behavior.
Changes:
- Refactors
Allocator::{from_raw_parts,set_cursor_ptr,data_end_ptr,end_ptr}to delegate toBumpAPIs instead of manually constructing/mutating chunk footers. - Adds
Bump::{from_raw_parts,set_cursor_ptr,data_end_ptr,end_ptr}behind thefrom_raw_partsfeature for raw-transfer use. - Centralizes raw-transfer sizing/alignment constants in
bump.rsand moves the fixed-sizeCHUNK_FOOTER_SIZEconsistency assertion there.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/oxc_allocator/src/from_raw_parts.rs | Removes allocator-side unsafe hacks and delegates raw-parts/cursor/end-pointer operations to Bump. |
| crates/oxc_allocator/src/bump.rs | Exposes chunk constants and implements from_raw_parts + cursor/end-pointer helpers under the from_raw_parts feature. |
562c8d0 to
c400e99
Compare
7e805d4 to
570545a
Compare
570545a to
2f76951
Compare
2f76951 to
86967f3
Compare
86967f3 to
b463e26
Compare
Merge activity
|
…1283) Until recently, our `Allocator` was based on `bumpalo`. `Allocator::from_raw_parts` (which is used by raw transfer) used some horrible hacks to determine the layout of `Bump`, and to reach in to `ChunkFooter` and mutate it, despite it not being exposed by `bumpalo`. Now that we've copied `bumpalo`'s code into `oxc_allocator` crate, we can remove these hacks and work with the actual `Bump` internals. This removes a bunch of very dodgy unsafe code. There wasn't a bug to fix, per se, but the hacks used by `Allocator::from_raw_parts` were so egregious that I think we can consider removing them a fix.
b463e26 to
bd8bd39
Compare
### 💥 BREAKING CHANGES - 36cdc31 str: [**BREAKING**] Remove identity `FromIn` impl for `Ident` (#21251) (overlookmotel) - 382958a span: [**BREAKING**] Remove re-exports of string types from `oxc_span` crate (#21246) (overlookmotel) - c4aedfa str: [**BREAKING**] Add `static_ident!` macro (#21245) (overlookmotel) ### 🚀 Features - e7e1aea transformer/typescript: Add `optimize_enums` option for regular enum inlining (#20539) (Dunqing) - 679f57f transformer/typescript: Implement const enum inlining and declaration removal (#20508) (Dunqing) - 6dd061c semantic: Extend `MemberWriteTarget` to cover all property modification patterns (#21205) (Dunqing) - f134e24 minifier: Support `property_write_side_effects` option to drop unused property assignments (#20773) (Dunqing) - 75663c0 semantic: Add enum member value evaluation for const enum support (#20602) (Dunqing) - 3cfe8ed semantic: Add `MemberWriteTarget` flag to `ReferenceFlags` (#20772) (Dunqing) ### 🐛 Bug Fixes - af1a586 transformer/class-properties: Use correct property name when converting parameter properties (#21268) (Amal Jossy) - b43250a allocator: Move allocation tracking into `Bump` (#21342) (overlookmotel) - 36f505f allocator: `StringBuilder` use `Allocator::alloc_layout` (#21340) (overlookmotel) - 7a08a6f allocator: Fix allocation counting in `Allocator::alloc_concat_strs_array` (#21336) (overlookmotel) - 2338e28 ecmascript: Treat `this` as potentially having side effects (#21297) (sapphi-red) - bd8bd39 allocator: Remove unsafe hacks from `from_raw_parts` methods (#21283) (overlookmotel) - 8f4c340 allocator: Remove dangerous pointer const to mut cast (#21279) (overlookmotel) - aa9259f parser: Add missing error code for optional param diagnostic (#21258) (camc314) - 04b3c2f str: Fix unsound casting const pointers to mut pointers (#21242) (overlookmotel) - ceadf6c str: Make `Ident::from_raw` an unsafe function (#21241) (overlookmotel) - eab13b3 transformer/decorators: Avoid accessor storage name collisions (#21106) (Dunqing) - 07e8a30 transformer/react-refresh: Handle parenthesized variable initializers (#21047) (camc314) ### ⚡ Performance - c3ca6f6 allocator: `StringBuilder::from_strs_array_in` check for 0 length earlier (#21338) (overlookmotel) - c2422bb allocator: `Allocator::alloc_concat_strs_array` check for 0 length earlier (#21337) (overlookmotel) - 04b0fdc allocator: Mark `Allocator::alloc_layout` as `#[inline(always)]` (#21335) (overlookmotel) - 17aee9e allocator: Use `offset_from_unsigned` in `ChunkFooter::as_raw_parts` (#21280) (overlookmotel) - 61adedd minifier: Fix O(n²) perf on very many var decls (#21062) (Gunnlaugur Thor Briem) - addcd02 napi/parser, linter/plugins: Raw transfer deserializer for `Vec`s use shift instead of multiply where possible (#21142) (overlookmotel) - 3068ded napi/parser, linter/plugins: Shift before add when calculating positions in raw transfer deserializer (#21141) (overlookmotel) - eb400b8 napi/parser, linter/plugins: Remove `uint32` buffer view (#21140) (overlookmotel) - 2675085 napi/parser: Lazy deserialization use only `Int32Array` (#21139) (overlookmotel) - 5b35a53 napi/parser: Deserializing tokens use only `int32` array (#21138) (overlookmotel) - f163d10 parser: Tokens raw deserialization use `Int32Array` (#21137) (overlookmotel) - 7a86613 linter/plugins: Use `Int32Array`s for tokens and comments buffers (#21136) (overlookmotel) - 8c51121 napi/parser, linter/plugins: Raw transfer deserialize `Span` fields as `i32`s (#21135) (overlookmotel) - bc1bcdd napi/parser, linter/plugins: Inline trivial raw transfer field deserializers into node object definitions (#21134) (overlookmotel) - c0278ab napi/parser, linter/plugins: Use `Int32Array` in raw transfer deserializer (#21132) (overlookmotel) - 43482c7 linter/plugins: Use `>>` not `>>>` in binary search loops (#21129) (overlookmotel) ### 📚 Documentation - f5e1845 allocator: Upgrade headers in doc comments for `Bump` (#21263) (overlookmotel) - 2870174 allocator: Upper case `SAFETY` in comments (#21253) (overlookmotel) - 01bc269 str: Reformat `Ident` doc comments (#21240) (overlookmotel) - dd47359 allocator: Add doc comments for panics and errors (#21230) (overlookmotel)

Until recently, our
Allocatorwas based onbumpalo.Allocator::from_raw_parts(which is used by raw transfer) used some horrible hacks to determine the layout ofBump, and to reach in toChunkFooterand mutate it, despite it not being exposed bybumpalo.Now that we've copied
bumpalo's code intooxc_allocatorcrate, we can remove these hacks and work with the actualBumpinternals. This removes a bunch of very dodgy unsafe code.There wasn't a bug to fix, per se, but the hacks used by
Allocator::from_raw_partswere so egregious that I think we can consider removing them a fix.