Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
53 changes: 46 additions & 7 deletions rhino/src/main/java/org/mozilla/javascript/IRFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -1210,22 +1210,61 @@ private Node transformTry(TryStatement node) {

Node catchBlocks = new Block();
for (CatchClause cc : node.getCatchClauses()) {
Name varName = cc.getVarName();
AstNode varName = cc.getVarName();
Node catchCond = null;
Node varNameNode = null;
Scope catchBody = cc.getBody();

if (varName != null) {
varNameNode = parser.createName(varName.getIdentifier());
if (varName instanceof Name) {
// Simple identifier
varNameNode = parser.createName(((Name) varName).getIdentifier());

AstNode ccc = cc.getCatchCondition();
if (ccc != null) {
catchCond = transform(ccc);
} else {
AstNode ccc = cc.getCatchCondition();
if (ccc != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

depressive if - maybe you can swap the branches

Copy link
Contributor Author

@andreabergia andreabergia Dec 9, 2025

Choose a reason for hiding this comment

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

I disagree, or rather, I don't particularly see the current version being particularly better or worse than the alternative. This to me reads about the same even if we invert the if.

                    AstNode ccc = cc.getCatchCondition();
                    if (ccc != null) {
                        catchCond = transform(ccc);
                    } else {
                        catchCond = new EmptyExpression();
                    }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally i think this is easier to read but i can live with the 'depressive' version also

AstNode ccc = cc.getCatchCondition();
if (ccc == null) {
    catchCond = new EmptyExpression();
} else {
    catchCond = transform(ccc);
}

catchCond = transform(ccc);
} else {
catchCond = new EmptyExpression();
}
} else if (varName instanceof org.mozilla.javascript.ast.ArrayLiteral
|| varName instanceof org.mozilla.javascript.ast.ObjectLiteral) {
// Destructuring pattern. We basically replace:
// catch ( {message} ) { body }
// into:
// catch ( $tempname ) { let {message} = $tempname; body }

// The exception will be stored in the temp name
String tempVarName = parser.currentScriptOrFn.getNextTempName();
varNameNode = parser.createName(tempVarName);

// The let statement will be used to do the destructuring
VariableDeclaration letStatement = new VariableDeclaration();
letStatement.setType(Token.LET);

VariableInitializer letVar = new VariableInitializer();
letStatement.addVariable(letVar);

// LHS: the destructuring declaration
letVar.setTarget(varName);

// RHS: the temp name (which we need to wrap in a new name node)
Name tempVarNameNode = new Name();
tempVarNameNode.setIdentifier(tempVarName);
letVar.setInitializer(tempVarNameNode);

// Prepend the destructuring "let" to the catch body
catchBody.addChildToFront(letStatement);

// Our non-standard condition is not supported for destructuring (we throw an
// error at parse time), so here we can simply force it to an empty expression
catchCond = new EmptyExpression();
} else {
throw new IllegalArgumentException(
"Unexpected catch parameter type: " + varName.getClass().getName());
}
}

Node body = transform(cc.getBody());
Node body = transform(catchBody);

catchBlocks.addChildToBack(
createCatch(varNameNode, catchCond, body, cc.getLineno(), cc.getColumn()));
Expand Down
50 changes: 32 additions & 18 deletions rhino/src/main/java/org/mozilla/javascript/Parser.java
Original file line number Diff line number Diff line change
Expand Up @@ -1821,35 +1821,49 @@ private TryStatement tryStatement() throws IOException {
guardPos = -1,
catchLine = lineNumber(),
catchColumn = columnNumber();
Name varName = null;
AstNode varName = null;
AstNode catchCond = null;

switch (peekToken()) {
case Token.LP:
{
matchToken(Token.LP, true);
lp = ts.tokenBeg;
if (!matchToken(Token.UNDEFINED, true)) {
mustMatchToken(Token.NAME, "msg.bad.catchcond", true);
}

varName = createNameNode();
Comment jsdocNodeForName = getAndResetJsDoc();
if (jsdocNodeForName != null) {
varName.setJsDocNode(jsdocNodeForName);
}
String varNameString = varName.getIdentifier();
if ("undefined".equals(varNameString)) {
hasUndefinedBeenRedefined = true;
}
if (inUseStrictDirective) {
if ("eval".equals(varNameString)
|| "arguments".equals(varNameString)) {
reportError("msg.bad.id.strict", varNameString);
int tt = peekToken();
if (tt == Token.LB || tt == Token.LC) {
// Destructuring pattern
if (compilerEnv.getLanguageVersion() >= Context.VERSION_ES6) {
varName = destructuringPrimaryExpr();
markDestructuring(varName);
} else {
reportError("msg.catch.destructuring.requires.es6");
}
} else {
// Simple identifier
if (!matchToken(Token.UNDEFINED, true)) {
mustMatchToken(Token.NAME, "msg.bad.catchcond", true);
}

varName = createNameNode();
Comment jsdocNodeForName = getAndResetJsDoc();
if (jsdocNodeForName != null) {
varName.setJsDocNode(jsdocNodeForName);
}
String varNameString = ((Name) varName).getIdentifier();
if ("undefined".equals(varNameString)) {
hasUndefinedBeenRedefined = true;
}
if (inUseStrictDirective) {
if ("eval".equals(varNameString)
|| "arguments".equals(varNameString)) {
reportError("msg.bad.id.strict", varNameString);
}
}
}

if (matchToken(Token.IF, true)) {
// Non-standard extension: we support "catch (e if cond)
if (varName instanceof Name && matchToken(Token.IF, true)) {
guardPos = ts.tokenBeg;
catchCond = expr(false);
} else {
Expand Down
17 changes: 11 additions & 6 deletions rhino/src/main/java/org/mozilla/javascript/ast/CatchClause.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@
* Node representing a catch-clause of a try-statement. Node type is {@link Token#CATCH}.
*
* <pre><i>CatchClause</i> :
* <b>catch</b> ( <i><b>Identifier</b></i> [<b>if</b> Expression] ) Block</pre>
* <b>catch</b> ( <i><b>Identifier</b></i> [<b>if</b> Expression] ) Block
* <b>catch</b> ( <i><b>ObjectPattern</b></i> ) Block
* <b>catch</b> ( <i><b>ArrayPattern</b></i> ) Block</pre>
*/
public class CatchClause extends AstNode {

private Name varName;
private AstNode varName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

at least the name of the var and the name of the getter/setter does no longer match the value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

varName sounds like this is a Name but now it is a AstNode but maybe it is more clear to stay with the current naming.

private AstNode catchCondition;
private Scope body;
private int ifPosition = -1;
Expand All @@ -40,20 +42,23 @@ public CatchClause(int pos, int len) {
/**
* Returns catch variable node
*
* @return catch variable
* @return catch variable (can be Name, ArrayLiteral, or ObjectLiteral)
*/
public Name getVarName() {
public AstNode getVarName() {
return varName;
}

/**
* Sets catch variable node, and sets its parent to this node.
*
* @param varName catch variable
* @param varName catch variable (can be Name, ArrayLiteral, or ObjectLiteral)
*/
public void setVarName(Name varName) {
public void setVarName(AstNode varName) {
this.varName = varName;
if (varName != null) {
assert varName instanceof Name
|| varName instanceof ArrayLiteral
|| varName instanceof ObjectLiteral;
varName.setParent(this);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,9 @@ msg.bad.catchcond =\
msg.catch.unreachable =\
any catch clauses following an unqualified catch are unreachable

msg.catch.destructuring.requires.es6 =\
Destructuring in catch blocks requires ES6 or later

msg.no.brace.try =\
missing '{' before try block

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

package org.mozilla.javascript.tests;

import org.junit.jupiter.api.Test;
import org.mozilla.javascript.testutils.Utils;

class CatchDestructuringTest {
@Test
void cannotUseObjectDestructuringInEs5() {
Utils.assertEvaluatorException_1_8(
"Destructuring in catch blocks requires ES6 or later (test#3)",
"try {\n"
+ " throw new Error('test');\n"
+ "} catch ({message}) {\n"
+ " message;\n"
+ "}\n");
}

@Test
void canUseObjectDestructuring() {
Utils.assertWithAllModes_ES6(
"hey",
"try {\n"
+ " throw new Error('hey');\n"
+ "} catch ({message}) {\n"
+ " message;\n"
+ "}\n");
}

@Test
void canUseObjectDestructuringWithRenaming() {
Utils.assertWithAllModes_ES6(
"hey",
"try {\n"
+ " throw new Error('hey');\n"
+ "} catch ({message: m}) {\n"
+ " m;\n"
+ "}\n");
}

@Test
void canUseNestedObjectDestructuring() {
Utils.assertWithAllModes_ES6(
"nested",
"try {\n"
+ " throw {error: {code: 'nested'}};\n"
+ "} catch ({error: {code}}) {\n"
+ " code;\n"
+ "}\n");
}

@Test
void canUseDefaultValuesInDestructuring() {
Utils.assertWithAllModes_ES6(
"default",
"try {\n"
+ " throw {};\n"
+ "} catch ({message = 'default'}) {\n"
+ " message;\n"
+ "}\n");
}

@Test
void cannotUseObjectDestructuringAndConditions() {
Utils.assertEvaluatorExceptionES6(
"invalid catch block condition (test#3)",
"try {\n"
+ " throw new Error('hey');\n"
+ "} catch ( {e} if e.message ) {\n"
+ " e.message;\n"
+ "}\n");
}

@Test
void cannotUseArrayDestructuringInEs5() {
Utils.assertEvaluatorException_1_8(
"Destructuring in catch blocks requires ES6 or later (test#3)",
"try {\n"
+ " throw ['h', 'e', 'y']\n"
+ "} catch ([h, e, y]) {\n"
+ " h + e + y;\n"
+ "}\n");
}

@Test
void canUseArrayDestructuring() {
Utils.assertWithAllModes_ES6(
"hey",
"try {\n"
+ " throw ['h', 'e', 'y']\n"
+ "} catch ([h, e, y]) {\n"
+ " h + e + y;\n"
+ "}\n");
}

@Test
void canSkipArrayElements() {
Utils.assertWithAllModes_ES6(
"c",
"try {\n"
+ " throw ['a', 'b', 'c'];\n"
+ "} catch ([, , third]) {\n"
+ " third;\n"
+ "}\n");
}

@Test
void canUseNestedArrayDestructuring() {
Utils.assertWithAllModes_ES6(
"b",
"try {\n"
+ " throw [['a', 'b'], ['c', 'd']];\n"
+ "} catch ([[x, y], [z, w]]) {\n"
+ " y;\n"
+ "}\n");
}

@Test
void canCombineObjectAndArrayDestructuring() {
Utils.assertWithAllModes_ES6(
"value",
"try {\n"
+ " throw {data: ['value', 'other']};\n"
+ "} catch ({data: [first]}) {\n"
+ " first;\n"
+ "}\n");
}

@Test
void canCombineArrayAndObjectDestructuring() {
Utils.assertWithAllModes_ES6(
"test",
"try {\n"
+ " throw [{message: 'test'}];\n"
+ "} catch ([{message}]) {\n"
+ " message;\n"
+ "}\n");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;

import java.io.IOException;
import java.util.List;
Expand Down Expand Up @@ -555,7 +556,7 @@ public void linenoTry() {
List<CatchClause> catchBlocks = tryStmt.getCatchClauses();
CatchClause catchClause = catchBlocks.get(0);
Scope catchVarBlock = catchClause.getBody();
Name catchVar = catchClause.getVarName();
Name catchVar = assertInstanceOf(Name.class, catchClause.getVarName());
AstNode finallyBlock = tryStmt.getFinallyBlock();
AstNode finallyStmt = (AstNode) finallyBlock.getFirstChild();

Expand Down
Loading