Java to Kotlin conversion: minor refactorings after code review

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

@@ -16,20 +16,8 @@
 
 package org.jetbrains.jet.j2k
 
-import com.intellij.psi.PsiMethod
+import com.intellij.psi.*
-import com.intellij.psi.PsiClass
-import com.intellij.psi.PsiReferenceExpression
-import com.intellij.psi.JavaRecursiveElementVisitor
 import java.util.LinkedHashSet
-import com.intellij.psi.PsiElement
-import com.intellij.psi.PsiMethodCallExpression
-import com.intellij.psi.PsiParameter
-import com.intellij.psi.PsiField
-import com.intellij.psi.PsiAssignmentExpression
-import com.intellij.psi.PsiThisExpression
-import com.intellij.psi.PsiStatement
-import com.intellij.psi.PsiExpressionStatement
-import com.intellij.psi.PsiBlockStatement
 import com.intellij.psi.util.PsiUtil
 
 fun PsiMethod.isPrimaryConstructor(): Boolean {
@@ -48,6 +36,7 @@ fun PsiClass.getPrimaryConstructor(): PsiMethod? {
 
         else -> {
             // if there is more than one constructor then choose one invoked by all others
+            //TODO: logic is incorrect - there can be a constructor which does not call any other
             class Visitor() : JavaRecursiveElementVisitor() {
                 //TODO: skip all non-constructor members (optimization)
                 private val invokedConstructors = LinkedHashSet<PsiMethod>()
@@ -89,7 +78,7 @@ fun PsiElement.getContainingConstructor(): PsiMethod? {
 
 fun PsiMethodCallExpression.isSuperConstructorCall(): Boolean {
     val ref = getMethodExpression()
-    if (ref.getCanonicalText().equals("super")) {
+    if (ref.getCanonicalText() == "super") {
         val target = ref.resolve()
         return target is PsiMethod && target.isConstructor()
     }

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

@@ -122,6 +122,27 @@ public class Converter(val project: Project, val settings: ConverterSettings) {
 
             else -> {
                 if (psiClass.getPrimaryConstructor() == null && psiClass.getConstructors().size > 1) {
+                    generateArtificialPrimaryConstructor(name, classBodyElements)
+                }
+
+                val baseClassParams: List<Expression> = run {
+                    val superVisitor = SuperVisitor()
+                    psiClass.accept(superVisitor)
+                    val resolvedSuperCallParameters = superVisitor.resolvedSuperCallParameters
+                    if (resolvedSuperCallParameters.size() == 1) {
+                        convertExpressions(resolvedSuperCallParameters.single().getExpressions())
+                    }
+                    else {
+                        listOf()
+                    }
+                }
+
+                return Class(this, name, getComments(psiClass), modifiers, typeParameters, extendsTypes, baseClassParams, implementsTypes, classBodyElements)
+            }
+        }
+    }
+
+    private fun generateArtificialPrimaryConstructor(className: Identifier, classBodyElements: MutableList<Element>) {
         val finalOrWithEmptyInitializerFields = classBodyElements.filterIsInstance(javaClass<Field>()).filter { it.isVal() || it.initializer.toKotlin().isEmpty() }
         val initializers = HashMap<String, String>()
         for (element in classBodyElements) {
@@ -153,7 +174,7 @@ public class Converter(val project: Project, val settings: ConverterSettings) {
                     }
 
                 }
-                            newStatements.add(0, DummyStringExpression("val __ = " + createPrimaryConstructorInvocation(name.toKotlin(), finalOrWithEmptyInitializerFields, initializers)))
+                newStatements.add(0, DummyStringExpression("val __ = " + createPrimaryConstructorInvocation(className.toKotlin(), finalOrWithEmptyInitializerFields, initializers)))
                 element.block = Block(newStatements)
             }
         }
@@ -171,53 +192,6 @@ public class Converter(val project: Project, val settings: ConverterSettings) {
         classBodyElements.removeAll(finalOrWithEmptyInitializerFields)
     }
 
-                val baseClassParams: List<Expression> = run {
-                    val superVisitor = SuperVisitor()
-                    psiClass.accept(superVisitor)
-                    val resolvedSuperCallParameters = superVisitor.resolvedSuperCallParameters
-                    if (resolvedSuperCallParameters.size() == 1) {
-                        convertExpressions(resolvedSuperCallParameters.single().getExpressions())
-                    }
-                    else {
-                        listOf()
-                    }
-                }
-
-                return Class(this, name, getComments(psiClass), modifiers, typeParameters, extendsTypes, baseClassParams, implementsTypes, classBodyElements)
-            }
-        }
-    }
-
-    private fun findBackingFieldForConstructorParameter(parameter: PsiParameter, constructor: PsiMethod): Pair<PsiField, PsiStatement>? {
-        val body = constructor.getBody() ?: return null
-
-        val refs = findExpressionReferences(parameter, body)
-
-        if (refs.any { PsiUtil.isAccessedForWriting(it) }) return null
-
-        for(ref in refs) {
-            val assignment = ref.getParent() as? PsiAssignmentExpression ?: continue
-            if (assignment.getOperationSign().getTokenType() != JavaTokenType.EQ) continue
-            val assignee = assignment.getLExpression() as? PsiReferenceExpression ?: continue
-            if (!isQualifierEmptyOrThis(assignee)) continue
-            val field = assignee.resolve() as? PsiField ?: continue
-            if (field.getContainingClass() != constructor.getContainingClass()) continue
-            if (field.getInitializer() != null) continue
-
-            // assignment should be a top-level statement
-            val statement = assignment.getParent() as? PsiExpressionStatement ?: continue
-            if (statement.getParent() != body) continue
-
-            // and no other assignments to field should exist in the constructor
-            if (findExpressionReferences(field, body).any { it != assignee && PsiUtil.isAccessedForWriting(it) && isQualifierEmptyOrThis(it) }) continue
-            //TODO: check access to field before assignment
-
-            return field to statement
-        }
-
-        return null
-    }
-
     private fun convertInitializer(initializer: PsiClassInitializer): Initializer {
         return Initializer(convertBlock(initializer.getBody()), convertModifierList(initializer.getModifierList()))
     }
@@ -283,19 +257,42 @@ public class Converter(val project: Project, val settings: ConverterSettings) {
 
             if (method.isConstructor()) {
                 if (method.isPrimaryConstructor()) {
-                    val params = method.getParameterList().getParameters()
+                    return convertPrimaryConstructor(method, modifiers, comments, membersToRemove)
+                }
+                else {
+                    val params = convertParameterList(method.getParameterList())
+                    return SecondaryConstructor(this, comments, modifiers, params, convertBlock(method.getBody()))
+                }
+            }
+            else {
+                val params = convertParameterList(method.getParameterList())
+                val typeParameterList = convertTypeParameterList(method.getTypeParameterList())
+                val block = convertBlock(method.getBody())
+                return Function(this, Identifier(method.getName()), comments, modifiers, returnType, typeParameterList, params, block)
+            }
+        }
+        finally {
+            dispatcher.expressionVisitor = ExpressionVisitor(this)
+        }
+    }
+
+    private fun convertPrimaryConstructor(constructor: PsiMethod,
+                                          modifiers: Set<Modifier>,
+                                          comments: MemberComments,
+                                          membersToRemove: MutableSet<PsiMember>): PrimaryConstructor {
+        val params = constructor.getParameterList().getParameters()
         val parameterToField = HashMap<PsiParameter, PsiField>()
-                    val body = method.getBody()
+        val body = constructor.getBody()
         val block = if (body != null) {
             val statementsToRemove = HashSet<PsiStatement>()
             val usageReplacementMap = HashMap<PsiVariable, String>()
             for (parameter in params) {
-                            val (field, initializationStatement) = findBackingFieldForConstructorParameter(parameter, method) ?: continue
+                val (field, initializationStatement) = findBackingFieldForConstructorParameter(parameter, constructor) ?: continue
-                            if (membersToRemove.contains(field)) continue // already used as backing field
                 if (convertVariableType(field) != convertVariableType(parameter)) continue
 
                 parameterToField.put(parameter, field)
                 statementsToRemove.add(initializationStatement)
+                membersToRemove.add(field)
 
                 if (field.getName() != parameter.getName()) {
                     usageReplacementMap.put(parameter, field.getName()!!)
@@ -314,7 +311,6 @@ public class Converter(val project: Project, val settings: ConverterSettings) {
                 convertParameter(it)
             }
             else {
-                            membersToRemove.add(field)
                 Parameter(Identifier(field.getName()!!),
                           convertVariableType(it),
                           if (field.hasModifierProperty(PsiModifier.FINAL)) Parameter.VarValModifier.Val else Parameter.VarValModifier.Var,
@@ -323,21 +319,36 @@ public class Converter(val project: Project, val settings: ConverterSettings) {
         })
         return PrimaryConstructor(this, comments, modifiers, parameterList, block)
     }
-                else {
+
-                    val params = convertParameterList(method.getParameterList())
+    private fun findBackingFieldForConstructorParameter(parameter: PsiParameter, constructor: PsiMethod): Pair<PsiField, PsiStatement>? {
-                    return SecondaryConstructor(this, comments, modifiers, params, convertBlock(method.getBody()))
+        val body = constructor.getBody() ?: return null
-                }
+
-            }
+        val refs = findExpressionReferences(parameter, body)
-            else {
+
-                val params = convertParameterList(method.getParameterList())
+        if (refs.any { PsiUtil.isAccessedForWriting(it) }) return null
-                val typeParameterList = convertTypeParameterList(method.getTypeParameterList())
+
-                val block = convertBlock(method.getBody())
+        for(ref in refs) {
-                return Function(this, Identifier(method.getName()), comments, modifiers, returnType, typeParameterList, params, block)
+            val assignment = ref.getParent() as? PsiAssignmentExpression ?: continue
-            }
+            if (assignment.getOperationSign().getTokenType() != JavaTokenType.EQ) continue
-        }
+            val assignee = assignment.getLExpression() as? PsiReferenceExpression ?: continue
-        finally {
+            if (!isQualifierEmptyOrThis(assignee)) continue
-            dispatcher.expressionVisitor = ExpressionVisitor(this)
+            val field = assignee.resolve() as? PsiField ?: continue
+            if (field.getContainingClass() != constructor.getContainingClass()) continue
+            if (field.hasModifierProperty(PsiModifier.STATIC)) continue
+            if (field.getInitializer() != null) continue
+
+            // assignment should be a top-level statement
+            val statement = assignment.getParent() as? PsiExpressionStatement ?: continue
+            if (statement.getParent() != body) continue
+
+            // and no other assignments to field should exist in the constructor
+            if (findExpressionReferences(field, body).any { it != assignee && PsiUtil.isAccessedForWriting(it) && isQualifierEmptyOrThis(it) }) continue
+            //TODO: check access to field before assignment
+
+            return field to statement
         }
+
+        return null
     }
 
     public fun convertBlock(block: PsiCodeBlock?): Block {

j2k/src/org/jetbrains/jet/j2k/ast/Field.kt

@@ -48,10 +48,7 @@ open class Field(
         val declaration: String = commentsToKotlin() +
         modifiersToKotlin() + identifier.toKotlin() + " : " + `type`.toKotlin()
         if (initializer.isEmpty) {
-            return declaration + ((if (isVal() && !isStatic() && writingAccesses != 0)
+            return declaration + (if (isVal() && !isStatic() && writingAccesses != 0) "" else " = " + getDefaultInitializer(this))
-                ""
-            else
-                " = " + getDefaultInitializer(this)))
         }
 
         return declaration + " = " + initializer.toKotlin()

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

@@ -80,11 +80,11 @@ class StatementVisitor(public val converter: Converter) : JavaElementVisitor() {
         val condition = statement.getCondition()
         val body = statement.getBody()
 
-        val initializationVar = initialization?.getFirstChild() as? PsiLocalVariable
+        val loopVar = initialization?.getFirstChild() as? PsiLocalVariable
-        val onceWritableIterator = initializationVar != null
+        val onceWritableIterator = loopVar != null
-                && initializationVar.countWriteAccesses(body) == 0
+                && loopVar.countWriteAccesses(body) == 0
-                && initializationVar.countWriteAccesses(condition) == 0
+                && loopVar.countWriteAccesses(condition) == 0
-                && initializationVar.countWriteAccesses(update) == 1
+                && loopVar.countWriteAccesses(update) == 1
 
         val operationTokenType = (condition as? PsiBinaryExpression)?.getOperationTokenType()
         if (initialization is PsiDeclarationStatement
@@ -94,16 +94,16 @@ class StatementVisitor(public val converter: Converter) : JavaElementVisitor() {
                 && update.getChildren().size == 1
                 && isPlusPlusExpression(update.getChildren().single())
                 && (operationTokenType == JavaTokenType.LT || operationTokenType == JavaTokenType.LE)
-                && initializationVar != null
+                && loopVar != null
-                && initializationVar.getNameIdentifier() != null
+                && loopVar.getNameIdentifier() != null
                 && onceWritableIterator) {
             val end = converter.convertExpression((condition as PsiBinaryExpression).getROperand())
             val endExpression = if (operationTokenType == JavaTokenType.LT)
                 BinaryExpression(end, Identifier("1"), "-")
             else
                 end
-            result = ForeachWithRangeStatement(Identifier(initializationVar.getName()!!),
+            result = ForeachWithRangeStatement(Identifier(loopVar.getName()!!),
-                                                 converter.convertExpression(initializationVar.getInitializer()),
+                                                 converter.convertExpression(loopVar.getInitializer()),
                                                  endExpression,
                                                  converter.convertStatement(body))
         }