feat: experimental oxc transform#113
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
+ Coverage 93.26% 95.12% +1.86%
==========================================
Files 2 5 +3
Lines 193 308 +115
Branches 72 115 +43
==========================================
+ Hits 180 293 +113
- Misses 13 15 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Iterating over the first draft, thanks for the feedback 🙏🏻 |
Resolve the ESM build and transform entry conflicts, keep Oxc dependencies optional, isolate plugin transformer instances, and update the experimental parser integration.
📝 WalkthroughWalkthroughThe transformer is refactored from a single file into a shared contracts module ( ChangesOptional Oxc Parser Backend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 142-144: The README install command is adding the optional Oxc
packages as runtime dependencies, but they should be installed as dev
dependencies for the build-time transform. Update the install instruction that
mentions oxc-parser and oxc-walker to use the dev-dependency form, and keep the
guidance aligned with their build-only usage.
In `@src/plugin.ts`:
- Line 37: The transform filter built in createTransformerFilter() is inserting
raw asyncFunctions and objectDefinitions keys into a RegExp, so identifiers
containing regex metacharacters can break matching and skip valid files. Update
createTransformerFilter() to escape each derived name before composing the
transform regex, then keep the filter wiring in plugin.ts using that corrected
filter so filenames with symbols like $, ., or + are matched consistently.
In `@src/transform/_shared.ts`:
- Around line 30-33: The public transformer contract in the shared transform
type is too restrictive because it declares options with force?: false even
though the implementation in the backend transform paths already checks
options_.force to bypass shouldTransform. Update the shared type so transform
accepts force: true as part of the public API, keeping the contract aligned with
the behavior in the transformer entry points and allowing TS consumers to use
the forced path without casts.
- Around line 52-61: The regex in the transformer filter is using configured
names from asyncFunctions and objectDefinitions directly, which can break
matching or throw when names contain regex metacharacters. Update the logic in
the transformer helper that builds the new RegExp so each configured name is
escaped before joining, while keeping the existing asyncFunctions and
objectDefinitions behavior intact. Use the same code path that constructs the
code filter to ensure all configured names are treated as literals.
In `@src/transform/acorn.ts`:
- Around line 61-69: The async wrapper transform in acorn.ts is appending the
sentinel argument too early in the CallExpression handling, so wrappers can get
a new positional arg even when their callback was not rewritten. Update the
logic around transformFunctionArguments(node) and the appendRight(..., ",1")
path to only add the sentinel when that specific callback actually triggered the
__executeAsync rewrite (for both the main wrapper path and the code at the other
referenced block). Use the existing functionName / node / detected flow to gate
the append based on whether the current callback was transformed.
- Around line 118-166: The async function body handling in transformFunctionBody
is assuming every async ArrowFunctionExpression/FunctionExpression has a
BlockStatement, which breaks concise async arrow bodies like async () => await
foo(). Update the logic to detect expression-bodied async arrows before casting
body, and either wrap/convert them into a block or skip the block-only injection
path so the prepend/append edits only target real BlockStatement bodies. Use
transformFunctionBody, body, and the existing injectVariable/appendLeft flow to
ensure the __temp/__restore injection happens inside braces rather than inside
an expression.
In `@src/transform/oxc.ts`:
- Around line 118-166: The async function rewrite in transformFunctionBody
assumes function_.body is always a BlockStatement, but expression-bodied async
arrows like async () => await foo() have an expression body and get corrupted by
the injected variable declaration. Update transformFunctionBody to detect
non-block bodies before casting and rewriting, and either wrap expression-bodied
async arrows into a block or skip this rewrite path; use the function_.body,
injectVariable, and s.appendLeft(body.start + 1, ...) logic as the key places to
adjust.
- Around line 65-73: Gate the extra appended “,1” in
transformCallExpression/transformFunctionArguments logic so it is only added
when that callback actually had an await rewritten, instead of for every
matching asyncFunctions wrapper. Track whether the current CallExpression in
src/transform/oxc.ts was rewritten before calling s.appendRight on the last
argument, and apply the same fix to the corresponding callback handling around
the other referenced block so untouched wrappers keep their original call
signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca6199a6-08d7-4098-b12f-76c6bfd27a81
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
README.mdbuild.config.tspackage.jsonsrc/plugin.tssrc/transform.tssrc/transform/_shared.tssrc/transform/acorn.tssrc/transform/oxc.tstest/plugin.test.tstest/transform.test.ts
| ```sh | ||
| pnpm add oxc-parser oxc-walker | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Install the optional Oxc packages as dev dependencies.
These packages are only needed for the build-time transform, so pnpm add puts them in runtime dependencies unnecessarily.
Proposed fix
-pnpm add oxc-parser oxc-walker
+pnpm add -D oxc-parser oxc-walker📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```sh | |
| pnpm add oxc-parser oxc-walker | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 142 - 144, The README install command is adding the
optional Oxc packages as runtime dependencies, but they should be installed as
dev dependencies for the build-time transform. Update the install instruction
that mentions oxc-parser and oxc-walker to use the dev-dependency form, and keep
the guidance aligned with their build-only usage.
| filter: options.transformFilter ?? transformer.filter, | ||
| handler(code, id) { | ||
| const result = transformer.transform(code); | ||
| filter: options.transformFilter ?? createTransformerFilter(options), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Escape helper names before deriving the transform regex.
createTransformerFilter() currently interpolates asyncFunctions and objectDefinitions keys directly into new RegExp(...). Names like use$ctx are valid here, but $, ., +, etc. change the regex and make the plugin skip files that the AST transformer would otherwise handle.
Proposed fix
+function escapeRegex(value: string) {
+ return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
+}
+
export function createTransformerFilter(options: TransformerOptions): {
code: RegExp;
} {
const {
asyncFunctions = defaultTransformerOptions().asyncFunctions,
objectDefinitions = defaultTransformerOptions().objectDefinitions,
} = options;
return {
code: new RegExp(
- `\\b(${[...asyncFunctions!, ...Object.keys(objectDefinitions!)].join(
- "|",
- )})\\(`,
+ `\\b(${[...asyncFunctions!, ...Object.keys(objectDefinitions!)]
+ .map(escapeRegex)
+ .join("|")})\\(`,
),
};
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugin.ts` at line 37, The transform filter built in
createTransformerFilter() is inserting raw asyncFunctions and objectDefinitions
keys into a RegExp, so identifiers containing regex metacharacters can break
matching and skip valid files. Update createTransformerFilter() to escape each
derived name before composing the transform regex, then keep the filter wiring
in plugin.ts using that corrected filter so filenames with symbols like $, ., or
+ are matched consistently.
| transform: ( | ||
| code: string, | ||
| options?: { force?: false }, | ||
| ) => { code: string; magicString: MagicString } | undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow force: true in the public transformer contract.
The shared type currently declares force?: false, but both backends branch on options_.force to bypass shouldTransform. TS consumers cannot call that path without a cast, so the exported API is stricter than the implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/transform/_shared.ts` around lines 30 - 33, The public transformer
contract in the shared transform type is too restrictive because it declares
options with force?: false even though the implementation in the backend
transform paths already checks options_.force to bypass shouldTransform. Update
the shared type so transform accepts force: true as part of the public API,
keeping the contract aligned with the behavior in the transformer entry points
and allowing TS consumers to use the forced path without casts.
| const { | ||
| asyncFunctions = defaultTransformerOptions().asyncFunctions, | ||
| objectDefinitions = defaultTransformerOptions().objectDefinitions, | ||
| } = options; | ||
| return { | ||
| code: new RegExp( | ||
| `\\b(${[...asyncFunctions!, ...Object.keys(objectDefinitions!)].join( | ||
| "|", | ||
| )})\\(`, | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Escape configured names before building the filter regex.
asyncFunctions and objectDefinitions keys are inserted raw into new RegExp(...). That makes the filter misbehave for valid configured names like foo$, and malformed patterns like a) can make plugin setup throw before any transform runs.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 56-60: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
\\b(${[...asyncFunctions!, ...Object.keys(objectDefinitions!)].join( "|", )})\\(,
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/transform/_shared.ts` around lines 52 - 61, The regex in the transformer
filter is using configured names from asyncFunctions and objectDefinitions
directly, which can break matching or throw when names contain regex
metacharacters. Update the logic in the transformer helper that builds the new
RegExp so each configured name is escaped before joining, while keeping the
existing asyncFunctions and objectDefinitions behavior intact. Use the same code
path that constructs the code filter to ensure all configured names are treated
as literals.
Source: Linters/SAST tools
| if (node.type === "CallExpression") { | ||
| const functionName = _getFunctionName(node.callee); | ||
| if (options.asyncFunctions!.includes(functionName)) { | ||
| transformFunctionArguments(node); | ||
| if (functionName !== "callAsync") { | ||
| const lastArgument = node.arguments[node.arguments.length - 1]; | ||
| if (lastArgument && lastArgument.loc) { | ||
| s.appendRight(toIndex(lastArgument.loc.end), ",1"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only append the sentinel arg when that specific callback was rewritten.
appendRight(..., ",1") runs for every matched wrapper before you know whether its callback injected any __executeAsync rewrite. If another wrapper in the same file flips detected, callbacks with no await still ship with a new positional arg.
Also applies to: 169-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/transform/acorn.ts` around lines 61 - 69, The async wrapper transform in
acorn.ts is appending the sentinel argument too early in the CallExpression
handling, so wrappers can get a new positional arg even when their callback was
not rewritten. Update the logic around transformFunctionArguments(node) and the
appendRight(..., ",1") path to only add the sentinel when that specific callback
actually triggered the __executeAsync rewrite (for both the main wrapper path
and the code at the other referenced block). Use the existing functionName /
node / detected flow to gate the append based on whether the current callback
was transformed.
| function transformFunctionBody(function_: Node) { | ||
| if ( | ||
| function_.type !== "ArrowFunctionExpression" && | ||
| function_.type !== "FunctionExpression" | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| // No need to transform non-async function | ||
| if (!function_.async) { | ||
| return; | ||
| } | ||
|
|
||
| const body = function_.body as BlockStatement; | ||
|
|
||
| let injectVariable = false; | ||
| walk(body, { | ||
| enter( | ||
| node: MaybeHandledNode, | ||
| parent: MaybeHandledNode | undefined | null, | ||
| ) { | ||
| if (node.type === "AwaitExpression" && !node[kInjected]) { | ||
| detected = true; | ||
| injectVariable = true; | ||
| injectForNode(node, parent); | ||
| } else if ( | ||
| node.type === "IfStatement" && | ||
| node.consequent.type === "ExpressionStatement" && | ||
| node.consequent.expression.type === "AwaitExpression" | ||
| ) { | ||
| detected = true; | ||
| injectVariable = true; | ||
| (node.consequent.expression as MaybeHandledNode)[kInjected] = true; | ||
| injectForNode(node.consequent.expression, node); | ||
| } | ||
| // Skip transform for nested functions | ||
| if ( | ||
| node.type === "ArrowFunctionExpression" || | ||
| node.type === "FunctionExpression" || | ||
| node.type === "FunctionDeclaration" | ||
| ) { | ||
| return this.skip(); | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| if (injectVariable && body.loc) { | ||
| s.appendLeft(toIndex(body.loc.start) + 1, "let __temp, __restore;"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Concise async arrow bodies currently transform into invalid syntax.
transformFunctionBody treats every async function body as a BlockStatement, but async () => await foo() has an expression body. The later appendLeft(toIndex(body.loc.start) + 1, "let __temp, __restore;") then injects inside that expression instead of inside braces.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/transform/acorn.ts` around lines 118 - 166, The async function body
handling in transformFunctionBody is assuming every async
ArrowFunctionExpression/FunctionExpression has a BlockStatement, which breaks
concise async arrow bodies like async () => await foo(). Update the logic to
detect expression-bodied async arrows before casting body, and either
wrap/convert them into a block or skip the block-only injection path so the
prepend/append edits only target real BlockStatement bodies. Use
transformFunctionBody, body, and the existing injectVariable/appendLeft flow to
ensure the __temp/__restore injection happens inside braces rather than inside
an expression.
| if (node.type === "CallExpression") { | ||
| const functionName = _getFunctionName(node.callee); | ||
| if (options.asyncFunctions!.includes(functionName)) { | ||
| transformFunctionArguments(node); | ||
| if (functionName !== "callAsync") { | ||
| const lastArgument = node.arguments[node.arguments.length - 1]; | ||
| if (lastArgument) { | ||
| s.appendRight(lastArgument.end, ",1"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate the extra ,1 argument on a rewrite in this callback.
Like the Acorn backend, this appends ,1 for every matched wrapper before knowing whether that callback actually contained a rewritten await. In files with multiple wrappers, untouched callbacks can be emitted with a changed call signature.
Also applies to: 169-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/transform/oxc.ts` around lines 65 - 73, Gate the extra appended “,1” in
transformCallExpression/transformFunctionArguments logic so it is only added
when that callback actually had an await rewritten, instead of for every
matching asyncFunctions wrapper. Track whether the current CallExpression in
src/transform/oxc.ts was rewritten before calling s.appendRight on the last
argument, and apply the same fix to the corresponding callback handling around
the other referenced block so untouched wrappers keep their original call
signature.
| function transformFunctionBody(function_: Node) { | ||
| if ( | ||
| function_.type !== "ArrowFunctionExpression" && | ||
| function_.type !== "FunctionExpression" | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| // No need to transform non-async function | ||
| if (!function_.async) { | ||
| return; | ||
| } | ||
|
|
||
| const body = function_.body as BlockStatement; | ||
|
|
||
| let injectVariable = false; | ||
| walk(body, { | ||
| enter( | ||
| node: MaybeHandledNode, | ||
| parent: MaybeHandledNode | undefined | null, | ||
| ) { | ||
| if (node.type === "AwaitExpression" && !node[kInjected]) { | ||
| detected = true; | ||
| injectVariable = true; | ||
| injectForNode(node, parent); | ||
| } else if ( | ||
| node.type === "IfStatement" && | ||
| node.consequent.type === "ExpressionStatement" && | ||
| node.consequent.expression.type === "AwaitExpression" | ||
| ) { | ||
| detected = true; | ||
| injectVariable = true; | ||
| (node.consequent.expression as MaybeHandledNode)[kInjected] = true; | ||
| injectForNode(node.consequent.expression, node); | ||
| } | ||
| // Skip transform for nested functions | ||
| if ( | ||
| node.type === "ArrowFunctionExpression" || | ||
| node.type === "FunctionExpression" || | ||
| node.type === "FunctionDeclaration" | ||
| ) { | ||
| return this.skip(); | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| if (injectVariable) { | ||
| s.appendLeft(body.start + 1, "let __temp, __restore;"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Expression-bodied async arrows break this rewrite path too.
async () => await foo() reaches this branch, but function_.body is an expression, not a BlockStatement. Injecting let __temp, __restore; at body.start + 1 therefore corrupts the expression instead of inserting into a block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/transform/oxc.ts` around lines 118 - 166, The async function rewrite in
transformFunctionBody assumes function_.body is always a BlockStatement, but
expression-bodied async arrows like async () => await foo() have an expression
body and get corrupted by the injected variable declaration. Update
transformFunctionBody to detect non-block bodies before casting and rewriting,
and either wrap expression-bodied async arrows into a block or skip this rewrite
path; use the function_.body, injectVariable, and s.appendLeft(body.start + 1,
...) logic as the key places to adjust.
|
No huge perf difference at the moment as other operations take more effort. Have some ideas but this is on hold for now. |
2 participants
This PR adds a naive plugin and transform based on oxc-parser
Related issue: #111
Summary by CodeRabbit
New Features
Bug Fixes