Java to Kotlin converter: more correct logic for specifying field type

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

@@ -251,8 +251,7 @@ public class Converter private(val project: Project, val settings: ConverterSett
                                          Modifiers.Empty,
                                          null,
                                          initializer,
-                                         true,
+                                         true).assignNoPrototype()
-                                         settings).assignNoPrototype()
             newStatements.add(0, DeclarationStatement(listOf(localVar)).assignNoPrototype())
             constructor.block = Block(newStatements, LBrace().assignNoPrototype(), RBrace().assignNoPrototype()).assignNoPrototype()
         }
@@ -286,30 +285,37 @@ public class Converter private(val project: Project, val settings: ConverterSett
                          convertElement(field.getArgumentList()))
         }
         else {
-            val convertedType = typeConverter.convertVariableType(field)
             val isVal = isVal(field)
+            val typeToDeclare = variableTypeToDeclare(field,
+                                                      settings.specifyFieldTypeByDefault || modifiers.isPublic || modifiers.isProtected,
+                                                      isVal && modifiers.isPrivate)
             Field(name,
                   annotations,
                   modifiers,
-                  convertedType,
+                  typeToDeclare ?: typeConverter.convertVariableType(field),
                   convertExpression(field.getInitializer(), field.getType()),
                   isVal,
-                  shouldDeclareType(field, isVal, convertedType),
+                  typeToDeclare != null,
                   field.hasWriteAccesses(field.getContainingClass()))
         }
         return converted.assignPrototype(field)
     }
 
-    private fun shouldDeclareType(field: PsiField, isVal: Boolean, convertedType: Type): Boolean {
+    fun variableTypeToDeclare(variable: PsiVariable, specifyAlways: Boolean, canChangeType: Boolean): Type? {
-        if (settings.specifyFieldTypeByDefault) return true
+        fun convertType() = typeConverter.convertVariableType(variable)
-        val initializer = field.getInitializer() ?: return true
+
-        fun typeMatches() = convertedType == typeConverter.convertExpressionType(initializer)
+        if (specifyAlways) return convertType()
-        return if (field.hasModifierProperty(PsiModifier.PRIVATE))
+
-            !isVal && !typeMatches()
+        val initializer = variable.getInitializer()
-        else if (field.hasModifierProperty(PsiModifier.PACKAGE_LOCAL))
+        if (initializer == null) return convertType()
-            !typeMatches()
+        if (initializer is PsiLiteralExpression && initializer.getType() == PsiType.NULL) return convertType()
-        else
+
-            true
+        if (canChangeType) return null
+
+        val convertedType = convertType()
+        var initializerType = convertedExpressionType(initializer, variable.getType())
+        if (initializerType is ErrorType) return null // do not add explicit type when initializer is not resolved, let user add it if really needed
+        return if (convertedType == initializerType) null else convertedType
     }
 
     private fun isVal(field: PsiField): Boolean {

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

@@ -92,7 +92,7 @@ class TypeConverter(val settings: ConverterSettings, val conversionScope: Conver
         if (nullability == Nullability.Default) {
             val initializer = variable.getInitializer()
             if (initializer != null) {
-                val initializerNullability = initializer.nullability(false)
+                val initializerNullability = initializer.nullability()
                 if (variable.isEffectivelyFinal()) {
                     nullability = initializerNullability
                 }
@@ -133,7 +133,7 @@ class TypeConverter(val settings: ConverterSettings, val conversionScope: Conver
                     for (call in findMethodCalls(method, scope)) {
                         val args = call.getArgumentList().getExpressions()
                         if (args.size == parameters.size) {
-                            if (args[parameterIndex].nullability(false) == Nullability.Nullable) {
+                            if (args[parameterIndex].nullability() == Nullability.Nullable) {
                                 nullability = Nullability.Nullable
                                 break
                             }
@@ -172,7 +172,7 @@ class TypeConverter(val settings: ConverterSettings, val conversionScope: Conver
         if (nullability == Nullability.Default) {
             method.getBody()?.accept(object: JavaRecursiveElementVisitor() {
                 override fun visitReturnStatement(statement: PsiReturnStatement) {
-                    if (statement.getReturnValue()?.nullability(false) == Nullability.Nullable) {
+                    if (statement.getReturnValue()?.nullability() == Nullability.Nullable) {
                         nullability = Nullability.Nullable
                     }
                 }
@@ -195,10 +195,6 @@ class TypeConverter(val settings: ConverterSettings, val conversionScope: Conver
         return nullability
     }
 
-    public fun convertExpressionType(expression: PsiExpression): Type {
-        return convertType(expression.getType(), expression.nullability(true))
-    }
-
     private fun searchScope(element: PsiElement): PsiElement? {
         return when(element) {
             is PsiParameter -> element.getDeclarationScope()
@@ -209,38 +205,22 @@ class TypeConverter(val settings: ConverterSettings, val conversionScope: Conver
         }
     }
 
-    private fun PsiExpression.nullability(useDeclarationsNullability: Boolean): Nullability {
+    private fun PsiExpression.nullability(): Nullability {
         return when (this) {
             is PsiLiteralExpression -> if (getType() != PsiType.NULL) Nullability.NotNull else Nullability.Nullable
 
             is PsiNewExpression -> Nullability.NotNull
 
             is PsiConditionalExpression -> {
-                val nullability1 = getThenExpression()?.nullability(useDeclarationsNullability)
+                val nullability1 = getThenExpression()?.nullability()
                 if (nullability1 == Nullability.Nullable) return Nullability.Nullable
-                val nullability2 = getElseExpression()?.nullability(useDeclarationsNullability)
+                val nullability2 = getElseExpression()?.nullability()
                 if (nullability2 == Nullability.Nullable) return Nullability.Nullable
                 if (nullability1 == Nullability.NotNull && nullability2 == Nullability.NotNull) return Nullability.NotNull
                 Nullability.Default
             }
 
-            is PsiParenthesizedExpression -> getExpression()?.nullability(useDeclarationsNullability) ?: Nullability.Default
+            is PsiParenthesizedExpression -> getExpression()?.nullability() ?: Nullability.Default
-
-            is PsiMethodCallExpression -> if (useDeclarationsNullability) {
-                val method = resolveMethod()
-                if (method != null) methodNullability(method) else Nullability.Default
-            }
-            else {
-                Nullability.Default
-            }
-
-            is PsiReferenceExpression -> if (useDeclarationsNullability) {
-                val variable = resolve() as? PsiVariable
-                if (variable != null) variableNullability(variable) else Nullability.Default
-            }
-            else {
-                Nullability.Default
-            }
 
 
         //TODO: some other cases
@@ -252,7 +232,7 @@ class TypeConverter(val settings: ConverterSettings, val conversionScope: Conver
     private fun isNullableFromUsage(usage: PsiExpression): Boolean {
         val parent = usage.getParent() ?: return false
         if (parent is PsiAssignmentExpression && parent.getOperationTokenType() == JavaTokenType.EQ && usage == parent.getLExpression()) {
-            return parent.getRExpression()?.nullability(false) == Nullability.Nullable
+            return parent.getRExpression()?.nullability() == Nullability.Nullable
         }
         else if (parent is PsiBinaryExpression) {
             val operationType = parent.getOperationTokenType()

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

@@ -26,8 +26,7 @@ class LocalVariable(
         private val modifiers: Modifiers,
         private val explicitType: Type?,
         private val initializer: Expression,
-        private val isVal: Boolean,
+        private val isVal: Boolean
-        private val settings: ConverterSettings
 ) : Element() {
 
     override fun generateCode(builder: CodeBuilder) {

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

@@ -27,26 +27,13 @@ class ElementVisitor(private val converter: Converter) : JavaElementVisitor() {
         protected set
 
     override fun visitLocalVariable(variable: PsiLocalVariable) {
+        val isVal = variable.hasModifierProperty(PsiModifier.FINAL) || !variable.hasWriteAccesses(variable.getContainingMethod())
         result = LocalVariable(variable.declarationIdentifier(),
                                converter.convertAnnotations(variable),
                                converter.convertModifiers(variable),
-                               explicitType(variable),
+                               converter.variableTypeToDeclare(variable, converter.settings.specifyLocalVariableTypeByDefault, isVal),
                                converter.convertExpression(variable.getInitializer(), variable.getType()),
-                               converter.settings.forceLocalVariableImmutability || variable.hasModifierProperty(PsiModifier.FINAL),
+                               converter.settings.forceLocalVariableImmutability || isVal)
-                               converter.settings)
-    }
-
-    private fun explicitType(variable: PsiLocalVariable): Type? {
-        fun convertType() = typeConverter.convertVariableType(variable)
-        if (converter.settings.specifyLocalVariableTypeByDefault) return convertType()
-        val initializer = variable.getInitializer()
-        if (initializer == null) return convertType()
-        if (initializer is PsiLiteralExpression && initializer.getType() == PsiType.NULL) return convertType()
-        if (variable.hasModifierProperty(PsiModifier.FINAL) || !variable.hasWriteAccesses(variable.getContainingMethod())) return null
-        val convertedType = convertType()
-        var initializerType = converter.convertedExpressionType(initializer, variable.getType())
-        if (initializerType is ErrorType) return null // do not add explicit type when initializer is not resolved, let user add it if really needed
-        return if (convertedType == initializerType) null else convertedType
     }
 
     override fun visitExpressionList(list: PsiExpressionList) {

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

@@ -1074,6 +1074,11 @@ public class JavaToKotlinConverterTestGenerated extends AbstractJavaToKotlinConv
             doTest("j2k/tests/testData/ast/field/classChildExtendsBase.java");
         }
         
+        @TestMetadata("conversion.java")
+        public void testConversion() throws Exception {
+            doTest("j2k/tests/testData/ast/field/conversion.java");
+        }
+        
         @TestMetadata("internalField.java")
         public void testInternalField() throws Exception {
             doTest("j2k/tests/testData/ast/field/internalField.java");

j2k/tests/testData/ast/annotations/jetbrainsNotNullChainExpr.java

@@ -11,7 +11,7 @@ class Foo {
 
 class Bar {
   @NotNull
-  Foo fooNotNull = Foo();
+  Foo fooNotNull = new Foo();
   Foo fooNullable = null;
 }
 

j2k/tests/testData/ast/annotations/jetbrainsNotNullChainExpr.kt

@@ -8,7 +8,7 @@ class Foo() {
 }
 
 class Bar() {
-    var fooNotNull: Foo = Foo()
+    var fooNotNull = Foo()
     var fooNullable: Foo? = null
 }
 

j2k/tests/testData/ast/field/conversion.java

@@ -0,0 +1,10 @@
+//file
+class A {
+    private Integer i = getByte();
+
+    static byte getByte() { return 0; }
+
+    void foo() {
+        i = 10;
+    }
+}

j2k/tests/testData/ast/field/conversion.kt

@@ -0,0 +1,14 @@
+class A() {
+    private var i = getByte().toInt()
+
+    fun foo() {
+        i = 10
+    }
+
+    class object {
+
+        fun getByte(): Byte {
+            return 0
+        }
+    }
+}

j2k/tests/testData/ast/issues/kt-820-field.kt

@@ -11,5 +11,5 @@ class One() {
 }
 
 class Test() {
-    var b: Byte = One.myContainer.myInt.toByte()
+    var b = One.myContainer.myInt.toByte()
 }

j2k/tests/testData/ast/settings/forceLocalVariableImmutability.java

@@ -2,6 +2,7 @@
 // !forceLocalVariableImmutability: false
 public void foo() {
   int i = 1;
+  i++;
   String s = "";
   s += "a";
 }

j2k/tests/testData/ast/settings/forceLocalVariableImmutability.kt

@@ -1,6 +1,7 @@
 // !forceLocalVariableImmutability: false
 public fun foo() {
     var i = 1
+    i++
     var s = ""
     s += "a"
 }