KT-3008 Fix UMD and plain module wrappers when module id is not a valid JS identifier

js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/MultiModuleWrappersTest.kt

@@ -51,10 +51,26 @@ class MultiModuleWrappersTest() : MultipleModulesTranslationTest("multiModuleWra
         runTest("simple")
     }
 
-    @WithModuleKind(ModuleKind.UMD) fun testPlain() {
+    @WithModuleKind(ModuleKind.PLAIN) fun testPlain() {
         runTest("simple")
     }
 
+    @WithModuleKind(ModuleKind.AMD) fun testAmdModuleWithNonIdentifierName() {
+        runTest("moduleWithNonIdentifierName")
+    }
+
+    @WithModuleKind(ModuleKind.COMMON_JS) fun testCommonJsModuleWithNonIdentifierName() {
+        runTest("moduleWithNonIdentifierName")
+    }
+
+    @WithModuleKind(ModuleKind.UMD) fun testUmdModuleWithNonIdentifierName() {
+        runTest("moduleWithNonIdentifierName")
+    }
+
+    @WithModuleKind(ModuleKind.PLAIN) fun testPlainModuleWithNonIdentifierName() {
+        runTest("moduleWithNonIdentifierName")
+    }
+
     fun runTest(name: String) {
         overridenTestName = name
         doTest("${pathToTestDir()}/cases/$name")

js/js.translator/src/org/jetbrains/kotlin/js/translate/context/Namer.java

@@ -539,4 +539,14 @@ public final class Namer {
 
         return sb.toString();
     }
+
+    public static boolean requiresEscaping(@NotNull String name) {
+        // TODO: remove if there is existing implementation of this method
+        // TODO: handle JavaScript keywords
+        if (name.isEmpty() || !Character.isJavaIdentifierStart(name.charAt(0))) return true;
+        for (int i = 1; i < name.length(); ++i) {
+            if (!Character.isJavaIdentifierPart(name.charAt(i))) return true;
+        }
+        return false;
+    }
 }

js/js.translator/src/org/jetbrains/kotlin/js/translate/general/Translation.java

@@ -290,9 +290,17 @@ public final class Translation {
         JsBlock amdBody = new JsBlock(wrapAmd(moduleId, factoryName.makeRef(), importedModules, program));
         JsBlock commonJsBody = new JsBlock(wrapCommonJs(factoryName.makeRef(), importedModules, program));
         JsInvocation plainInvocation = makePlainInvocation(factoryName.makeRef(), importedModules, program);
-        JsExpression plainExpr = moduleId != null ?
+
-                                 JsAstUtils.assignment(new JsNameRef(moduleId, rootName.makeRef()), plainInvocation) :
+        JsExpression plainExpr;
-                                 plainInvocation;
+        if (moduleId != null) {
+            JsExpression lhs = Namer.requiresEscaping(moduleId) ?
+                               new JsArrayAccess(rootName.makeRef(), program.getStringLiteral(moduleId)) :
+                               new JsNameRef(scope.declareName(moduleId), rootName.makeRef());
+            plainExpr = JsAstUtils.assignment(lhs, plainInvocation);
+        }
+        else {
+            plainExpr = plainInvocation;
+        }
 
         JsStatement selector = JsAstUtils.newJsIf(amdTest, amdBody, JsAstUtils.newJsIf(commonJsTest, commonJsBody, plainExpr.makeStmt()));
         JsFunction adapter = new JsFunction(program.getScope(), new JsBlock(selector), "UMD adapter");
@@ -363,7 +371,9 @@ public final class Translation {
             statement = invocation.makeStmt();
         }
         else {
-            statement = JsAstUtils.newVar(program.getScope().declareName(moduleId), invocation);
+            statement = Namer.requiresEscaping(moduleId) ?
+                        JsAstUtils.assignment(makePlainModuleRef(moduleId, program), invocation).makeStmt() :
+                        JsAstUtils.newVar(program.getRootScope().declareName(moduleId), invocation);
         }
 
         return Collections.singletonList(statement);
@@ -375,12 +385,21 @@ public final class Translation {
         List<JsExpression> invocationArgs = new ArrayList<JsExpression>(importedModules.size());
 
         for (ImportedModule importedModule : importedModules) {
-            invocationArgs.add(program.getRootScope().declareName(importedModule.id).makeRef());
+            invocationArgs.add(makePlainModuleRef(importedModule.id, program));
         }
 
         return new JsInvocation(function, invocationArgs);
     }
 
+    @NotNull
+    private static JsExpression makePlainModuleRef(@NotNull String moduleId, @NotNull JsProgram program) {
+        // TODO: we could use `this.moduleName` syntax. However, this does not work for `kotlin` module in Rhino, since
+        // we run kotlin.js in a parent scope. Consider better solution
+        return Namer.requiresEscaping(moduleId) ?
+               new JsArrayAccess(JsLiteral.THIS, program.getStringLiteral(moduleId)) :
+               program.getScope().declareName(moduleId).makeRef();
+    }
+
     private static void defineModule(@NotNull TranslationContext context, @NotNull List<JsStatement> statements, @NotNull String moduleId) {
         JsName rootPackageName = context.scope().findName(Namer.getRootPackageName());
         if (rootPackageName != null) {

js/js.translator/testData/multiModuleWrappers/cases/moduleWithNonIdentifierName/dependencies.txt

@@ -0,0 +1,2 @@
+module-1->
+main->module-1

js/js.translator/testData/multiModuleWrappers/cases/moduleWithNonIdentifierName/main/main.kt

@@ -0,0 +1,4 @@
+fun box(): String {
+    assertEquals("bar", bar())
+    return "OK"
+}

js/js.translator/testData/multiModuleWrappers/cases/moduleWithNonIdentifierName/module-1/module-1.kt

@@ -0,0 +1 @@
+fun bar() = "bar"