fix(parser): SyntaxError for new import.defer/source(...) and SequenceExpression specifiers; clean up test262 known-bugs list#20917
Merged
Conversation
Removed three duplicate entries from the import-attributes 2nd-param section and added explanatory comments to previously uncommented groups so each unfixable entry documents the underlying root cause (global \`this\` wrap, namespace exotic semantics, build-time vs runtime error reporting, nested \`import(import(...))\`, top-level-await ordering, etc.). https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
…) specifier
`import((1, 0, "./mod.js"))` (and any other comma expression whose leading
operands are pure) now resolves to `import("./mod.js")` instead of being
rejected as unresolvable. Previously the parser had no `evaluate` hook for
`SequenceExpression`, so the specifier was unknown and the dependency could
not be created. The new hook only folds when each leading expression is
side-effect free, so e.g. `import((effectfulFn(), "./mod.js"))` is still
left dynamic and its side effects are preserved.
Removes test262 case `expressions/dynamic-import/assignment-expression/cover-parenthesized-expr.js`
from \`knownBugs\` (it now passes) and clarifies the comment for
\`statements/async-function/evaluation-body.js\` (failure is Jest's per-test
unhandled-rejection tracking, not microtask ordering).
https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
Per spec ImportCall is a CallExpression, never a MemberExpression, so
\`new import.defer(...)\` and \`new import.source(...)\` are SyntaxErrors.
Acorn rejects bare \`new import(...)\`, but the \`acorn-import-phases\` plugin
currently accepts the phased variants. We now detect this in
\`walkNewExpression\` and throw a SyntaxError, which gets surfaced as a
\`ModuleParseError\` by NormalModule. Parenthesized forms
\`new (import.defer(...))\` produce the same AST shape but are valid syntax;
we tell them apart by checking for an opening paren in the source between
\`new\` and the callee, leaving them to throw \`TypeError\` at runtime.
Removes 21 corresponding test262 entries from \`knownBugs\`:
- the 19 \`syntax/invalid/*-import-defer-no-new-call-expression.js\` cases
- \`syntax/invalid/{nested-with,top-level}-import-defer-no-new-call-expression.js\`
(previously misclassified as acorn bugs)
And one valid-syntax entry that was incorrectly listed:
- \`syntax/valid/new-covered-expression-is-valid.js\`
Also clarifies the comment for
\`expressions/dynamic-import/custom-primitive.js\` (root cause is
\`__webpack_exports__\` inheriting \`Object.prototype\` rather than having
\`null\` prototype like a real module namespace exotic, so
\`Object.prototype.toString\` shadows the missing-\`toString\` fallback path).
https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
… wrap dynamic options in IIFE
* Moved the SequenceExpression evaluator and the new \`new import.defer/source(...)\` SyntaxError check out of \`JavascriptParser\` and into \`ImportParserPlugin\`. \`JavascriptParser\` now exposes a \`newImportCall\` SyncBailHook that fires whenever a \`NewExpression\`'s callee is an \`ImportExpression\`; \`ImportParserPlugin\` taps it.
* Added \`dynamicOptionsRange\` to \`ImportDependency\` (and the Eager / Weak subclasses). When set, the dependency template wraps the runtime require chain in \`((opts) => <chain>)((<options-source>))\` so the original second-argument expression is still evaluated for side effects, even though webpack overwrites the entire \`import(...)\` source range.
* Removes \`expressions/dynamic-import/import-attributes/2nd-param-yield-expr.js\` and the \`2nd-param-evaluation-abrupt-{return,throw}.js\` cases from \`knownBugs\` (they pass now). Comments updated for the remaining 2nd-param cases that need spec-level runtime validation of the options object.
https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
The comma-expression fold isn't import-specific — any \`evaluateExpression\` caller can benefit, so it lives alongside the other generic language-level evaluators (\`LogicalExpression\`, \`ConditionalExpression\`, ...) in \`JavascriptParser._initializeEvaluating\` instead of \`ImportParserPlugin\`. The \`new import.defer/source(...)\` SyntaxError check stays in \`ImportParserPlugin\` (it taps the \`newImportCall\` hook on the parser) because that one truly is import-specific. https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
The wrap fixed three test262 cases (\`2nd-param-yield-expr\`,
\`2nd-param-evaluation-abrupt-{return,throw}\`) but added \`dynamicOptionsRange\`
plumbing across \`ImportDependency\`, \`ImportEagerDependency\`,
\`ImportWeakDependency\` (constructor + serialize/deserialize), a new
\`wrapWithDynamicOptions\` helper, and template branches in three places, plus
matching changes in \`ImportParserPlugin\`. Net cost was ~140 lines for 3 tests
and the spec also requires runtime validation of the options object that
the wrap doesn't address. Roll it back; the affected entries go back into
\`knownBugs\` with a single comment explaining the architectural cause.
https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
The hook + tap was overkill for a workaround whose only purpose is to fire a SyntaxError that acorn-import-phases (or acorn itself) should be raising. Move the check back inline in \`walkNewExpression\` with a TODO that says it is not a webpack bug and should be removed once the upstream parser native handling is fixed. Removes the \`newImportCall\` hook and the \`ImportParserPlugin\` tap. https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
🦋 Changeset detectedLatest commit: 4f1f2e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
This PR is packaged and the instant preview is available (3032f8c). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@3032f8c
yarn add -D webpack@https://pkg.pr.new/webpack@3032f8c
pnpm add -D webpack@https://pkg.pr.new/webpack@3032f8c |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (56.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #20917 +/- ##
==========================================
+ Coverage 91.34% 91.35% +0.01%
==========================================
Files 563 566 +3
Lines 55887 56026 +139
Branches 14819 14875 +56
==========================================
+ Hits 51050 51184 +134
- Misses 4837 4842 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates webpack’s Test262 harness skip list to remove duplicates and document known-spec divergences more clearly, and it also includes two small parser fixes (with changesets) to improve dynamic import() handling and reject invalid new import.*(...) forms.
Changes:
- Clean up
test/test262.spectest.jsknown-bugs entries (remove duplicates) and add root-cause comments for skipped groups. JavascriptParser: evaluate side-effect-freeSequenceExpressiontails to allow resolvingimport((1, 0, "./mod.js")).JavascriptParser: rejectnew import.defer(...)/new import.source(...)as a parse-timeSyntaxError, and add changesets for both user-facing fixes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
test/test262.spectest.js |
Removes duplicate skip entries and expands comments explaining why certain Test262 cases remain skipped. |
lib/javascript/JavascriptParser.js |
Adds a SequenceExpression evaluator for safe folding, and throws a SyntaxError for invalid new import.*(...) forms. |
.changeset/new-import-call-syntax-error.md |
Patch release note for rejecting new import.defer/source(...) at parse time. |
.changeset/dynamic-import-sequence-expression.md |
Patch release note for resolving static specifiers from side-effect-free sequence expressions in import(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3891
to
+3901
| if (between !== undefined && !/\(/.test(between)) { | ||
| const err = | ||
| /** @type {SyntaxError & { loc?: { line: number, column: number } }} */ | ||
| (new SyntaxError("import call cannot be the target of `new`")); | ||
| if (expression.loc) { | ||
| err.loc = { | ||
| line: expression.loc.start.line, | ||
| column: expression.loc.start.column | ||
| }; | ||
| } | ||
| throw err; |
Comment on lines
+1985
to
+1998
| // Only fold a comma expression to its tail when the leading | ||
| // expressions are side-effect free; otherwise downstream | ||
| // transforms that replace the whole expression range (e.g. | ||
| // `ImportDependency` for `import(...)`) would silently drop the | ||
| // leading side effects. | ||
| let commentsStartPos = /** @type {Range} */ (expr.range)[0]; | ||
| for (let i = 0; i < expr.expressions.length - 1; i++) { | ||
| const item = expr.expressions[i]; | ||
| if (!this.isPure(item, commentsStartPos)) return; | ||
| commentsStartPos = /** @type {Range} */ (item.range)[1]; | ||
| } | ||
| const last = expr.expressions[expr.expressions.length - 1]; | ||
| return this.evaluateExpression(last).setRange( | ||
| /** @type {Range} */ (expr.range) |
- \`walkNewExpression\`: strip block and line comments from the source between \`new\` and the \`ImportExpression\` callee before checking for \`(\`, so e.g. \`new /* ( */ import.defer(...)\` is still rejected. - \`SequenceExpression\` evaluator: bail out when \`expr.range\` or any \`expr.expressions[i].range\` is missing. \`JavascriptParser.prototype.evaluate(source)\` parses without \`ranges: true\` (e.g. when \`DefinePlugin\` evaluates a config string), so the previous unconditional \`expr.range[0]\` access would throw and surface as a \`console.warn\` from \`evaluateExpression\`'s catch block. https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
Merging this PR will degrade performance by 27.21%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | benchmark "asset-modules-resource", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
188.1 ms | 72.2 ms | ×2.6 |
| ⚡ | Memory | benchmark "asset-modules-source", scenario '{"name":"mode-development","mode":"development"}' |
833.2 KB | 659.5 KB | +26.34% |
| ❌ | Memory | benchmark "many-chunks-commonjs", scenario '{"name":"mode-production","mode":"production"}' |
6.6 MB | 8.6 MB | -22.87% |
| ❌ | Memory | benchmark "devtool-eval-source-map", scenario '{"name":"mode-development","mode":"development"}' |
1,017.5 KB | 1,397.8 KB | -27.21% |
| ❌ | Memory | benchmark "future-defaults", scenario '{"name":"mode-development","mode":"development"}' |
1.4 MB | 1.8 MB | -25.72% |
Comparing claude/fix-test262-bugs-YA4VY (4f1f2e7) with main (d060ace)
Comment on lines
+2003
to
+2005
| return this.evaluateExpression(last).setRange( | ||
| /** @type {Range} */ (expr.range) | ||
| ); |
… value
Copilot review: with the previous fold, \`(0, eval)("...")\` evaluated to the
\`eval\` identifier and triggered \`parser.hooks.call.for("eval")\`, which made
\`JavascriptMetaInfoPlugin\` bail out of module concatenation for the
canonical indirect-eval pattern (and would similarly conflate other
\`(0, ident)(...)\` direct/indirect-call distinctions). Restrict the fold to
\`isCompileTimeValue()\` results — string / number / boolean / RegExp / null /
undefined / BigInt / const-array — so the static \`import((1, 0, "./mod.js"))\`
case still resolves while identifier and member-chain tails are left alone.
Verified: with this guard, \`(0, eval)("2 + 2")\` no longer sets
\`buildInfo.moduleConcatenationBailout = "eval()"\`, and
\`expressions/dynamic-import/assignment-expression/cover-parenthesized-expr.js\`
still passes.
https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
new import.defer/source(...) and SequenceExpression specifiers; clean up test262 known-bugs list
…tly in walkNewExpression
\`parse()\` already converts a Buffer source to UTF-8 string at the top, but
only the local variable was updated; \`state.source\` could still be a Buffer.
Mirror the conversion onto \`state.source\` so downstream walkers (currently
the \`new ImportExpression\` workaround) can read it as a string without an
extra \`Buffer.toString("utf8")\` per check, simplifying the call site.
https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
The range guard stays — \`parser.evaluate(source)\` (used by \`DefinePlugin\`) parses with \`defaultParserOptions.ranges = false\`, so AST nodes from that path don't carry ranges and the unconditional \`expr.range[0]\` access would otherwise throw and surface as a \`console.warn\`. Just drop the multi-line explanations. https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R
Contributor
aryanraj45
pushed a commit
to aryanraj45/webpack
that referenced
this pull request
May 8, 2026
…nceExpression specifiers; clean up test262 known-bugs list (webpack#20917)
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.
Summary
Two small parser fixes (with changesets), plus a cleanup of
test/test262.spectest.js'sknownBugslist:SyntaxError for
new import.defer(...)/new import.source(...).acorn-import-phasesaccepts these even thoughImportCallis aCallExpressionper spec (so it's not a validnewoperand — barenew import(...)is correctly rejected by acorn).walkNewExpressionnow throws aSyntaxError(surfaced byNormalModuleasModuleParseError) when the callee is anImportExpressionand the source betweennewand the callee contains no(outside comments. Parenthesized forms (new (import.defer(...))) stay valid and continue to throwTypeErrorat runtime. Inline TODO notes that this is an upstream-parser workaround that should be removed onceacorn-import-phases(or acorn) reports the SyntaxError natively.Fold side-effect-free
SequenceExpressionto its tail in evaluation. Adds anevaluatehook forSequenceExpressioninJavascriptParser._initializeEvaluatingsoimport((1, 0, "./mod.js"))resolves the same asimport("./mod.js"). The fold is gated onparser.isPurefor every leading expression so side effects are never silently dropped, and onevaluated.isCompileTimeValue()for the tail so identifier / member-chain tails like(0, eval)("…")are not folded (which would otherwise conflate the canonical indirect-eval pattern with a directeval()call and triggerJavascriptMetaInfoPlugin's concatenation bailout).knownBugscleanup. Removed three duplicate entries from theimport-attributes/2nd-param-evaluation-*block, removed entries that now pass thanks to the parser fixes (the*-import-defer-no-new-call-expression.jsfamily andcover-parenthesized-expr.js), and rewrote terse / missing comments on the remaining entries with the actual root cause (globalthiswrap, namespace-exotic semantics, build-time vs runtime error reporting, top-level-await ordering, etc.).What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes — fixes are covered by removing skips from
test/test262.spectest.js. Net test262 wins on top ofmain:dynamic-import2,252 → 2,348 (+96), 21 invalid-syntax*-import-defer-no-new-call-expression.jscases now pass, pluscover-parenthesized-expr.jsandnew-covered-expression-is-valid.js.Does this PR introduce a breaking change?
No.
If relevant, what needs to be documented…
n/a
Use of AI
Claude Code drafted the implementation, the test262 audit and the changesets under human review.
https://claude.ai/code/session_01BCUNtcBHAZPzjvuUbnW37R