From ee132858e5a0ff569834fa902e66cb836ed5e3d1 Mon Sep 17 00:00:00 2001 From: "andrea.bergia" Date: Tue, 2 Dec 2025 16:44:01 +0100 Subject: [PATCH 1/5] Handle incorrect destructuring in for loops at parse time Code like this: ```js for (var {};;) {} ``` was parsed correctly, but then failed during the code generation with various exceptions because the destructuring expression after `var` doesn't have a right-hand side. This change implements a check at parse time, to throw a `SyntaxError`, coherently with what `node` or `spidermonkey` or `quickjs` do. I'm a bit surprised that this doesn't seem to show any improvement in test262 coverage. Perhaps a test there might be warranted? Fixes https://github.com/mozilla/rhino/issues/1922 --- rhino/src/main/java/org/mozilla/javascript/Parser.java | 10 ++++++++++ .../java/org/mozilla/javascript/tests/ParserTest.java | 5 +++++ tests/testsrc/jstests/harmony/destructuring.js | 1 + 3 files changed, 16 insertions(+) diff --git a/rhino/src/main/java/org/mozilla/javascript/Parser.java b/rhino/src/main/java/org/mozilla/javascript/Parser.java index 6807ec081f..c1952c2e10 100644 --- a/rhino/src/main/java/org/mozilla/javascript/Parser.java +++ b/rhino/src/main/java/org/mozilla/javascript/Parser.java @@ -1677,6 +1677,16 @@ && matchToken(Token.NAME, true) markDestructuring(init); cond = expr(false); // object over which we're iterating } else { // ordinary for-loop + // For ordinary for loops, destructuring declarations must have initializers + if (init instanceof VariableDeclaration) { + VariableDeclaration varDecl = (VariableDeclaration) init; + for (VariableInitializer vi : varDecl.getVariables()) { + if (vi.isDestructuring() && vi.getInitializer() == null) { + reportError("msg.destruct.assign.no.init"); + } + } + } + mustMatchToken(Token.SEMI, "msg.no.semi.for", true); if (peekToken() == Token.SEMI) { // no loop condition diff --git a/tests/src/test/java/org/mozilla/javascript/tests/ParserTest.java b/tests/src/test/java/org/mozilla/javascript/tests/ParserTest.java index 11a86b6efc..ae5afade6a 100644 --- a/tests/src/test/java/org/mozilla/javascript/tests/ParserTest.java +++ b/tests/src/test/java/org/mozilla/javascript/tests/ParserTest.java @@ -1490,6 +1490,11 @@ public void oomOnInvalidInput() { expectParseErrors("`\\u{8", new String[] {"syntax error"}); } + @Test + public void errorOnInvalidDestructuringDeclaration() { + expectParseErrors("for(var {};;) {}", new String[] {"Missing = in destructuring declaration", "syntax error"}); + } + private void expectParseErrors(String string, String[] errors) { parse(string, errors, null, false); } diff --git a/tests/testsrc/jstests/harmony/destructuring.js b/tests/testsrc/jstests/harmony/destructuring.js index 7886b66517..eaf4da38df 100644 --- a/tests/testsrc/jstests/harmony/destructuring.js +++ b/tests/testsrc/jstests/harmony/destructuring.js @@ -29,5 +29,6 @@ assertEquals(obj.b, 345); assertThrows("(1 ? {} : 490) = 1", SyntaxError); assertThrows("(1 ? [] : 490) = 1", SyntaxError); +assertThrows("for (var {};;) {}", SyntaxError); "success"; From cdf9649a3a3a0602a4691d49142bdd3aa9e22d02 Mon Sep 17 00:00:00 2001 From: "andrea.bergia" Date: Tue, 2 Dec 2025 16:51:24 +0100 Subject: [PATCH 2/5] spotless --- .../java/org/mozilla/javascript/tests/ParserTest.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/src/test/java/org/mozilla/javascript/tests/ParserTest.java b/tests/src/test/java/org/mozilla/javascript/tests/ParserTest.java index ae5afade6a..420e314385 100644 --- a/tests/src/test/java/org/mozilla/javascript/tests/ParserTest.java +++ b/tests/src/test/java/org/mozilla/javascript/tests/ParserTest.java @@ -1490,10 +1490,12 @@ public void oomOnInvalidInput() { expectParseErrors("`\\u{8", new String[] {"syntax error"}); } - @Test - public void errorOnInvalidDestructuringDeclaration() { - expectParseErrors("for(var {};;) {}", new String[] {"Missing = in destructuring declaration", "syntax error"}); - } + @Test + public void errorOnInvalidDestructuringDeclaration() { + expectParseErrors( + "for(var {};;) {}", + new String[] {"Missing = in destructuring declaration", "syntax error"}); + } private void expectParseErrors(String string, String[] errors) { parse(string, errors, null, false); From e0b7485309d2129945eeca9184945a57742017fc Mon Sep 17 00:00:00 2001 From: "andrea.bergia" Date: Tue, 2 Dec 2025 21:25:41 +0100 Subject: [PATCH 3/5] Dummy commit to trigger the CI From 9e6e5909d76d7141025437e4fb71baf0b1ca896b Mon Sep 17 00:00:00 2001 From: "andrea.bergia" Date: Wed, 3 Dec 2025 08:49:00 +0100 Subject: [PATCH 4/5] Added a test case for `let` as well --- tests/testsrc/jstests/harmony/destructuring.js | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testsrc/jstests/harmony/destructuring.js b/tests/testsrc/jstests/harmony/destructuring.js index eaf4da38df..edb2001834 100644 --- a/tests/testsrc/jstests/harmony/destructuring.js +++ b/tests/testsrc/jstests/harmony/destructuring.js @@ -30,5 +30,6 @@ assertEquals(obj.b, 345); assertThrows("(1 ? {} : 490) = 1", SyntaxError); assertThrows("(1 ? [] : 490) = 1", SyntaxError); assertThrows("for (var {};;) {}", SyntaxError); +assertThrows("for (let {};;) {}", SyntaxError); "success"; From 84ee3cc7caae8b94702488df411ed054e9ac3267 Mon Sep 17 00:00:00 2001 From: "andrea.bergia" Date: Wed, 3 Dec 2025 08:50:42 +0100 Subject: [PATCH 5/5] Added more tests --- tests/testsrc/jstests/harmony/destructuring.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/testsrc/jstests/harmony/destructuring.js b/tests/testsrc/jstests/harmony/destructuring.js index edb2001834..ea0865946a 100644 --- a/tests/testsrc/jstests/harmony/destructuring.js +++ b/tests/testsrc/jstests/harmony/destructuring.js @@ -31,5 +31,7 @@ assertThrows("(1 ? {} : 490) = 1", SyntaxError); assertThrows("(1 ? [] : 490) = 1", SyntaxError); assertThrows("for (var {};;) {}", SyntaxError); assertThrows("for (let {};;) {}", SyntaxError); +assertThrows("for (var {a};;) {}", SyntaxError); +assertThrows("for (let {a};;) {}", SyntaxError); "success";