Check class hierarchy when checking for overrides

There was a bug in overrides check where superClass.foo overrides subClass.foo would return true even though super class cannot override a subClass. This PR fixes that issue by checking for class hiearchy in addition to the method signature. Fixes: #123 Test: CheckOverrideProcessor
google · Oct 18, 2020 · eae30cf · eae30cf
1 parent d4b637c
commit eae30cf
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 2 deletions.
diff --git a/compiler-plugin/src/main/kotlin/com/google/devtools/ksp/processing/impl/ResolverImpl.kt b/compiler-plugin/src/main/kotlin/com/google/devtools/ksp/processing/impl/ResolverImpl.kt
@@ -18,6 +18,7 @@
 
 package com.google.devtools.ksp.processing.impl
 
+import com.google.devtools.ksp.closestClassDeclaration
 import com.google.devtools.ksp.isOpen
 import com.google.devtools.ksp.isVisibleFrom
 import com.intellij.openapi.project.Project
@@ -59,6 +60,7 @@ import org.jetbrains.kotlin.resolve.*
 import org.jetbrains.kotlin.resolve.calls.smartcasts.DataFlowInfo
 import org.jetbrains.kotlin.resolve.constants.ConstantValue
 import org.jetbrains.kotlin.resolve.constants.evaluate.ConstantExpressionEvaluator
+import org.jetbrains.kotlin.resolve.descriptorUtil.getAllSuperClassifiers
 import org.jetbrains.kotlin.resolve.jvm.multiplatform.JavaActualAnnotationArgumentExtractor
 import org.jetbrains.kotlin.resolve.lazy.DeclarationScopeProvider
 import org.jetbrains.kotlin.resolve.lazy.ForceResolveUtil
@@ -245,7 +247,6 @@ class ResolverImpl(
             return false
         if (!overridee.isVisibleFrom(overrider))
             return false
-
         if (!(overridee is KSFunctionDeclaration || overrider is KSFunctionDeclaration || (overridee is KSPropertyDeclaration && overrider is KSPropertyDeclaration)))
             return false
 
@@ -254,8 +255,21 @@ class ResolverImpl(
 
         val superDescriptor = resolveForOverride(overridee) as? CallableMemberDescriptor ?: return false
         val subDescriptor = resolveForOverride(overrider) as? CallableMemberDescriptor ?: return false
+        val subClassDescriptor = overrider.closestClassDeclaration()?.let {
+            resolveClassDeclaration(it)
+        } ?: return false
+        val superClassDescriptor = overridee.closestClassDeclaration()?.let {
+            resolveClassDeclaration(it)
+        } ?: return false
+        val typeOverride = subClassDescriptor.getAllSuperClassifiers()
+            .filter { it != subClassDescriptor } // exclude subclass itself as it cannot override its own methods
+            .any {
+                it == superClassDescriptor
+            }
+        if (!typeOverride) return false
+
         return OverridingUtil.DEFAULT.isOverridableBy(
-                superDescriptor, subDescriptor, null
+                superDescriptor, subDescriptor, subClassDescriptor, true
         ).result == OverridingUtil.OverrideCompatibilityInfo.Result.OVERRIDABLE
     }
 

diff --git a/compiler-plugin/src/main/kotlin/com/google/devtools/ksp/processor/CheckOverrideProcessor.kt b/compiler-plugin/src/main/kotlin/com/google/devtools/ksp/processor/CheckOverrideProcessor.kt
@@ -53,21 +53,36 @@ class CheckOverrideProcessor : AbstractTestProcessor() {
         checkOverride(getFunKt,getFunJava)
         checkOverride(fooFunKt,fooFunJava)
         checkOverride(foooFunKt,fooFunJava)
+        checkOverride(fooFunKt,fooFunKt)
         checkOverride(equalFunKt,equalFunJava)
         checkOverride(bazPropKt,baz2PropKt)
         checkOverride(bazPropKt,bazz2PropKt)
         checkOverride(bazzPropKt,bazz2PropKt)
         checkOverride(bazzPropKt,baz2PropKt)
+        checkOverride(bazPropKt,bazPropKt)
         val JavaImpl = resolver.getClassDeclarationByName("JavaImpl")!!
         val MyInterface = resolver.getClassDeclarationByName("MyInterface")!!
         val getX = JavaImpl.getDeclaredFunctions().first { it.simpleName.asString() == "getX" }
         val getY = JavaImpl.getDeclaredFunctions().first { it.simpleName.asString() == "getY" }
         val setY = JavaImpl.getDeclaredFunctions().first { it.simpleName.asString() == "setY" }
+        val setX = JavaImpl.getDeclaredFunctions().first { it.simpleName.asString() == "setX" }
         val myInterfaceX = MyInterface.declarations.first{ it.simpleName.asString() == "x" }
         val myInterfaceY = MyInterface.declarations.first{ it.simpleName.asString() == "y" }
         checkOverride(getY, getX)
         checkOverride(getY, myInterfaceX)
         checkOverride(getX, myInterfaceX)
         checkOverride(setY, myInterfaceY)
+        checkOverride(setX, myInterfaceX)
+        checkOverride(getY, getY)
+        checkOverride(myInterfaceX, getY)
+        checkOverride(myInterfaceX, getX)
+        checkOverride(myInterfaceY, setY)
+        checkOverride(myInterfaceY, myInterfaceY)
+
+        val JavaDifferentReturnTypes =
+            resolver.getClassDeclarationByName("JavaDifferentReturnType")!!
+        val diffGetX = JavaDifferentReturnTypes.getDeclaredFunctions()
+            .first { it.simpleName.asString() == "foo" }
+        checkOverride(diffGetX, fooFunJava)
     }
 }
diff --git a/compiler-plugin/testData/api/checkOverride.kt b/compiler-plugin/testData/api/checkOverride.kt
@@ -20,15 +20,24 @@
 // KotlinList.get overrides JavaList.get: false
 // KotlinList.foo overrides JavaList.foo: true
 // KotlinList.fooo overrides JavaList.foo: false
+// KotlinList.foo overrides KotlinList.foo: false
 // KotlinList.equals overrides JavaList.equals: true
 // KotlinList2.baz overrides KotlinList.baz: true
 // KotlinList2.baz overrides KotlinList.bazz: false
 // KotlinList2.bazz overrides KotlinList.bazz: true
 // KotlinList2.bazz overrides KotlinList.baz: false
+// KotlinList2.baz overrides KotlinList2.baz: false
 // JavaImpl.getY overrides JavaImpl.getX: false
 // JavaImpl.getY overrides MyInterface.x: false
 // JavaImpl.getX overrides MyInterface.x: true
 // JavaImpl.setY overrides MyInterface.y: true
+// JavaImpl.setX overrides MyInterface.x: false
+// JavaImpl.getY overrides JavaImpl.getY: false
+// MyInterface.x overrides JavaImpl.getY: false
+// MyInterface.x overrides JavaImpl.getX: false
+// MyInterface.y overrides JavaImpl.setY: false
+// MyInterface.y overrides MyInterface.y: false
+// JavaDifferentReturnType.foo overrides JavaList.foo: true
 // END
 // FILE: a.kt
 
@@ -112,4 +121,17 @@ public class JavaImpl implements MyInterface {
     public void setY(int value) {
 
     }
+
+    // intentional override check for a val property
+    public void setX(int value) {
+        return value;
+    }
 }
+
+// FILE: JavaDifferentReturnType.java
+public abstract class JavaDifferentReturnType extends JavaList {
+    // intentional different return type
+    protected String foo() {
+        return "";
+    }
+}