From b184f12aafe1322d3b0f8fa861641b81fc5fb0bc Mon Sep 17 00:00:00 2001
From: Yonatan Goldschmidt <yon.goldschmidt@gmail.com>
Date: Tue, 5 May 2020 01:58:53 +0300
Subject: [PATCH] LibJS: Limit scope of 'for' loop variables

This required 2 changes:
1. In the parser, create a new variable scope, so the variable is
   declared in it instead of the scope in which the 'for' is found.
2. On execute, push the variable into the newly created block. Existing
   code created an empty block (no variables, no arguments) which
   allows Interpreter::enter_scope() to skip the creation of a new
   environment, therefore when the variable initializer is executed, it
   sets the variable to the outer scope. By attaching the variable to
   the new block, the block gets a new environment.

This is only needed for 'let' / 'const' declarations, since 'var'
declarations are expected to leak.

Fixes: #2103
---
 Libraries/LibJS/AST.cpp             |  3 +++
 Libraries/LibJS/Parser.cpp          | 10 ++++++++++
 Libraries/LibJS/Tests/for-scopes.js | 22 ++++++++++++++++++++++
 3 files changed, 35 insertions(+)
 create mode 100644 Libraries/LibJS/Tests/for-scopes.js

diff --git a/Libraries/LibJS/AST.cpp b/Libraries/LibJS/AST.cpp
index 0a758197c0..c9538e61dd 100644
--- a/Libraries/LibJS/AST.cpp
+++ b/Libraries/LibJS/AST.cpp
@@ -251,6 +251,9 @@ Value ForStatement::execute(Interpreter& interpreter) const
 
     if (m_init && m_init->is_variable_declaration() && static_cast<const VariableDeclaration*>(m_init.ptr())->declaration_kind() != DeclarationKind::Var) {
         wrapper = create_ast_node<BlockStatement>();
+        NonnullRefPtrVector<VariableDeclaration> decls;
+        decls.append(*static_cast<const VariableDeclaration*>(m_init.ptr()));
+        wrapper->add_variables(decls);
         interpreter.enter_scope(*wrapper, {}, ScopeType::Block);
     }
 
diff --git a/Libraries/LibJS/Parser.cpp b/Libraries/LibJS/Parser.cpp
index 9ca17144a5..454c94922a 100644
--- a/Libraries/LibJS/Parser.cpp
+++ b/Libraries/LibJS/Parser.cpp
@@ -1133,6 +1133,7 @@ NonnullRefPtr<ForStatement> Parser::parse_for_statement()
     consume(TokenType::ParenOpen);
 
     bool first_semicolon_consumed = false;
+    bool in_scope = false;
     RefPtr<ASTNode> init;
     switch (m_parser_state.m_current_token.type()) {
     case TokenType::Semicolon:
@@ -1141,6 +1142,11 @@ NonnullRefPtr<ForStatement> Parser::parse_for_statement()
         if (match_expression()) {
             init = parse_expression(0);
         } else if (match_variable_declaration()) {
+            if (m_parser_state.m_current_token.type() != TokenType::Var) {
+                m_parser_state.m_let_scopes.append(NonnullRefPtrVector<VariableDeclaration>());
+                in_scope = true;
+            }
+
             init = parse_variable_declaration();
             first_semicolon_consumed = true;
         } else {
@@ -1176,6 +1182,10 @@ NonnullRefPtr<ForStatement> Parser::parse_for_statement()
 
     auto body = parse_statement();
 
+    if (in_scope) {
+        m_parser_state.m_let_scopes.take_last();
+    }
+
     return create_ast_node<ForStatement>(move(init), move(test), move(update), move(body));
 }
 
diff --git a/Libraries/LibJS/Tests/for-scopes.js b/Libraries/LibJS/Tests/for-scopes.js
new file mode 100644
index 0000000000..00c6a96c88
--- /dev/null
+++ b/Libraries/LibJS/Tests/for-scopes.js
@@ -0,0 +1,22 @@
+load("test-common.js");
+
+try {
+    for (var v = 5; false; );
+    assert(v == 5);
+
+    const options = {error: ReferenceError};
+
+    assertThrowsError(() => {
+        for (let l = 5; false; );
+        l;
+    }, options);
+
+    assertThrowsError(() => {
+        for (const c = 5; false; );
+        c;
+    }, options)
+
+    console.log("PASS");
+} catch (e) {
+    console.log("FAIL: " + e);
+}