From de25d13f1a089f76ac166244348d5a75ba161ce6 Mon Sep 17 00:00:00 2001 From: Anton Bannykh Date: Tue, 21 Apr 2020 18:49:57 +0300 Subject: [PATCH] JS: fix JsName renamer (KT-36484 fixed) The following code was treated incorrectly: ``` function (_) { function foo() { try {} catch (_) {} try {} catch (_) {} } // _ is linked to the catch parameter here } ``` --- .../kotlin/js/inline/util/collectUtils.kt | 14 +- .../inline/util/fixForwardNameReferences.kt | 35 ++- .../js/test/ast/FixForwardReferencesTest.kt | 267 ++++++++++++++++++ 3 files changed, 294 insertions(+), 22 deletions(-) create mode 100644 js/js.tests/test/org/jetbrains/kotlin/js/test/ast/FixForwardReferencesTest.kt diff --git a/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/collectUtils.kt b/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/collectUtils.kt index 6e4152b053d..845903b645c 100644 --- a/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/collectUtils.kt +++ b/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/collectUtils.kt @@ -83,7 +83,9 @@ fun collectUsedNames(scope: JsNode): Set { return references } -fun collectDefinedNames(scope: JsNode): Set { +fun collectDefinedNames(scope: JsNode) = collectDefinedNames(scope, false) + +fun collectDefinedNames(scope: JsNode, skipLabelsAndCatches: Boolean): Set { val names = mutableSetOf() object : RecursiveJsVisitor() { @@ -107,12 +109,16 @@ fun collectDefinedNames(scope: JsNode): Set { } override fun visitLabel(x: JsLabel) { - x.name?.let { names += it } + if (!skipLabelsAndCatches) { + x.name?.let { names += it } + } super.visitLabel(x) } override fun visitCatch(x: JsCatch) { - names += x.parameter.name + if (!skipLabelsAndCatches) { + names += x.parameter.name + } super.visitCatch(x) } @@ -158,7 +164,7 @@ fun collectDefinedNamesInAllScopes(scope: JsNode): Set { fun JsFunction.collectFreeVariables() = collectUsedNames(body) - collectDefinedNames(body) - parameters.map { it.name } -fun JsFunction.collectLocalVariables() = collectDefinedNames(body) + parameters.map { it.name } +fun JsFunction.collectLocalVariables(skipLabelsAndCatches: Boolean = false) = collectDefinedNames(body, skipLabelsAndCatches) + parameters.map { it.name } fun collectNamedFunctions(scope: JsNode) = collectNamedFunctionsAndMetadata(scope).mapValues { it.value.first.function } diff --git a/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/fixForwardNameReferences.kt b/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/fixForwardNameReferences.kt index 81f6debdd35..517e3c93d2b 100644 --- a/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/fixForwardNameReferences.kt +++ b/js/js.inliner/src/org/jetbrains/kotlin/js/inline/util/fixForwardNameReferences.kt @@ -23,26 +23,30 @@ fun JsNode.fixForwardNameReferences() { val currentScope = mutableMapOf() init { - currentScope += collectDefinedNames(this@fixForwardNameReferences).associateBy { it.ident } + currentScope += collectDefinedNames(this@fixForwardNameReferences, skipLabelsAndCatches = true).associateBy { it.ident } + } + + private fun restore(ident: String, oldName: JsName?) { + if (oldName == null) { + currentScope -= ident + } else { + currentScope[ident] = oldName + } } override fun visitFunction(x: JsFunction) { - val scopeBackup = mutableMapOf() - val localVars = x.collectLocalVariables() - for (localVar in localVars) { - scopeBackup[localVar.ident] = currentScope[localVar.ident] + val localVars = x.collectLocalVariables(skipLabelsAndCatches = true).toList() + val backup = arrayOfNulls(localVars.size) + + localVars.forEachIndexed { index, localVar -> + backup[index] = currentScope[localVar.ident] currentScope[localVar.ident] = localVar } super.visitFunction(x) - for ((ident, oldName) in scopeBackup) { - if (oldName == null) { - currentScope -= ident - } - else { - currentScope[ident] = oldName - } + for (index in localVars.indices.reversed()) { + restore(localVars[index].ident, backup[index]) } } @@ -53,12 +57,7 @@ fun JsNode.fixForwardNameReferences() { super.visitCatch(x) - if (oldName != null) { - currentScope[name.ident] = name - } - else { - currentScope -= name.ident - } + restore(name.ident, oldName) } override fun visitNameRef(nameRef: JsNameRef) { diff --git a/js/js.tests/test/org/jetbrains/kotlin/js/test/ast/FixForwardReferencesTest.kt b/js/js.tests/test/org/jetbrains/kotlin/js/test/ast/FixForwardReferencesTest.kt new file mode 100644 index 00000000000..ea04595965e --- /dev/null +++ b/js/js.tests/test/org/jetbrains/kotlin/js/test/ast/FixForwardReferencesTest.kt @@ -0,0 +1,267 @@ +/* + * Copyright 2010-2020 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. + */ + +package org.jetbrains.kotlin.js.test.ast + +import com.google.gwt.dev.js.ThrowExceptionOnErrorReporter +import org.jetbrains.kotlin.js.backend.ast.* +import org.jetbrains.kotlin.js.inline.util.fixForwardNameReferences +import org.jetbrains.kotlin.js.parser.parse +import org.junit.Test +import kotlin.test.assertEquals + +class FixForwardReferencesTest { + + @Test + fun simple() { + """ + function foo() { + var a = 1; + return a; + } + """ parsesInto """ + function foo_0() { + var a_0 = 1; + return a_0; + } + """ transformsInto """ + function foo_0() { + var a_0 = 1; + return a_0; + } + """ + } + + @Test + fun functionParameterClashesWithCatch() { + """ + function foo(a) { + try {} catch (a) { + a; + } + try {} catch (a) { + a; + } + return a; + } + """ parsesInto """ + function foo_0(a_0) { + try {} catch (a_1) { + a_1; + } + try {} catch (a_2) { + a_2; + } + return a_0; + } + """ transformsInto """ + function foo_0(a_0) { + try {} catch (a_1) { + a_1; + } + try {} catch (a_2) { + a_2; + } + return a_0; + } + """ + } + + @Test + fun nestedCatchParameters() { + """ + try {} catch (a) { + a; + try { + a; + } catch (a) { + a; + } + a; + } + """ parsesInto """ + try {} catch (a_0) { + a_0; + try { + a_0; + } catch (a_1) { + a_1; + } + a_0; + } + """ transformsInto """ + try {} catch (a_0) { + a_0; + try { + a_0; + } catch (a_1) { + a_1; + } + a_0; + } + """ + } + + @Test + fun labelsAndVarClash() { + """ + function () { + var a; + a: while (a) { + break a; + } + a; + } + """ parsesInto """ + function () { + var a_0; + a_1: while (a_0) { + break a_1; + } + a_0; + } + """ transformsInto """ + function () { + var a_0; + a_1: while (a_0) { + break a_1; + } + a_0; + } + """ + } + + @Test fun catchParamAndVarClash() { + """ + { + var a; + try {} catch (a) { a } + a; + } + """ parsesInto """ + { + var a_0; + try {} catch (a_1) { a_1 } + a_0; + } + """ transformsInto """ + { + var a_0; + try {} catch (a_1) { a_1 } + a_0; + } + """ + } + + @Test fun catchParamAndVarClashInsideFunction() { + """ + function () { + var a; + try {} catch (a) { a } + a; + } + """ parsesInto """ + function () { + var a_0; + try {} catch (a_1) { a_1 } + a_0; + } + """ transformsInto """ + function () { + var a_0; + try {} catch (a_1) { a_1 } + a_0; + } + """ + } + + private infix fun String.parsesInto(result: String): String { + assertEquals(result.parse().toString(), parse().makeNamesUnique().toString(), "Unexpected parsing result") + return result + } + + private infix fun String.transformsInto(result: String): String { + val node = toAst() + node.fixForwardNameReferences() + node.makeNamesUnique() + + assertEquals(result.parse().toString(), node.toString(), "Unexpected transformation result") + return result + } + +} + +private fun String.parse(): JsNode = parse(this, ThrowExceptionOnErrorReporter, JsProgram().scope, "test")!!.single() + +private fun JsNode.transformEachName(transform: JsName.() -> JsName): JsNode { + accept(object : RecursiveJsVisitor() { + override fun visitFunction(x: JsFunction) { + x.name?.let { x.name = it.transform() } + super.visitFunction(x) + } + + override fun visitNameRef(nameRef: JsNameRef) { + nameRef.name?.let { nameRef.name = it.transform() } + super.visitNameRef(nameRef) + } + + override fun visitParameter(x: JsParameter) { + x.name = x.name.transform() + super.visitParameter(x) + } + + override fun visitLabel(x: JsLabel) { + x.name = x.name.transform() + super.visitLabel(x) + } + + override fun visit(x: JsVars.JsVar) { + x.name = x.name.transform() + + super.visit(x) + } + }) + + return this +} + +private fun String.toAst(): JsNode { + val nameMapping = mutableMapOf>() + + return parse().transformEachName { + nameMapping[ident]?.let { (oldName, newName) -> + if (oldName != this) error("Duplicate unique name: ${ident}") + newName + } ?: run { + + val parts = ident.split('_') + if (parts.size != 2) error("Unable to parse: $ident") + val (name, index) = parts + if (ident != "${name}_$index") error("Unable to parse: $ident") + + JsName(name).also { + nameMapping[ident] = this to it + } + } + } +} + +private fun JsNode.makeNamesUnique(): JsNode { + + val nameCounter = mutableMapOf() + val nameMap = mutableMapOf() + + return transformEachName { + nameMap[this] ?: run { + val index = nameCounter.getOrDefault(ident, 0).also { + nameCounter[ident] = it + 1 + } + + JsName("${ident}_$index").also { + nameMap[this] = it + } + } + } +} \ No newline at end of file