JS: refactorings & cleanup

js/js.inliner/src/org/jetbrains/kotlin/js/coroutine/CoroutineTransformer.kt

@@ -19,6 +19,8 @@ package org.jetbrains.kotlin.js.coroutine
 import org.jetbrains.kotlin.js.backend.ast.*
 import org.jetbrains.kotlin.js.backend.ast.metadata.coroutineMetadata
 import org.jetbrains.kotlin.js.backend.ast.metadata.isInlineableCoroutineBody
+import org.jetbrains.kotlin.js.inline.InlineSuspendFunctionSplitter
+import org.jetbrains.kotlin.js.inline.ProgramFragmentInliningScope
 import org.jetbrains.kotlin.js.translate.declaration.transformCoroutineMetadataToSpecialFunctions
 import org.jetbrains.kotlin.js.translate.expression.InlineMetadata
 import org.jetbrains.kotlin.js.translate.utils.JsAstUtils
@@ -70,8 +72,9 @@ class CoroutineTransformer : JsVisitorWithContextImpl() {
 fun transformCoroutines(fragments: List<JsProgramFragment>) {
     val coroutineTransformer = CoroutineTransformer()
     for (fragment in fragments) {
-        fragment.inlinedFunctionWrappers.values.forEach { coroutineTransformer.accept(it) }
+        val scope = ProgramFragmentInliningScope(fragment)
-        coroutineTransformer.accept(fragment.declarationBlock)
+        InlineSuspendFunctionSplitter(scope).accept(scope.allCode)
-        coroutineTransformer.accept(fragment.initializerBlock)
+        coroutineTransformer.accept(scope.allCode)
+        scope.update()
     }
 }

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

@@ -1,68 +0,0 @@
-/*
- * Copyright 2010-2017 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
-
-import org.jetbrains.kotlin.descriptors.PropertyGetterDescriptor
-import org.jetbrains.kotlin.descriptors.PropertySetterDescriptor
-import org.jetbrains.kotlin.js.backend.ast.*
-import org.jetbrains.kotlin.js.backend.ast.metadata.descriptor
-import org.jetbrains.kotlin.js.backend.ast.metadata.inlineStrategy
-import org.jetbrains.kotlin.js.backend.ast.metadata.psiElement
-
-class DummyAccessorInvocationTransformer : JsVisitorWithContextImpl() {
-    override fun endVisit(x: JsNameRef, ctx: JsContext<in JsNode>) {
-        super.endVisit(x, ctx)
-        val dummy = tryCreatePropertyGetterInvocation(x)
-        if (dummy != null) {
-            ctx.replaceMe(dummy)
-        }
-    }
-
-    override fun endVisit(x: JsBinaryOperation, ctx: JsContext<in JsNode>) {
-        super.endVisit(x, ctx)
-        val dummy = tryCreatePropertySetterInvocation(x)
-        if (dummy != null) {
-            ctx.replaceMe(dummy)
-        }
-    }
-
-    private fun tryCreatePropertyGetterInvocation(x: JsNameRef): JsInvocation? {
-        if (x.inlineStrategy != null && x.descriptor is PropertyGetterDescriptor) {
-            val dummyInvocation = JsInvocation(x)
-            copyInlineMetadata(x, dummyInvocation)
-            return dummyInvocation
-        }
-        return null
-    }
-
-    private fun tryCreatePropertySetterInvocation(x: JsBinaryOperation): JsInvocation? {
-        if (!x.operator.isAssignment || x.arg1 !is JsNameRef) return null
-        val name = x.arg1 as JsNameRef
-        if (name.inlineStrategy != null && name.descriptor is PropertySetterDescriptor) {
-            val dummyInvocation = JsInvocation(name, x.arg2)
-            copyInlineMetadata(name, dummyInvocation)
-            return dummyInvocation
-        }
-        return null
-    }
-
-    private fun copyInlineMetadata(from: JsNameRef, to: JsInvocation) {
-        to.inlineStrategy = from.inlineStrategy
-        to.descriptor = from.descriptor
-        to.psiElement = from.psiElement
-    }
-}

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

@@ -14,7 +14,6 @@ import org.jetbrains.kotlin.js.backend.ast.metadata.inlineStrategy
 import org.jetbrains.kotlin.js.backend.ast.metadata.psiElement
 import org.jetbrains.kotlin.js.inline.context.FunctionContext
 import org.jetbrains.kotlin.js.inline.util.FunctionWithWrapper
-import org.jetbrains.kotlin.js.inline.util.IdentitySet
 import org.jetbrains.kotlin.resolve.inline.InlineStrategy
 import java.util.*
 
@@ -30,18 +29,19 @@ class InlinerCycleReporter(
     private val functionContext: FunctionContext
 ) {
 
-    private val processedFunctions = IdentitySet<JsFunction>()
+    private enum class VisitedState { IN_PROCESS, PROCESSED }
-    private val inProcessFunctions = IdentitySet<JsFunction>()
+
+    private val functionVisitingState = mutableMapOf<JsFunction, VisitedState>()
+
     // these are needed for error reporting, when inliner detects cycle
     private val namedFunctionsStack = Stack<JsFunction>()
 
-    private val inlineCallInfos = LinkedList<JsCallInfo>()
+    private val currentNamedFunction: JsFunction?
-
-    // TODO This looks like a hack
-    val currentNamedFunction: JsFunction?
         get() = if (namedFunctionsStack.empty()) null else namedFunctionsStack.peek()
 
+    private val inlineCallInfos = LinkedList<JsCallInfo>()
 
+    // Puts `function` on the `namedFunctionsStack` for inline call cycles reporting
     fun <T> withFunction(function: JsFunction, body: () -> T): T {
         if (function in functionContext.functionsByFunctionNodes.keys) {
             namedFunctionsStack.push(function)
@@ -49,34 +49,41 @@ class InlinerCycleReporter(
 
         val result = body()
 
-        if (!namedFunctionsStack.empty() && namedFunctionsStack.peek() == function) {
+        if (currentNamedFunction == function) {
             namedFunctionsStack.pop()
         }
 
         return result
     }
 
-    fun <T> withInlineFunctionDefinition(function: JsFunction, body: () -> T): T {
+    fun processInlineFunction(definition: FunctionWithWrapper, call: JsInvocation?, doProcess: () -> Unit) {
-        assert(!inProcessFunctions.contains(function)) { "Inliner has revisited function" }
-        inProcessFunctions.add(function)
 
-        val result = withFunction(function, body)
+        when (functionVisitingState[definition.function]) {
+            VisitedState.IN_PROCESS -> {
+                reportInlineCycle(call, definition.function)
+                return
+            }
+            VisitedState.PROCESSED -> return
+        }
 
-        processedFunctions.add(function)
+        val function = definition.function
 
-        assert(function in inProcessFunctions)
+        functionVisitingState[function] = VisitedState.IN_PROCESS
-        inProcessFunctions.remove(function)
+
+        val result = withFunction(function, doProcess)
+
+        functionVisitingState[function] = VisitedState.PROCESSED
 
         return result
     }
 
 
-    fun <T> withInlining(call: JsInvocation, body: () -> T): T {
+    fun <T> inlineCall(call: JsInvocation, doInline: () -> T): T {
         currentNamedFunction?.let {
             inlineCallInfos.add(JsCallInfo(call, it))
         }
 
-        val result = body()
+        val result = doInline()
 
         if (!inlineCallInfos.isEmpty()) {
             if (inlineCallInfos.last.call == call) {
@@ -87,17 +94,6 @@ class InlinerCycleReporter(
         return result
     }
 
-    // Return true iff the definition should be visited by the inliner
-    fun shouldProcess(definition: FunctionWithWrapper, call: JsInvocation?): Boolean {
-        if (definition.function in inProcessFunctions) {
-            reportInlineCycle(call, definition.function)
-        } else if (definition.function !in processedFunctions) {
-            return true
-        }
-
-        return false
-    }
-
     private fun reportInlineCycle(call: JsInvocation?, calledFunction: JsFunction) {
         call?.inlineStrategy = InlineStrategy.NOT_INLINE
         val it = inlineCallInfos.descendingIterator()

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

@@ -5,9 +5,13 @@
 
 package org.jetbrains.kotlin.js.inline
 
+import org.jetbrains.kotlin.descriptors.PropertyGetterDescriptor
+import org.jetbrains.kotlin.descriptors.PropertySetterDescriptor
 import org.jetbrains.kotlin.js.backend.ast.*
+import org.jetbrains.kotlin.js.backend.ast.metadata.descriptor
 import org.jetbrains.kotlin.js.backend.ast.metadata.forcedReturnVariable
 import org.jetbrains.kotlin.js.backend.ast.metadata.inlineStrategy
+import org.jetbrains.kotlin.js.backend.ast.metadata.psiElement
 import org.jetbrains.kotlin.js.inline.clean.FunctionPostProcessor
 import org.jetbrains.kotlin.js.inline.clean.removeUnusedLocalFunctionDeclarations
 import org.jetbrains.kotlin.js.inline.util.extractFunction
@@ -72,6 +76,18 @@ class InlinerImpl(
     }
 
 
+    override fun endVisit(x: JsNameRef, ctx: JsContext<in JsNode>) {
+        tryCreatePropertyGetterInvocation(x)?.let {
+            endVisit(it, ctx)
+        }
+    }
+
+    override fun endVisit(x: JsBinaryOperation, ctx: JsContext<in JsNode>) {
+        tryCreatePropertySetterInvocation(x)?.let {
+            endVisit(it, ctx)
+        }
+    }
+
     override fun endVisit(call: JsInvocation, ctx: JsContext<JsNode>) {
         if (hasToBeInlined(call)) {
             val (inlineableBody, resultExpression) = jsInliner.inline(scope, call, lastStatementLevelContext.currentNode)
@@ -92,9 +108,7 @@ class InlinerImpl(
         var i = 0
 
         while (i < statements.size) {
-            val additionalStatements = ExpressionDecomposer.preserveEvaluationOrder(statements[i]) { node ->
+            val additionalStatements = ExpressionDecomposer.preserveEvaluationOrder(statements[i], ::hasToBeInlined)
-                node is JsInvocation && hasToBeInlined(node)
-            }
             statements.addAll(i, additionalStatements)
             i += additionalStatements.size + 1
         }
@@ -102,6 +116,16 @@ class InlinerImpl(
         super.doAcceptStatementList(statements)
     }
 
+    private fun hasToBeInlined(node: JsNode): Boolean {
+        return when (node) {
+            is JsInvocation -> hasToBeInlined(node)
+            is JsNameRef -> node.inlineStrategy != null && tryCreatePropertyGetterInvocation(node)?.let { hasToBeInlined(it) } ?: false
+            is JsBinaryOperation -> node.operator.isAssignment && node.arg1?.let { left ->
+                left is JsNameRef && left.inlineStrategy != null && tryCreatePropertySetterInvocation(node)?.let { hasToBeInlined(it) } ?: false
+            } ?: false
+            else -> false
+        }
+    }
 
     private fun hasToBeInlined(call: JsInvocation): Boolean {
         val strategy = call.inlineStrategy
@@ -119,4 +143,30 @@ class InlinerImpl(
             })
         }
     }
+
+    private fun tryCreatePropertyGetterInvocation(x: JsNameRef): JsInvocation? {
+        if (x.inlineStrategy != null && x.descriptor is PropertyGetterDescriptor) {
+            val dummyInvocation = JsInvocation(x)
+            copyInlineMetadata(x, dummyInvocation)
+            return dummyInvocation
+        }
+        return null
+    }
+
+    private fun tryCreatePropertySetterInvocation(x: JsBinaryOperation): JsInvocation? {
+        if (!x.operator.isAssignment || x.arg1 !is JsNameRef) return null
+        val name = x.arg1 as JsNameRef
+        if (name.inlineStrategy != null && name.descriptor is PropertySetterDescriptor) {
+            val dummyInvocation = JsInvocation(name, x.arg2)
+            copyInlineMetadata(name, dummyInvocation)
+            return dummyInvocation
+        }
+        return null
+    }
+
+    private fun copyInlineMetadata(from: JsNameRef, to: JsInvocation) {
+        to.inlineStrategy = from.inlineStrategy
+        to.descriptor = from.descriptor
+        to.psiElement = from.psiElement
+    }
 }

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

@@ -137,6 +137,17 @@ class ProgramFragmentInliningScope(
     override val fragment: JsProgramFragment
 ) : InliningScope() {
 
+    val allCode: JsBlock
+        get() = JsBlock(
+            JsBlock(fragment.inlinedFunctionWrappers.values.toList()),
+            fragment.declarationBlock,
+            fragment.exportBlock,
+            JsExpressionStatement(JsFunction(JsDynamicScope, fragment.initializerBlock, ""))
+        ).also { block ->
+            fragment.tests?.let { block.statements.add(it) }
+            fragment.mainFunction?.let { block.statements.add(it) }
+        }
+
     private val existingModules = fragment.importedModules.associateTo(mutableMapOf()) { it.key to it }
 
     private val existingBindings = fragment.nameBindings.associateTo(mutableMapOf()) { it.key to it.name }
@@ -148,18 +159,9 @@ class ProgramFragmentInliningScope(
         fragment.declarationBlock.statements.addAll(0, additionalDeclarations)
 
         // post-processing
-        val block = JsBlock(
+        simplifyWrappedFunctions(allCode)
-            JsBlock(fragment.inlinedFunctionWrappers.values.toList()),
+        removeUnusedFunctionDefinitions(allCode, collectNamedFunctions(allCode))
-            fragment.declarationBlock,
+        removeUnusedImports(fragment, allCode)
-            fragment.initializerBlock,
-            fragment.exportBlock
-        )
-        fragment.tests?.let { block.statements.add(it) }
-        fragment.mainFunction?.let { block.statements.add(it) }
-
-        simplifyWrappedFunctions(block)
-        removeUnusedFunctionDefinitions(block, collectNamedFunctions(block))
-        removeUnusedImports(fragment)
     }
 
     override fun hasImport(name: JsName, tag: String): JsName? {

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

@@ -21,16 +21,6 @@ class JsInliner(
     val translationResult: AstGenerationResult
 ) {
 
-    init {
-        // TODO Isn't there a better way to achieve this? Also there is a bug with private inline properties
-        DummyAccessorInvocationTransformer().let {
-            for (fragment in translationResult.newFragments) {
-                it.accept<JsGlobalBlock>(fragment.declarationBlock)
-                it.accept<JsGlobalBlock>(fragment.initializerBlock)
-            }
-        }
-    }
-
     val functionContext = FunctionContext(this)
 
     val cycleReporter = InlinerCycleReporter(trace, functionContext)
@@ -44,36 +34,21 @@ class JsInliner(
     fun process(fragment: JsProgramFragment) {
         val fragmentScope = functionContext.scopeForFragment(fragment) ?: return
 
-        // TODO any way and/or need to visit everything inside the fragment?
+        fragmentScope.process(fragmentScope.allCode)
-        fragmentScope.process(fragment.declarationBlock)
-
-        // TODO Atm it's placed after inliner in order not to perform the body inlining twice. Is that OK?
-        // Ideally it could be moved to the coroutine transformers. The info regarding which inline function wrappers have been imported
-        // on top level should be persisted for that sake. Also it going to be needed in order to avoid duplicate code.
-        InlineSuspendFunctionSplitter(fragmentScope).accept(fragment.declarationBlock)
-
-        // Mostly for the sake of post-processor
-        // TODO are inline function marked with @Test possible?
-        fragment.tests?.let { fragmentScope.process(it) }
-
-        // TODO wrap in a function in order to do the post-processing
-        fragmentScope.process(fragment.initializerBlock)
 
         fragmentScope.update()
     }
 
     fun process(inlineFn: InlineFunctionDefinition, call: JsInvocation?, containingScope: InliningScope) {
-        if (cycleReporter.shouldProcess(inlineFn.fn, call)) {
+        cycleReporter.processInlineFunction(inlineFn.fn, call) {
             val (function, wrapperBody) = inlineFn.fn
 
-            cycleReporter.withInlineFunctionDefinition(function) {
+            if (wrapperBody != null) {
-                if (wrapperBody != null) {
+                val scope = PublicInlineFunctionInliningScope(function, wrapperBody, containingScope.fragment)
-                    val scope = PublicInlineFunctionInliningScope(function, wrapperBody, containingScope.fragment)
+                scope.process(wrapperBody)
-                    scope.process(wrapperBody)
+                scope.update()
-                    scope.update()
+            } else {
-                } else {
+                containingScope.process(function)
-                    containingScope.process(function)
-                }
             }
         }
     }
@@ -85,7 +60,7 @@ class JsInliner(
     fun inline(scope: InliningScope, call: JsInvocation, currentStatement: JsStatement?): InlineableResult {
         val definition = functionContext.getFunctionDefinition(call, scope)
 
-        return cycleReporter.withInlining(call) {
+        return cycleReporter.inlineCall(call) {
 
             val function = scope.importFunctionDefinition(definition)
 

js/js.inliner/src/org/jetbrains/kotlin/js/inline/clean/removeUnusedImports.kt

@@ -21,19 +21,10 @@ import org.jetbrains.kotlin.js.backend.ast.metadata.coroutineMetadata
 import org.jetbrains.kotlin.js.backend.ast.metadata.imported
 
 
-fun removeUnusedImports(fragment: JsProgramFragment) {
+fun removeUnusedImports(fragment: JsProgramFragment, code: JsBlock) {
     val usedImports = mutableSetOf<JsName>()
 
-    with(fragment) {
+    collectUsedImports(code, usedImports)
-        inlinedFunctionWrappers.values.forEach {
-            collectUsedImports(it, usedImports)
-        }
-        collectUsedImports(declarationBlock, usedImports)
-        collectUsedImports(initializerBlock, usedImports)
-        tests?.let { collectUsedImports(it, usedImports) }
-        mainFunction?.let { collectUsedImports(it, usedImports) }
-        collectUsedImports(exportBlock, usedImports)
-    }
 
     fragment.nameBindings.retainAll { !it.name.imported || it.name in usedImports }
 

js/js.inliner/src/org/jetbrains/kotlin/js/inline/context/FunctionContext.kt

@@ -27,20 +27,6 @@ import java.util.HashMap
 class FunctionContext(
     val inliner: JsInliner
 ) {
-    private val functionReader = FunctionReader(inliner.reporter, inliner.config, inliner.translationResult.innerModuleName)
-
-    private data class FunctionsAndAccessors(
-        val functions: Map<JsName, FunctionWithWrapper>,
-        val accessors: Map<String, FunctionWithWrapper>)
-
-    private val fragmentInfo = mutableMapOf<JsProgramFragment, FunctionsAndAccessors>()
-
-    private fun lookUpStaticFunction(functionName: JsName?, fragment: JsProgramFragment): FunctionWithWrapper? =
-        fragmentInfo[fragment]?.run { functions[functionName] }
-
-    private fun lookUpStaticFunctionByTag(functionTag: String, fragment: JsProgramFragment): FunctionWithWrapper? =
-        fragmentInfo[fragment]?.run { accessors[functionTag] }
-
     fun getFunctionDefinition(call: JsInvocation, scope: InliningScope): InlineFunctionDefinition {
         return getFunctionDefinitionImpl(call, scope)!!
     }
@@ -51,6 +37,12 @@ class FunctionContext(
 
     val functionsByFunctionNodes = HashMap<JsFunction, FunctionWithWrapper>()
 
+    fun scopeForFragment(fragment: JsProgramFragment) = if (fragment in newFragments) {
+        inliningScopeCache.computeIfAbsent(fragment) {
+            ProgramFragmentInliningScope(fragment)
+        }
+    } else null
+
     /**
      * Gets function definition by invocation.
      *
@@ -89,6 +81,21 @@ class FunctionContext(
         return lookUpFunctionDirect(call) ?: lookUpFunctionIndirect(call, scope) ?: lookUpFunctionExternal(call, scope.fragment)
     }
 
+    private val functionReader = FunctionReader(inliner.reporter, inliner.config, inliner.translationResult.innerModuleName)
+
+    private data class FunctionsAndAccessors(
+        val functions: Map<JsName, FunctionWithWrapper>,
+        val accessors: Map<String, FunctionWithWrapper>)
+
+    private val fragmentInfo = mutableMapOf<JsProgramFragment, FunctionsAndAccessors>()
+
+    private fun lookUpStaticFunction(functionName: JsName?, fragment: JsProgramFragment): FunctionWithWrapper? =
+        fragmentInfo[fragment]?.run { functions[functionName] }
+
+    private fun lookUpStaticFunctionByTag(functionTag: String, fragment: JsProgramFragment): FunctionWithWrapper? =
+        fragmentInfo[fragment]?.run { accessors[functionTag] }
+
+
     private fun lookUpFunctionIndirect(call: JsInvocation, scope: InliningScope): InlineFunctionDefinition? {
         /** remove ending `()` */
         val callQualifier: JsExpression = if (isCallInvocation(call)) {
@@ -124,11 +131,6 @@ class FunctionContext(
 
     private val newFragments = inliner.translationResult.newFragments.toIdentitySet()
 
-    fun scopeForFragment(fragment: JsProgramFragment) = if (fragment in newFragments) {
-        inliningScopeCache.computeIfAbsent(fragment) {
-            ProgramFragmentInliningScope(fragment)
-        }
-    } else null
 
     private fun loadFragment(fragment: JsProgramFragment) {
         fragmentInfo.computeIfAbsent(fragment) {
@@ -143,27 +145,32 @@ class FunctionContext(
         }
     }
 
-    fun fragmentByTag(tag: String): JsProgramFragment? {
+    private fun fragmentByTag(tag: String): JsProgramFragment? {
         return inliner.translationResult.inlineFunctionTagMap[tag]?.let { unit ->
             inliner.translationResult.translate(unit).fragment.also { loadFragment(it) }
         }
     }
 
-    private fun lookUpFunctionDirect(call: JsInvocation): InlineFunctionDefinition? =
+    private fun lookUpFunctionDirect(call: JsInvocation): InlineFunctionDefinition? {
-        call.descriptor?.let { descriptor ->
+        val descriptor = call.descriptor ?: return null
-            Namer.getFunctionTag(descriptor, inliner.config).let { tag ->
+
-                fragmentByTag(tag)?.let { definitionFragment ->
+        val tag = Namer.getFunctionTag(descriptor, inliner.config)
-                    return lookUpStaticFunctionByTag(tag, definitionFragment)?.let { fn ->
+
-                        InlineFunctionDefinition(fn, tag).also { definition ->
+        val definitionFragment = fragmentByTag(tag) ?: return null
-                            scopeForFragment(definitionFragment)?.let { definitionScope ->
+
-                                inliner.process(definition, call, definitionScope)
+        val fn = lookUpStaticFunctionByTag(tag, definitionFragment) ?: return null
-                            }
+
-                        }
+        val definition = InlineFunctionDefinition(fn, tag)
-                    }
+
-                }
+        // Process definition if it is in the new fragments
-            }
+        scopeForFragment(definitionFragment)?.let { definitionScope ->
+            inliner.process(definition, call, definitionScope)
         }
 
+        return definition
+    }
+
+
     private fun lookUpFunctionExternal(call: JsInvocation, fragment: JsProgramFragment): InlineFunctionDefinition? =
         call.descriptor?.let { descriptor ->
             functionReader[descriptor, fragment]?.let {