assignments in for-in/for-of header not allowed
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 Mar 2016 18:38:28 +0000 (18:38 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 Mar 2016 18:38:28 +0000 (18:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=155384

Reviewed by Darin Adler.

Source/JavaScriptCore:

This patch prevents assignments to the loop variable
in for in/of loops in all but one situation. The following
syntax is still allowed even though the spec prevents it:
```
for (var i = X in blah) ;
```
If the loop contains let/const, destructuring, or is a for-of
loop, we always throw a syntax error if there is an assignment.
We can do this with full backwards compatibility.
We only allow the above type of for-in loops because Oliver told
me that when he tried to make such programs illegal he ran
into real websites breaking.

This patch also removed the !::CreatesAST compile-time branch when checking
assignments to new.target. This was a dangerous thing for me
to introduce into our parser. There are times where ::CreatesAST
is true but we also want to check for syntax errors. For example,
when parsing the top-level AST of a program. Though this check
was technically correct, it's dangerous to have. It was correct
because we would always be reparsing the new.target assignment
because new.target is only allowed inside a function. That made it
so that (!::CreatesAST <=> we care about new.target assignment syntax errors).
But, (!::CreatesAST <=> we care about syntax error X) is not true in general.
I think it's safer to remove such code.

* parser/ASTBuilder.h:
(JSC::ASTBuilder::createNewTargetExpr):
(JSC::ASTBuilder::isNewTarget):
(JSC::ASTBuilder::createResolve):
* parser/Nodes.h:
(JSC::ExpressionNode::isBoolean):
(JSC::ExpressionNode::isSpreadExpression):
(JSC::ExpressionNode::isSuperNode):
(JSC::ExpressionNode::isNewTarget):
(JSC::ExpressionNode::isBytecodeIntrinsicNode):
* parser/Parser.cpp:
(JSC::Parser<LexerType>::parseForStatement):
(JSC::Parser<LexerType>::parseAssignmentExpression):
(JSC::Parser<LexerType>::parseUnaryExpression):

LayoutTests:

* js/parser-syntax-check-expected.txt:
* js/script-tests/parser-syntax-check.js:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@198144 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/js/parser-syntax-check-expected.txt
LayoutTests/js/script-tests/parser-syntax-check.js
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/parser/ASTBuilder.h
Source/JavaScriptCore/parser/Nodes.h
Source/JavaScriptCore/parser/Parser.cpp

index b23ae9c..c25d1f8 100644 (file)
@@ -1,3 +1,13 @@
+2016-03-14  Saam barati  <sbarati@apple.com>
+
+        assignments in for-in/for-of header not allowed
+        https://bugs.webkit.org/show_bug.cgi?id=155384
+
+        Reviewed by Darin Adler.
+
+        * js/parser-syntax-check-expected.txt:
+        * js/script-tests/parser-syntax-check.js:
+
 2016-03-14  Zalan Bujtas  <zalan@apple.com>
 
         Negative outline offset could break curved outline-style: auto
index c3f4c03..97ef09b 100644 (file)
@@ -526,6 +526,36 @@ PASS Invalid: "for (var (a) in b) { }"
 PASS Invalid: "function f() { for (var (a) in b) { } }"
 PASS Valid:   "for (var a = 7, b = c < d >= d ; f()[6]++ ; --i()[1]++ ) {}" with ReferenceError
 PASS Valid:   "function f() { for (var a = 7, b = c < d >= d ; f()[6]++ ; --i()[1]++ ) {} }"
+PASS Invalid: "for (var {a} = 20 in b) { }"
+PASS Invalid: "function f() { for (var {a} = 20 in b) { } }"
+PASS Invalid: "for (var {a} = 20 of b) { }"
+PASS Invalid: "function f() { for (var {a} = 20 of b) { } }"
+PASS Invalid: "for (var {a} = 20 in b) { }"
+PASS Invalid: "function f() { for (var {a} = 20 in b) { } }"
+PASS Valid:   "for (var i = 20 in b) { }" with ReferenceError
+PASS Valid:   "function f() { for (var i = 20 in b) { } }"
+PASS Invalid: "for (var i = 20 of b) { }"
+PASS Invalid: "function f() { for (var i = 20 of b) { } }"
+PASS Invalid: "for (var {i} = 20 of b) { }"
+PASS Invalid: "function f() { for (var {i} = 20 of b) { } }"
+PASS Invalid: "for (var [i] = 20 of b) { }"
+PASS Invalid: "function f() { for (var [i] = 20 of b) { } }"
+PASS Invalid: "for (let [i] = 20 of b) { }"
+PASS Invalid: "function f() { for (let [i] = 20 of b) { } }"
+PASS Invalid: "for (const [i] = 20 of b) { }"
+PASS Invalid: "function f() { for (const [i] = 20 of b) { } }"
+PASS Invalid: "for (const i = 20 of b) { }"
+PASS Invalid: "function f() { for (const i = 20 of b) { } }"
+PASS Invalid: "for (let i = 20 of b) { }"
+PASS Invalid: "function f() { for (let i = 20 of b) { } }"
+PASS Invalid: "for (let i = 20 in b) { }"
+PASS Invalid: "function f() { for (let i = 20 in b) { } }"
+PASS Invalid: "for (const i = 20 in b) { }"
+PASS Invalid: "function f() { for (const i = 20 in b) { } }"
+PASS Invalid: "for (const {i} = 20 in b) { }"
+PASS Invalid: "function f() { for (const {i} = 20 in b) { } }"
+PASS Invalid: "for (let {i} = 20 in b) { }"
+PASS Invalid: "function f() { for (let {i} = 20 in b) { } }"
 try statement
 PASS Invalid: "try { break } catch(e) {}"
 PASS Invalid: "function f() { try { break } catch(e) {} }"
index 7c8726d..845dae7 100644 (file)
@@ -344,6 +344,21 @@ valid("for (var a = (b in c) in d) break");
 invalid("for (var a = (b in c in d) break");
 invalid("for (var (a) in b) { }");
 valid  ("for (var a = 7, b = c < d >= d ; f()[6]++ ; --i()[1]++ ) {}");
+invalid("for (var {a} = 20 in b) { }");
+invalid("for (var {a} = 20 of b) { }");
+invalid("for (var {a} = 20 in b) { }");
+valid("for (var i = 20 in b) { }");
+invalid("for (var i = 20 of b) { }");
+invalid("for (var {i} = 20 of b) { }");
+invalid("for (var [i] = 20 of b) { }");
+invalid("for (let [i] = 20 of b) { }");
+invalid("for (const [i] = 20 of b) { }");
+invalid("for (const i = 20 of b) { }");
+invalid("for (let i = 20 of b) { }");
+invalid("for (let i = 20 in b) { }");
+invalid("for (const i = 20 in b) { }");
+invalid("for (const {i} = 20 in b) { }");
+invalid("for (let {i} = 20 in b) { }");
 
 debug  ("try statement");
 
index 1019d23..8822df6 100644 (file)
@@ -1,3 +1,50 @@
+2016-03-14  Saam barati  <sbarati@apple.com>
+
+        assignments in for-in/for-of header not allowed
+        https://bugs.webkit.org/show_bug.cgi?id=155384
+
+        Reviewed by Darin Adler.
+
+        This patch prevents assignments to the loop variable
+        in for in/of loops in all but one situation. The following
+        syntax is still allowed even though the spec prevents it:
+        ```
+        for (var i = X in blah) ;
+        ```
+        If the loop contains let/const, destructuring, or is a for-of
+        loop, we always throw a syntax error if there is an assignment.
+        We can do this with full backwards compatibility.
+        We only allow the above type of for-in loops because Oliver told
+        me that when he tried to make such programs illegal he ran
+        into real websites breaking.
+
+        This patch also removed the !::CreatesAST compile-time branch when checking
+        assignments to new.target. This was a dangerous thing for me
+        to introduce into our parser. There are times where ::CreatesAST
+        is true but we also want to check for syntax errors. For example,
+        when parsing the top-level AST of a program. Though this check
+        was technically correct, it's dangerous to have. It was correct
+        because we would always be reparsing the new.target assignment
+        because new.target is only allowed inside a function. That made it
+        so that (!::CreatesAST <=> we care about new.target assignment syntax errors).
+        But, (!::CreatesAST <=> we care about syntax error X) is not true in general.
+        I think it's safer to remove such code.
+
+        * parser/ASTBuilder.h:
+        (JSC::ASTBuilder::createNewTargetExpr):
+        (JSC::ASTBuilder::isNewTarget):
+        (JSC::ASTBuilder::createResolve):
+        * parser/Nodes.h:
+        (JSC::ExpressionNode::isBoolean):
+        (JSC::ExpressionNode::isSpreadExpression):
+        (JSC::ExpressionNode::isSuperNode):
+        (JSC::ExpressionNode::isNewTarget):
+        (JSC::ExpressionNode::isBytecodeIntrinsicNode):
+        * parser/Parser.cpp:
+        (JSC::Parser<LexerType>::parseForStatement):
+        (JSC::Parser<LexerType>::parseAssignmentExpression):
+        (JSC::Parser<LexerType>::parseUnaryExpression):
+
 2016-03-13  Joseph Pecoraro  <pecoraro@apple.com>
 
         Remove ENABLE(ES6_TEMPLATE_LITERAL_SYNTAX) guards
index e6cda90..947f028 100644 (file)
@@ -182,7 +182,7 @@ public:
         usesNewTarget();
         return new (m_parserArena) NewTargetNode(location);
     }
-    NO_RETURN_DUE_TO_CRASH bool isNewTarget(ExpressionNode*) { RELEASE_ASSERT_NOT_REACHED(); }
+    bool isNewTarget(ExpressionNode* node) { return node->isNewTarget(); }
     ExpressionNode* createResolve(const JSTokenLocation& location, const Identifier& ident, const JSTextPosition& start, const JSTextPosition& end)
     {
         if (m_vm->propertyNames->arguments == ident)
index 9fa3f57..07f672c 100644 (file)
@@ -174,6 +174,7 @@ namespace JSC {
         virtual bool isBoolean() const { return false; }
         virtual bool isSpreadExpression() const { return false; }
         virtual bool isSuperNode() const { return false; }
+        virtual bool isNewTarget() const { return false; }
         virtual bool isBytecodeIntrinsicNode() const { return false; }
 
         virtual void emitBytecodeInConditionContext(BytecodeGenerator&, Label*, Label*, FallThroughMode);
@@ -560,6 +561,7 @@ namespace JSC {
         NewTargetNode(const JSTokenLocation&);
 
     private:
+        bool isNewTarget() const final { return true; }
         RegisterID* emitBytecode(BytecodeGenerator&, RegisterID* = 0) override;
     };
 
index b30579d..c5561a4 100644 (file)
@@ -1154,12 +1154,8 @@ template <class TreeBuilder> TreeStatement Parser<LexerType>::parseForStatement(
         // Remainder of a standard for loop is handled identically
         if (match(SEMICOLON))
             goto standardForLoop;
-        
-        failIfFalse(declarations == 1, "can only declare a single variable in an enumeration");
-        failIfTrueIfStrict(forInInitializer, "Cannot use initialiser syntax in a strict mode enumeration");
 
-        if (forInInitializer)
-            failIfFalse(context.isBindingNode(forInTarget), "Cannot use initialiser syntax when binding to a pattern during enumeration");
+        failIfFalse(declarations == 1, "can only declare a single variable in an enumeration");
 
         // Handle for-in with var declaration
         JSTextPosition inLocation = tokenStartPosition();
@@ -1167,9 +1163,15 @@ template <class TreeBuilder> TreeStatement Parser<LexerType>::parseForStatement(
         if (!consume(INTOKEN)) {
             failIfFalse(match(IDENT) && *m_token.m_data.ident == m_vm->propertyNames->of, "Expected either 'in' or 'of' in enumeration syntax");
             isOfEnumeration = true;
-            failIfTrue(forInInitializer, "Cannot use initialiser syntax in a for-of enumeration");
             next();
         }
+        bool hasAnyAssignments = !!forInInitializer;
+        if (hasAnyAssignments) {
+            if (isOfEnumeration)
+                internalFailWithMessage(false, "Cannot assign to the loop variable inside a for-of loop header");
+            if (strictMode() || (isLetDeclaration || isConstDeclaration) || !context.isBindingNode(forInTarget))
+                internalFailWithMessage(false, "Cannot assign to the loop variable inside a for-in loop header");
+        }
         TreeExpression expr = parseExpression(context);
         failIfFalse(expr, "Expected expression to enumerate");
         JSTextPosition exprEnd = lastTokenEndPosition();
@@ -3044,10 +3046,8 @@ template <typename TreeBuilder> TreeExpression Parser<LexerType>::parseAssignmen
         }
         m_parserState.nonTrivialExpressionCount++;
         hadAssignment = true;
-        if (!TreeBuilder::CreatesAST) { // We only need to do this check with the syntax checker.
-            if (UNLIKELY(context.isNewTarget(lhs)))
-                internalFailWithMessage(false, "new.target can't be the left hand side of an assignment expression");
-        }
+        if (UNLIKELY(context.isNewTarget(lhs)))
+            internalFailWithMessage(false, "new.target can't be the left hand side of an assignment expression");
         context.assignmentStackAppend(assignmentStack, lhs, start, tokenStartPosition(), m_parserState.assignmentCount, op);
         start = tokenStartPosition();
         m_parserState.assignmentCount++;
@@ -4009,10 +4009,8 @@ template <class TreeBuilder> TreeExpression Parser<LexerType>::parseUnaryExpress
             failWithMessage("Cannot parse subexpression of ", operatorString(true, lastOperator), "operator");
         failWithMessage("Cannot parse member expression");
     }
-    if (!TreeBuilder::CreatesAST) { // We only need to do this check with the syntax checker.
-        if (UNLIKELY(lastOperator && context.isNewTarget(expr)))
-            internalFailWithMessage(false, "new.target can't come after a prefix operator");
-    }
+    if (UNLIKELY(lastOperator && context.isNewTarget(expr)))
+        internalFailWithMessage(false, "new.target can't come after a prefix operator");
     bool isEvalOrArguments = false;
     if (strictMode() && !m_syntaxAlreadyValidated) {
         if (context.isResolve(expr))
@@ -4021,10 +4019,8 @@ template <class TreeBuilder> TreeExpression Parser<LexerType>::parseUnaryExpress
     failIfTrueIfStrict(isEvalOrArguments && modifiesExpr, "Cannot modify '", m_parserState.lastIdentifier->impl(), "' in strict mode");
     switch (m_token.m_type) {
     case PLUSPLUS:
-        if (!TreeBuilder::CreatesAST) { // We only need to do this check with the syntax checker.
-            if (UNLIKELY(context.isNewTarget(expr)))
-                internalFailWithMessage(false, "new.target can't come before a postfix operator");
-        }
+        if (UNLIKELY(context.isNewTarget(expr)))
+            internalFailWithMessage(false, "new.target can't come before a postfix operator");
         m_parserState.nonTrivialExpressionCount++;
         m_parserState.nonLHSCount++;
         expr = context.makePostfixNode(location, expr, OpPlusPlus, subExprStart, lastTokenEndPosition(), tokenEndPosition());
@@ -4035,10 +4031,8 @@ template <class TreeBuilder> TreeExpression Parser<LexerType>::parseUnaryExpress
         next();
         break;
     case MINUSMINUS:
-        if (!TreeBuilder::CreatesAST) { // We only need to do this check with the syntax checker.
-            if (UNLIKELY(context.isNewTarget(expr)))
-                internalFailWithMessage(false, "new.target can't come before a postfix operator");
-        }
+        if (UNLIKELY(context.isNewTarget(expr)))
+            internalFailWithMessage(false, "new.target can't come before a postfix operator");
         m_parserState.nonTrivialExpressionCount++;
         m_parserState.nonLHSCount++;
         expr = context.makePostfixNode(location, expr, OpMinusMinus, subExprStart, lastTokenEndPosition(), tokenEndPosition());