From 900adcf29bd62929ff2415e54c3b3d713f6679f6 Mon Sep 17 00:00:00 2001 From: Alexey Andreev Date: Wed, 28 Dec 2016 13:49:03 +0300 Subject: [PATCH] JS: prohibit stable (public) names with non-identifier chars. Rewrite unstable (private) names with non-identifier chars. See KT-4160 --- .../testsWithJsStdLib/name/illegalName.kt | 20 +++++++ .../testsWithJsStdLib/name/illegalName.txt | 16 +++++ .../DiagnosticsTestWithJsStdLibGenerated.java | 6 ++ .../kotlin/js/naming/NameSuggestion.kt | 7 +++ .../js/resolve/JsPlatformConfigurator.kt | 1 + .../diagnostics/DefaultErrorMessagesJs.kt | 4 ++ .../js/resolve/diagnostics/ErrorsJs.java | 5 ++ .../resolve/diagnostics/JsNameCharsChecker.kt | 58 +++++++++++++++++++ .../js/test/semantics/BoxJsTestGenerated.java | 6 ++ .../js/translate/context/StaticContext.java | 6 +- .../js/translate/context/UsageTracker.kt | 4 +- .../utils/FunctionBodyTranslator.java | 3 +- .../box/jsName/peculiarIdentifiers.kt | 34 +++++++++++ .../testData/jsTester/jsTester.kt | 6 +- 14 files changed, 166 insertions(+), 10 deletions(-) create mode 100644 compiler/testData/diagnostics/testsWithJsStdLib/name/illegalName.kt create mode 100644 compiler/testData/diagnostics/testsWithJsStdLib/name/illegalName.txt create mode 100644 js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/JsNameCharsChecker.kt create mode 100644 js/js.translator/testData/box/jsName/peculiarIdentifiers.kt diff --git a/compiler/testData/diagnostics/testsWithJsStdLib/name/illegalName.kt b/compiler/testData/diagnostics/testsWithJsStdLib/name/illegalName.kt new file mode 100644 index 00000000000..c0529ded3d3 --- /dev/null +++ b/compiler/testData/diagnostics/testsWithJsStdLib/name/illegalName.kt @@ -0,0 +1,20 @@ +private fun ` .private `(): String = TODO("") + +fun ` .public `(): String = TODO("") + +@JsName(" __ ") +fun foo(): String = TODO("") + +@JsName(" ___ ") +private fun bar(): String = TODO("") + +@JsName("validName") +private fun ` .private with @JsName `(): String = TODO("") + +private class ` .private class ` { + val ` .field. ` = "" +} + +val x: Int + @JsName(".") + get() = TODO("") \ No newline at end of file diff --git a/compiler/testData/diagnostics/testsWithJsStdLib/name/illegalName.txt b/compiler/testData/diagnostics/testsWithJsStdLib/name/illegalName.txt new file mode 100644 index 00000000000..bdfdc841ee7 --- /dev/null +++ b/compiler/testData/diagnostics/testsWithJsStdLib/name/illegalName.txt @@ -0,0 +1,16 @@ +package + +public val x: kotlin.Int +private fun ` .private `(): kotlin.String +@kotlin.js.JsName(name = "validName") private fun ` .private with @JsName `(): kotlin.String +public fun ` .public `(): kotlin.String +@kotlin.js.JsName(name = " ___ ") private fun bar(): kotlin.String +@kotlin.js.JsName(name = " __ ") public fun foo(): kotlin.String + +private final class ` .private class ` { + public constructor ` .private class `() + public final val ` .field. `: kotlin.String = "" + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String +} diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestWithJsStdLibGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestWithJsStdLibGenerated.java index 737cd8f58b2..222546a7448 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestWithJsStdLibGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestWithJsStdLibGenerated.java @@ -440,6 +440,12 @@ public class DiagnosticsTestWithJsStdLibGenerated extends AbstractDiagnosticsTes doTest(fileName); } + @TestMetadata("illegalName.kt") + public void testIllegalName() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/testsWithJsStdLib/name/illegalName.kt"); + doTest(fileName); + } + @TestMetadata("jsNameAndNamedNative.kt") public void testJsNameAndNamedNative() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/testsWithJsStdLib/name/jsNameAndNamedNative.kt"); diff --git a/js/js.frontend/src/org/jetbrains/kotlin/js/naming/NameSuggestion.kt b/js/js.frontend/src/org/jetbrains/kotlin/js/naming/NameSuggestion.kt index 703a42a888f..08bc25db4f4 100644 --- a/js/js.frontend/src/org/jetbrains/kotlin/js/naming/NameSuggestion.kt +++ b/js/js.frontend/src/org/jetbrains/kotlin/js/naming/NameSuggestion.kt @@ -330,5 +330,12 @@ class NameSuggestion { private val DeclarationDescriptorWithVisibility.ownEffectiveVisibility get() = visibility.effectiveVisibility(this, checkPublishedApi = true).toVisibility() + + @JvmStatic fun sanitizeName(name: String): String { + if (name.isEmpty()) return "_" + + val first = name.first().let { if (java.lang.Character.isJavaIdentifierStart(it)) it else '_' } + return first.toString() + name.drop(1).map { if (java.lang.Character.isJavaIdentifierPart(it)) it else '_' }.joinToString("") + } } } diff --git a/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/JsPlatformConfigurator.kt b/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/JsPlatformConfigurator.kt index 29929b6f8c2..b5ac05d2758 100644 --- a/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/JsPlatformConfigurator.kt +++ b/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/JsPlatformConfigurator.kt @@ -57,6 +57,7 @@ object JsPlatformConfigurator : PlatformConfigurator( container.useInstance(SyntheticConstructorsProvider.Empty) container.useInstance(JsTypeSpecificityComparator) container.useInstance(JsNameClashChecker()) + container.useInstance(JsNameCharsChecker()) container.useImpl() container.useImpl() container.useImpl() diff --git a/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/DefaultErrorMessagesJs.kt b/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/DefaultErrorMessagesJs.kt index a4a629caa41..0c7e5d6b91a 100644 --- a/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/DefaultErrorMessagesJs.kt +++ b/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/DefaultErrorMessagesJs.kt @@ -39,6 +39,7 @@ private val DIAGNOSTIC_FACTORY_TO_RENDERER by lazy { put(ErrorsJs.JSCODE_NO_JAVASCRIPT_PRODUCED, "Argument must be non-empty JavaScript code") put(ErrorsJs.NESTED_EXTERNAL_DECLARATION, "Non top-level `external` declaration") put(ErrorsJs.WRONG_EXTERNAL_DECLARATION, "Declaration of such kind ({0}) can't be external", Renderers.STRING) + put(ErrorsJs.JS_NAME_CLASH, "JavaScript name ({0}) generated for this declaration clashes with another declaration: {1}", Renderers.STRING, Renderers.COMPACT) put(ErrorsJs.JS_FAKE_NAME_CLASH, "JavaScript name {0} is generated for different inherited members: {1} and {2}", @@ -49,6 +50,9 @@ private val DIAGNOSTIC_FACTORY_TO_RENDERER by lazy { put(ErrorsJs.JS_NAME_PROHIBITED_FOR_OVERRIDE, "@JsName is prohibited for overridden members") put(ErrorsJs.JS_NAME_PROHIBITED_FOR_EXTENSION_PROPERTY, "@JsName is prohibited for extension properties") put(ErrorsJs.JS_NAME_PROHIBITED_FOR_NAMED_NATIVE, "@JsName is prohibited for external declaration with explicit name") + + put(ErrorsJs.NAME_CONTAINS_ILLEGAL_CHARS, "Name contains illegal chars that can't appear in JavaScript identifier") + put(ErrorsJs.JS_MODULE_PROHIBITED_ON_VAR, "@JsModule and @JsNonModule annotations prohibited for 'var' declarations. " + "Use 'val' instead.") put(ErrorsJs.JS_MODULE_PROHIBITED_ON_NON_NATIVE, "@JsModule and @JsNonModule annotations prohibited for non-external declarations.") diff --git a/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/ErrorsJs.java b/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/ErrorsJs.java index 31001b4c3af..e5bb1c4d979 100644 --- a/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/ErrorsJs.java +++ b/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/ErrorsJs.java @@ -42,6 +42,7 @@ public interface ErrorsJs { DiagnosticFactory0 JSCODE_NO_JAVASCRIPT_PRODUCED = DiagnosticFactory0.create(ERROR, DEFAULT); DiagnosticFactory1 WRONG_EXTERNAL_DECLARATION = DiagnosticFactory1.create(ERROR, DECLARATION_SIGNATURE_OR_DEFAULT); DiagnosticFactory0 NESTED_EXTERNAL_DECLARATION = DiagnosticFactory0.create(ERROR, DECLARATION_SIGNATURE_OR_DEFAULT); + DiagnosticFactory2 JS_NAME_CLASH = DiagnosticFactory2.create( ERROR, DECLARATION_SIGNATURE_OR_DEFAULT); DiagnosticFactory3 JS_FAKE_NAME_CLASH = @@ -52,6 +53,10 @@ public interface ErrorsJs { DiagnosticFactory0 JS_NAME_PROHIBITED_FOR_OVERRIDE = DiagnosticFactory0.create(ERROR); DiagnosticFactory0 JS_NAME_PROHIBITED_FOR_EXTENSION_PROPERTY = DiagnosticFactory0.create(ERROR); DiagnosticFactory0 JS_NAME_PROHIBITED_FOR_NAMED_NATIVE = DiagnosticFactory0.create(ERROR); + + DiagnosticFactory0 NAME_CONTAINS_ILLEGAL_CHARS = DiagnosticFactory0.create( + ERROR, DECLARATION_SIGNATURE_OR_DEFAULT); + DiagnosticFactory0 JS_MODULE_PROHIBITED_ON_VAR = DiagnosticFactory0.create(ERROR, DECLARATION_SIGNATURE_OR_DEFAULT); DiagnosticFactory0 JS_MODULE_PROHIBITED_ON_NON_NATIVE = DiagnosticFactory0.create(ERROR, DECLARATION_SIGNATURE_OR_DEFAULT); DiagnosticFactory0 NESTED_JS_MODULE_PROHIBITED = DiagnosticFactory0.create(ERROR, DECLARATION_SIGNATURE_OR_DEFAULT); diff --git a/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/JsNameCharsChecker.kt b/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/JsNameCharsChecker.kt new file mode 100644 index 00000000000..2e45bdbdeae --- /dev/null +++ b/js/js.frontend/src/org/jetbrains/kotlin/js/resolve/diagnostics/JsNameCharsChecker.kt @@ -0,0 +1,58 @@ +/* + * 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. + */ + +/* + * 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.resolve.diagnostics + +import org.jetbrains.kotlin.descriptors.DeclarationDescriptor +import org.jetbrains.kotlin.descriptors.PropertyAccessorDescriptor +import org.jetbrains.kotlin.diagnostics.DiagnosticSink +import org.jetbrains.kotlin.js.naming.NameSuggestion +import org.jetbrains.kotlin.js.translate.utils.AnnotationsUtils +import org.jetbrains.kotlin.psi.KtDeclaration +import org.jetbrains.kotlin.resolve.BindingContext +import org.jetbrains.kotlin.resolve.checkers.SimpleDeclarationChecker + +class JsNameCharsChecker() : SimpleDeclarationChecker { + val suggestion = NameSuggestion() + + override fun check( + declaration: KtDeclaration, descriptor: DeclarationDescriptor, + diagnosticHolder: DiagnosticSink, bindingContext: BindingContext + ) { + if (descriptor is PropertyAccessorDescriptor && AnnotationsUtils.getJsName(descriptor) == null) return + + val suggestedName = suggestion.suggest(descriptor) ?: return + if (suggestedName.stable && suggestedName.names.any { NameSuggestion.sanitizeName(it) != it }) { + diagnosticHolder.report(ErrorsJs.NAME_CONTAINS_ILLEGAL_CHARS.on(declaration)) + } + } +} \ No newline at end of file diff --git a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java index e41dd188d51..5b8b5af56e0 100644 --- a/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java +++ b/js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java @@ -5165,6 +5165,12 @@ public class BoxJsTestGenerated extends AbstractBoxJsTest { doTest(fileName); } + @TestMetadata("peculiarIdentifiers.kt") + public void testPeculiarIdentifiers() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("js/js.translator/testData/box/jsName/peculiarIdentifiers.kt"); + doTest(fileName); + } + @TestMetadata("privateMethod.kt") public void testPrivateMethod() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("js/js.translator/testData/box/jsName/privateMethod.kt"); diff --git a/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/StaticContext.java b/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/StaticContext.java index bab9f699bee..b8045311c11 100644 --- a/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/StaticContext.java +++ b/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/StaticContext.java @@ -368,7 +368,7 @@ public final class StaticContext { assert suggested.getNames().size() == 1 : "Private names must always consist of exactly one name"; JsName name = nameCache.get(suggested.getDescriptor()); if (name == null) { - String baseName = suggested.getNames().get(0); + String baseName = NameSuggestion.sanitizeName(suggested.getNames().get(0)); if (suggested.getDescriptor() instanceof LocalVariableDescriptor || suggested.getDescriptor() instanceof ValueParameterDescriptor ) { @@ -530,14 +530,14 @@ public final class StaticContext { } } else { - suggestedName = descriptor.getName().asString(); + suggestedName = NameSuggestion.sanitizeName(descriptor.getName().asString()); } } if (!(descriptor instanceof PackageFragmentDescriptor) && !DescriptorUtils.isTopLevelDeclaration(descriptor)) { DeclarationDescriptor container = descriptor.getContainingDeclaration(); assert container != null : "We just figured out that descriptor is not for a top-level declaration: " + descriptor; - suggestedName = getSuggestedName(container) + "$" + suggestedName; + suggestedName = getSuggestedName(container) + "$" + NameSuggestion.sanitizeName(suggestedName); } return suggestedName; diff --git a/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/UsageTracker.kt b/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/UsageTracker.kt index 949bc37caef..504fe90b7dd 100644 --- a/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/UsageTracker.kt +++ b/js/js.translator/src/org/jetbrains/kotlin/js/translate/context/UsageTracker.kt @@ -166,12 +166,12 @@ class UsageTracker( // Append 'closure$' prefix to avoid name clash between closure and member fields in case of local classes else -> { - val mangled = NameSuggestion().suggest(this)!!.names.last() + val mangled = NameSuggestion.sanitizeName(NameSuggestion().suggest(this)!!.names.last()) "closure\$$mangled" } } - return scope.declareFreshName(suggestedName) + return scope.declareTemporaryName(suggestedName) } } diff --git a/js/js.translator/src/org/jetbrains/kotlin/js/translate/utils/FunctionBodyTranslator.java b/js/js.translator/src/org/jetbrains/kotlin/js/translate/utils/FunctionBodyTranslator.java index c33539f47de..a2d1f2b456f 100644 --- a/js/js.translator/src/org/jetbrains/kotlin/js/translate/utils/FunctionBodyTranslator.java +++ b/js/js.translator/src/org/jetbrains/kotlin/js/translate/utils/FunctionBodyTranslator.java @@ -24,6 +24,7 @@ import org.jetbrains.kotlin.descriptors.ConstructorDescriptor; import org.jetbrains.kotlin.descriptors.DeclarationDescriptor; import org.jetbrains.kotlin.descriptors.FunctionDescriptor; import org.jetbrains.kotlin.descriptors.ValueParameterDescriptor; +import org.jetbrains.kotlin.js.naming.NameSuggestion; import org.jetbrains.kotlin.js.translate.context.Namer; import org.jetbrains.kotlin.js.translate.context.TranslationContext; import org.jetbrains.kotlin.js.translate.expression.LocalFunctionCollector; @@ -58,7 +59,7 @@ public final class FunctionBodyTranslator extends AbstractTranslator { for (FunctionDescriptor localFunction : functionCollector.getFunctions()) { String localIdent = localFunction.getName().isSpecial() ? "lambda" : localFunction.getName().asString(); - JsName localName = functionBodyContext.scope().getParent().declareTemporaryName(localIdent); + JsName localName = functionBodyContext.scope().getParent().declareTemporaryName(NameSuggestion.sanitizeName(localIdent)); JsExpression alias = JsAstUtils.pureFqn(localName, null); aliases.put(localFunction, alias); } diff --git a/js/js.translator/testData/box/jsName/peculiarIdentifiers.kt b/js/js.translator/testData/box/jsName/peculiarIdentifiers.kt new file mode 100644 index 00000000000..bfdce4a60fd --- /dev/null +++ b/js/js.translator/testData/box/jsName/peculiarIdentifiers.kt @@ -0,0 +1,34 @@ +private fun `+`(a: Int, b: Int) = a + b + +@JsName("minus") +fun `-`(a: Int, b: Int) = a - b + +fun test1(): Int { + val ` x ` = 23 + val `*` = 3 + return `-`(` x `, `*`) +} + +fun test2(`p 1`: Int, `.p 2`: Int) = `+`(`p 1`, `.p 2`) + +fun test3(): String { + val `#` = "K" + class ` `(private val `::`: String) { + internal fun `@`() = `::` + `#` + } + return ` `("O").`@`() +} + +fun test4(): String { + val `()` = "OK" + fun `[]`() = `()` + return `[]`() +} + +fun box(): String { + if (test1() != 20) return "fail1" + if (test2(10, 13) != 23) return "fail2" + if (test3() != "OK") return "fail3" + if (test4() != "OK") return "fail4" + return "OK" +} \ No newline at end of file diff --git a/js/js.translator/testData/jsTester/jsTester.kt b/js/js.translator/testData/jsTester/jsTester.kt index d24618cc769..3ed9bd6ac72 100644 --- a/js/js.translator/testData/jsTester/jsTester.kt +++ b/js/js.translator/testData/jsTester/jsTester.kt @@ -8,8 +8,6 @@ public class JsTestsAsserter() : Asserter { public override fun fail(message: String?): Nothing = failWithMessage(message) } -@JsName("JsTests.assert") -public external fun assert(value: Boolean, message: String?): Unit = noImpl +public inline fun assert(value: Boolean, message: String?): Unit = js("JsTests").assert(value, message) -@JsName("JsTests.fail") -private external fun failWithMessage(message: String?): Nothing = noImpl +private inline fun failWithMessage(message: String?): Nothing = js("JsTests").fail(message)