JS: extract expressions preceding inline call

js/js.inliner/src/org/jetbrains/kotlin/js/inline/ExpressionDecomposer.kt

@@ -0,0 +1,191 @@
+/*
+ * Copyright 2010-2015 JetBrains s.r.o.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.jetbrains.kotlin.js.inline.util
+
+import com.google.dart.compiler.backend.js.ast.*
+import com.google.dart.compiler.backend.js.ast.metadata.*
+import org.jetbrains.kotlin.js.translate.utils.JsAstUtils.*
+
+import kotlin.platform.platformStatic
+
+import com.intellij.util.SmartList
+
+/**
+ * If inline function consists of multiple statements,
+ * its statements will be extracted before last visited statement.
+ * This action can potentially break evaluation order.
+ *
+ * An example:
+ *      var x = foo() + inlineBox();
+ * Inliner could extract statements from inlineBox():
+ *      // inlineBox body..
+ *      var x = foo() + inlineBox$result;
+ * So foo is evaluated before inlineBox() after inline.
+ *
+ * So we need to extract all nodes, that evaluate before inline call,
+ * to temporary vars (preserving evaluation order).
+ *
+ * For the example, defined before, evaluation order could be preserved this way:
+ *      var tmp = foo();
+ *      // inlineBox body..
+ *      var x = tmp + inlineBox$result;
+ *
+ * It's desirable to create temporary var only if node can have side effect,
+ * and precedes inline call (in JavaScript evaluation order).
+ */
+class ExpressionDecomposer private (
+        private val scope: JsScope,
+        private val containsExtractable: Set<JsNode>,
+        private val containsNodeWithSideEffect: Set<JsNode>
+) : JsExpressionVisitor() {
+
+    private var additionalStatements: MutableList<JsStatement> = SmartList()
+
+    companion object {
+        platformStatic
+        public fun preserveEvaluationOrder(
+                scope: JsScope, statement: JsStatement, canBeExtractedByInliner: (JsNode)->Boolean
+        ): List<JsStatement> {
+            val decomposer = with (statement) {
+                val extractable = match(canBeExtractedByInliner)
+                val containsExtractable = withParentsOfNodes(extractable)
+                val nodesWithSideEffect = match { it is JsExpression && it.canHaveOwnSideEffect() }
+                val containsNodeWithSideEffect = withParentsOfNodes(nodesWithSideEffect)
+
+                ExpressionDecomposer(scope, containsExtractable, containsNodeWithSideEffect)
+            }
+
+            decomposer.accept(statement)
+            return decomposer.additionalStatements
+        }
+    }
+}
+
+/**
+ * Visits only expressions that can potentially be extracted by [JsInliner],
+ * that directly referenced by statements.
+ *
+ * For example, won't visit [JsBlock] statements, but will visit test expression of [JsWhile].
+ */
+private open class JsExpressionVisitor() : JsVisitorWithContextImpl() {
+
+    override fun visit(x: JsBlock, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsTry, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsDebugger, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsLabel, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsFunction, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsObjectLiteral, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsPropertyInitializer, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsProgramFragment, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsProgram, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsParameter, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsCatch, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsBreak, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsContinue, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsCase, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsDefault, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsEmpty, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsLiteral.JsBooleanLiteral, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsLiteral.JsThisRef, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsNullLiteral, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsNumberLiteral, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsRegExp, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsStringLiteral, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsName, ctx: JsContext<*>): Boolean = false
+
+    // TODO: support these
+    // Not generated by compiler now, (can be generated in future or used in js() block)
+    override fun visit(x: JsForIn, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsSwitch, ctx: JsContext<*>): Boolean = false
+
+    // Compiler generates restricted version of for,
+    // where init and test do not contain inline calls.
+    override fun visit(x: JsFor, ctx: JsContext<*>): Boolean = false
+
+    override fun visit(x: JsIf, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsWhile, ctx: JsContext<*>): Boolean = false
+    override fun visit(x: JsDoWhile, ctx: JsContext<*>): Boolean = false
+
+    override fun visit(x: JsArrayAccess, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsArrayLiteral, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsBinaryOperation, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsConditional, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsInvocation, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsNameRef, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsNew, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsVars.JsVar, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsPostfixOperation, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsPrefixOperation, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsExpressionStatement, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsReturn, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsThrow, ctx: JsContext<*>): Boolean = true
+    override fun visit(x: JsVars, ctx: JsContext<*>): Boolean = true
+}
+
+/**
+ * Returns descendants of receiver, matched by [predicate].
+ */
+private fun JsNode.match(predicate: (JsNode) -> Boolean): Set<JsNode> {
+    val visitor = object : JsExpressionVisitor() {
+        val matched = IdentitySet<JsNode>()
+
+        override fun <R : JsNode?> doTraverse(node: R, ctx: JsContext<*>?) {
+            super.doTraverse(node, ctx)
+
+            if (node !in matched && predicate(node)) {
+                matched.add(node)
+            }
+        }
+    }
+
+    visitor.accept(this)
+    return visitor.matched
+}
+
+/**
+ * Returns set of nodes, that satisfy transitive closure of `is parent` relation, starting from [nodes].
+ */
+private fun JsNode.withParentsOfNodes(nodes: Set<JsNode>): Set<JsNode> {
+    val visitor = object : JsExpressionVisitor() {
+        private val stack = SmartList<JsNode>()
+        val matched = IdentitySet<JsNode>()
+
+        override fun <R : JsNode?> doTraverse(node: R, ctx: JsContext<*>?) {
+            stack.add(node)
+            super.doTraverse(node, ctx)
+
+            if (node in nodes) {
+                addAllUntilMatchedOrStatement(stack)
+            }
+
+            stack.remove(stack.lastIndex)
+        }
+
+        fun addAllUntilMatchedOrStatement(nodesOnStack: List<JsNode>) {
+            for (i in nodesOnStack.lastIndex downTo 0) {
+                val currentNode = nodesOnStack[i]
+                if (currentNode in matched) break
+
+                matched.add(currentNode)
+                if (currentNode is JsStatement) break
+            }
+        }
+    }
+
+    visitor.accept(this)
+    return visitor.matched
+}

js/js.inliner/src/org/jetbrains/kotlin/js/inline/JsInliner.java

@@ -23,11 +23,13 @@ import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.kotlin.diagnostics.DiagnosticSink;
 import org.jetbrains.kotlin.js.inline.context.*;
+import org.jetbrains.kotlin.js.inline.util.ExpressionDecomposer;
 import org.jetbrains.kotlin.js.resolve.diagnostics.ErrorsJs;
 import org.jetbrains.kotlin.js.translate.context.TranslationContext;
 import org.jetbrains.kotlin.resolve.inline.InlineStrategy;
 
 import java.util.*;
+import kotlin.Function1;
 
 import static org.jetbrains.kotlin.js.inline.FunctionInlineMutator.getInlineableCallReplacement;
 import static org.jetbrains.kotlin.js.inline.clean.CleanPackage.removeUnusedFunctionDefinitions;
@@ -47,6 +49,15 @@ public class JsInliner extends JsVisitorWithContextImpl {
     // these are needed for error reporting, when inliner detects cycle
     private final Stack<JsFunction> namedFunctionsStack = new Stack<JsFunction>();
     private final LinkedList<JsCallInfo> inlineCallInfos = new LinkedList<JsCallInfo>();
+    private final Function1<JsNode, Boolean> canBeExtractedByInliner = new Function1<JsNode, Boolean>() {
+        @Override
+        public Boolean invoke(JsNode node) {
+            if (!(node instanceof JsInvocation)) return false;
+
+            JsInvocation call = (JsInvocation) node;
+            return hasToBeInlined(call);
+        }
+    };
 
     public static JsProgram process(@NotNull TranslationContext context) {
         JsProgram program = context.program();
@@ -100,24 +111,21 @@ public class JsInliner extends JsVisitorWithContextImpl {
 
     @Override
     public boolean visit(@NotNull JsInvocation call, @NotNull JsContext context) {
-        if (shouldInline(call) && canInline(call)) {
+        if (!hasToBeInlined(call)) return true;
-            JsFunction containingFunction = getCurrentNamedFunction();
-            if (containingFunction != null) {
-                inlineCallInfos.add(new JsCallInfo(call, containingFunction));
-            }
 
-            JsFunction definition = getFunctionContext().getFunctionDefinition(call);
+        JsFunction containingFunction = getCurrentNamedFunction();
 
-            if (inProcessFunctions.contains(definition))  {
+        if (containingFunction != null) {
-                reportInlineCycle(call, definition);
+            inlineCallInfos.add(new JsCallInfo(call, containingFunction));
-                return false;
+        }
-            }
 
-            if (!processedFunctions.contains(definition)) {
+        JsFunction definition = getFunctionContext().getFunctionDefinition(call);
-                accept(definition);
-            }
 
-            inline(call, context);
+        if (inProcessFunctions.contains(definition))  {
+            reportInlineCycle(call, definition);
+        }
+        else if (!processedFunctions.contains(definition)) {
+            accept(definition);
         }
 
         return true;
@@ -125,6 +133,10 @@ public class JsInliner extends JsVisitorWithContextImpl {
 
     @Override
     public void endVisit(@NotNull JsInvocation x, @NotNull JsContext ctx) {
+        if (hasToBeInlined(x)) {
+            inline(x, ctx);
+        }
+
         JsCallInfo lastCallInfo = null;
 
         if (!inlineCallInfos.isEmpty()) {
@@ -136,6 +148,25 @@ public class JsInliner extends JsVisitorWithContextImpl {
         }
     }
 
+    @Override
+    protected void doAcceptStatementList(List<JsStatement> statements) {
+        // at top level of js ast, contexts stack can be empty,
+        // but there is no inline calls anyway
+        if(!inliningContexts.isEmpty()) {
+            JsScope scope = getFunctionContext().getScope();
+            int i = 0;
+
+            while (i < statements.size()) {
+                List<JsStatement> additionalStatements =
+                        ExpressionDecomposer.preserveEvaluationOrder(scope, statements.get(i), canBeExtractedByInliner);
+                statements.addAll(i, additionalStatements);
+                i += additionalStatements.size() + 1;
+            }
+        }
+
+        super.doAcceptStatementList(statements);
+    }
+
     private void inline(@NotNull JsInvocation call, @NotNull JsContext context) {
         JsInliningContext inliningContext = getInliningContext();
         FunctionContext functionContext = getFunctionContext();
@@ -146,7 +177,9 @@ public class JsInliner extends JsVisitorWithContextImpl {
         JsExpression resultExpression = inlineableResult.getResultExpression();
         JsContext<JsStatement> statementContext = inliningContext.getStatementContext();
         // body of inline function can contain call to lambdas that need to be inlined
-        accept(inlineableBody);
+        JsStatement statement = accept(inlineableBody);
+        assert inlineableBody == statement;
+
         statementContext.addPrevious(flattenStatement(inlineableBody));
 
         /**
@@ -195,14 +228,11 @@ public class JsInliner extends JsVisitorWithContextImpl {
         }
     }
 
-    private boolean canInline(@NotNull JsInvocation call) {
+    public boolean hasToBeInlined(@NotNull JsInvocation call) {
-        FunctionContext functionContext = getFunctionContext();
-        return functionContext.hasFunctionDefinition(call);
-    }
-
-    private static boolean shouldInline(@NotNull JsInvocation call) {
         InlineStrategy strategy = MetadataPackage.getInlineStrategy(call);
-        return strategy != null && strategy.isInline();
+        if (strategy == null || !strategy.isInline()) return false;
+
+        return getFunctionContext().hasFunctionDefinition(call);
     }
 
     private class JsInliningContext implements InliningContext {

js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/namingUtils.kt

@@ -32,7 +32,7 @@ public fun aliasArgumentsIfNeeded(
     for ((arg, param) in arguments zip parameters) {
         val paramName = param.getName()
         val replacement =
-                if (needToAlias(arg)) {
+                if (arg.needToAlias()) {
                     val freshName = context.getFreshName(paramName)
                     context.newVar(freshName, arg)
                     freshName.makeRef()

js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/rewriters/ReturnReplacingVisitor.kt

@@ -59,7 +59,7 @@ class ReturnReplacingVisitor(private val resultRef: JsNameRef?, private val brea
             if (resultRef != null)
                 return JsAstUtils.assignment(resultRef, returnExpression)
 
-            if (canHaveSideEffect(returnExpression))
+            if (returnExpression.canHaveSideEffect())
                 return returnExpression
         }
 

js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/sideEffectUtils.kt

@@ -16,67 +16,33 @@
 
 package org.jetbrains.kotlin.js.inline.util
 
-import com.google.dart.compiler.backend.js.ast.JsArrayAccess
+import com.google.dart.compiler.backend.js.ast.JsLiteral.*
-import com.google.dart.compiler.backend.js.ast.JsArrayLiteral
+import com.google.dart.compiler.backend.js.ast.*
-import com.google.dart.compiler.backend.js.ast.JsBinaryOperation
+import org.jetbrains.kotlin.js.translate.utils.ast.any
-import com.google.dart.compiler.backend.js.ast.JsConditional
-import com.google.dart.compiler.backend.js.ast.JsExpression
-import com.google.dart.compiler.backend.js.ast.JsInvocation
-import com.google.dart.compiler.backend.js.ast.JsLiteral.JsThisRef
-import com.google.dart.compiler.backend.js.ast.JsLiteral.JsValueLiteral
-import com.google.dart.compiler.backend.js.ast.JsNameRef
-import com.google.dart.compiler.backend.js.ast.JsNode
-import com.google.dart.compiler.backend.js.ast.RecursiveJsVisitor
 
-public fun canHaveSideEffect(x: JsExpression): Boolean {
+public fun JsExpression.canHaveSideEffect(): Boolean =
-    return with(SideEffectVisitor()) {
+        any { it is JsExpression && it.canHaveOwnSideEffect() }
-        accept(x)
-        !sideEffectFree
-    }
-}
 
-public fun needToAlias(x: JsExpression): Boolean {
+public fun JsExpression.canHaveOwnSideEffect(): Boolean =
-    return with(NeedToAliasVisitor()) {
+    when (this) {
-        accept(x)
+        is JsValueLiteral,
-        !sideEffectFree
+        is JsConditional,
-    }
+        is JsArrayAccess,
-}
+        is JsArrayLiteral,
-
+        is JsNameRef -> false
-private open class SideEffectVisitor() : RecursiveJsVisitor() {
+        is JsBinaryOperation -> getOperator().isAssignment()
-    public var sideEffectFree: Boolean = true
+        else -> true
-        protected set
-
-    override fun visitElement(node: JsNode) {
-        sideEffectFree = sideEffectFree && isSideEffectFree(node)
-
-        if (sideEffectFree) {
-            super.visitElement(node)
-        }
     }
 
-    protected open fun isSideEffectFree(node: JsNode): Boolean =
+public fun JsExpression.needToAlias(): Boolean =
-        when (node) {
+        any { it is JsExpression && it.shouldHaveOwnAlias() }
-            is JsValueLiteral,
-            is JsConditional,
-            is JsArrayAccess,
-            is JsArrayLiteral,
-            is JsNameRef ->
-                true
-            is JsBinaryOperation ->
-                !node.getOperator().isAssignment()
-            else ->
-                false
-        }
-}
 
-private class NeedToAliasVisitor() : SideEffectVisitor() {
+public fun JsExpression.shouldHaveOwnAlias(): Boolean =
-    override fun isSideEffectFree(node: JsNode): Boolean =
+        when (this) {
-        when (node) {
             is JsThisRef,
             is JsConditional,
             is JsBinaryOperation,
-            is JsArrayLiteral -> false
+            is JsArrayLiteral -> true
-            is JsInvocation -> isFunctionCreatorInvocation(node)
+            is JsInvocation -> !isFunctionCreatorInvocation(this)
-            else -> super.isSideEffectFree(node)
+            else -> canHaveOwnSideEffect()
         }
-}

js/js.translator/src/org/jetbrains/kotlin/js/translate/utils/astUtils.kt

@@ -16,9 +16,8 @@
 
 package org.jetbrains.kotlin.js.translate.utils.ast
 
-import com.google.dart.compiler.backend.js.ast.JsFunction
+import com.google.dart.compiler.backend.js.ast.*
-import com.google.dart.compiler.backend.js.ast.JsStatement
+import org.jetbrains.kotlin.js.inline.util.IdentitySet
-import com.google.dart.compiler.backend.js.ast.JsParameter
 
 public fun JsFunction.addStatement(stmt: JsStatement) {
     getBody().getStatements().add(stmt)
@@ -36,3 +35,23 @@ public fun JsFunction.addParameter(identifier: String, index: Int? = null): JsPa
 
     return parameter
 }
+
+/**
+ * Tests, if any node containing in receiver's AST matches, [predicate].
+ */
+public fun JsNode.any(predicate: (JsNode) -> Boolean): Boolean {
+    val visitor = object : RecursiveJsVisitor() {
+        public var matched: Boolean = false
+
+        override fun visitElement(node: JsNode) {
+            matched = matched || predicate(node)
+
+            if (!matched) {
+                super.visitElement(node)
+            }
+        }
+    }
+
+    visitor.accept(this)
+    return visitor.matched
+}