JS: when resolving temporary functions, don't rely on JsScope hierarchy, build this hierarchy instead, based on scoping rules for function, var, etc expressions

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

@@ -14,37 +14,13 @@
  * limitations under the License.
  */
 
-/*
- * Copyright 2010-2016 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.clean
 
 import org.jetbrains.kotlin.js.backend.ast.*
 import org.jetbrains.kotlin.js.backend.ast.metadata.coroutineMetadata
-import org.jetbrains.kotlin.js.inline.util.collectReferencedTemporaryNames
 
 fun JsNode.resolveTemporaryNames() {
-    val allNames = collectReferencedTemporaryNames(this)
+    val renamings = resolveNames()
-
-    val scopeTree = ScopeCollector().let {
-        it.accept(this)
-        it.scopeTree
-    }
-
-    val renamings = scopeTree.resolveNames(allNames)
     accept(object : RecursiveJsVisitor() {
         override fun visitElement(node: JsNode) {
             super.visitElement(node)
@@ -66,52 +42,126 @@ fun JsNode.resolveTemporaryNames() {
     })
 }
 
-private fun Map<JsScope, Set<JsScope>>.resolveNames(knownNames: Set<JsName>): Map<JsName, JsName> {
+private fun JsNode.resolveNames(): Map<JsName, JsName> {
+    val rootScope = computeScopes().liftUsedNames()
     val replacements = mutableMapOf<JsName, JsName>()
-    fun traverse(scope: JsScope) {
+    fun traverse(scope: Scope) {
-        for (temporaryName in scope.temporaryNames.filter { it in knownNames }.sortedBy { it.ordinal }) {
+        // Don't clash with non-temporary names declared in current scope. It's for rare cases like `_` or `Kotlin` names,
-            replacements[temporaryName] = scope.declareFreshName(temporaryName.ident).apply { copyMetadataFrom(temporaryName) }
+        // since most of local declarations are temporary.
+        val occupiedNames = scope.declaredNames.asSequence().filter { !it.isTemporary }.map { it.ident }.toMutableSet()
+
+        // Don't clash with non-temporary names used in current scope. It's ok to clash with unused names.
+        // Don't clash with used temporary names from outer scopes that get their resolved names. For example,
+        // when function declares temporary name `foo` and inner function both declares temporary name `foo` and uses
+        // (directly or propagates to inner scope) outer `foo`, we should resolve distinct strings for these `foo` names.
+        // Outer `foo` resolves first, so when traversing inner scope, we should take it into account.
+        occupiedNames += scope.usedNames.asSequence().mapNotNull { if (!it.isTemporary) it.ident else replacements[it]?.ident }
+
+        for (temporaryName in scope.declaredNames.asSequence().filter { it.isTemporary }) {
+            var resolvedName = temporaryName.ident
+            var suffix = 0
+            while (resolvedName in JsDeclarationScope.RESERVED_WORDS || !occupiedNames.add(resolvedName)) {
+                resolvedName = "${temporaryName.ident}_${suffix++}"
+            }
+            replacements[temporaryName] = JsDynamicScope.declareName(resolvedName).apply { copyMetadataFrom(temporaryName) }
+            occupiedNames += resolvedName
         }
-        this[scope]!!.forEach(::traverse)
+        scope.children.forEach(::traverse)
     }
 
-    val roots = keys.toMutableSet()
+    traverse(rootScope)
-    values.forEach { roots -= it }
 
-    for (root in roots) {
+    // Labels in JS are in separate scope from local variables. It's only important that nested labels don't clash.
-        traverse(root)
+    // Adjacent labels in one function is OK
-    }
+    accept(object : RecursiveJsVisitor() {
+        var labels = mutableSetOf<String>()
+
+        override fun visitLabel(x: JsLabel) {
+            val addedNames = mutableSetOf<String>()
+            if (x.name.isTemporary) {
+                var resolvedName = x.name.ident
+                var suffix = 0
+                while (!labels.add(resolvedName)) {
+                    resolvedName = "${x.name.ident}_${suffix++}"
+                }
+                replacements[x.name] = JsDynamicScope.declareName(resolvedName)
+                addedNames += resolvedName
+            }
+            super.visitLabel(x)
+            labels.removeAll(addedNames)
+        }
+
+        override fun visitFunction(x: JsFunction) {
+            val oldLabels = labels
+            labels = mutableSetOf<String>()
+            super.visitFunction(x)
+            labels = oldLabels
+        }
+    })
 
     return replacements
 }
 
-private class ScopeCollector : RecursiveJsVisitor() {
+// When name is used by inner scope, it's implicitly used by outer scopes
-    val scopeTree = mutableMapOf<JsScope, MutableSet<JsScope>>()
+private fun Scope.liftUsedNames(): Scope {
-
+    fun traverse(scope: Scope) {
-    override fun visitCatch(x: JsCatch) {
+        scope.children.forEach { child ->
-        recordScope(x.scope)
+            scope.usedNames += scope.declaredNames
-        super.visitCatch(x)
+            traverse(child)
-    }
+            scope.usedNames += child.usedNames.filter { !it.isTemporary }
-
-    override fun visitFunction(x: JsFunction) {
-        recordScope(x.scope)
-        super.visitFunction(x)
-    }
-    override fun visitElement(node: JsNode) {
-        if (node is HasName) {
-            node.name?.let { recordScope(it.enclosing) }
-        }
-        super.visitElement(node)
-    }
-
-    private fun recordScope(scope: JsScope) {
-        if (scope !in scopeTree) {
-            scopeTree[scope] = mutableSetOf()
-            val parent = scope.parent
-            if (parent != null) {
-                recordScope(parent)
-                scopeTree[parent]!!.add(scope)
-            }
         }
     }
+    traverse(this)
+    return this
+}
+
+private fun JsNode.computeScopes(): Scope {
+    val rootScope = Scope()
+    accept(object : RecursiveJsVisitor() {
+        var currentScope: Scope = rootScope
+
+        override fun visitFunction(x: JsFunction) {
+            x.name?.let { currentScope.declaredNames += it }
+            val oldScope = currentScope
+            currentScope = Scope().apply {
+                parent = currentScope
+                currentScope.children += this
+            }
+            currentScope.declaredNames += x.parameters.map { it.name }
+            super.visitFunction(x)
+            currentScope = oldScope
+        }
+
+        override fun visitCatch(x: JsCatch) {
+            currentScope.declaredNames += x.parameter.name
+            super.visitCatch(x)
+        }
+
+        override fun visit(x: JsVars.JsVar) {
+            currentScope.declaredNames += x.name
+            super.visit(x)
+        }
+
+        override fun visitNameRef(nameRef: JsNameRef) {
+            if (nameRef.qualifier == null) {
+                val name = nameRef.name
+                currentScope.usedNames += name ?: JsDynamicScope.declareName(nameRef.ident)
+            }
+
+            super.visitNameRef(nameRef)
+        }
+
+        override fun visitBreak(x: JsBreak) {}
+
+        override fun visitContinue(x: JsContinue) {}
+    })
+
+    return rootScope
+}
+
+private class Scope {
+    var parent: Scope? = null
+    val declaredNames = mutableSetOf<JsName>()
+    val usedNames = mutableSetOf<JsName>()
+    val children = mutableSetOf<Scope>()
 }

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

@@ -25,21 +25,6 @@ import org.jetbrains.kotlin.js.translate.utils.JsAstUtils
 fun collectFunctionReferencesInside(scope: JsNode): List<JsName> =
         collectReferencedNames(scope).filter { it.staticRef is JsFunction }
 
-fun collectReferencedTemporaryNames(scope: JsNode): Set<JsName> {
-    val references = mutableSetOf<JsName>()
-
-    object : RecursiveJsVisitor() {
-        override fun visitElement(node: JsNode) {
-            if (node is HasName) {
-                node.name?.let { references += it }
-            }
-            super.visitElement(node)
-        }
-    }.accept(scope)
-
-    return references
-}
-
 private fun collectReferencedNames(scope: JsNode): Set<JsName> {
     val references = mutableSetOf<JsName>()
 

js/js.tests/test/org/jetbrains/kotlin/js/test/ast/NameResolutionTest.kt

@@ -0,0 +1,92 @@
+/*
+ * 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.test.ast
+
+import com.google.gwt.dev.js.rhino.CodePosition
+import com.google.gwt.dev.js.rhino.ErrorReporter
+import com.intellij.openapi.util.io.FileUtil
+import org.jetbrains.kotlin.js.backend.ast.*
+import org.jetbrains.kotlin.js.inline.clean.resolveTemporaryNames
+import org.jetbrains.kotlin.js.parser.parse
+import org.jetbrains.kotlin.js.test.BasicTest
+import org.junit.Assert.assertEquals
+import org.junit.Assert.fail
+import org.junit.Rule
+import org.junit.Test
+import org.junit.rules.TestName
+import java.io.File
+
+class NameResolutionTest {
+    @Rule
+    @JvmField
+    var testName = TestName()
+
+    @Test
+    fun simple() = doTest()
+
+    @Test
+    fun reuseName() = doTest()
+
+    @Test
+    fun globalName() = doTest()
+
+    @Test
+    fun labels() = doTest()
+
+    private fun doTest() {
+        val methodName = testName.methodName
+        val baseName = "${BasicTest.TEST_DATA_DIR_PATH}/js-name-resolution/"
+        val originalName = "$baseName/$methodName.original.js"
+        val expectedName = "$baseName/$methodName.expected.js"
+
+        val originalCode = FileUtil.loadFile(File(originalName))
+        val expectedCode = FileUtil.loadFile(File(expectedName))
+
+        val parserScope = JsFunctionScope(JsRootScope(JsProgram()), "<js fun>")
+        val originalAst = JsGlobalBlock().apply { statements += parse(originalCode, errorReporter, parserScope) }
+        val expectedAst = JsGlobalBlock().apply { statements += parse(expectedCode, errorReporter, parserScope) }
+
+        originalAst.accept(object : RecursiveJsVisitor() {
+            val cache = mutableMapOf<JsName, JsName>()
+
+            override fun visitElement(node: JsNode) {
+                super.visitElement(node)
+                if (node is HasName) {
+                    node.name = node.name?.let { name ->
+                        if (name.ident.startsWith("$")) {
+                            cache.getOrPut(name) { JsDynamicScope.declareTemporaryName("x") }
+                        }
+                        else {
+                            name
+                        }
+                    }
+                }
+            }
+        })
+        originalAst.resolveTemporaryNames()
+
+        assertEquals(expectedAst.toString(), originalAst.toString())
+    }
+
+    private val errorReporter = object : ErrorReporter {
+        override fun warning(message: String, startPosition: CodePosition, endPosition: CodePosition) { }
+
+        override fun error(message: String, startPosition: CodePosition, endPosition: CodePosition) {
+            fail("Error parsing JS file: $message at $startPosition")
+        }
+    }
+}

js/js.translator/testData/js-name-resolution/globalName.expected.js

@@ -0,0 +1,10 @@
+var x_0 = 23;
+var x_1 = 42;
+
+function foo() {
+    println(x_0);
+}
+
+function bar() {
+    println(x + x_1);
+}

js/js.translator/testData/js-name-resolution/globalName.original.js

@@ -0,0 +1,10 @@
+var $a = 23;
+var $b = 42;
+
+function foo() {
+    println($a);
+}
+
+function bar() {
+    println(x + $b);
+}

js/js.translator/testData/js-name-resolution/labels.expected.js

@@ -0,0 +1,28 @@
+function foo() {
+    x: {
+        x_0: {
+            console.log("1");
+            console.log(function() {
+                x: {
+                    console.log("2");
+                }
+            });
+
+            if (condition()) {
+                break x;
+            }
+            else {
+                break x_0;
+            }
+        }
+    }
+
+    x: {
+        x_0: {
+            console.log("1");
+        }
+        x_0: {
+            console.log("1");
+        }
+    }
+}

js/js.translator/testData/js-name-resolution/labels.original.js

@@ -0,0 +1,28 @@
+function foo() {
+    $a: {
+        $b: {
+            console.log("1");
+            console.log(function() {
+                $aa: {
+                    console.log("2");
+                }
+            });
+
+            if (condition()) {
+                break $a;
+            }
+            else {
+                break $b;
+            }
+        }
+    }
+
+    $c: {
+        $d: {
+            console.log("1");
+        }
+        $e: {
+            console.log("1");
+        }
+    }
+}

js/js.translator/testData/js-name-resolution/reuseName.expected.js

@@ -0,0 +1,12 @@
+var x = 23;
+var x_0 = 42;
+
+function foo() {
+    var x_0 = 99;
+    console.log(x + x_0);
+}
+
+function bar() {
+    var x = 101;
+    console.log(x_0 + x);
+}

js/js.translator/testData/js-name-resolution/reuseName.original.js

@@ -0,0 +1,12 @@
+var $a = 23;
+var $b = 42;
+
+function foo() {
+    var $c = 99;
+    console.log($a + $c);
+}
+
+function bar() {
+    var $d = 101;
+    console.log($b + $d);
+}

js/js.translator/testData/js-name-resolution/simple.expected.js

@@ -0,0 +1,7 @@
+var x = 23;
+
+function x_0() {
+    var x_0 = 42;
+    var x_1 = 99;
+    return x + x_0 + x_1;
+}

js/js.translator/testData/js-name-resolution/simple.original.js

@@ -0,0 +1,7 @@
+var $a = 23;
+
+function $f() {
+    var $b = 42;
+    var $c = 99;
+    return $a + $b + $c;
+}