8151810: for-in iteration does not provide per-iteration scope
authorhannesw
Tue, 22 Mar 2016 14:23:16 +0100
changeset 36690 06b714373aa4
parent 36689 3370f6f942ff
child 36691 56a7257458e7
8151810: for-in iteration does not provide per-iteration scope Reviewed-by: attila, lagergren
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/CodeGenerator.java
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/FieldObjectCreator.java
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/Lower.java
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/Block.java
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/ForNode.java
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/parser/Parser.java
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/SetMethodCreator.java
nashorn/test/script/basic/es6/JDK-8151810.js
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/CodeGenerator.java	Mon Mar 21 12:38:23 2016 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/CodeGenerator.java	Tue Mar 22 14:23:16 2016 +0100
@@ -257,6 +257,9 @@
     //is this a rest of compilation
     private final int[] continuationEntryPoints;
 
+    // Scope object creators needed for for-of and for-in loops
+    private Deque<FieldObjectCreator<?>> scopeObjectCreators = new ArrayDeque<>();
+
     /**
      * Constructor.
      *
@@ -1297,6 +1300,9 @@
     private void popBlockScope(final Block block) {
         final Label breakLabel = block.getBreakLabel();
 
+        if (block.providesScopeCreator()) {
+            scopeObjectCreators.pop();
+        }
         if(!block.needsScope() || lc.isFunctionBody()) {
             emitBlockBreakLabel(breakLabel);
             return;
@@ -1812,6 +1818,14 @@
         }.store();
         body.accept(this);
 
+        if (forNode.needsScopeCreator() && lc.getCurrentBlock().providesScopeCreator()) {
+            // for-in loops with lexical declaration need a new scope for each iteration.
+            final FieldObjectCreator<?> creator = scopeObjectCreators.peek();
+            assert creator != null;
+            creator.createForInIterationScope(method);
+            method.storeCompilerConstant(SCOPE);
+        }
+
         if(method.isReachable()) {
             method._goto(continueLabel);
         }
@@ -1923,12 +1937,16 @@
              * Create a new object based on the symbols and values, generate
              * bootstrap code for object
              */
-            new FieldObjectCreator<Symbol>(this, tuples, true, hasArguments) {
+            final FieldObjectCreator<Symbol> creator = new FieldObjectCreator<Symbol>(this, tuples, true, hasArguments) {
                 @Override
                 protected void loadValue(final Symbol value, final Type type) {
                     method.load(value, type);
                 }
-            }.makeObject(method);
+            };
+            creator.makeObject(method);
+            if (block.providesScopeCreator()) {
+                scopeObjectCreators.push(creator);
+            }
             // program function: merge scope into global
             if (isFunctionBody && function.isProgram()) {
                 method.invoke(ScriptRuntime.MERGE_SCOPE);
@@ -4480,7 +4498,7 @@
                     final Symbol symbol = node.getSymbol();
                     assert symbol != null;
                     if (symbol.isScope()) {
-                        final int flags = getScopeCallSiteFlags(symbol);
+                        final int flags = getScopeCallSiteFlags(symbol) | (node.isDeclaredHere() ? CALLSITE_DECLARE : 0);
                         if (isFastScope(symbol)) {
                             storeFastScopeVar(symbol, flags);
                         } else {
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/FieldObjectCreator.java	Mon Mar 21 12:38:23 2016 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/FieldObjectCreator.java	Tue Mar 22 14:23:16 2016 +0100
@@ -120,6 +120,25 @@
         }
     }
 
+    /**
+     * Create a scope for a for-in/of loop as defined in ES6 13.7.5.13 step 5.g.iii
+     *
+     * @param method the method emitter
+     */
+    void createForInIterationScope(final MethodEmitter method) {
+        assert fieldObjectClass != null;
+        assert isScope();
+        assert getMap() != null;
+
+        final String className = getClassName();
+        method._new(fieldObjectClass).dup();
+        loadMap(method); //load the map
+        loadScope(method);
+        // We create a scope identical to the currently active one, so use its parent as our parent
+        method.invoke(ScriptObject.GET_PROTO);
+        method.invoke(constructorNoLookup(className, PropertyMap.class, ScriptObject.class));
+    }
+
     @Override
     public void populateRange(final MethodEmitter method, final Type objectType, final int objectSlot, final int start, final int end) {
         method.load(objectType, objectSlot);
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/Lower.java	Mon Mar 21 12:38:23 2016 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/Lower.java	Tue Mar 22 14:23:16 2016 +0100
@@ -97,6 +97,7 @@
 final class Lower extends NodeOperatorVisitor<BlockLexicalContext> implements Loggable {
 
     private final DebugLogger log;
+    private final boolean es6;
 
     // Conservative pattern to test if element names consist of characters valid for identifiers.
     // This matches any non-zero length alphanumeric string including _ and $ and not starting with a digit.
@@ -144,6 +145,7 @@
         });
 
         this.log = initLogger(compiler.getContext());
+        this.es6 = compiler.getScriptEnvironment()._es6;
     }
 
     @Override
@@ -257,8 +259,9 @@
         }
 
         newForNode = checkEscape(newForNode);
-        if(newForNode.isForIn()) {
-            // Wrap it in a block so its internally created iterator is restricted in scope
+        if(!es6 && newForNode.isForIn()) {
+            // Wrap it in a block so its internally created iterator is restricted in scope, unless we are running
+            // in ES6 mode, in which case the parser already created a block to capture let/const declarations.
             addStatementEnclosedInBlock(newForNode);
         } else {
             addStatement(newForNode);
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/Block.java	Mon Mar 21 12:38:23 2016 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/Block.java	Tue Mar 22 14:23:16 2016 +0100
@@ -462,6 +462,19 @@
         return next;
     }
 
+    /**
+     * Determine whether this block needs to provide its scope object creator for use by its child nodes.
+     * This is only necessary for synthetic parent blocks of for-in loops with lexical declarations.
+     *
+     * @see ForNode#needsScopeCreator()
+     * @return true if child nodes need access to this block's scope creator
+     */
+    public boolean providesScopeCreator() {
+        return needsScope() && isSynthetic()
+                && (getLastStatement() instanceof ForNode)
+                && ((ForNode) getLastStatement()).needsScopeCreator();
+    }
+
     @Override
     public boolean isBreakableWithoutLabel() {
         return false;
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/ForNode.java	Mon Mar 21 12:38:23 2016 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/ForNode.java	Tue Mar 22 14:23:16 2016 +0100
@@ -274,4 +274,15 @@
     public boolean hasPerIterationScope() {
         return (flags & PER_ITERATION_SCOPE) != 0;
     }
+
+    /**
+     * Returns true if this for-node needs the scope creator of its containing block to create
+     * per-iteration scope. This is only true for for-in loops with lexical declarations.
+     *
+     * @see Block#providesScopeCreator()
+     * @return true if the containing block's scope object creator is required in codegen
+     */
+    public boolean needsScopeCreator() {
+        return isForIn() && hasPerIterationScope();
+    }
 }
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/parser/Parser.java	Mon Mar 21 12:38:23 2016 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/parser/Parser.java	Tue Mar 22 14:23:16 2016 +0100
@@ -1104,12 +1104,14 @@
                 } finally {
                     defaultNames.pop();
                 }
-            } else if (varType == CONST) {
+            } else if (varType == CONST && isStatement) {
                 throw error(AbstractParser.message("missing.const.assignment", name.getName()));
             }
 
+            // Only set declaration flag on lexically scoped let/const as it adds runtime overhead.
+            final IdentNode actualName = varType == LET || varType == CONST ? name.setIsDeclaredHere() : name;
             // Allocate var node.
-            final VarNode var = new VarNode(varLine, varToken, sourceOrder, finish, name.setIsDeclaredHere(), init, varFlags);
+            final VarNode var = new VarNode(varLine, varToken, sourceOrder, finish, actualName, init, varFlags);
             vars.add(var);
             appendStatement(var);
 
@@ -1247,7 +1249,6 @@
 
             expect(LPAREN);
 
-
             switch (type) {
             case VAR:
                 // Var declaration captured in for outer block.
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/SetMethodCreator.java	Mon Mar 21 12:38:23 2016 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/SetMethodCreator.java	Tue Mar 22 14:23:16 2016 +0100
@@ -145,8 +145,7 @@
         final boolean isStrict  = NashornCallSiteDescriptor.isStrict(desc);
         final MethodHandle methodHandle;
 
-        if (NashornCallSiteDescriptor.isDeclaration(desc)) {
-            assert property.needsDeclaration();
+        if (NashornCallSiteDescriptor.isDeclaration(desc) && property.needsDeclaration()) {
             // This is a LET or CONST being declared. The property is already there but flagged as needing declaration.
             // We create a new PropertyMap with the flag removed. The map is installed with a fast compare-and-set
             // method if the pre-callsite map is stable (which should be the case for function scopes except for
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/test/script/basic/es6/JDK-8151810.js	Tue Mar 22 14:23:16 2016 +0100
@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ * 
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ * 
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ * 
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ * 
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * JDK-8151810: for-in iteration does not provide per-iteration scope
+ *
+ * @test
+ * @run
+ * @option --language=es6
+ */
+
+"use strict";
+
+let array = ["a", "b", "c"];
+let funcs = [];
+
+for (let i in array) {
+    funcs.push(function() { return array[i]; });
+}
+
+Assert.assertEquals(funcs.length, 3);
+
+for (let i = 0; i < 3; i++) {
+    Assert.assertEquals(funcs[i](), array[i]);
+}
+
+funcs = [];
+
+for (let i in array) {
+    for (let j in array) {
+        for (let k in array) {
+            funcs.push(function () {
+                return array[i] + array[j] + array[k];
+            });
+        }
+    }
+}
+
+Assert.assertEquals(funcs.length, 3 * 3 * 3);
+let count = 0;
+
+for (let i = 0; i < 3; i++) {
+    for (let j = 0; j < 3; j++) {
+        for (let k = 0; k < 3; k++) {
+            Assert.assertEquals(funcs[count++](), array[i] + array[j] + array[k]);
+        }
+    }
+}
+