Java to Kotlin converter: correct nullability in signatures of overrides

#KT-5269 Fixed

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

@@ -330,23 +330,6 @@ public class Converter private(val project: Project, val settings: ConverterSett
             }
 
             var params = convertParameterList(method.getParameterList())
-
-            // if we override equals from Object, change parameter type to nullable
-            if (isOverride && method.getName() == "equals") {
-                val superSignatures = method.getHierarchicalMethodSignature().getSuperSignatures()
-                val overridesMethodFromObject = superSignatures.any {
-                    it.getMethod().getContainingClass()?.getQualifiedName() == JAVA_LANG_OBJECT
-                }
-                if (overridesMethodFromObject) {
-                    val correctedParameter = Parameter(Identifier("other"),
-                                                       ClassType(Identifier("Any"), listOf(), Nullability.Nullable, settings),
-                                                       Parameter.VarValModifier.None,
-                                                       params.parameters.single().annotations,
-                                                       Modifiers.Empty)
-                    params = ParameterList(listOf(correctedParameter))
-                }
-            }
-
             val typeParameterList = convertTypeParameterList(method.getTypeParameterList())
             val block = convertBlock(method.getBody())
             return Function(this, method.declarationIdentifier(), annotations, modifiers, returnType, typeParameterList, params, block, containingClass?.isInterface() ?: false)

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

@@ -25,6 +25,7 @@ import java.util.HashSet
 import org.jetbrains.jet.j2k.ast.Import
 import org.jetbrains.jet.j2k.ast.ImportList
 import org.jetbrains.jet.j2k.ast.assignPrototype
+import com.intellij.psi.CommonClassNames.JAVA_LANG_OBJECT
 
 class TypeConverter(val settings: ConverterSettings, val conversionScope: ConversionScope) {
     private val nullabilityCache = HashMap<PsiElement, Nullability>()
@@ -72,6 +73,19 @@ class TypeConverter(val settings: ConverterSettings, val conversionScope: Conver
 
         var nullability = variable.nullabilityFromAnnotations()
 
+        if (nullability == Nullability.Default && variable is PsiParameter) {
+            val scope = variable.getDeclarationScope()
+            if (scope is PsiMethod) {
+                val paramIndex = scope.getParameterList().getParameters().indexOf(variable)
+                assert(paramIndex >= 0)
+                val superSignatures = scope.getHierarchicalMethodSignature().getSuperSignatures()
+                nullability = superSignatures.map { signature ->
+                    val params = signature.getMethod().getParameterList().getParameters()
+                    if (paramIndex < params.size) variableNullability(params[paramIndex]) else Nullability.Default
+                }.firstOrNull { it != Nullability.Default } ?: Nullability.Default
+            }
+        }
+
         if (nullability == Nullability.Default) {
             val initializer = variable.getInitializer()
             if (initializer != null) {
@@ -85,7 +99,17 @@ class TypeConverter(val settings: ConverterSettings, val conversionScope: Conver
             }
         }
 
-        if (!conversionScope.contains(variable)) return nullability // do not analyze usages of fields out of our conversion scope
+        if (!conversionScope.contains(variable)) { // do not analyze usages out of our conversion scope
+            if (variable is PsiParameter) {
+                // Object.equals corresponds to Any.equals which has nullable parameter:
+                val scope = variable.getDeclarationScope()
+                if (scope is PsiMethod && scope.getName() == "equals" && scope.getContainingClass()?.getQualifiedName() == JAVA_LANG_OBJECT) {
+                    return Nullability.Nullable
+                }
+            }
+
+            return nullability
+        }
 
         if (nullability == Nullability.Default) {
             val scope = searchScope(variable)
@@ -133,6 +157,11 @@ class TypeConverter(val settings: ConverterSettings, val conversionScope: Conver
     private fun methodNullabilityNoCache(method: PsiMethod): Nullability {
         var nullability = method.nullabilityFromAnnotations()
 
+        if (nullability == Nullability.Default) {
+            val superSignatures = method.getHierarchicalMethodSignature().getSuperSignatures()
+            nullability = superSignatures.map { methodNullability(it.getMethod()) }.firstOrNull { it != Nullability.Default } ?: Nullability.Default
+        }
+
         if (!conversionScope.contains(method)) return nullability // do not analyze body and usages of methods out of our conversion scope
 
         if (nullability == Nullability.Default) {

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

@@ -1326,6 +1326,11 @@ public class JavaToKotlinConverterTestGenerated extends AbstractJavaToKotlinConv
             doTest("j2k/tests/testData/ast/function/overrideObject2.java");
         }
         
+        @TestMetadata("overrideObject3.java")
+        public void testOverrideObject3() throws Exception {
+            doTest("j2k/tests/testData/ast/function/overrideObject3.java");
+        }
+        
         @TestMetadata("ownGenericParam.java")
         public void testOwnGenericParam() throws Exception {
             doTest("j2k/tests/testData/ast/function/ownGenericParam.java");
@@ -2004,6 +2009,11 @@ public class JavaToKotlinConverterTestGenerated extends AbstractJavaToKotlinConv
             doTest("j2k/tests/testData/ast/nullability/FieldInitializedWithNull.java");
         }
         
+        @TestMetadata("IndirectOverride.java")
+        public void testIndirectOverride() throws Exception {
+            doTest("j2k/tests/testData/ast/nullability/IndirectOverride.java");
+        }
+        
         @TestMetadata("MethodInvokedWithNullArg.java")
         public void testMethodInvokedWithNullArg() throws Exception {
             doTest("j2k/tests/testData/ast/nullability/MethodInvokedWithNullArg.java");
@@ -2069,6 +2079,16 @@ public class JavaToKotlinConverterTestGenerated extends AbstractJavaToKotlinConv
             doTest("j2k/tests/testData/ast/nullability/NullableVariableDotAccess.java");
         }
         
+        @TestMetadata("OverrideWithInheritanceLoop.java")
+        public void testOverrideWithInheritanceLoop() throws Exception {
+            doTest("j2k/tests/testData/ast/nullability/OverrideWithInheritanceLoop.java");
+        }
+        
+        @TestMetadata("Overrides.java")
+        public void testOverrides() throws Exception {
+            doTest("j2k/tests/testData/ast/nullability/Overrides.java");
+        }
+        
         @TestMetadata("ParameterComparedWithNull.java")
         public void testParameterComparedWithNull() throws Exception {
             doTest("j2k/tests/testData/ast/nullability/ParameterComparedWithNull.java");

j2k/tests/testData/JavaApi.java

@@ -1,5 +1,10 @@
 package javaApi;
 
+import org.jetbrains.annotations.Nullable;
+
+import java.lang.Override;
+import java.lang.String;
+
 public @interface Anon1 {
     String[] value();
     String[] stringArray();
@@ -43,3 +48,11 @@ public @interface Anon8 {
 public enum E {
     A, B, C
 }
+
+class Base {
+    public @Nullable String foo(@Nullable String s) { return s; }
+}
+
+class Derived extends Base {
+    public String foo(String s) { return s; }
+}

j2k/tests/testData/ast/function/extendsBaseWhichExtendsObject.kt

@@ -5,7 +5,7 @@ class Test() : Base() {
         return super.hashCode()
     }
 
-    override fun equals(o: Any): Boolean {
+    override fun equals(o: Any?): Boolean {
         return super.equals(o)
     }
 
@@ -29,7 +29,7 @@ class Base() {
         return super.hashCode()
     }
 
-    override fun equals(other: Any?): Boolean {
+    override fun equals(o: Any?): Boolean {
         return super.equals(o)
     }
 

j2k/tests/testData/ast/function/overrideObject.kt

@@ -3,7 +3,7 @@ class X() {
         return super.hashCode()
     }
 
-    override fun equals(other: Any?): Boolean {
+    override fun equals(o: Any?): Boolean {
         return super.equals(o)
     }
 

j2k/tests/testData/ast/function/overrideObject2.kt

@@ -5,7 +5,7 @@ class X() : Base() {
         return super.hashCode()
     }
 
-    override fun equals(other: Any?): Boolean {
+    override fun equals(o: Any?): Boolean {
         return super.equals(o)
     }
 

j2k/tests/testData/ast/function/overrideObject3.kt

@@ -1,5 +1,5 @@
 class Base() {
-    override fun equals(other: Any?): Boolean {
+    override fun equals(o: Any?): Boolean {
         return super.equals(o)
     }
 }

j2k/tests/testData/ast/issues/kt-696.kt

@@ -5,7 +5,7 @@ class Base() {
         return super.hashCode()
     }
 
-    override fun equals(other: Any?): Boolean {
+    override fun equals(o: Any?): Boolean {
         return super.equals(o)
     }
 
@@ -19,7 +19,7 @@ class Child() : Base() {
         return super.hashCode()
     }
 
-    override fun equals(o: Any): Boolean {
+    override fun equals(o: Any?): Boolean {
         return super.equals(o)
     }
 

j2k/tests/testData/ast/nullability/IndirectOverride.java

@@ -0,0 +1,4 @@
+//file
+class C extends javaApi.Derived {
+    public String foo(String s) { return s; }
+}

j2k/tests/testData/ast/nullability/IndirectOverride.kt

@@ -0,0 +1,5 @@
+class C() : javaApi.Derived() {
+    override fun foo(s: String?): String? {
+        return s
+    }
+}

j2k/tests/testData/ast/nullability/OverrideWithInheritanceLoop.java

@@ -0,0 +1,8 @@
+//file
+class A extends B {
+    public void foo(String s) {}
+}
+
+class B extends A {
+    public void foo(String s) {}
+}

j2k/tests/testData/ast/nullability/OverrideWithInheritanceLoop.kt

@@ -0,0 +1,9 @@
+class A() : B() {
+    public fun foo(s: String) {
+    }
+}
+
+class B() : A() {
+    public fun foo(s: String) {
+    }
+}

j2k/tests/testData/ast/nullability/Overrides.java

@@ -0,0 +1,26 @@
+//file
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+class Base {
+    @Nullable
+    public String foo(@Nullable String s) { return ""; }
+
+    public String bar(String s) {
+        return s != null ? s + 1 : null;
+    }
+
+    public String zoo(Object o){ return ""; }
+}
+
+interface I {
+    @Nullable String zoo(@Nullable Object o);
+}
+
+class C extends Base implements I {
+    public String foo(String s) { return ""; }
+
+    public String bar(String s) { return ""; }
+
+    public String zoo(Object o) { return ""; }
+}

j2k/tests/testData/ast/nullability/Overrides.kt

@@ -0,0 +1,31 @@
+class Base() {
+    public fun foo(s: String?): String? {
+        return ""
+    }
+
+    public fun bar(s: String?): String? {
+        return if (s != null) s + 1 else null
+    }
+
+    public fun zoo(o: Any): String {
+        return ""
+    }
+}
+
+trait I {
+    public fun zoo(o: Any?): String?
+}
+
+class C() : Base(), I {
+    override fun foo(s: String?): String? {
+        return ""
+    }
+
+    override fun bar(s: String?): String? {
+        return ""
+    }
+
+    override fun zoo(o: Any?): String? {
+        return ""
+    }
+}