diff --git a/src/com/google/javascript/jscomp/AstValidator.java b/src/com/google/javascript/jscomp/AstValidator.java index 6ade15b6e00..646e70dc182 100644 --- a/src/com/google/javascript/jscomp/AstValidator.java +++ b/src/com/google/javascript/jscomp/AstValidator.java @@ -840,38 +840,79 @@ private void validateNameDeclarationHelper(Token type, Node n) { private void validateNameDeclarationChild(Token type, Node n) { if (n.isName()) { - validateLHS(type, n); + // Don't use validateName here since this NAME node may have + // a child. + validateNonEmptyString(n); + validateMaximumChildCount(n, 1); + if (n.hasChildren()) { + validateExpression(n.getFirstChild()); + } } else if (n.isDestructuringLhs()) { - validateLHS(type, n.getFirstChild()); + validateDestructuringLhs(type, n); } else { violation("Invalid child for " + type + " node", n); } } + private void validateDestructuringLhs(Token type, Node n) { + validateChildCountIn(n, 1, 2); + Node c = n.getFirstChild(); + switch (c.getToken()) { + case ARRAY_PATTERN: + validateArrayPattern(type, c); + break; + case OBJECT_PATTERN: + validateObjectPattern(type, c); + break; + default: + violation("Invalid destructuring lhs first child for " + type + " node", n); + } + if (n.hasTwoChildren()) { + validateExpression(n.getSecondChild()); + } + } + /** * @param contextType A {@link Token} constant value indicating that {@code n} should be validated * appropriately for a descendant of a {@link Node} of this type. * @param n */ private void validateLHS(Token contextType, Node n) { - if (n.isName()) { - // Don't use validateName here since this NAME node may have - // a child. - validateNonEmptyString(n); - validateMaximumChildCount(n, 1); - if (n.hasChildren()) { - validateExpression(n.getFirstChild()); - } - } else if (n.isArrayPattern()) { - validateArrayPattern(contextType, n); - } else if (n.isObjectPattern()) { - validateObjectPattern(contextType, n); - } else if (n.isDefaultValue()) { - validateDefaultValue(contextType, n); - } else if (n.isComputedProp()) { - validateObjectPatternComputedPropKey(contextType, n); - } else { + switch (n.getToken()) { + case NAME: + validateName(n); + break; + case ARRAY_PATTERN: + validateArrayPattern(contextType, n); + break; + case OBJECT_PATTERN: + validateObjectPattern(contextType, n); + break; + case GETPROP: + case GETELEM: + validateGetPropGetElemInLHS(contextType, n); + break; + default: + violation("Invalid child for " + contextType + " node", n); + } + } + + private void validateGetPropGetElemInLHS(Token contextType, Node n) { + if (contextType == Token.CONST || contextType == Token.LET || contextType == Token.VAR + || contextType == Token.PARAM_LIST) { violation("Invalid child for " + contextType + " node", n); + return; + } + switch (n.getToken()) { + case GETPROP: + validateGetProp(n); + break; + case GETELEM: + validateBinaryOp(n); + break; + default: + throw new IllegalStateException( + "Expected GETPROP or GETELEM but instead got node " + n.getToken()); } } @@ -879,18 +920,18 @@ private void validateArrayPattern(Token type, Node n) { validateFeature(Feature.DESTRUCTURING, n); validateNodeType(Token.ARRAY_PATTERN, n); for (Node c = n.getFirstChild(); c != null; c = c.getNext()) { - // When the array pattern is a direct child of a var/let/const node, - // the last element is the RHS of the assignment. - if (c == n.getLastChild() && NodeUtil.isNameDeclaration(n.getParent())) { - validateExpression(c); - } else if (c.isRest()) { - validateRest(type, c); - } else if (c.isEmpty()) { - validateChildless(c); - } else { - // The members of the array pattern can be simple names, - // or nested array/object patterns, e.g. "var [a,[b,c]]=[1,[2,3]];" - validateLHS(type, c); + switch (c.getToken()) { + case DEFAULT_VALUE: + validateDefaultValue(type, c); + break; + case REST: + validateRest(type, c); + break; + case EMPTY: + validateChildless(c); + break; + default: + validateLHS(type, c); } } } @@ -899,15 +940,21 @@ private void validateObjectPattern(Token type, Node n) { validateFeature(Feature.DESTRUCTURING, n); validateNodeType(Token.OBJECT_PATTERN, n); for (Node c = n.getFirstChild(); c != null; c = c.getNext()) { - // When the object pattern is a direct child of a var/let/const node, - // the last element is the RHS of the assignment. - if (c == n.getLastChild() && NodeUtil.isNameDeclaration(n.getParent())) { - validateExpression(c); - } else if (c.isStringKey()) { - validateObjectPatternStringKey(type, c); - } else { - // Nested destructuring pattern. - validateLHS(type, c); + switch (c.getToken()) { + case STRING_KEY: + validateObjectPatternStringKey(type, c); + break; + case OBJECT_PATTERN: + validateObjectPattern(type, c); + break; + case DEFAULT_VALUE: + validateDefaultValue(type, c); + break; + case COMPUTED_PROP: + validateObjectPatternComputedPropKey(type, c); + break; + default: + violation("Invalid object pattern child for " + type + " node", n); } } } @@ -951,7 +998,7 @@ private void validateVarOrAssignmentTarget(Node n) { validateChildCount(n, 1); validateNameDeclarationHelper(n.getToken(), n); } else { - validateAssignmentTarget(n); + validateLHS(n.getParent().getToken(), n); } } @@ -1120,16 +1167,10 @@ private void validateChildless(Node n) { private void validateAssignmentExpression(Node n) { validateChildCount(n); - validateAssignmentTarget(n.getFirstChild()); + validateLHS(n.getToken(), n.getFirstChild()); validateExpression(n.getLastChild()); } - private void validateAssignmentTarget(Node n) { - if (!n.isValidAssignmentTarget()) { - violation("Expected assignment target expression but was " + n.getToken(), n); - } - } - private void validateGetProp(Node n) { validateNodeType(Token.GETPROP, n); validateChildCount(n); @@ -1258,7 +1299,14 @@ private void validateObjectPatternStringKey(Token type, Node n) { validateChildCountIn(n, 0, 1); if (n.hasOneChild()) { - validateLHS(type, n.getFirstChild()); + Node c = n.getFirstChild(); + switch (c.getToken()) { + case DEFAULT_VALUE: + validateDefaultValue(type, c); + break; + default: + validateLHS(type, c); + } } } diff --git a/src/com/google/javascript/rhino/Node.java b/src/com/google/javascript/rhino/Node.java index 10b5f28a534..d8df6427407 100644 --- a/src/com/google/javascript/rhino/Node.java +++ b/src/com/google/javascript/rhino/Node.java @@ -2088,10 +2088,6 @@ public boolean isUnscopedQualifiedName() { public boolean isValidAssignmentTarget() { switch (this.getToken()) { - // TODO(tbreisacher): Remove CAST from this list, and disallow - // the cryptic case from cl/41958159. - case CAST: - case DEFAULT_VALUE: case NAME: case GETPROP: case GETELEM: diff --git a/test/com/google/javascript/jscomp/AstValidatorTest.java b/test/com/google/javascript/jscomp/AstValidatorTest.java index 1f2c2f8363a..b771b55eb18 100644 --- a/test/com/google/javascript/jscomp/AstValidatorTest.java +++ b/test/com/google/javascript/jscomp/AstValidatorTest.java @@ -164,6 +164,32 @@ public void testAwaitExpressionNoFunction() { expectInvalid(n, Check.EXPRESSION); } + public void testValidDestructuringAssignment() { + setAcceptedLanguage(LanguageMode.ECMASCRIPT_2015); + valid("var [x, ...y] = obj;"); + valid("([...this.x] = obj);"); + valid("var {a:b} = obj;"); + valid("({a:this.b} = obj);"); + } + + public void testInvalidDestructuringAssignment() { + setAcceptedLanguage(LanguageMode.ECMASCRIPT_2015); + + Node n = IR.assign( + new Node(Token.OBJECT_PATTERN, new Node(Token.ARRAY_PATTERN)), IR.objectlit()); + expectInvalid(n, Check.EXPRESSION); + + n = IR.assign( + new Node(Token.ARRAY_PATTERN, IR.computedProp(IR.string("x"), IR.number(1))), + IR.objectlit()); + expectInvalid(n, Check.EXPRESSION); + + Node stringkey = IR.stringKey("x"); + stringkey.addChildToFront(IR.computedProp(IR.string("x"), IR.number(1))); + n = IR.assign(new Node(Token.OBJECT_PATTERN, stringkey), IR.objectlit()); + expectInvalid(n, Check.EXPRESSION); + } + private void valid(String code) { testSame(code); assertTrue(lastCheckWasValid);