feat(data_structures): introduce NonNullConst and NonNullMut pointer types#21425
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
|
There was a problem hiding this comment.
Pull request overview
Introduces new non-null pointer wrapper types to encode read-only vs read-write permissions in the type system, aiming to reduce accidental UB compared to using std::ptr::NonNull<T> directly.
Changes:
- Adds
NonNullConst<T>andNonNullMut<T>wrappers (plus supporting APIs/traits and tests) under a newnon_nullfeature. - Wires the new module behind a crate feature flag and includes it in the crate’s
allfeature. - Updates crate README to mention the new pointer utilities.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/oxc_data_structures/src/non_null.rs | Adds NonNullConst/NonNullMut, supporting APIs (slice/array helpers, sealed IntoNonNull), and unit tests. |
| crates/oxc_data_structures/src/lib.rs | Exposes the non_null module behind the non_null feature. |
| crates/oxc_data_structures/README.md | Documents availability of the new non-null pointer types. |
| crates/oxc_data_structures/Cargo.toml | Adds the non_null feature and includes it in the all feature set. |
12e5a7b to
9bac6e5
Compare
Merge activity
|
…ter types (#21425) Most of our low-level data structures e.g. `Arena`, `Vec`, `Box`, and `Stack` types use `NonNull` pointers. `NonNull` is fundamentally dangerous. A `NonNull` can be created from a `&T`, and so only has read permissions. Yet you can call `as_mut()` on such a pointer to gain a `&mut T`, and that's instant undefined behavior. The onus is on the author to manually track how pointers were created and so whether they have write permissions or not, to avoid this hazard. In complex code like the arena allocator, pointers are created in various different places, stored in structs, and passed around, so it is not trivial to track their permissions, and neither the type system nor Clippy gives you any help. In comparison, raw pointers `*const T` and `*mut T` *do* make such a distinction between read-only and write-enabled pointers, but they have other shortcomings - they lack the non-null guarantee of `NonNull` (which can improve perf in some cases), and `*mut T` is not covariant, so it is unsuitable for use in types like `Vec`. This PR intoduces 2 new pointer types `NonNullConst` and `NonNullMut`. Both are just wrappers around `NonNull` and are equivalent to `NonNull` at runtime. What they add is type system-level guarantees of the read/write permissions that these pointers have. i.e. they bring together the const/mut distinction of raw `*const` / `*mut` pointers, and the advantages of `NonNull`. This PR only introduces these new pointer types, but does not use them in the codebase yet. It is my intention to migrate `Arena` allocator over to these new pointer types, to make it harder to blunder into causing UB while reworking the implementation. Other types like `Vec`, `Box`, `Stack`, and lexer's `Source` could also be migrated over later on.
9bac6e5 to
24b03de
Compare
### 💥 BREAKING CHANGES - 24fb7eb allocator: [**BREAKING**] Rename `Box` and `Vec` methods (#21395) (overlookmotel) ### 🚀 Features - ce5072d parser: Support `turbopack` magic comments (#20803) (Kane Wang) - f5deb55 napi/transform: Expose `optimizeConstEnums` and `optimizeEnums` options (#21388) (Dunqing) - 24b03de data_structures: Introduce `NonNullConst` and `NonNullMut` pointer types (#21425) (overlookmotel) ### 🐛 Bug Fixes - d7a359a ecmascript: Treat update expressions as unconditionally side-effectful (#21456) (Dunqing) - 56af2f4 transformer/async-to-generator: Correct scope of inferred named FE in async-to-generator (#21458) (Dunqing) - b3ed467 minifier: Avoid illegal `var;` when folding unused arguments copy loop (#21421) (fazba) - b0e8f13 minifier: Preserve `var` inside `catch` with same-named parameter (#21366) (Dunqing) - 4fb73a7 transformer/typescript: Preserve execution order for accessor with `useDefineForClassFields: false` (#21369) (Dunqing) ### ⚡ Performance - da3cc16 parser: Refactor out `LexerContext` (#21275) (Ulrich Stark) ### 📚 Documentation - c5b19bb allocator: Reformat comments in `Arena` (#21448) (overlookmotel) - 091e88e lexer: Update doc comment about perf benefit of reading through references (#21423) (overlookmotel) - 922cbee allocator: Remove references to "bump" from comments (#21397) (overlookmotel) Co-authored-by: Dunqing <29533304+Dunqing@users.noreply.github.com>
`just doc` run rustdoc with `--all-features` flag. I discovered that `just doc` was not, for example, catching errors in doc comments in #21425, because the code is behind a feature which is not enabled by default.

Most of our low-level data structures e.g.
Arena,Vec,Box, andStacktypes useNonNullpointers.NonNullis fundamentally dangerous. ANonNullcan be created from a&T, and so only has read permissions. Yet you can callas_mut()on such a pointer to gain a&mut T, and that's instant undefined behavior. The onus is on the author to manually track how pointers were created and so whether they have write permissions or not, to avoid this hazard.In complex code like the arena allocator, pointers are created in various different places, stored in structs, and passed around, so it is not trivial to track their permissions, and neither the type system nor Clippy gives you any help.
In comparison, raw pointers
*const Tand*mut Tdo make such a distinction between read-only and write-enabled pointers, but they have other shortcomings - they lack the non-null guarantee ofNonNull(which can improve perf in some cases), and*mut Tis not covariant, so it is unsuitable for use in types likeVec.This PR intoduces 2 new pointer types
NonNullConstandNonNullMut. Both are just wrappers aroundNonNulland are equivalent toNonNullat runtime. What they add is type system-level guarantees of the read/write permissions that these pointers have. i.e. they bring together the const/mut distinction of raw*const/*mutpointers, and the advantages ofNonNull.This PR only introduces these new pointer types, but does not use them in the codebase yet. It is my intention to migrate
Arenaallocator over to these new pointer types, to make it harder to blunder into causing UB while reworking the implementation. Other types likeVec,Box,Stack, and lexer'sSourcecould also be migrated over later on.