-
Notifications
You must be signed in to change notification settings - Fork 903
Implement support for destructuring in catch
#2195
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you mean here.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
|
@@ -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); | ||
| } | ||
| } | ||
|
|
||
| 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"); | ||
| } | ||
| } |
There was a problem hiding this comment.
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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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