refactor(linter/plugins): store source text in end of buffer#18714
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. |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR refactors how source text is stored in buffers for Oxlint JS plugins, moving it from the start to the end of the buffer. This change simplifies the code significantly by removing unsafe workarounds and making the implementation more maintainable.
Changes:
- Refactored string deserializer to detect source text by checking if position is >= sourceStartPos (for linter) instead of < sourceEndPos
- Updated parse function to accept source_start parameter and position source text at end of buffer
- Removed RawTransferFileSystem and its complex unsafe file reading implementation, now using standard OsFileSystem
- Simplified source text allocation from unsafe
alloc_bytes_startto safealloc_str - Added ACTIVE_SIZE and CHUNK_FOOTER_SIZE constants for better buffer size calculations
- Removed complex allocator reset logic that was restoring data pointer to start
Reviewed changes
Copilot reviewed 11 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tasks/ast_tools/src/generators/raw_transfer.rs | Updated string deserializer to check pos >= sourceStartPos for linter; added CHUNK_FOOTER_SIZE constant and ACTIVE_SIZE calculation |
| napi/parser/src/generated/raw_transfer_constants.rs | Added CHUNK_FOOTER_SIZE constant |
| napi/parser/src-js/generated/constants.js | Added ACTIVE_SIZE constant for buffer size calculations |
| crates/oxc_linter/src/lint_runner.rs | Removed file_system parameter, now always uses OsFileSystem |
| crates/oxc_linter/src/lib.rs | Simplified source text allocation to use alloc_str instead of unsafe alloc_bytes_start |
| crates/oxc_allocator/src/pool/fixed_size.rs | Removed complex data pointer restoration logic from reset method |
| crates/oxc_allocator/src/generated/fixed_size_constants.rs | Added CHUNK_FOOTER_SIZE constant |
| crates/oxc_allocator/src/from_raw_parts.rs | Added compile-time check to verify CHUNK_FOOTER_SIZE matches expected value |
| apps/oxlint/src/lint.rs | Removed file_system parameter from lint_files call and removed associated configuration logic |
| apps/oxlint/src/js_plugins/raw_fs.rs | Deleted entire file - no longer needed with new approach |
| apps/oxlint/src/js_plugins/parse.rs | Updated to accept source_start parameter, position source text at end, and set allocator cursor accordingly |
| apps/oxlint/src/js_plugins/mod.rs | Removed RawTransferFileSystem export |
| apps/oxlint/src/generated/raw_transfer_constants.rs | Added CHUNK_FOOTER_SIZE constant |
| apps/oxlint/src-js/package/parse.ts | Calculate sourceStartPos as ACTIVE_SIZE - maxSourceByteLen and pass to parseRawSync |
| apps/oxlint/src-js/generated/deserialize.js | Removed sourceEndPos variable; updated string detection to use pos >= sourceStartPos |
| apps/oxlint/src-js/generated/constants.ts | Added ACTIVE_SIZE constant |
| apps/oxlint/src-js/bindings.d.ts | Updated parseRawSync signature to include sourceStart parameter |
| apps/oxlint/Cargo.toml | Removed unused simdutf8 dependency |
| Cargo.lock | Updated to reflect removed simdutf8 dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
71058c7 to
2c6c42b
Compare
|
I wrote this shameful hacky code originally, so I think I know it better than anyone. So merging without review. |
Merge activity
|
Previously we had to store source text at start of buffers sent to JS via raw transfer. #18376 made changes to how raw transfer deserializer handles strings, in order to support files containing a BOM. Building on that, we're now able to remove the requirement that source text be at start of the buffer entirely. This PR changes the deserializer used in Oxlint JS plugins to accept source text being anywhere in the buffer, *as long as no other strings are after it*. In practice this just means that the source text must be allocated before anything else, which is easy to satisfy. Now the source text can be allocated with just the usual safe `allocator.alloc_str(source_text)` method. This change removes a ton of dodgy workarounds and unsafe code we used previously to get source text at the start of buffer. It makes the code less labyrinthine and far less likely a slip up can inadvertently introduce UB. Note: In `napi/parser`, source text still *is* at start of the buffer, as that's simpler and more efficient when the source text is written into the buffer on JS side. This change only affects Oxlint.
2c6c42b to
e39a983
Compare
Remove several unsafe and potentially dangerous methods from `Allocator`. These methods were only required in order to support writing source text into start of allocator chunk, instead of bumping downwards in the usual way. #18714 has removed the need for source text to be at start of allocator chunks, so we can now remove these methods. This is a breaking change, but the docs stated that use of these methods was inadvisable, so hopefully no-one is depending on them. https://github.com/oxc-project/oxc/blob/b82faec35c486c0fcdb8a242a626554d2c99a310/crates/oxc_allocator/src/lib.rs#L24-L25

Previously we had to store source text at start of buffers sent to JS via raw transfer.
#18376 made changes to how raw transfer deserializer handles strings, in order to support files containing a BOM. Building on that, we're now able to remove the requirement that source text be at start of the buffer entirely.
This PR changes the deserializer used in Oxlint JS plugins to accept source text being anywhere in the buffer, as long as no other strings are after it. In practice this just means that the source text must be allocated before anything else, which is easy to satisfy.
Now the source text can be allocated with just the usual safe
allocator.alloc_str(source_text)method.This change removes a ton of dodgy workarounds and unsafe code we used previously to get source text at the start of buffer. It makes the code less labyrinthine and far less likely a slip up can inadvertently introduce UB.
Note: In
napi/parser, source text still is at start of the buffer, as that's simpler and more efficient when the source text is written into the buffer on JS side. This change only affects Oxlint.