8013419: Streamline handling of with and eval

Reviewed-by: hannesw, lagergren

nashorn/src/jdk/nashorn/internal/codegen/Attr.java

@@ -79,6 +79,7 @@ import jdk.nashorn.internal.ir.TernaryNode;
 import jdk.nashorn.internal.ir.TryNode;
 import jdk.nashorn.internal.ir.UnaryNode;
 import jdk.nashorn.internal.ir.VarNode;
+import jdk.nashorn.internal.ir.WithNode;
 import jdk.nashorn.internal.ir.visitor.NodeOperatorVisitor;
 import jdk.nashorn.internal.ir.visitor.NodeVisitor;
 import jdk.nashorn.internal.parser.TokenType;
@@ -495,10 +496,8 @@ final class Attr extends NodeOperatorVisitor {
             }
 
             identNode.setSymbol(symbol);
-            // non-local: we need to put symbol in scope (if it isn't already)
-            if (!isLocal(lc.getCurrentFunction(), symbol) && !symbol.isScope()) {
-                Symbol.setSymbolIsScope(lc, symbol);
-            }
+            // if symbol is non-local or we're in a with block, we need to put symbol in scope (if it isn't already)
+            maybeForceScope(symbol);
         } else {
             LOG.info("No symbol exists. Declare undefined: ", symbol);
             symbol = defineSymbol(block, name, IS_GLOBAL, identNode);
@@ -520,6 +519,50 @@ final class Attr extends NodeOperatorVisitor {
         return false;
     }
 
+    /**
+     * If the symbol isn't already a scope symbol, and it is either not local to the current function, or it is being
+     * referenced from within a with block, we force it to be a scope symbol.
+     * @param symbol the symbol that might be scoped
+     */
+    private void maybeForceScope(final Symbol symbol) {
+        if(!symbol.isScope() && symbolNeedsToBeScope(symbol)) {
+            Symbol.setSymbolIsScope(getLexicalContext(), symbol);
+        }
+    }
+
+    private boolean symbolNeedsToBeScope(Symbol symbol) {
+        if(symbol.isThis() || symbol.isInternal()) {
+            return false;
+        }
+        boolean previousWasBlock = false;
+        for(final Iterator<LexicalContextNode> it = getLexicalContext().getAllNodes(); it.hasNext();) {
+            final LexicalContextNode node = it.next();
+            if(node instanceof FunctionNode) {
+                // We reached the function boundary without seeing a definition for the symbol - it needs to be in
+                // scope.
+                return true;
+            } else if(node instanceof WithNode) {
+                if(previousWasBlock) {
+                    // We reached a WithNode; the symbol must be scoped. Note that if the WithNode was not immediately
+                    // preceded by a block, this means we're currently processing its expression, not its body,
+                    // therefore it doesn't count.
+                    return true;
+                }
+                previousWasBlock = false;
+            } else if(node instanceof Block) {
+                if(((Block)node).getExistingSymbol(symbol.getName()) == symbol) {
+                    // We reached the block that defines the symbol without reaching either the function boundary, or a
+                    // WithNode. The symbol need not be scoped.
+                    return false;
+                }
+                previousWasBlock = true;
+            } else {
+                previousWasBlock = false;
+            }
+        }
+        throw new AssertionError();
+    }
+
     private void setBlockScope(final String name, final Symbol symbol) {
         assert symbol != null;
         if (symbol.isGlobal()) {
@@ -963,18 +1006,17 @@ final class Attr extends NodeOperatorVisitor {
         final Node lhs = binaryNode.lhs();
 
         if (lhs instanceof IdentNode) {
-            final LexicalContext lc    = getLexicalContext();
-            final Block          block = lc.getCurrentBlock();
-            final IdentNode      ident = (IdentNode)lhs;
-            final String         name  = ident.getName();
+            final Block     block = getLexicalContext().getCurrentBlock();
+            final IdentNode ident = (IdentNode)lhs;
+            final String    name  = ident.getName();
 
             Symbol symbol = findSymbol(block, name);
 
             if (symbol == null) {
                 symbol = defineSymbol(block, name, IS_GLOBAL, ident);
                 binaryNode.setSymbol(symbol);
-            } else if (!isLocal(lc.getCurrentFunction(), symbol)) {
-                Symbol.setSymbolIsScope(lc, symbol);
+            } else {
+                maybeForceScope(symbol);
             }
 
             addLocalDef(name);

nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java

@@ -87,6 +87,7 @@ import jdk.nashorn.internal.ir.IdentNode;
 import jdk.nashorn.internal.ir.IfNode;
 import jdk.nashorn.internal.ir.IndexNode;
 import jdk.nashorn.internal.ir.LexicalContext;
+import jdk.nashorn.internal.ir.LexicalContextNode;
 import jdk.nashorn.internal.ir.LineNumberNode;
 import jdk.nashorn.internal.ir.LiteralNode;
 import jdk.nashorn.internal.ir.LiteralNode.ArrayLiteralNode;
@@ -201,6 +202,7 @@ final class CodeGenerator extends NodeOperatorVisitor {
      * @param compiler
      */
     CodeGenerator(final Compiler compiler) {
+        super(new DynamicScopeTrackingLexicalContext());
         this.compiler      = compiler;
         this.callSiteFlags = compiler.getEnv()._callsite_flags;
     }
@@ -284,6 +286,53 @@ final class CodeGenerator extends NodeOperatorVisitor {
         }
     }
 
+    /**
+     * A lexical context that also tracks if we have any dynamic scopes in the context. Such scopes can have new
+     * variables introduced into them at run time - a with block or a function directly containing an eval call.
+     */
+    private static class DynamicScopeTrackingLexicalContext extends LexicalContext {
+        int dynamicScopeCount = 0;
+
+        @Override
+        public <T extends LexicalContextNode> T push(T node) {
+            if(isDynamicScopeBoundary(node)) {
+                ++dynamicScopeCount;
+            }
+            return super.push(node);
+        }
+
+        @Override
+        public <T extends LexicalContextNode> T pop(T node) {
+            final T popped = super.pop(node);
+            if(isDynamicScopeBoundary(popped)) {
+                --dynamicScopeCount;
+            }
+            return popped;
+        }
+
+        private boolean isDynamicScopeBoundary(LexicalContextNode node) {
+            if(node instanceof Block) {
+                // Block's immediate parent is a with node. Note we aren't testing for a WithNode, as that'd capture
+                // processing of WithNode.expression too, but it should be unaffected.
+                return !isEmpty() && peek() instanceof WithNode;
+            } else if(node instanceof FunctionNode) {
+                // Function has a direct eval in it (so a top-level "var ..." in the eval code can introduce a new
+                // variable into the function's scope), and it isn't strict (as evals in strict functions get an
+                // isolated scope).
+                return isFunctionDynamicScope((FunctionNode)node);
+            }
+            return false;
+        }
+    }
+
+    boolean inDynamicScope() {
+        return ((DynamicScopeTrackingLexicalContext)getLexicalContext()).dynamicScopeCount > 0;
+    }
+
+    static boolean isFunctionDynamicScope(FunctionNode fn) {
+        return fn.hasEval() && !fn.isStrict();
+    }
+
     /**
      * Check if this symbol can be accessed directly with a putfield or getfield or dynamic load
      *
@@ -291,17 +340,46 @@ final class CodeGenerator extends NodeOperatorVisitor {
      * @return true if fast scope
      */
     private boolean isFastScope(final Symbol symbol) {
-        if (!symbol.isScope() || !getLexicalContext().getDefiningBlock(symbol).needsScope()) {
+        if(!symbol.isScope()) {
             return false;
         }
-        // Allow fast scope access if no function contains with or eval
-        for (final Iterator<FunctionNode> it = getLexicalContext().getFunctions(); it.hasNext();) {
-            final FunctionNode func = it.next();
-            if (func.hasWith() || func.hasEval()) {
-                return false;
+        final LexicalContext lc = getLexicalContext();
+        if(!inDynamicScope()) {
+            // If there's no with or eval in context, and the symbol is marked as scoped, it is fast scoped. Such a
+            // symbol must either be global, or its defining block must need scope.
+            assert symbol.isGlobal() || lc.getDefiningBlock(symbol).needsScope() : symbol.getName();
+            return true;
+        }
+        if(symbol.isGlobal()) {
+            // Shortcut: if there's a with or eval in context, globals can't be fast scoped
+            return false;
+        }
+        // Otherwise, check if there's a dynamic scope between use of the symbol and its definition
+        final String name = symbol.getName();
+        boolean previousWasBlock = false;
+        for (final Iterator<LexicalContextNode> it = lc.getAllNodes(); it.hasNext();) {
+            final LexicalContextNode node = it.next();
+            if(node instanceof Block) {
+                // If this block defines the symbol, then we can fast scope the symbol.
+                final Block block = (Block)node;
+                if(block.getExistingSymbol(name) == symbol) {
+                    assert block.needsScope();
+                    return true;
+                }
+                previousWasBlock = true;
+            } else {
+                if((node instanceof WithNode && previousWasBlock) || (node instanceof FunctionNode && isFunctionDynamicScope((FunctionNode)node))) {
+                    // If we hit a scope that can have symbols introduced into it at run time before finding the defining
+                    // block, the symbol can't be fast scoped. A WithNode only counts if we've immediately seen a block
+                    // before - its block. Otherwise, we are currently processing the WithNode's expression, and that's
+                    // obviously not subjected to introducing new symbols.
+                    return false;
+                }
+                previousWasBlock = false;
             }
         }
-        return true;
+        // Should've found the symbol defined in a block
+        throw new AssertionError();
     }
 
     private MethodEmitter loadSharedScopeVar(final Type valueType, final Symbol symbol, final int flags) {
@@ -671,7 +749,7 @@ final class CodeGenerator extends NodeOperatorVisitor {
                         evalCall(node, flags);
                     } else if (useCount <= SharedScopeCall.FAST_SCOPE_CALL_THRESHOLD
                             || (!isFastScope(symbol) && useCount <= SharedScopeCall.SLOW_SCOPE_CALL_THRESHOLD)
-                            || callNode.inWithBlock()) {
+                            || CodeGenerator.this.inDynamicScope()) {
                         scopeCall(node, flags);
                     } else {
                         sharedScopeCall(node, flags);
@@ -2114,9 +2192,11 @@ final class CodeGenerator extends NodeOperatorVisitor {
     }
 
     private void closeWith() {
-        method.loadCompilerConstant(SCOPE);
-        method.invoke(ScriptRuntime.CLOSE_WITH);
-        method.storeCompilerConstant(SCOPE);
+        if(method.hasScope()) {
+            method.loadCompilerConstant(SCOPE);
+            method.invoke(ScriptRuntime.CLOSE_WITH);
+            method.storeCompilerConstant(SCOPE);
+        }
     }
 
     @Override
@@ -2124,38 +2204,58 @@ final class CodeGenerator extends NodeOperatorVisitor {
         final Node expression = withNode.getExpression();
         final Node body       = withNode.getBody();
 
-        final Label tryLabel   = new Label("with_try");
-        final Label endLabel   = new Label("with_end");
-        final Label catchLabel = new Label("with_catch");
-        final Label exitLabel  = new Label("with_exit");
+        // It is possible to have a "pathological" case where the with block does not reference *any* identifiers. It's
+        // pointless, but legal. In that case, if nothing else in the method forced the assignment of a slot to the
+        // scope object, its' possible that it won't have a slot assigned. In this case we'll only evaluate expression
+        // for its side effect and visit the body, and not bother opening and closing a WithObject.
+        final boolean hasScope = method.hasScope();
 
-        method.label(tryLabel);
-
-        method.loadCompilerConstant(SCOPE);
-        load(expression);
-
-        assert expression.getType().isObject() : "with expression needs to be object: " + expression;
-
-        method.invoke(ScriptRuntime.OPEN_WITH);
-        method.storeCompilerConstant(SCOPE);
-
-        body.accept(this);
-
-        if (!body.isTerminal()) {
-            closeWith();
-            method._goto(exitLabel);
+        final Label tryLabel;
+        if(hasScope) {
+            tryLabel = new Label("with_try");
+            method.label(tryLabel);
+            method.loadCompilerConstant(SCOPE);
+        } else {
+            tryLabel = null;
         }
 
-        method.label(endLabel);
+        load(expression);
+        assert expression.getType().isObject() : "with expression needs to be object: " + expression;
 
-        method._catch(catchLabel);
-        closeWith();
-        method.athrow();
+        if(hasScope) {
+            // Construct a WithObject if we have a scope
+            method.invoke(ScriptRuntime.OPEN_WITH);
+            method.storeCompilerConstant(SCOPE);
+        } else {
+            // We just loaded the expression for its side effect; discard it
+            method.pop();
+        }
 
-        method.label(exitLabel);
 
-        method._try(tryLabel, endLabel, catchLabel);
+        // Always process body
+        body.accept(this);
 
+        if(hasScope) {
+            // Ensure we always close the WithObject
+            final Label endLabel   = new Label("with_end");
+            final Label catchLabel = new Label("with_catch");
+            final Label exitLabel  = new Label("with_exit");
+
+            if (!body.isTerminal()) {
+                closeWith();
+                method._goto(exitLabel);
+            }
+
+            method.label(endLabel);
+
+            method._catch(catchLabel);
+            closeWith();
+            method.athrow();
+
+            method.label(exitLabel);
+
+            method._try(tryLabel, endLabel, catchLabel);
+        }
         return false;
     }
 

nashorn/src/jdk/nashorn/internal/codegen/MethodEmitter.java

@@ -818,7 +818,7 @@ public class MethodEmitter implements Emitter {
     }
 
     /**
-     * Push an local variable to the stack. If the symbol representing
+     * Push a local variable to the stack. If the symbol representing
      * the local variable doesn't have a slot, this is a NOP
      *
      * @param symbol the symbol representing the local variable.
@@ -900,6 +900,15 @@ public class MethodEmitter implements Emitter {
         return functionNode.getBody().getExistingSymbol(cc.symbolName());
     }
 
+    /**
+     * True if this method has a slot allocated for the scope variable (meaning, something in the method actually needs
+     * the scope).
+     * @return if this method has a slot allocated for the scope variable.
+     */
+    boolean hasScope() {
+        return compilerConstant(SCOPE).hasSlot();
+    }
+
     MethodEmitter loadCompilerConstant(final CompilerConstants cc) {
         final Symbol symbol = compilerConstant(cc);
         if (cc == SCOPE && peekType() == Type.SCOPE) {

nashorn/src/jdk/nashorn/internal/ir/CallNode.java

@@ -50,9 +50,6 @@ public final class CallNode extends LexicalContextNode implements TypeOverride<C
     /** Is this a "new" operation */
     public static final int IS_NEW        = 0x1;
 
-    /** Is this call tagged as inside a with block */
-    public static final int IN_WITH_BLOCK = 0x2;
-
     private final int flags;
 
     /**
@@ -145,14 +142,13 @@ public final class CallNode extends LexicalContextNode implements TypeOverride<C
      * @param finish   finish
      * @param function the function to call
      * @param args     args to the call
-     * @param flags    flags
      */
-    public CallNode(final Source source, final long token, final int finish, final Node function, final List<Node> args, final int flags) {
+    public CallNode(final Source source, final long token, final int finish, final Node function, final List<Node> args) {
         super(source, token, finish);
 
         this.function = function;
         this.args     = args;
-        this.flags    = flags;
+        this.flags    = 0;
         this.type     = null;
         this.evalArgs = null;
     }
@@ -333,14 +329,6 @@ public final class CallNode extends LexicalContextNode implements TypeOverride<C
         return setFlags(IS_NEW);
     }
 
-    /**
-     * Check if this call is inside a {@code with} block
-     * @return true if the call is inside a {@code with} block
-     */
-    public boolean inWithBlock() {
-        return (flags & IN_WITH_BLOCK) == IN_WITH_BLOCK;
-    }
-
     private CallNode setFlags(final int flags) {
         if (this.flags == flags) {
             return this;

nashorn/src/jdk/nashorn/internal/ir/FunctionNode.java

@@ -145,14 +145,12 @@ public final class FunctionNode extends LexicalContextNode implements Flags<Func
     /** Has this node been split because it was too large? */
     public static final int IS_SPLIT                    = 1 << 4;
 
-    /** Does the function call eval? */
+    /** Does the function call eval? If it does, then all variables in this function might be get/set by it and it can
+     * introduce new variables into this function's scope too.*/
     public static final int HAS_EVAL                    = 1 << 5;
 
-    /** Does the function contain a with block ? */
-    public static final int HAS_WITH                    = 1 << 6;
-
-    /** Does a descendant function contain a with or eval? */
-    public static final int HAS_DESCENDANT_WITH_OR_EVAL = 1 << 7;
+    /** Does a nested function contain eval? If it does, then all variables in this function might be get/set by it. */
+    public static final int HAS_NESTED_EVAL = 1 << 6;
 
     /**
      * Flag this function as one that defines the identifier "arguments" as a function parameter or nested function
@@ -178,18 +176,18 @@ public final class FunctionNode extends LexicalContextNode implements Flags<Func
     /** Does this function have nested declarations? */
     public static final int HAS_FUNCTION_DECLARATIONS   = 1 << 13;
 
-    /** Does this function or any nested functions contain a with or an eval? */
-    private static final int HAS_DEEP_WITH_OR_EVAL = HAS_EVAL | HAS_WITH | HAS_DESCENDANT_WITH_OR_EVAL;
+    /** Does this function or any nested functions contain an eval? */
+    private static final int HAS_DEEP_EVAL = HAS_EVAL | HAS_NESTED_EVAL;
 
     /** Does this function need to store all its variables in scope? */
-    private static final int HAS_ALL_VARS_IN_SCOPE = HAS_DEEP_WITH_OR_EVAL | IS_SPLIT | HAS_LAZY_CHILDREN;
+    private static final int HAS_ALL_VARS_IN_SCOPE = HAS_DEEP_EVAL | IS_SPLIT | HAS_LAZY_CHILDREN;
 
     /** Does this function potentially need "arguments"? Note that this is not a full test, as further negative check of REDEFINES_ARGS is needed. */
     private static final int MAYBE_NEEDS_ARGUMENTS = USES_ARGUMENTS | HAS_EVAL;
 
-    /** Does this function need the parent scope? It needs it if either it or its descendants use variables from it, or have a deep with or eval.
+    /** Does this function need the parent scope? It needs it if either it or its descendants use variables from it, or have a deep eval.
      *  We also pessimistically need a parent scope if we have lazy children that have not yet been compiled */
-    private static final int NEEDS_PARENT_SCOPE = USES_ANCESTOR_SCOPE | HAS_DEEP_WITH_OR_EVAL | HAS_LAZY_CHILDREN;
+    private static final int NEEDS_PARENT_SCOPE = USES_ANCESTOR_SCOPE | HAS_DEEP_EVAL | HAS_LAZY_CHILDREN;
 
     /** What is the return type of this function? */
     private Type returnType = Type.UNKNOWN;
@@ -405,15 +403,6 @@ public final class FunctionNode extends LexicalContextNode implements Flags<Func
         return getFlag(IS_LAZY);
     }
 
-    /**
-     * Check if the {@code with} keyword is used in this function
-     *
-     * @return true if {@code with} is used
-     */
-    public boolean hasWith() {
-        return getFlag(HAS_WITH);
-    }
-
     /**
      * Check if the {@code eval} keyword is used in this function
      *
@@ -423,18 +412,6 @@ public final class FunctionNode extends LexicalContextNode implements Flags<Func
         return getFlag(HAS_EVAL);
     }
 
-    /**
-     * Test whether this function or any of its nested functions contains a <tt>with</tt> statement
-     * or an <tt>eval</tt> call.
-     *
-     * @see #hasWith()
-     * @see #hasEval()
-     * @return true if this or a nested function contains with or eval
-     */
-    public boolean hasDeepWithOrEval() {
-        return getFlag(HAS_DEEP_WITH_OR_EVAL);
-    }
-
     /**
      * Get the first token for this function
      * @return the first token

nashorn/src/jdk/nashorn/internal/ir/LexicalContext.java

@@ -419,14 +419,6 @@ public class LexicalContext {
         return null;
     }
 
-    /**
-     * Check if lexical context is currently inside a with block
-     * @return true if in a with block
-     */
-    public boolean inWith() {
-        return getScopeNestingLevelTo(null) > 0;
-    }
-
     /**
      * Count the number of with scopes until a given node
      * @param until node to stop counting at, or null if all nodes should be counted

nashorn/src/jdk/nashorn/internal/parser/Parser.java

@@ -398,7 +398,7 @@ loop:
         final String name = ident.getName();
 
         if (EVAL.symbolName().equals(name)) {
-            markWithOrEval(lc, FunctionNode.HAS_EVAL);
+            markEval(lc);
         }
     }
 
@@ -1353,7 +1353,6 @@ loop:
 
         // Get WITH expression.
         WithNode withNode = new WithNode(source, withToken, finish);
-        markWithOrEval(lc, FunctionNode.HAS_WITH);
 
         try {
             lc.push(withNode);
@@ -1742,7 +1741,7 @@ loop:
         // Skip ending of edit string expression.
         expect(RBRACE);
 
-        return new CallNode(source, primaryToken, finish, execIdent, arguments, 0);
+        return new CallNode(source, primaryToken, finish, execIdent, arguments);
     }
 
     /**
@@ -2041,10 +2040,6 @@ loop:
         return new PropertyNode(source, propertyToken, finish, propertyName, assignmentExpression(false), null, null);
     }
 
-    private int callNodeFlags() {
-        return lc.inWith() ? CallNode.IN_WITH_BLOCK : 0;
-    }
-
     /**
      * LeftHandSideExpression :
      *      NewExpression
@@ -2074,7 +2069,7 @@ loop:
                 detectSpecialFunction((IdentNode)lhs);
             }
 
-            lhs = new CallNode(source, callToken, finish, lhs, arguments, callNodeFlags());
+            lhs = new CallNode(source, callToken, finish, lhs, arguments);
         }
 
 loop:
@@ -2088,7 +2083,7 @@ loop:
                 final List<Node> arguments = argumentList();
 
                 // Create call node.
-                lhs = new CallNode(source, callToken, finish, lhs, arguments, callNodeFlags());
+                lhs = new CallNode(source, callToken, finish, lhs, arguments);
 
                 break;
 
@@ -2167,7 +2162,7 @@ loop:
             arguments.add(objectLiteral());
         }
 
-        final CallNode callNode = new CallNode(source, constructor.getToken(), finish, constructor, arguments, callNodeFlags());
+        final CallNode callNode = new CallNode(source, constructor.getToken(), finish, constructor, arguments);
 
         return new UnaryNode(source, newToken, callNode);
     }
@@ -2822,16 +2817,16 @@ loop:
         return "[JavaScript Parsing]";
     }
 
-    private static void markWithOrEval(final LexicalContext lc, int flag) {
+    private static void markEval(final LexicalContext lc) {
         final Iterator<FunctionNode> iter = lc.getFunctions();
         boolean flaggedCurrentFn = false;
         while (iter.hasNext()) {
             final FunctionNode fn = iter.next();
             if (!flaggedCurrentFn) {
-                lc.setFlag(fn, flag);
+                lc.setFlag(fn, FunctionNode.HAS_EVAL);
                 flaggedCurrentFn = true;
             } else {
-                lc.setFlag(fn, FunctionNode.HAS_DESCENDANT_WITH_OR_EVAL);
+                lc.setFlag(fn, FunctionNode.HAS_NESTED_EVAL);
             }
             lc.setFlag(lc.getFunctionBody(fn), Block.NEEDS_SCOPE);
         }

nashorn/src/jdk/nashorn/internal/runtime/linker/NashornCallSiteDescriptor.java

@@ -43,9 +43,8 @@ public class NashornCallSiteDescriptor extends AbstractCallSiteDescriptor {
     public static final int CALLSITE_SCOPE                = 0x01;
     /** Flags that the call site is in code that uses ECMAScript strict mode. */
     public static final int CALLSITE_STRICT               = 0x02;
-    /** Flags that a property getter or setter call site references a scope variable that is not in the global scope
-     * (it is in a function lexical scope), and the function's scope object class is fixed and known in advance. Such
-     * getters and setters can often be linked more optimally using these assumptions. */
+    /** Flags that a property getter or setter call site references a scope variable that is located at a known distance
+     * in the scope chain. Such getters and setters can often be linked more optimally using these assumptions. */
     public static final int CALLSITE_FAST_SCOPE    = 0x400;
 
     /** Flags that the call site is profiled; Contexts that have {@code "profile.callsites"} boolean property set emit