Skip to content

Commit

Permalink
Fix methods in ASTValidator so that they are not too permissive.
Browse files Browse the repository at this point in the history 
Remove CAST and DEFAULT_VALUE from isValidAssignmentTarget.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=162540103
  • Loading branch information
@EatingW @blickly
EatingW authored and blickly committed Jul 20, 2017
1 parent f2d4742 commit 529fcd7
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 53 deletions.
146 changes: 97 additions & 49 deletions src/com/google/javascript/jscomp/AstValidator.java
View file
Expand Up @@ -840,57 +840,98 @@ 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());
}
}

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);
}
}
}
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -951,7 +998,7 @@ private void validateVarOrAssignmentTarget(Node n) {
validateChildCount(n, 1);
validateNameDeclarationHelper(n.getToken(), n);
} else {
validateAssignmentTarget(n);
validateLHS(n.getParent().getToken(), n);
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
}
}

Expand Down
4 changes: 0 additions & 4 deletions src/com/google/javascript/rhino/Node.java
View file
Expand Up @@ -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:
Expand Down
26 changes: 26 additions & 0 deletions test/com/google/javascript/jscomp/AstValidatorTest.java
View file
Expand Up @@ -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);
Expand Down

0 comments on commit 529fcd7

Please sign in to comment.