Java to Kotlin converter: more correct detection of primary constructor + constructor chains are supported (not completely correct code generated yet)

j2k/src/org/jetbrains/jet/j2k/ConstructorConverter.kt

@@ -28,10 +28,10 @@ class ConstructorConverter(private val converter: Converter) {
     private val typeConverter = converter.typeConverter
 
     public fun convertConstructor(constructor: PsiMethod,
-                                          annotations: Annotations,
+                                  annotations: Annotations,
-                                          modifiers: Modifiers,
+                                  modifiers: Modifiers,
-                                          membersToRemove: MutableSet<PsiMember>,
+                                  membersToRemove: MutableSet<PsiMember>,
-                                          postProcessBody: (Block) -> Block): Member {
+                                  postProcessBody: (Block) -> Block): Member {
         if (constructor.isPrimaryConstructor()) {
             return convertPrimaryConstructor(constructor, annotations, modifiers, membersToRemove, postProcessBody)
         }
@@ -164,64 +164,24 @@ class ConstructorConverter(private val converter: Converter) {
 
     public fun postProcessConstructors(classBody: ClassBody, psiClass: PsiClass): ClassBody {
         if (psiClass.getPrimaryConstructor() == null && psiClass.getConstructors().size > 1) {
-            return generateArtificialPrimaryConstructor(psiClass.declarationIdentifier(), classBody)
+            return generateArtificialPrimaryConstructor(psiClass.getName()!!, classBody)
         }
         else {
-            correctFactoryFunctions(classBody, psiClass.getName()!!)
+            replaceConstructorCallsInFactoryFunctions(classBody, psiClass.getName()!!)
             return classBody
         }
     }
 
-    private fun generateArtificialPrimaryConstructor(className: Identifier, classBody: ClassBody): ClassBody {
+    private fun generateArtificialPrimaryConstructor(className: String, classBody: ClassBody): ClassBody {
         assert(classBody.primaryConstructorSignature == null)
 
         val fieldsToInitialize = classBody.members.filterIsInstance(javaClass<Field>()).filter { it.isVal }
-        val initializers = HashMap<Field, Expression?>()
         for (factoryFunction in classBody.factoryFunctions()) {
-            for (field in fieldsToInitialize) {
+            val body = factoryFunction.body!!
-                initializers.put(field, getDefaultInitializer(field))
+            // 2 cases: secondary constructor either calls another constructor or does not call any
-            }
+            val newStatements = replaceConstructorCallInFactoryFunction(body, className) ?:
-
+                    insertCallToArtificialPrimary(body, className, fieldsToInitialize)
-            val statements = ArrayList<Statement>()
+            factoryFunction.body = Block(newStatements, LBrace().assignNoPrototype(), RBrace().assignNoPrototype()).assignNoPrototype()
-            for (statement in factoryFunction.body!!.statements) {
-                var keepStatement = true
-                if (statement is AssignmentExpression) {
-                    val assignee = statement.left
-                    if (assignee is QualifiedExpression && (assignee.qualifier as? Identifier)?.name == tempValName) {
-                        val name = (assignee.identifier as Identifier).name
-                        for (field in fieldsToInitialize) {
-                            if (name == field.identifier.name) {
-                                initializers.put(field, statement.right)
-                                keepStatement = false
-                            }
-
-                        }
-                    }
-
-                }
-
-                if (keepStatement) {
-                    statements.add(statement)
-                }
-            }
-
-            val arguments = fieldsToInitialize.map { initializers[it] ?: LiteralExpression("null").assignNoPrototype() }
-            val initializer = MethodCallExpression.buildNotNull(null, className.name, arguments).assignNoPrototype()
-            if (statements.isNotEmpty()) {
-                val localVar = LocalVariable(tempValIdentifier(),
-                                             Annotations.Empty,
-                                             Modifiers.Empty,
-                                             null,
-                                             initializer,
-                                             true).assignNoPrototype()
-                statements.add(0, DeclarationStatement(listOf(localVar)).assignNoPrototype())
-                statements.add(ReturnStatement(tempValIdentifier()).assignNoPrototype())
-            }
-            else {
-                statements.add(ReturnStatement(initializer).assignNoPrototype())
-            }
-
-            factoryFunction.body = Block(statements, LBrace().assignNoPrototype(), RBrace().assignNoPrototype()).assignNoPrototype()
         }
 
         val parameters = fieldsToInitialize.map { field ->
@@ -236,15 +196,17 @@ class ConstructorConverter(private val converter: Converter) {
         return ClassBody(constructorSignature, updatedMembers, classBody.classObjectMembers, classBody.lBrace, classBody.rBrace)
     }
 
-    private fun correctFactoryFunctions(classBody: ClassBody, className: String) {
+    private fun replaceConstructorCallsInFactoryFunctions(classBody: ClassBody, className: String) {
         for (factoryFunction in classBody.factoryFunctions()) {
             val body = factoryFunction.body!!
-            val statements = correctFactoryFunctionStatements(body, className)
+            val statements = replaceConstructorCallInFactoryFunction(body, className)
-            factoryFunction.body = Block(statements, body.lBrace, body.rBrace).assignPrototypesFrom(body)
+            if (statements != null) {
+                factoryFunction.body = Block(statements, body.lBrace, body.rBrace).assignPrototypesFrom(body)
+            }
         }
     }
 
-    private fun correctFactoryFunctionStatements(body: Block, className: String): List<Statement> {
+    private fun replaceConstructorCallInFactoryFunction(body: Block, className: String): List<Statement>? {
         val statements = ArrayList(body.statements)
 
         // searching for other constructor call in form "this(...)"
@@ -260,12 +222,59 @@ class ConstructorConverter(private val converter: Converter) {
                     }
                     val localVar = LocalVariable(tempValIdentifier(), Annotations.Empty, Modifiers.Empty, null, constructorCall, true).assignNoPrototype()
                     statements[i] = DeclarationStatement(listOf(localVar)).assignNoPrototype()
-                    break
+                    statements.add(ReturnStatement(tempValIdentifier()).assignNoPrototype())
+                    return statements
                 }
             }
         }
 
-        statements.add(ReturnStatement(tempValIdentifier()).assignNoPrototype())
+        return null
+    }
+
+    private fun insertCallToArtificialPrimary(body: Block, className: String, fieldsToInitialize: Collection<Field>): List<Statement> {
+        val initializers = HashMap<Field, Expression?>()
+        for (field in fieldsToInitialize) {
+            initializers.put(field, getDefaultInitializer(field))
+        }
+
+        val statements = ArrayList<Statement>()
+        for (statement in body.statements) {
+            var keepStatement = true
+            if (statement is AssignmentExpression) {
+                val assignee = statement.left
+                if (assignee is QualifiedExpression && (assignee.qualifier as? Identifier)?.name == tempValName) {
+                    val name = (assignee.identifier as Identifier).name
+                    for (field in fieldsToInitialize) {
+                        if (name == field.identifier.name) {
+                            initializers.put(field, statement.right)
+                            keepStatement = false
+                        }
+
+                    }
+                }
+
+            }
+
+            if (keepStatement) {
+                statements.add(statement)
+            }
+        }
+
+        val arguments = fieldsToInitialize.map { initializers[it] ?: LiteralExpression("null").assignNoPrototype() }
+        val initializer = MethodCallExpression.buildNotNull(null, className, arguments).assignNoPrototype()
+        if (statements.isNotEmpty()) {
+            val localVar = LocalVariable(tempValIdentifier(),
+                                         Annotations.Empty,
+                                         Modifiers.Empty,
+                                         null,
+                                         initializer,
+                                         true).assignNoPrototype()
+            statements.add(0, DeclarationStatement(listOf(localVar)).assignNoPrototype())
+            statements.add(ReturnStatement(tempValIdentifier()).assignNoPrototype())
+        }
+        else {
+            statements.add(ReturnStatement(initializer).assignNoPrototype())
+        }
         return statements
     }
 

j2k/src/org/jetbrains/jet/j2k/ConstructorUtils.kt

@@ -17,7 +17,7 @@
 package org.jetbrains.jet.j2k
 
 import com.intellij.psi.*
-import java.util.LinkedHashSet
+import java.util.HashMap
 
 fun PsiMethod.isPrimaryConstructor(): Boolean {
     if (!isConstructor()) return false
@@ -28,33 +28,39 @@ fun PsiMethod.isPrimaryConstructor(): Boolean {
 
 fun PsiClass.getPrimaryConstructor(): PsiMethod? {
     val constructors = getConstructors()
-    return when (constructors.size) {
+    when (constructors.size) {
-        0 -> null
+        0 -> return null
 
-        1 -> constructors.single()
+        1 -> return constructors.single()
 
         else -> {
-            // if there is more than one constructor then choose one invoked by all others
+            val toTargetConstructorMap = HashMap<PsiMethod, PsiMethod>()
-            //TODO: logic is incorrect - there can be a constructor which does not call any other
+            for (constructor in constructors) {
-            class Visitor() : JavaRecursiveElementVisitor() {
+                val firstStatement = constructor.getBody()?.getStatements()?.firstOrNull()
-                //TODO: skip all non-constructor members (optimization)
+                val refExpr = ((firstStatement as? PsiExpressionStatement)
-                private val invokedConstructors = LinkedHashSet<PsiMethod>()
+                        ?.getExpression() as? PsiMethodCallExpression)
-
+                            ?.getMethodExpression()
-                override fun visitReferenceExpression(expression: PsiReferenceExpression) {
+                if (refExpr != null && refExpr.getCanonicalText() == "this") {
-                    expression.getReferences()
+                    val target = refExpr.resolve() as? PsiMethod
-                            .filter { it.getCanonicalText() == "this" }
+                    if (target != null && target.isConstructor()) {
-                            .map { it.resolve() }
+                        val finalTarget = toTargetConstructorMap[target] ?: target!!/*TODO: see KT-5335*/
-                            .filterIsInstance(javaClass<PsiMethod>())
+                        toTargetConstructorMap[constructor] = finalTarget
-                            .filterTo(invokedConstructors) { it.isConstructor() }
+                        for (entry in toTargetConstructorMap.entrySet()) {
+                            if (entry.getValue() == constructor) {
+                                entry.setValue(finalTarget)
+                            }
+                        }
+                    }
                 }
-
-                val primaryConstructor: PsiMethod?
-                    get() = if (invokedConstructors.size == 1) invokedConstructors.single() else null
             }
 
-            val visitor = Visitor()
+            val candidates = constructors.filter { it !in toTargetConstructorMap }
-            accept(visitor)
+            if (candidates.size != 1) return null // there should be only one constructor which does not call other constructor
-            visitor.primaryConstructor
+            val candidate = candidates.single()
+            return if (toTargetConstructorMap.values().all { it == candidate } /* all other constructors call our candidate (directly or indirectly)*/)
+                candidate
+            else
+                null
         }
     }
 }
@@ -82,7 +88,4 @@ fun PsiMethodCallExpression.isSuperConstructorCall(): Boolean {
     return false
 }
 
-fun PsiReferenceExpression.isThisConstructorCall(): Boolean
-        = getReferences().filter { it.getCanonicalText() == "this" }.map { it.resolve() }.any { it is PsiMethod && it.isConstructor() }
-
 fun PsiElement.isConstructor(): Boolean = this is PsiMethod && this.isConstructor()

j2k/src/org/jetbrains/jet/j2k/visitors/ExpressionVisitor.kt

@@ -267,7 +267,7 @@ open class ExpressionVisitor(private val converter: Converter,
         var arguments = expression.getArgumentList()?.getExpressions() ?: array()
 
         val constructor = expression.resolveMethod()
-        if (constructor != null && !constructor.isPrimaryConstructor() && converter.conversionScope.contains(constructor)) {
+        if (constructor != null && converter.conversionScope.contains(constructor) && !constructor.isPrimaryConstructor()) {
             //TODO: handle anonymous class!
             // non-primary constructor converted to factory method in class object
             val reference = expression.getClassReference()

j2k/tests/test/org/jetbrains/jet/j2k/test/JavaToKotlinConverterTestGenerated.java

@@ -811,6 +811,11 @@ public class JavaToKotlinConverterTestGenerated extends AbstractJavaToKotlinConv
             JetTestUtils.assertAllTestsPresentByMetadata(this.getClass(), "org.jetbrains.jet.generators.tests.TestsPackage", new File("j2k/tests/testData/ast/constructors"), Pattern.compile("^(.+)\\.java$"), true);
         }
         
+        @TestMetadata("chain.java")
+        public void testChain() throws Exception {
+            doTest("j2k/tests/testData/ast/constructors/chain.java");
+        }
+        
         @TestMetadata("customerBuilder.java")
         public void testCustomerBuilder() throws Exception {
             doTest("j2k/tests/testData/ast/constructors/customerBuilder.java");
@@ -866,6 +871,11 @@ public class JavaToKotlinConverterTestGenerated extends AbstractJavaToKotlinConv
             doTest("j2k/tests/testData/ast/constructors/methodCallInFactoryFun.java");
         }
         
+        @TestMetadata("noPrimary.java")
+        public void testNoPrimary() throws Exception {
+            doTest("j2k/tests/testData/ast/constructors/noPrimary.java");
+        }
+        
         @TestMetadata("parameterModification.java")
         public void testParameterModification() throws Exception {
             doTest("j2k/tests/testData/ast/constructors/parameterModification.java");

j2k/tests/testData/ast/constructors/chain.java

@@ -0,0 +1,23 @@
+//file
+class C {
+  C(int arg1, int arg2, int arg3) {
+  }
+
+  C(int arg1, int arg2) {
+    this(arg1, arg2, 0);
+    System.out.println();
+  }
+
+  C(int arg1) {
+    this(arg1, 0);
+    System.out.println();
+  }
+}
+
+public class User {
+  public static void main() {
+     C c1 = new C(1, 2, 3);
+     C c2 = new C(5, 6);
+     C c3 = new C(7);
+  }
+}

j2k/tests/testData/ast/constructors/chain.kt

@@ -0,0 +1,26 @@
+class C(arg1: Int, arg2: Int, arg3: Int) {
+    class object {
+
+        fun create(arg1: Int, arg2: Int): C {
+            val __ = C(arg1, arg2, 0)
+            System.out.println()
+            return __
+        }
+
+        fun create(arg1: Int): C {
+            val __ = C(arg1, 0)
+            System.out.println()
+            return __
+        }
+    }
+}
+
+public class User {
+    class object {
+        public fun main() {
+            val c1 = C(1, 2, 3)
+            val c2 = C.create(5, 6)
+            val c3 = C.create(7)
+        }
+    }
+}

j2k/tests/testData/ast/constructors/noPrimary.java

@@ -0,0 +1,22 @@
+//file
+class C {
+  C(int arg1, int arg2, int arg3) {
+  }
+
+  C(int arg1, int arg2) {
+    this(arg1, arg2, 0);
+    System.out.println();
+  }
+
+  C(int arg) {
+    System.out.println(arg);
+  }
+}
+
+public class User {
+  public static void main() {
+     C c1 = new C(1, 2, 3);
+     C c2 = new C(5, 6);
+     C c3 = new C(7);
+  }
+}

j2k/tests/testData/ast/constructors/noPrimary.kt

@@ -0,0 +1,29 @@
+class C private() {
+    class object {
+        fun create(arg1: Int, arg2: Int, arg3: Int): C {
+            return C()
+        }
+
+        fun create(arg1: Int, arg2: Int): C {
+            val __ = C(arg1, arg2, 0)
+            System.out.println()
+            return __
+        }
+
+        fun create(arg: Int): C {
+            val __ = C()
+            System.out.println(arg)
+            return __
+        }
+    }
+}
+
+public class User {
+    class object {
+        public fun main() {
+            val c1 = C.create(1, 2, 3)
+            val c2 = C.create(5, 6)
+            val c3 = C.create(7)
+        }
+    }
+}