JVM_IR: avoid redundant accessors in private inline funs

Their call sites are all in the same file, so we can check whether the declarations used in the inline function are accessible from all the places where it will be inlined. #KT-48736 Fixed
2021-09-13 19:03:01 +02:00
parent ef684c0dff
commit d07070e184
11 changed files with 161 additions and 47 deletions
@@ -41884,6 +41884,12 @@ public class FirBlackBoxCodegenTestGenerated extends AbstractFirBlackBoxCodegenT
            runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivate.kt");
        }

+        @Test
+        @TestMetadata("packagePrivateInPrivateInline.kt")
+        public void testPackagePrivateInPrivateInline() throws Exception {
+            runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt");
+        }
+
        @Test
        @TestMetadata("protectedFromLambda.kt")
        public void testProtectedFromLambda() throws Exception {
@@ -367,6 +367,12 @@ public class FirBytecodeTextTestGenerated extends AbstractFirBytecodeTextTest {
        runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackageCrossinline.kt");
    }

+    @Test
+    @TestMetadata("noAccessorForProtectedInSamePackagePrivateInline.kt")
+    public void testNoAccessorForProtectedInSamePackagePrivateInline() throws Exception {
+        runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackagePrivateInline.kt");
+    }
+
    @Test
    @TestMetadata("noFlagAnnotations.kt")
    public void testNoFlagAnnotations() throws Exception {
@@ -11,6 +11,7 @@ import org.jetbrains.kotlin.backend.common.descriptors.synthesizedString
 import org.jetbrains.kotlin.backend.common.ir.*
 import org.jetbrains.kotlin.backend.jvm.JvmBackendContext
 import org.jetbrains.kotlin.backend.jvm.JvmLoweredDeclarationOrigin
+import org.jetbrains.kotlin.backend.jvm.codegen.fileParent
 import org.jetbrains.kotlin.backend.jvm.codegen.isJvmInterface
 import org.jetbrains.kotlin.backend.jvm.intrinsics.receiverAndArgs
 import org.jetbrains.kotlin.backend.jvm.ir.IrInlineReferenceLocator
@@ -19,6 +20,7 @@ import org.jetbrains.kotlin.codegen.AsmUtil
 import org.jetbrains.kotlin.descriptors.DescriptorVisibilities
 import org.jetbrains.kotlin.descriptors.DescriptorVisibility
 import org.jetbrains.kotlin.descriptors.Modality
+import org.jetbrains.kotlin.ir.IrElement
 import org.jetbrains.kotlin.ir.IrStatement
 import org.jetbrains.kotlin.ir.UNDEFINED_OFFSET
 import org.jetbrains.kotlin.ir.builders.declarations.addValueParameter
@@ -42,6 +44,7 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle

    private val pendingAccessorsToAdd = mutableListOf<IrFunction>()
    private val inlineLambdaToCallSite = mutableMapOf<IrFunction, LambdaCallSite>()
+    private val inlineFunctionToCallSites = mutableMapOf<IrFunction, Set<IrElement>>()

    override fun lower(irFile: IrFile) {
        irFile.accept(object : IrInlineReferenceLocator(context) {
@@ -56,6 +59,24 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle
                inlineLambdaToCallSite[argument.symbol.owner] =
                    LambdaCallSite(scope, parameter.isCrossinline && !callee.isCoroutineIntrinsic())
            }
+
+            override fun visitSimpleFunction(declaration: IrSimpleFunction, data: IrDeclaration?) {
+                if (declaration.isPrivateInline) {
+                    inlineFunctionToCallSites.putIfAbsent(declaration, mutableSetOf())
+                }
+                super.visitSimpleFunction(declaration, data)
+            }
+
+            override fun visitCall(expression: IrCall, data: IrDeclaration?) {
+                val callee = expression.symbol.owner
+                if (callee.isPrivateInline && callee.fileParent == irFile && data != null) {
+                    (inlineFunctionToCallSites.getOrPut(callee) { mutableSetOf() } as MutableSet).add(data)
+                }
+                super.visitCall(expression, data)
+            }
+
+            private inline val IrSimpleFunction.isPrivateInline
+                get() = isInline && DescriptorVisibilities.isPrivate(visibility)
        }, null)

        irFile.transformChildrenVoid(this)
@@ -752,58 +773,76 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle
        }
    }

+    private fun getScopeClassOrPackage(): IrDeclarationContainer? =
+        getScopeClassOrPackage(currentScope?.irElement, approximateToPackage = false)
+
    // Get the class from which all accesses in the current scope will be done after bytecode generation.
    // If the current scope is a crossinline lambda, this is not possible, as the lambda maybe inlined
    // into some other class; in that case, get at least the package.
-    private fun getScopeClassOrPackage(): IrDeclarationContainer? {
-        var context = currentScope?.irElement
-        var throughCrossinlineLambda = false
-        while (context is IrDeclaration) {
-            val callSite = inlineLambdaToCallSite[context]
-            when {
-                // Crossinline lambdas can be inlined into some other class in the same package. However,
-                // classes within crossinline lambdas should not be regenerated, so if we've already found
-                // a class *before* reaching this lambda, it's valid:
-                //     class C {
-                //         fun f() {}
-                //         fun g() = inlineFunctionWithCrossinlineArgument {
-                //             f() // this call is done in some unknown class within C's package
-                //             object { val x = f() } // this call is done in C$g$1$1
-                //         }
-                //     }
-                callSite != null -> throughCrossinlineLambda = throughCrossinlineLambda || callSite.crossinline
-                // Inline functions can be inlined into anywhere. Not even private inline functions are safe:
-                //     class C {
-                //         fun f() {}
-                //         private inline fun g1() = f() // `f` is called from C?
-                //         fun g2() = { g1() } // ...or from C$g2$1 in the same package?
-                //         inline fun g3() = g1() // ...or from some other package that calls g3?
-                //     }
-                // TODO: this has some weird effects for inline functions in local classes, e.g. they
-                //   access the capture fields (package-private) through accessors; this may or may not
-                //   be necessary - local types should in theory not be usable outside the current file.
-                context is IrFunction && context.isInline -> return null
-                // TODO: if this class is an object local to an inline function, it could be regenerated,
-                //  so the scope depends on the declaration accessed (see KT-48508):
-                //     class C {
-                //         fun f1()
-                //         inline fun inlineFun() = object {
-                //             fun f2() {}
-                //             fun g1() {
-                //                 f1() // this access can be anywhere
-                //                 f2() // can pretend this access is from C$foo$1
-                //             }
-                //         }
-                //     }
-                //  Further complicating things, the accessor for `f1` cannot be in `C$foo$1`, as otherwise
-                //  the accessor itself will be regenerated (and thus not work) at `inlineFun` call sites.
-                context is IrClass && !throughCrossinlineLambda -> return context
+    private tailrec fun getScopeClassOrPackage(context: IrElement?, approximateToPackage: Boolean): IrDeclarationContainer? {
+        val callSite = inlineLambdaToCallSite[context]
+        return when {
+            // Crossinline lambdas can be inlined into some other class in the same package. However,
+            // classes within crossinline lambdas should not be regenerated, so if we've already found
+            // a class *before* reaching this lambda, it's valid:
+            //     class C {
+            //         fun f() {}
+            //         fun g() = inlineFunctionWithCrossinlineArgument {
+            //             f() // this call is done in some unknown class within C's package
+            //             object { val x = f() } // this call is done in C$g$1$1
+            //         }
+            //     }
+            callSite != null -> getScopeClassOrPackage(callSite.scope, approximateToPackage || callSite.crossinline)
+            // Inline functions can be inlined into anywhere. Not even private inline functions are safe:
+            //     class C {
+            //         fun f() {}
+            //         private inline fun g1() = f() // `f` is called from C?
+            //         fun g2() = { g1() } // ...or from C$g2$1 in the same package?
+            //         inline fun g3() = g1() // ...or from some other package that calls g3?
+            //     }
+            // However, for private ones we at least know where they're called, so just like inline lambdas,
+            // we can navigate there.
+            //
+            // TODO: this has some weird effects for inline functions in local classes, e.g. they
+            //   access the capture fields (package-private) through accessors; this may or may not
+            //   be necessary - local types should in theory not be usable outside the current file.
+            context is IrFunction && context.isInline -> {
+                val callSites = inlineFunctionToCallSites[context] ?: return null
+                when {
+                    callSites.isEmpty() -> getScopeClassOrPackage(context.parent, approximateToPackage)
+                    callSites.size == 1 -> getScopeClassOrPackage(callSites.single(), approximateToPackage)
+                    else -> {
+                        // TODO: cache the results
+                        @Suppress("NON_TAIL_RECURSIVE_CALL")
+                        val results = callSites.map { getScopeClassOrPackage(it, approximateToPackage = false) ?: return null }
+                        // If all call sites are within a single class, use it. Otherwise, all scopes must be within
+                        // the current file's package.
+                        val single = results.first().takeIf { results.all { other -> it === other } }
+                        getScopeClassOrPackage(single ?: context.parent, approximateToPackage || single == null)
+                    }
+                }
            }
+            // TODO: if this class is an object local to an inline function, it could be regenerated,
+            //  so the scope depends on the declaration accessed (see KT-48508):
+            //     class C {
+            //         fun f1()
+            //         inline fun inlineFun() = object {
+            //             fun f2() {}
+            //             fun g1() {
+            //                 f1() // this access can be anywhere
+            //                 f2() // can pretend this access is from C$foo$1
+            //             }
+            //         }
+            //     }
+            //  Further complicating things, the accessor for `f1` cannot be in `C$inlineFun$1`, as otherwise
+            //  the accessor itself will be regenerated (and thus not work) at `inlineFun` call sites.
+            context is IrClass && !approximateToPackage -> context
            // Inline lambdas have already been moved out to the containing class, but we still need to check
            // the containing function (again, see above), so navigate there instead.
-            context = callSite?.scope ?: context.parent
+            context is IrDeclaration -> getScopeClassOrPackage(context.parent, approximateToPackage)
+            // The only non-declaration parent should be the package.
+            else -> context as? IrPackageFragment
        }
-        return context as? IrPackageFragment
    }
 }

@@ -0,0 +1,19 @@
+// TARGET_BACKEND: JVM
+// FILE: test/J.java
+package test;
+
+public class J {
+    String packagePrivate = "OK";
+}
+
+// FILE: test/main.kt
+package test
+
+object O {
+    fun box() = privateInline()
+
+    // no accessor needed - the only call site is in the same package
+    private inline fun privateInline(): String = J().packagePrivate
+}
+
+fun box() = O.box()
@@ -0,0 +1,17 @@
+package a
+
+open class A {
+    protected fun protectedFun(): String = "OK"
+}
+
+class BSamePackage: A() {
+    // known to only be called within `BSamePackage`, so accessors are redundant
+    private inline fun test(): String = protectedFun()
+
+    fun onlyTestCallSite() = test()
+}
+
+// JVM_TEMPLATES
+// 2 INVOKESTATIC a/BSamePackage.access
+// JVM_IR_TEMPLATES
+// 0 INVOKESTATIC a/BSamePackage.access
@@ -39,8 +39,6 @@ suspend fun box() {
 // test.kt:25 box:
 // test.kt:26 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null
 // test.kt:19 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, $i$f$suspendBar:int=0:int
-// test.kt:19 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, $i$f$suspendBar:int=0:int
-// test.kt:14 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, $i$f$suspendBar:int=0:int, $this$extensionFun$iv$iv:AtomicInt=AtomicInt, $i$f$extensionFun:int=0:int
 // test.kt:14 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, $i$f$suspendBar:int=0:int, $this$extensionFun$iv$iv:AtomicInt=AtomicInt, $i$f$extensionFun:int=0:int
 // test.kt:15 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, $i$f$suspendBar:int=0:int, $this$extensionFun$iv$iv:AtomicInt=AtomicInt, $i$f$extensionFun:int=0:int
 // test.kt:6 getValue:
@@ -41728,6 +41728,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest {
            runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivate.kt");
        }

+        @Test
+        @TestMetadata("packagePrivateInPrivateInline.kt")
+        public void testPackagePrivateInPrivateInline() throws Exception {
+            runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt");
+        }
+
        @Test
        @TestMetadata("protectedFromLambda.kt")
        public void testProtectedFromLambda() throws Exception {
@@ -355,6 +355,12 @@ public class BytecodeTextTestGenerated extends AbstractBytecodeTextTest {
        runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackageCrossinline.kt");
    }

+    @Test
+    @TestMetadata("noAccessorForProtectedInSamePackagePrivateInline.kt")
+    public void testNoAccessorForProtectedInSamePackagePrivateInline() throws Exception {
+        runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackagePrivateInline.kt");
+    }
+
    @Test
    @TestMetadata("noFlagAnnotations.kt")
    public void testNoFlagAnnotations() throws Exception {
@@ -41884,6 +41884,12 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes
            runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivate.kt");
        }

+        @Test
+        @TestMetadata("packagePrivateInPrivateInline.kt")
+        public void testPackagePrivateInPrivateInline() throws Exception {
+            runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt");
+        }
+
        @Test
        @TestMetadata("protectedFromLambda.kt")
        public void testProtectedFromLambda() throws Exception {
@@ -367,6 +367,12 @@ public class IrBytecodeTextTestGenerated extends AbstractIrBytecodeTextTest {
        runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackageCrossinline.kt");
    }

+    @Test
+    @TestMetadata("noAccessorForProtectedInSamePackagePrivateInline.kt")
+    public void testNoAccessorForProtectedInSamePackagePrivateInline() throws Exception {
+        runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackagePrivateInline.kt");
+    }
+
    @Test
    @TestMetadata("noFlagAnnotations.kt")
    public void testNoFlagAnnotations() throws Exception {
@@ -33568,6 +33568,11 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes
            runTest("compiler/testData/codegen/box/syntheticAccessors/kt9958Interface.kt");
        }

+        @TestMetadata("packagePrivateInPrivateInline.kt")
+        public void testPackagePrivateInPrivateInline() throws Exception {
+            runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt");
+        }
+
        @TestMetadata("protectedFromLambda.kt")
        public void testProtectedFromLambda() throws Exception {
            runTest("compiler/testData/codegen/box/syntheticAccessors/protectedFromLambda.kt");