Fix a bug with private superclass interceptors (#175)

We weren't publishing our added functions in class metadata,
and that was causing private @InterceptTest properties to be
missed.

Adding it was made complicated by the fact that we started
getting fake overrides, that we now need to remove.

Co-authored-by: Jesse Wilson <jwilson@squareup.com>

Author: swankjesse

burst-gradle-plugin/src/test/kotlin/app/cash/burst/gradle/TestInterceptorGradlePluginTest.kt

@@ -131,4 +131,27 @@ class TestInterceptorGradlePluginTest {
       )
     }
   }
+
+  @Test
+  fun abstractClassHasSuperClassThatIntercepts() {
+    val tester = GradleTester("interceptorAcrossModules")
+    tester.cleanAndBuild(":lib:test")
+
+    with(tester.readTestSuite("app.cash.burst.tests.BottomTest")) {
+      assertThat(systemOut).isEqualTo(
+        """
+        |> intercepting top
+        |  running bottom
+        |< intercepted top
+        |> intercepting top
+        |  running middle
+        |< intercepted top
+        |> intercepting top
+        |  running top
+        |< intercepted top
+        |
+        """.trimMargin(),
+      )
+    }
+  }
 }

burst-gradle-plugin/src/test/projects/interceptorAcrossModules/lib/src/test/kotlin/app/cash/burst/tests/BottomTest.kt

@@ -0,0 +1,10 @@
+package app.cash.burst.tests
+
+import kotlin.test.Test
+
+class BottomTest : MiddleTest() {
+  @Test
+  fun testBottom() {
+    println("  running bottom")
+  }
+}

burst-gradle-plugin/src/test/projects/interceptorAcrossModules/lib/src/test/kotlin/app/cash/burst/tests/MiddleTest.kt

@@ -0,0 +1,11 @@
+package app.cash.burst.tests
+
+import app.cash.burst.testlib.TopTest
+import kotlin.test.Test
+
+abstract class MiddleTest : TopTest() {
+  @Test
+  fun testMiddle() {
+    println("  running middle")
+  }
+}

burst-gradle-plugin/src/test/projects/interceptorAcrossModules/testlib/src/main/kotlin/app/cash/burst/testlib/TopTest.kt

@@ -0,0 +1,14 @@
+package app.cash.burst.testlib
+
+import app.cash.burst.InterceptTest
+import kotlin.test.Test
+
+abstract class TopTest {
+  @InterceptTest
+  private val interceptor = BasicInterceptor("top")
+
+  @Test
+  fun testTop() {
+    println("  running top")
+  }
+}

burst-kotlin-plugin-tests/src/test/kotlin/app/cash/burst/kotlin/TestInterceptorKotlinPluginTest.kt

@@ -1535,4 +1535,70 @@ class TestInterceptorKotlinPluginTest {
       "intercepted",
     )
   }
+
+  @Test
+  fun abstractClassInTheMiddle() {
+    val log = BurstTester(
+      packageName = "com.example",
+    ).compileAndRun(
+      SourceFile.kotlin(
+        "Main.kt",
+        """
+        package com.example
+
+        import app.cash.burst.InterceptTest
+        import app.cash.burst.TestFunction
+        import app.cash.burst.TestInterceptor
+        import kotlin.test.AfterTest
+        import kotlin.test.BeforeTest
+        import kotlin.test.Test
+
+        abstract class TopTest {
+          @InterceptTest
+          val interceptor = object : TestInterceptor {
+            override fun intercept(testFunction: TestFunction) {
+              log("intercepting ${'$'}{testFunction.className}.${'$'}{testFunction.functionName}")
+              testFunction()
+            }
+          }
+
+          @Test
+          fun testTop() {
+            log("top")
+          }
+        }
+
+        abstract class MiddleTest : TopTest() {
+          @Test
+          fun testMiddle() {
+            log("middle")
+          }
+        }
+
+        class BottomTest : MiddleTest() {
+          @Test
+          fun testBottom() {
+            log("bottom")
+          }
+        }
+
+        fun main(vararg args: String) {
+          val test = BottomTest()
+          test.testTop()
+          test.testMiddle()
+          test.testBottom()
+        }
+        """,
+      ),
+    )
+
+    assertThat(log).containsExactly(
+      "intercepting TopTest.testTop",
+      "top",
+      "intercepting MiddleTest.testMiddle",
+      "middle",
+      "intercepting BottomTest.testBottom",
+      "bottom",
+    )
+  }
 }

burst-kotlin-plugin/src/main/kotlin/app/cash/burst/kotlin/HierarchyInterceptorInjector.kt

@@ -18,10 +18,12 @@
 package app.cash.burst.kotlin
 
 import org.jetbrains.kotlin.backend.common.extensions.IrPluginContext
+import org.jetbrains.kotlin.ir.backend.js.utils.isDispatchReceiver
 import org.jetbrains.kotlin.ir.declarations.IrClass
 import org.jetbrains.kotlin.ir.declarations.IrProperty
 import org.jetbrains.kotlin.ir.declarations.IrSimpleFunction
 import org.jetbrains.kotlin.ir.symbols.UnsafeDuringIrConstructionAPI
+import org.jetbrains.kotlin.ir.types.classFqName
 import org.jetbrains.kotlin.ir.util.functions
 import org.jetbrains.kotlin.ir.util.isSubtypeOfClass
 import org.jetbrains.kotlin.ir.util.properties
@@ -47,7 +49,7 @@ internal class HierarchyInterceptorInjector(
     // If this class directly declares an intercept() function, return that. Otherwise, our injected
     // symbol would collide with that one.
     val existing = classDeclaration.interceptFunction
-    if (existing != null) return existing
+    if (existing != null && !existing.isFakeOverride) return existing
 
     // Rewrite the superclass first!
     val superClass = classDeclaration.superClass
@@ -77,6 +79,7 @@ internal class HierarchyInterceptorInjector(
       originalParent = classDeclaration,
       interceptorProperties = interceptorProperties,
       superclassIntercept = superClassInterceptFunction,
+      existingIntercept = existing,
     )
 
     for (function in originalFunctions) {
@@ -103,7 +106,15 @@ internal class HierarchyInterceptorInjector(
 
   /** The `intercept()` function declared by this class. */
   private val IrClass.interceptFunction: IrSimpleFunction?
-    get() = functions.firstOrNull { burstApis.testInterceptorIntercept in it.overriddenSymbols }
+    get() {
+      val other = burstApis.testInterceptorIntercept.owner
+      return functions.firstOrNull {
+        it.name == other.name &&
+          it.parameters.size == other.parameters.size &&
+          it.parameters[0].isDispatchReceiver &&
+          it.parameters[1].type.classFqName == other.parameters[1].type.classFqName
+      }
+    }
 
   private fun unexpectedInterceptTest(property: IrProperty): Nothing {
     throw BurstCompilationException(

burst-kotlin-plugin/src/main/kotlin/app/cash/burst/kotlin/InterceptorInjector.kt

@@ -120,6 +120,7 @@ internal class InterceptorInjector(
   private val originalParent: IrClass,
   private val interceptorProperties: List<IrProperty>,
   private val superclassIntercept: IrSimpleFunction?,
+  private val existingIntercept: IrSimpleFunction?,
 ) {
   private val packageName = originalParent.packageFqName?.asString() ?: ""
   private val className = generateSequence(originalParent) { it.parentClassOrNull }
@@ -157,6 +158,11 @@ internal class InterceptorInjector(
   fun defineIntercept(): IrSimpleFunction {
     check(interceptFunctionSymbol == null) { "already defined?!" }
 
+    // If there's already a fake override, remove it.
+    if (existingIntercept != null) {
+      originalParent.declarations.remove(existingIntercept)
+    }
+
     // TODO: don't add 'implements TestInterceptor' if it already does.
     originalParent.superTypes += burstApis.testInterceptorType
 
@@ -295,6 +301,11 @@ internal class InterceptorInjector(
 
     this.interceptFunctionSymbol = function.symbol
 
+    // Make this function visible to subclasses in other modules.
+    if (existingIntercept == null) {
+      pluginContext.metadataDeclarationRegistrar.registerFunctionAsMetadataVisible(function)
+    }
+
     return function
   }