fix(linter/plugins): alter method for obtaining mutable Program when sending AST to JS plugins - #26077
Merged
Conversation
Member
Author
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
|
Contributor
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
@camc314 This is probably worth an AI review, but it seems I've exhausted my Copilot quota. Could you please set Codex reviewer on it? |
Contributor
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Contributor
Merge activity
|
…n sending AST to JS plugins (#26077) ### What's needed for JS plugins When running JS plugins, we need to obtain a mutable copy of the AST (`&mut Program`) in order to alter it (update spans to UTF-16 offsets). But `Semantic` only contains an immutable `&Program` reference, and also its `AstNodes` contains immutable references to _all_ nodes in the AST. So we need to do 2 things: 1. Convert the `&Program` from `Semantic` to `&mut Program`. 2. Get rid of all the active references to AST nodes stored in `AstNodes` so that the `&mut Program` doesn't illegally alias with those references. ### Previous solution and its problems Previously we achieved this with a dodgy hack where we discarded all of `Semantic` (to get rid of the AST node references) and then "laundered" the pointer to the `Program`, upgrading it to write permission, by taking the provenance of the `Allocator`'s cursor pointer (which does have write permission). However, this relied on the `Program` being in the `Allocator`'s current chunk, so that the `Allocator`'s current cursor and the `Program` are within the same allocation (a requirement for the pointer returned from `NonNull::with_addr` being legal to dereference). This assumption was always a bit shaky, but when we enabled React Compiler rules by default in last release, it became clear it was completely untenable. React Compiler rules allocate a lot, and the debug assertions which checked that the `Program` was in current chunk started failing - so many allocations had occured that the `Allocator` had grown a fresh chunk to accomodate them. ### How this PR fixes the problem This PR alters the method by which we obtain the `&mut Program`. It still discards all the references to AST nodes held in `Semantic`, but now performs a bitwise copy of the original `Program` and allocates that copy back into the arena, yielding a `&mut Program`. This avoids the need for pointer "laudering" and removes the requirement for `Program` to be in the current `Allocator` chunk. As noted in the comments, this is still not sound. We have no way to prove that no other references to AST nodes exist, stashed somewhere, which would be an aliasing violation (UB). However, a thorough review of the code shows that we currently do not do this, so there is no actual UB at present. So, in short, this change does not achieve soundness, but it does at least swap definite proven UB for a _potential risk_ of UB which isn't currently triggered. Not great, but better. ### Side effects Previously we nuked the whole of `Semantic`, replacing it all with a new empty copy. Instead this PR operates more "surgically", just removing the AST node and `Comment` references that we _need_ to get rid of to avoid aliasing violations, and leaving the rest of `Semantic` alone. This is much cheaper.
graphite-app
Bot
force-pushed
the
om/08-24-fix_linter_plugins_alter_method_for_obtaining_mutable_program_when_sending_ast_to_js_plugins
branch
from
August 25, 2026 12:53
554c2bd to
8531b9b
Compare
graphite-app
Bot
deleted the
om/08-24-fix_linter_plugins_alter_method_for_obtaining_mutable_program_when_sending_ast_to_js_plugins
branch
August 25, 2026 12:59
graphite-app Bot
pushed a commit
that referenced
this pull request
Aug 25, 2026
…26079) `source_text` field was added to `ContextSubHost` in #25280 to work around the code that runs JS plugins (`run_external_rules`) nuking the whole of `Semantic`, replacing `semantic.source_text` with an empty string. #26077 has removed that behavior, so we can now return to using `semantic.source_text`, and it remains what it should be, even after `run_external_rules`.
graphite-app Bot
pushed a commit
that referenced
this pull request
Aug 25, 2026
…#26080) While working on #26077, Claude spotted another related bug. JS plugins have to run last because they need to mutate the AST, and in the process they destroy the stored AST. For files with multiple sections (Vue, Astro, Svelte), we ran native rules then JS rules for each section in turn. That appeared fine - JS rules do run last. But the problem is that some rules e.g. `vue/valid-define-emits` also access _earlier_ sections. Because JS plugins already ran on those previous sections, their ASTs have been discarded already, leading to incorrect results. Instead, lint in 3 separate passes which each run on _all_ sections before going on to the next pass: 1. Native rules. 2. JS rules. 3. Unused directives. i.e.: - Before: section 1 native, section 1 JS, section 2 native, section 2 JS. - After: section 1 native, section 2 native, section 1 JS, section 2 JS. Unused directives check has to be in a separate final pass, as it requires both native rules and JS rules to have run before it. Looping over sections is cheap compared to the work that happens in each turn of the loops, so I very much doubt this has a measurable impact on perf. This change does have one visible effect: The order of diagnostics will change for multi-section files. Previously diagnostics would be output in section order, now they're in rule type (native/JS) order. I don't _think_ that matters.
graphite-app Bot
pushed a commit
that referenced
this pull request
Aug 25, 2026
…vate (#26081) `Allocator::cursor_ptr` and `Allocator::data_end_ptr` were only exposed for `oxc_linter` to use for some dodgy pointer laundering. #26077 removed that code, so these methods no longer need to be exposed. Remove them from public API - it's not ideal to be exposing the internals of the allocator to user code. Both are still used within `oxc_allocator` crate, so they're not removed entirely, but become `pub(crate)`.
This was referenced Aug 31, 2026
graphite-app Bot
pushed a commit
that referenced
this pull request
Sep 1, 2026
### 💥 BREAKING CHANGES - a31567a allocator: [**BREAKING**] Make `Allocator::cursor_ptr` and `data_end_ptr` private (#26081) (overlookmotel) ### 🚀 Features - 784e9fa minifier: Invert `!0` and `!1` in place for boolean context to `1` and `0` (#26050) (Armano) - 1e902cc minifier: Expand fold leading assignments into the var decl (#26142) (Armano) - 3ed4f6a minifier: Expand de morgan's optimization to allow move of `!` (#25930) (Armano) - 5672585 parser: Attach all comments to nodes (#25944) (camc314) ### 🐛 Bug Fixes - 4adfb4c semantic: Validate chained continue labels (#26157) (camc314) - b874f48 ecmascript: `Math.round` only exact half ties (#26150) (camc314) - a625378 minifier: Coerce omitted `indexOf` search argument (#26149) (camc314) - dc7398b ecmascript: Trim trailing whitespace in string to number (#26148) (camc314) - 243b685 transformer/object-rest: Lower multiple declarators correctly (#26147) (camc314) - dc09a3a parser: Avoid panic on escaped string export names (#26146) (camc314) - e412cf2 linter: Clamp invalid JS plugin locations (#26144) (camc314) - c3dedc9 semantic: Skip function body bindings in parameters (#26099) (Dunqing) - e74de61 transform-react: Match Babel diagnostic reporting (#26128) (Boshen) - d5163d0 parser: Correctly classify unapplied pure annotations (#26084) (camc314) - 8531b9b linter/plugins: Alter method for obtaining mutable `Program` when sending AST to JS plugins (#26077) (overlookmotel) - 9b51658 regular_expression: Allow oversized decimal escape for Annex B (#26070) (leaysgur) - c8de4df regular_expression: Reject oversized backreferences (#26055) (camc314) ### ⚡ Performance - ac4785a diagnostics: Use fixed ANSI styles (#26130) (Boshen) - 07a0793 packages/codegen: Flatten output in chunks (#26109) (overlookmotel) - 487427a packages/codegen: Faster string flattening (#26108) (overlookmotel) - 7785583 packages/codegen: Ensure indent strings are flattened (#26107) (overlookmotel) - 0dd4db3 packages/codegen: Store mapping positions in an `Int32Array` (#26085) (overlookmotel)
graphite-app Bot
pushed a commit
that referenced
this pull request
Sep 1, 2026
# Oxlint ### 🚀 Features - 60b945d linter/nextjs/no-typos: Implement suggestion (#26091) (Mikhail Baev) ### 🐛 Bug Fixes - 33ac4b0 linter/lsp: Prevent tsgolint from holding onto processes (#25570) (Adrian Schaedle) - baf4b1e linter/import/no-empty-named-blocks: Make empty value import removal a suggestion (#26155) (camc314) - 77cfaec linter/eslint/object-shorthand: Preserve `__proto__` semantics (#26154) (camc314) - fa3c082 linter/unicorn/prefer-set-size: Ignore shadowed `Set` constructors (#26153) (camc314) - e412cf2 linter: Clamp invalid JS plugin locations (#26144) (camc314) - d86c113 linter: Normalize reversed JS plugin locations (#26138) (camc314) - 73c09b2 linter/eslint/no-use-before-define: Run on JS, JSX files (#26114) (camc314) - 03ef0f2 linter/unicorn/no-useless-spread: Treat typed arrays as a distinct value hint (#26067) (Aadharsh Pannirselvam) - bd15905 linter/react/no-unstable-nested-components: Check nested component object property names (#26101) (camc314) - 3910e2b linter/eslint/no-unassigned-vars: Skip Svelte and Vue files (#26042) (Hamody We) - 047f7ca linter/plugins: Fix interaction between JS plugins and Vue rules (#26080) (overlookmotel) - 8531b9b linter/plugins: Alter method for obtaining mutable `Program` when sending AST to JS plugins (#26077) (overlookmotel) - dc464ff linter/unicorn/prefer-math-min-max: Avoid unsafe autofix (#26060) (camc314) ### 📚 Documentation - 464ddd1 linter: Support a shared short description for jest/vitest rules (#26186) (connorshea) - 9db5ad3 linter: Add short description to `vue/no-dupe-keys` (#26183) (connorshea) - db66f58 linter: Correct export/import mismatch in bar and foo example (#25927) (billychannnnnn) - d5be037 linter/typescript/switch-exhaustiveness-check: Clarify default case comment pattern (#26100) (camc314) # Oxfmt ### 🚀 Features - 1fb37b6 formatter/css: Format declaration-shaped raw-prelude rules (postcss nested config blocks) (#26194) (leaysgur) ### 🐛 Bug Fixes - 57e8e22 formatter_css: Space a folded sign after a call in Less operations (#26134) (leaysgur) - e4298fb formatter/jsdoc: Follow CommonMark for interrupting lists and guard wrapping from creating them (#26098) (leaysgur) - a1e21c2 formatter: Apply head body policy everywhere (#26074) (leaysgur) - 676b7e1 formatter: Keep comments in their for-head slot, before an empty-statement body and do-while (#26073) (leaysgur) - 3bbcea6 formatter: Keep comments between a head and its body out of the braces across all constructs (#26071) (leaysgur) - 57537a2 formatter: Place comments between a head and open paren (#26058) (leaysgur) - e2d53e7 formatter: Place comments between a statement head and its body (#26043) (leaysgur) - 662556c formatter: Print the idempotent placement for comments in dropped parens (#26041) (leaysgur) - faf11d9 formatter_yaml: Keep trailing whitespace in block scalars (#26072) (leaysgur) ### ⚡ Performance - ac4785a diagnostics: Use fixed ANSI styles (#26130) (Boshen) ### 📚 Documentation - b879608 formatter,formatter_graphql,formatter_yaml: Annotate own-line comment inlining as known policy violation (#26131) (leaysgur) - 181953b oxfmt,formatter_core,formatter,formatter_yaml,formatter_css,formatter_graphql: Extract `DIVERGENCES.md` out from `AGENTS.md` (#26121) (leaysgur) - a67cb9d formatter,formatter_core: Document comment moving policy (#26075) (leaysgur) - fc175b0 formatter_core: Clarify idempotency test infra (#26069) (leaysgur)
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
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.
What's needed for JS plugins
When running JS plugins, we need to obtain a mutable copy of the AST (
&mut Program) in order to alter it (update spans to UTF-16 offsets). ButSemanticonly contains an immutable&Programreference, and also itsAstNodescontains immutable references to all nodes in the AST. So we need to do 2 things:&ProgramfromSemanticto&mut Program.AstNodesso that the&mut Programdoesn't illegally alias with those references.Previous solution and its problems
Previously we achieved this with a dodgy hack where we discarded all of
Semantic(to get rid of the AST node references) and then "laundered" the pointer to theProgram, upgrading it to write permission, by taking the provenance of theAllocator's cursor pointer (which does have write permission).However, this relied on the
Programbeing in theAllocator's current chunk, so that theAllocator's current cursor and theProgramare within the same allocation (a requirement for the pointer returned fromNonNull::with_addrbeing legal to dereference).This assumption was always a bit shaky, but when we enabled React Compiler rules by default in last release, it became clear it was completely untenable. React Compiler rules allocate a lot, and the debug assertions which checked that the
Programwas in current chunk started failing - so many allocations had occured that theAllocatorhad grown a fresh chunk to accomodate them.How this PR fixes the problem
This PR alters the method by which we obtain the
&mut Program. It still discards all the references to AST nodes held inSemantic, but now performs a bitwise copy of the originalProgramand allocates that copy back into the arena, yielding a&mut Program.This avoids the need for pointer "laudering" and removes the requirement for
Programto be in the currentAllocatorchunk.As noted in the comments, this is still not sound. We have no way to prove that no other references to AST nodes exist, stashed somewhere, which would be an aliasing violation (UB).
However, a thorough review of the code shows that we currently do not do this, so there is no actual UB at present.
So, in short, this change does not achieve soundness, but it does at least swap definite proven UB for a potential risk of UB which isn't currently triggered. Not great, but better.
Side effects
Previously we nuked the whole of
Semantic, replacing it all with a new empty copy. Instead this PR operates more "surgically", just removing the AST node andCommentreferences that we need to get rid of to avoid aliasing violations, and leaving the rest ofSemanticalone. This is much cheaper.