Skip to content

Check syntax eagerly in vm.Script constructor #28667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 3 commits into from
+51 −1
Closed

Check syntax eagerly in vm.Script constructor #28667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c78386b
Check syntax eagerly in vm.Script constructor
robobun May 5, 2026
1a654fd
Update vm.test.ts assertion for eager syntax checking
robobun May 5, 2026
68b2343
Retrigger CI
robobun May 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/jsc/bindings/NodeVMScript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "ErrorCode.h"

#include "JavaScriptCore/Completion.h"
#include "JavaScriptCore/ParserError.h"
#include "JavaScriptCore/JIT.h"
#include "JavaScriptCore/JSWeakMap.h"
#include "JavaScriptCore/JSWeakMapInlines.h"
Expand Down Expand Up @@ -130,6 +131,17 @@ constructScript(JSGlobalObject* globalObject, CallFrame* callFrame, JSValue newT
SourceCode source = makeSource(sourceString, JSC::SourceOrigin(WTF::URL::fileURLWithFileSystemPath(options.filename), *fetcher), JSC::SourceTaintedOrigin::Untainted, options.filename, TextPosition(options.lineOffset, options.columnOffset));
RETURN_IF_EXCEPTION(scope, {});

{
JSC::ParserError error;
if (!JSC::checkSyntax(vm, source, error)) {
ASSERT(error.isValid());
auto exception = error.toErrorObject(globalObject, source);
RETURN_IF_EXCEPTION(scope, {});
JSC::throwException(globalObject, scope, exception);
return {};
}
Comment thread
robobun marked this conversation as resolved.
Comment on lines +134 to +142

@claude claude Bot Mar 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Every followed by execution now incurs two full JSC parses instead of one: in the constructor fully parses the source and discards the AST, then re-parses from scratch at run time. Pre-fix Bun did one parse (at run time); Node.js/V8 does one compile at construction time and reuses cached bytecode — this PR is strictly worse than both for the common case of valid scripts that are executed.

Extended reasoning...

What the bug is and how it manifests

The new call added to (NodeVMScript.cpp lines 134–142) performs a full source parse internally — it calls the JSC parser, builds a complete AST, validates it, and then discards everything. When the script is later executed via , , or , creates a new and re-parses the same source from scratch to produce bytecode. This means every object that is created and then run now incurs two full parses instead of one.

The specific code path that triggers it

  1. calls → JSC parser runs a full pass, builds an AST, returns true/false, discards the AST.
  2. Later, calls → JSC creates a new , re-parses the same source string via the same parser path, and compiles to bytecode.
    There is no mechanism to pass any intermediate representation between these two calls.

Why existing code doesn't prevent it / addressing the refutation

The refutation suggests "JSC may have internal caching that mitigates the double-parse cost." JSC does have a that caches bytecode keyed by source. However, does NOT produce bytecode — it produces only a parsed AST that is immediately discarded. The sourceProviderCache is populated only by successful compilation (via or similar). Therefore, the cache cannot be populated by , and the second parse in is a full cold parse. The double-parse is confirmed real.

The refutation is correct that this is intentional and mirrors the pattern in NodeVM.cpp. This is a known tradeoff accepted by the PR author to achieve eager correctness checking. It is not a hidden defect.

What the impact would be

For valid scripts (the common case — invalid scripts throw at construction and are never run), every construction + execution now costs ~2× the parsing work compared to pre-fix Bun. For large scripts or hot paths that create many instances (template engines, test runners, repeated patterns), this is a measurable regression. Node.js/V8 avoids this entirely by eagerly compiling in the constructor (producing cacheable bytecode), so V8 does one compile and zero re-parses on execution. Pre-fix Bun did one parse (deferred to run time). This PR makes Bun do two parses for every valid script that gets executed.

How to fix it and step-by-step proof

The correct fix is to replace + discard with an eager compile that stores the resulting or its bytecode for reuse by subsequent calls — matching Node.js/V8 behavior. This is a larger change outside this PR's scope and is noted as a follow-up in the synthesis description. A concrete step-by-step showing the regression: (1) is called; (2) parses , builds AST, returns , AST is freed; (3) the object is created storing only the string; (4) is called; (5) creates a fresh , parses again from the source string, compiles to bytecode, runs it. Two parses for one script execution — where pre-fix Bun did one and Node.js does zero (after construction-time compile).

}

const bool produceCachedData = options.produceCachedData;
auto filename = options.filename;

Expand Down
2 changes: 1 addition & 1 deletion test/js/node/vm/vm.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ test("can't use export syntax in vm.Script", () => {
expect(() => {
const script = new Script("export default {};");
script.createCachedData();
}).toThrow({ message: "createCachedData failed" });
}).toThrow({ name: "SyntaxError", message: "Unexpected keyword 'export'" });
});

test("rejects invalid bytecode", () => {
Expand Down
38 changes: 38 additions & 0 deletions test/regression/issue/28666.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { expect, test } from "bun:test";
import { Script } from "node:vm";
Comment on lines +1 to +2

@claude claude Bot May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Per root CLAUDE.md (line 66) and test/CLAUDE.md (line 153), test/regression/issue/${issueNumber}.test.ts is reserved for true regressions — behavior that worked in a previous release and then broke. Issue #28666 is a Node.js-compat gap (vm.Script never eagerly checked syntax in any prior Bun release), so these five tests belong in test/js/node/vm/vm.test.ts, which this PR already modifies.

Extended reasoning...

What the issue is

Both governing CLAUDE.md files for this directory tree state the same rule:

  • CLAUDE.md line 66: "test/regression/issue/${issueNumber}.test.ts is reserved for bugs that have a GitHub issue number and are true regressions (worked in a previous release, then broke). An issue number alone is not enough — if the behavior was never correct, it's not a regression and the test belongs in the existing file for that module."
  • test/CLAUDE.md line 153: "An issue number alone does not qualify — if it was never correct, put the test in the module's existing test file instead."

Issue #28666 is not a regression. The PR description itself states "The source was stored but only parsed at execution time" and "Bun silently created the Script object" — i.e., the eager syntax check never existed in any prior Bun release. The pre-PR assertion in vm.test.ts (createCachedData() throwing "createCachedData failed" rather than the constructor throwing) confirms the constructor never threw before. This is a never-implemented Node.js-compat behavior, not something that worked and then broke.

Why the convention matters here

The repo's documented intent is that compat-gap coverage lives alongside the module's other tests so future readers find all vm.Script behavior in one place, and so test/regression/issue/ remains a clean signal of "this once worked, don't let it break again." Filing compat gaps under regression/ dilutes that signal.

Addressing the objection

One reviewer noted that test/regression/issue/ already contains many compat-gap tests, suggesting loose enforcement, and that file placement is purely stylistic. That is true historically, but the rule is now explicitly documented in two CLAUDE.md files — including the very sentence "An issue number alone does not qualify" — which exists precisely to stop new instances of this pattern. Since this PR already touches test/js/node/vm/vm.test.ts, moving the five tests there is essentially zero-cost. This is not a functional defect, hence nit severity, but it is a documented convention being newly violated rather than a pre-existing one.

Step-by-step

  1. test/regression/issue/28666.test.ts is created with 5 tests covering construction-time SyntaxError behavior.
  2. Per CLAUDE.md:66 and test/CLAUDE.md:153, this path is reserved for behavior that worked in a prior release and then broke.
  3. new Script(invalidSource) has never thrown at construction time in any released Bun — confirmed by the pre-PR vm.test.ts expectation and the PR's own problem statement.
  4. Therefore Bun does not throw when using invalid Syntax within vm.Script #28666 is a compat gap, not a regression, and the tests belong in test/js/node/vm/vm.test.ts.

How to fix

Move the five test cases from test/regression/issue/28666.test.ts into test/js/node/vm/vm.test.ts (e.g., a describe("Script constructor syntax checking", ...) block near the existing "can't use export syntax in vm.Script" test) and delete the new regression file.


test("vm.Script throws SyntaxError for missing closing paren", () => {
expect(() => {
new Script("Math.max(a, b", { filename: "main" });
}).toThrow(SyntaxError);
});

test("vm.Script throws SyntaxError for unterminated string", () => {
expect(() => {
new Script('"hello', { filename: "main" });
}).toThrow(SyntaxError);
});

test("vm.Script throws SyntaxError for invalid token", () => {
expect(() => {
new Script("let @x = 1;", { filename: "main" });
}).toThrow(SyntaxError);
});

test("vm.Script throws SyntaxError at construction, not at run time", () => {
let reachedRun = false;
try {
const script = new Script("Math.max(a, b", { filename: "main" });
reachedRun = true;
script.runInThisContext();
} catch (error: unknown) {
expect(error).toBeInstanceOf(SyntaxError);
}
expect(reachedRun).toBe(false);
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Comment thread
robobun marked this conversation as resolved.
test("vm.Script does not throw for valid syntax", () => {
expect(() => {
new Script("Math.max(1, 2)", { filename: "main" });
}).not.toThrow();
});
Loading