js_parser: reject a trailing comma in a parenthesized expression

src/js_parser/parse/mod.rs

@@ -432,6 +432,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         let mut spread_range = bun_ast::Range::default();
         let mut type_colon_range = bun_ast::Range::default();
         let mut comma_after_spread = bun_ast::Loc::EMPTY;
+        let mut close_paren_after_comma = bun_ast::Loc::EMPTY;
 
         // Push a scope assuming this is an arrow function. It may not be, in which
         // case we'll need to roll this change back. This has to be done ahead of
@@ -501,6 +502,12 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
 
             // Eat the comma token
             p.lexer.next()?;
+
+            // A comma before the ")" is only valid if these turn out to be arguments
+            if p.lexer.token == T::TCloseParen {
+                close_paren_after_comma = p.lexer.loc();
+                break;
+            }
         }
         let items: &'a mut [Expr] = items_list.into_bump_slice_mut();
 
@@ -652,6 +659,19 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                 return Err(crate::Error::SyntaxError);
             }
 
+            // Not in esbuild, which accepts "(a,)". Node refuses it.
+            if !close_paren_after_comma.is_empty() {
+                p.log().add_range_error(
+                    Some(p.source),
+                    bun_ast::Range {
+                        loc: close_paren_after_comma,
+                        len: 1,
+                    },
+                    b"Unexpected )",
+                );
+                return Err(crate::Error::SyntaxError);
+            }
+
             let mut value = Expr::join_all_with_comma(items);
             p.mark_expr_as_parenthesized(&mut value);
             return Ok(value);

src/jsc/RuntimeTranspilerCache.rs

@@ -77,7 +77,8 @@ bun_core::declare_scope!(cache, visible);
 /// Version 42: The body of a class with lowered class decorators names the class by the
 /// binding that holds what the decorators return.
 /// Version 43: A `bun:` specifier that names no builtin is printed as it is written.
-const EXPECTED_VERSION: u32 = 43;
+/// Version 44: A parenthesized expression that ends with a comma, `(a,)`, is a syntax error.
+const EXPECTED_VERSION: u32 = 44;
 
 /// Source files smaller than this are not written to / read from the on-disk
 /// transpiler cache. Originally 50 KiB, which excluded almost every file in a

test/bundler/transpiler/transpiler.test.js

@@ -1,5 +1,6 @@
 import { describe, expect, it } from "bun:test";
 import { bunEnv, bunExe, bunRun, hideFromStackTrace, integersThatNeedRounding, isDebug, tempDir } from "harness";
+import vm from "node:vm";
 import { join } from "path";
 
 describe("Bun.Transpiler", () => {
@@ -3922,6 +3923,250 @@ console.log(resolve.length)
       expectPrintedNoTrim("for (const {a} of y) ;", "for (const { a } of y)\n  ;\n");
     });
 
+    describe("a comma before the `)` of a parenthesized list", () => {
+      const loaders = {
+        js: new Bun.Transpiler({ loader: "js" }),
+        jsx: new Bun.Transpiler({ loader: "jsx" }),
+        ts: new Bun.Transpiler({ loader: "ts" }),
+        tsx: new Bun.Transpiler({ loader: "tsx" }),
+      };
+      const errorsOf = parse => {
+        try {
+          parse();
+        } catch (er) {
+          return (er instanceof AggregateError ? er.errors : [er]).map(e => ({
+            message: e.message,
+            offset: e.position?.offset,
+          }));
+        }
+        return [];
+      };
+      // `scanImports` uses the scan-only parser.
+      const everyEntry = (code, loader) => ({
+        transformSync: errorsOf(() => loaders[loader].transformSync(code)),
+        scan: errorsOf(() => loaders[loader].scan(code)),
+        scanImports: errorsOf(() => loaders[loader].scanImports(code)),
+      });
+      const prints = (code, out, loader = "js") => {
+        expect({ code, out: loaders[loader].transformSync(code).trim(), ...everyEntry(code, loader) }).toEqual({
+          code,
+          out,
+          transformSync: [],
+          scan: [],
+          scanImports: [],
+        });
+      };
+
+      it("is an error when the list is an expression", () => {
+        // The caret marks the ")" that the one error points at.
+        const rejects = (marked, loader = "js") => {
+          const offset = marked.indexOf("^");
+          const code = marked.slice(0, offset) + marked.slice(offset + 1);
+          const error = [{ message: "Unexpected )", offset }];
+          expect({ code, ...everyEntry(code, loader) }).toEqual({
+            code,
+            transformSync: error,
+            scan: error,
+            scanImports: error,
+          });
+        };
+
+        rejects("x = (a,^)");
+        rejects("x = (a, b,^)");
+        rejects("x = (a,\n^)");
+        rejects("x = (a, /* ) */ ^)");
+        rejects("x = ((a,^))");
+        rejects("x = ((a),^)");
+        rejects("x = (a, (b,^))");
+        rejects("x = ((a,^), b,)");
+        rejects("x = (a = 1,^)");
+        rejects("x = ([a,],^)");
+        rejects("x = ({ a, },^)");
+        rejects("x = (a,^).b");
+        rejects("x = (a,^) ? b : c");
+        rejects("x = y ? (a,^) : b");
+        rejects("x = y ? a : (b,^)");
+        rejects("x = f((a,^))");
+        rejects("x = [(a,^)]");
+        rejects("x = () => (a,^)");
+        rejects("x = (a,) => (b,^)");
+        rejects("x = ((a,) => a,^)");
+        rejects("x = ((a,^)) => a");
+        rejects("x = (b = (a,^)) => b");
+        rejects("(a,^) = 1");
+        rejects("for ((a,^);;) ;");
+        rejects("function f() { return (a,^) }");
+        rejects("x = <div>{(a,^)}</div>", "jsx");
+        rejects("x = (a,^)", "ts");
+        rejects("x = (a as any,^)", "ts");
+        rejects("x = <T>(a,^)", "ts");
+        rejects("x = 1 + <T>(a,^)", "ts");
+        rejects("x = (a,^): T", "ts");
+        rejects("x = y ? (a,^) : b", "ts");
+        rejects("x = y ? (a,^) : b => c", "ts");
+        rejects("x = <div>{(a,^)}</div>", "tsx");
+        // A line of TypeScript's commaOperatorWithoutOperand.ts. tsc reports TS1109 at the same place.
+        rejects("(ANY, ^);", "ts");
+
+        // The same list after an operator gets the same error, from the lexer.
+        rejects("x = b || (a,^)");
+        rejects("x = typeof (a,^)");
+
+        // replMode reads `{a: 1},` as `({a: 1},)`. The position of an error is one in that text, so it is not checked.
+        const repl = new Bun.Transpiler({ loader: "tsx", replMode: true });
+        expect(errorsOf(() => repl.transformSync("{a: 1},")).map(e => e.message)).toEqual(["Unexpected )"]);
+      });
+
+      it("is valid in arrow function parameters and in call arguments", () => {
+        prints("x = (a,) => a;", "x = (a) => a;");
+        prints("x = (a, b,) => a;", "x = (a, b) => a;");
+        prints("x = (\n  a,\n  b,\n) => a;", "x = (a, b) => a;");
+        prints("x = (a, /* ) */ ) => a;", "x = (a) => a;");
+        prints("x = (a = 1,) => a;", "x = (a = 1) => a;");
+        prints("x = ({ a = 1 }, [b],) => a;", "x = ({ a = 1 }, [b]) => a;");
+        prints("x = ((a,) => a, b);", "x = ((a) => a, b);");
+        prints("x = async (a,) => a;", "x = async (a) => a;");
+        prints("x = async(a,);", "x = async(a);");
+        prints("x = async(a, b,);", "x = async(a, b);");
+        prints("x = async(...a,);", "x = async(...a);");
+        prints("x = f(a,);", "x = f(a);");
+        prints("x = new f(a,);", "x = new f(a);");
+        prints("x = (a,): T => a;", "x = (a) => a;", "ts");
+        prints("x = (a?,) => a;", "x = (a) => a;", "ts");
+        prints("x = (a: T,) => a;", "x = (a) => a;", "ts");
+        prints("x = <T>(a,) => a;", "x = (a) => a;", "ts");
+        prints("x = async <T>(a,) => a;", "x = async (a) => a;", "ts");
+        prints("x = async <T>(a,);", "x = async(a);", "ts");
+        prints("x = y ? (a,): T => a : c;", "x = y ? (a) => a : c;", "ts");
+        prints("x = y ? (a,) : b => c : d;", "x = y ? (a) => c : d;", "ts");
+        prints("x = y ? async (a,) : b => c;", "x = y ? async(a) : (b) => c;", "ts");
+        prints("x = y ? async (a,) : b => c : d;", "x = y ? async (a) => c : d;", "ts");
+        prints("x = y ? (a,): T => (b) : (c) => d;", "x = y ? (a) => b : (c) => d;", "ts");
+        prints(
+          "x = y ? ({ a, b },): T => ({ a, b }) : (c) => d;",
+          "x = y ? ({ a, b }) => ({ a, b }) : (c) => d;",
+          "ts",
+        );
+        prints("x = <T,>(a,) => a;", "x = (a) => a;", "tsx");
+
+        // A setter has no parenthesized list. JavaScriptCore refuses this comma. V8 and tsc accept it.
+        prints("x = { set m(a,) {} };", "x = { set m(a) {} };");
+        prints("class A { set m(a,) {} }", "class A {\n  set m(a) {}\n}");
+      });
+
+      // tsc reads each of these as `y ? <arrow function> : <arrow function>`. The parser takes `: c =>` for a
+      // return type of `(b)`, with and without a trailing comma (the first three rows are wrong on main too).
+      // The last two rows were the program `y ? a : (T) => (b) => d` before, and are an error now.
+      describe("does not repair a TypeScript arrow function before `:` that has a body in parentheses", () => {
+        for (const [code, out] of [
+          ["x = y ? (a) => (b) : c => d;", "x = y ? (a) => b : (c) => d;"],
+          ["x = y ? (a): T => (b) : c => d;", "x = y ? (a) => b : (c) => d;"],
+          ["x = y ? async (a,): T => (b) : c => d;", "x = y ? async (a) => b : (c) => d;"],
+          ["x = y ? (a,): T => (b) : c => d;", "x = y ? (a) => b : (c) => d;"],
+          ["x = y ? ({ a, b },): T => ({ a, b }) : c => d;", "x = y ? ({ a, b }) => ({ a, b }) : (c) => d;"],
+        ]) {
+          it.failing(code, () => prints(code, out, "ts"));
+        }
+      });
+
+      it("comes after an earlier error in the list", () => {
+        const rejectsWith = (code, messages, loader = "js") => {
+          const errors = errorsOf(() => loaders[loader].transformSync(code)).map(e => e.message);
+          expect({ code, errors }).toEqual({ code, errors: messages });
+        };
+
+        // An error that stops the parser is the only one.
+        rejectsWith("x = (...a,)", ['Unexpected "..."']);
+        rejectsWith("x = (...a,) => a", ['Unexpected "," after rest pattern']);
+        rejectsWith("x = (a,)\n=> a", ['Unexpected newline before "=>"']);
+        rejectsWith("x = (a: T,)", ['Unexpected ":"'], "ts");
+        rejectsWith("x = (a,); y = (b,)", ["Unexpected )"]);
+        // Other errors come first, as they do for the same list after an operator.
+        rejectsWith("x = ({ a = 1 },)", ['Unexpected "="', "Unexpected )"]);
+        rejectsWith("x = b || ({ a = 1 },)", ['Unexpected "="', "Unexpected )"]);
+        rejectsWith("x = (super.a,)", ['Unexpected "super"', "Unexpected )"]);
+        rejectsWith("x = b || (super.a,)", ['Unexpected "super"', "Unexpected )"]);
+        rejectsWith("x = (a?,)", ["Unexpected ,", "Unexpected )"], "ts");
+      });
+
+      it("is accepted only where JavaScriptCore accepts it", () => {
+        const accepts = parse => {
+          try {
+            parse();
+            return true;
+          } catch {
+            return false;
+          }
+        };
+        const lists = ["a", "a, b", "a = 1", "[a]", "{ a }", "a, [b] = c", "...a", "a, ...b"];
+        const places = [
+          list => `x = (${list});`,
+          list => `x = (${list}) => y;`,
+          list => `x = async (${list}) => y;`,
+          list => `x = async(${list});`,
+          list => `x = f(${list});`,
+          list => `x = new f(${list});`,
+          list => `x = ((${list}));`,
+          list => `x = ((${list}), z);`,
+          list => `x = (z, (${list}));`,
+          list => `x = y ? (${list}) : z;`,
+          list => `x = y ? z : (${list});`,
+          list => `x = y || (${list});`,
+          list => `x = (${list}).p;`,
+          list => `x = [(${list})];`,
+          list => `x = { k: (${list}) };`,
+          list => `x = () => (${list});`,
+          list => `x = (p = (${list})) => p;`,
+          list => `if ((${list})) ;`,
+          list => `for ((${list});;) ;`,
+          list => `function g() { return (${list}); }`,
+          list => `(${list}) = 1;`,
+          list => `x = \`\${(${list})}\`;`,
+        ];
+        const differ = [];
+        let accepted = 0;
+        let refused = 0;
+        for (const place of places) {
+          for (const list of lists) {
+            for (const code of [place(list), place(list + ",")]) {
+              // `vm.Script` gives the text to JavaScriptCore as it is, with no transpiler before it.
+              const engine = accepts(() => new vm.Script(code));
+              if (engine) accepted++;
+              else refused++;
+              if (accepts(() => loaders.js.transformSync(code)) !== engine) differ.push(code);
+            }
+          }
+        }
+        expect(differ).toEqual([]);
+        expect({ accepted: accepted > 0, refused: refused > 0 }).toEqual({ accepted: true, refused: true });
+      });
+
+      it("is an error when bun runs the file", async () => {
+        const source = "var a = 1;\nvar x = (a,);\nconsole.log('RAN', x);\n";
+        using dir = tempDir("paren-trailing-comma", { "esm.mjs": source, "cjs.cjs": source });
+        const run = async file => {
+          await using proc = Bun.spawn({
+            cmd: [bunExe(), file],
+            env: bunEnv,
+            cwd: String(dir),
+            stdout: "pipe",
+            stderr: "pipe",
+          });
+          const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+          return {
+            stdout,
+            error: stderr.match(/^error: (.*)$/m)?.[1],
+            at: stderr.match(/^\s+at .*?(\w+\.\w+:\d+:\d+)$/m)?.[1],
+            exitCode,
+          };
+        };
+        expect(await Promise.all([run("esm.mjs"), run("cjs.cjs")])).toEqual([
+          { stdout: "", error: "Unexpected )", at: "esm.mjs:2:12", exitCode: 1 },
+          { stdout: "", error: "Unexpected )", at: "cjs.cjs:2:12", exitCode: 1 },
+        ]);
+      });
+    });
+
     it("regexp", () => {
       expectPrinted("/x/g", "/x/g");
       expectPrinted("/x/i", "/x/i");