JVM_IR: delay writes to the source map until generateMethod

`generateMethodNode` should not have any side effects for the output to be stable under incremental compilation.
2020-03-26 15:46:18 +01:00
parent 495cefe65d
commit 9ed73439f8
18 changed files with 167 additions and 50 deletions
@@ -2460,6 +2460,16 @@ public class IncrementalJvmCompilerRunnerTestGenerated extends AbstractIncrement
            runTest("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/changeAnnotationInJavaClass/");
        }

+        @TestMetadata("inlineFunctionRegeneratedObjectStability")
+        public void testInlineFunctionRegeneratedObjectStability() throws Exception {
+            runTest("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionRegeneratedObjectStability/");
+        }
+
+        @TestMetadata("inlineFunctionSmapStability")
+        public void testInlineFunctionSmapStability() throws Exception {
+            runTest("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionSmapStability/");
+        }
+
        @TestMetadata("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/addAnnotationToJavaClass")
        @TestDataPath("$PROJECT_ROOT")
        @RunWith(JUnit3RunnerWithInners.class)
@@ -2498,5 +2508,31 @@ public class IncrementalJvmCompilerRunnerTestGenerated extends AbstractIncrement
                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/changeAnnotationInJavaClass"), Pattern.compile("^([^\\.]+)$"), null, TargetBackend.JVM, true);
            }
        }
+
+        @TestMetadata("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionRegeneratedObjectStability")
+        @TestDataPath("$PROJECT_ROOT")
+        @RunWith(JUnit3RunnerWithInners.class)
+        public static class InlineFunctionRegeneratedObjectStability extends AbstractIncrementalJvmCompilerRunnerTest {
+            private void runTest(String testDataFilePath) throws Exception {
+                KotlinTestUtils.runTest(this::doTest, TargetBackend.JVM, testDataFilePath);
+            }
+
+            public void testAllFilesPresentInInlineFunctionRegeneratedObjectStability() throws Exception {
+                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionRegeneratedObjectStability"), Pattern.compile("^([^\\.]+)$"), null, TargetBackend.JVM, true);
+            }
+        }
+
+        @TestMetadata("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionSmapStability")
+        @TestDataPath("$PROJECT_ROOT")
+        @RunWith(JUnit3RunnerWithInners.class)
+        public static class InlineFunctionSmapStability extends AbstractIncrementalJvmCompilerRunnerTest {
+            private void runTest(String testDataFilePath) throws Exception {
+                KotlinTestUtils.runTest(this::doTest, TargetBackend.JVM, testDataFilePath);
+            }
+
+            public void testAllFilesPresentInInlineFunctionSmapStability() throws Exception {
+                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionSmapStability"), Pattern.compile("^([^\\.]+)$"), null, TargetBackend.JVM, true);
+            }
+        }
    }
 }
@@ -2460,6 +2460,16 @@ public class IrIncrementalJvmCompilerRunnerTestGenerated extends AbstractIrIncre
            runTest("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/changeAnnotationInJavaClass/");
        }

+        @TestMetadata("inlineFunctionRegeneratedObjectStability")
+        public void testInlineFunctionRegeneratedObjectStability() throws Exception {
+            runTest("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionRegeneratedObjectStability/");
+        }
+
+        @TestMetadata("inlineFunctionSmapStability")
+        public void testInlineFunctionSmapStability() throws Exception {
+            runTest("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionSmapStability/");
+        }
+
        @TestMetadata("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/addAnnotationToJavaClass")
        @TestDataPath("$PROJECT_ROOT")
        @RunWith(JUnit3RunnerWithInners.class)
@@ -2498,5 +2508,31 @@ public class IrIncrementalJvmCompilerRunnerTestGenerated extends AbstractIrIncre
                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/changeAnnotationInJavaClass"), Pattern.compile("^([^\\.]+)$"), null, TargetBackend.JVM_IR, true);
            }
        }
+
+        @TestMetadata("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionRegeneratedObjectStability")
+        @TestDataPath("$PROJECT_ROOT")
+        @RunWith(JUnit3RunnerWithInners.class)
+        public static class InlineFunctionRegeneratedObjectStability extends AbstractIrIncrementalJvmCompilerRunnerTest {
+            private void runTest(String testDataFilePath) throws Exception {
+                KotlinTestUtils.runTest(this::doTest, TargetBackend.JVM_IR, testDataFilePath);
+            }
+
+            public void testAllFilesPresentInInlineFunctionRegeneratedObjectStability() throws Exception {
+                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionRegeneratedObjectStability"), Pattern.compile("^([^\\.]+)$"), null, TargetBackend.JVM_IR, true);
+            }
+        }
+
+        @TestMetadata("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionSmapStability")
+        @TestDataPath("$PROJECT_ROOT")
+        @RunWith(JUnit3RunnerWithInners.class)
+        public static class InlineFunctionSmapStability extends AbstractIrIncrementalJvmCompilerRunnerTest {
+            private void runTest(String testDataFilePath) throws Exception {
+                KotlinTestUtils.runTest(this::doTest, TargetBackend.JVM_IR, testDataFilePath);
+            }
+
+            public void testAllFilesPresentInInlineFunctionSmapStability() throws Exception {
+                KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("jps-plugin/testData/incremental/incrementalJvmCompilerOnly/inlineFunctionSmapStability"), Pattern.compile("^([^\\.]+)$"), null, TargetBackend.JVM_IR, true);
+            }
+        }
    }
 }
@@ -96,14 +96,7 @@ class ClassCodegen private constructor(
        )
    }

-    private var sourceMapper: DefaultSourceMapper? = null
-
-    fun getOrCreateSourceMapper(): DefaultSourceMapper {
-        if (sourceMapper == null) {
-            sourceMapper = context.getSourceMapper(irClass)
-        }
-        return sourceMapper!!
-    }
+    internal var writeSourceMap: Boolean = withinInline

    private val serializerExtension = JvmSerializerExtension(visitor.serializationBindings, state, typeMapper)
    private val serializer: DescriptorSerializer? =
@@ -134,11 +127,12 @@ class ClassCodegen private constructor(
        if (generated) return reifiedTypeParametersUsages
        generated = true

+        val smap = context.getSourceMapper(irClass)
        for (declaration in irClass.declarations) {
            when (declaration) {
                is IrClass, classInitializer -> Unit // see below
                is IrField -> generateField(declaration)
-                is IrFunction -> generateMethod(declaration)
+                is IrFunction -> generateMethod(declaration, smap)
                else -> throw AssertionError("unexpected class member $declaration at codegen")
            }
        }
@@ -147,7 +141,7 @@ class ClassCodegen private constructor(
        // might need to generate the `$assertionsDisabled` field initializer.
        classInitializer?.let {
            generatingClInit = true
-            generateMethod(it)
+            generateMethod(it, smap)
        }

        // Generate nested classes at the end, to ensure that when the companion's metadata is serialized
@@ -165,18 +159,12 @@ class ClassCodegen private constructor(
        }.genAnnotations(irClass, null, null)
        generateKotlinMetadataAnnotation()

-        val fileEntry = context.psiSourceManager.getFileEntry(irClass.fileParent)
-        if (fileEntry != null) {
-            /* TODO: Temporary workaround: ClassBuilder needs a pathless name. */
-            val shortName = File(fileEntry.name).name
-            visitor.visitSource(shortName, null)
-        }
-
        generateInnerAndOuterClasses()

-        if (withinInline || sourceMapper != null) {
-            val smap = getOrCreateSourceMapper()
+        if (writeSourceMap) {
            visitor.visitSMAP(smap, !context.state.languageVersionSettings.supportsFeature(LanguageFeature.CorrectSourceMappingSyntax))
+        } else {
+            visitor.visitSource(smap.sourceInfo.source, null)
        }

        visitor.done()
@@ -343,46 +331,51 @@ class ClassCodegen private constructor(
        }
    }

-    private val generatedInlineMethods = mutableMapOf<IrFunction, MethodNode>()
+    private val generatedInlineMethods = mutableMapOf<IrFunction, SMAPAndMethodNode>()

-    fun generateMethodNode(method: IrFunction): MethodNode {
+    fun generateMethodNode(method: IrFunction): SMAPAndMethodNode {
        if (!method.isInline && !method.isSuspend) {
            // Inline methods can be used multiple times by `IrSourceCompilerForInline`, suspend methods
            // could be used twice if they capture crossinline lambdas, and everything else is only
            // generated by `generateMethod` below so does not need caching.
            return FunctionCodegen(method, this).generate()
        }
-        val node = generatedInlineMethods.getOrPut(method) { FunctionCodegen(method, this).generate() }
+        val (node, smap) = generatedInlineMethods.getOrPut(method) { FunctionCodegen(method, this).generate() }
        val copy = with(node) { MethodNode(Opcodes.API_VERSION, access, name, desc, signature, exceptions.toTypedArray()) }
        node.instructions.resetLabels()
        node.accept(copy)
-        return copy
+        return SMAPAndMethodNode(copy, smap)
    }

-    private fun generateMethod(method: IrFunction) {
+    private fun generateMethod(method: IrFunction, classSMAP: DefaultSourceMapper) {
        if (method.isFakeOverride) {
            jvmSignatureClashDetector.trackFakeOverrideMethod(method)
            return
        }

-        val node = generateMethodNode(method)
+        val (node, smap) = generateMethodNode(method)
        node.preprocessSuspendMarkers(
            method.origin == JvmLoweredDeclarationOrigin.FOR_INLINE_STATE_MACHINE_TEMPLATE,
            method.origin == JvmLoweredDeclarationOrigin.FOR_INLINE_STATE_MACHINE_TEMPLATE_CAPTURES_CROSSINLINE
        )
        val mv = with(node) { visitor.newMethod(method.OtherOrigin, access, name, desc, signature, exceptions.toTypedArray()) }
+        val smapCopier = NestedSourceMapper(classSMAP, smap, sameFile = true)
+        val smapCopyingVisitor = object : MethodVisitor(Opcodes.API_VERSION, mv) {
+            override fun visitLineNumber(line: Int, start: Label) =
+                super.visitLineNumber(smapCopier.mapLineNumber(line), start)
+        }
        if (method.hasContinuation() || method.isInvokeSuspendOfLambda()) {
            // Generate a state machine within this method. The continuation class for it should be generated
            // lazily so that if tail call optimization kicks in, the unused class will not be written to the output.
            val continuationClassCodegen = lazy { getOrCreate(method.continuationClass()!!, context, method) }
-            node.acceptWithStateMachine(method, this, mv) {
+            node.acceptWithStateMachine(method, this, smapCopyingVisitor) {
                if (method.isSuspend) continuationClassCodegen.value.visitor else visitor
            }
            if (continuationClassCodegen.isInitialized() || method.alwaysNeedsContinuation()) {
                continuationClassCodegen.value.generate()
            }
        } else {
-            node.accept(mv)
+            node.accept(smapCopyingVisitor)
        }
        jvmSignatureClashDetector.trackMethod(method, RawSignature(node.name, node.desc, MemberKind.METHOD))

@@ -107,7 +107,7 @@ class ExpressionCodegen(
    val mv: InstructionAdapter,
    val classCodegen: ClassCodegen,
    val inlinedInto: ExpressionCodegen?,
-    val smapOverride: DefaultSourceMapper?
+    val smap: DefaultSourceMapper
 ) : IrElementVisitor<PromisedValue, BlockInfo>, BaseExpressionCodegen {

    var finallyDepth = 0
@@ -12,9 +12,7 @@ import org.jetbrains.kotlin.backend.jvm.JvmBackendContext
 import org.jetbrains.kotlin.backend.jvm.JvmLoweredDeclarationOrigin
 import org.jetbrains.kotlin.backend.jvm.lower.suspendFunctionOriginal
 import org.jetbrains.kotlin.codegen.AsmUtil
-import org.jetbrains.kotlin.codegen.inline.DefaultSourceMapper
-import org.jetbrains.kotlin.codegen.inline.MethodBodyVisitor
-import org.jetbrains.kotlin.codegen.inline.wrapWithMaxLocalCalc
+import org.jetbrains.kotlin.codegen.inline.*
 import org.jetbrains.kotlin.codegen.mangleNameIfNeeded
 import org.jetbrains.kotlin.codegen.state.GenerationState
 import org.jetbrains.kotlin.codegen.visitAnnotableParameterCount
@@ -44,14 +42,14 @@ class FunctionCodegen(
 ) {
    private val context = classCodegen.context

-    fun generate(smapOverride: DefaultSourceMapper? = null): MethodNode =
+    fun generate(): SMAPAndMethodNode =
        try {
-            doGenerate(smapOverride)
+            doGenerate()
        } catch (e: Throwable) {
            throw RuntimeException("Exception while generating code for:\n${irFunction.dump()}", e)
        }

-    private fun doGenerate(smapOverride: DefaultSourceMapper?): MethodNode {
+    private fun doGenerate(): SMAPAndMethodNode {
        val signature = context.methodSignatureMapper.mapSignatureWithGeneric(irFunction)
        val flags = irFunction.calculateMethodFlags()
        val methodNode = MethodNode(
@@ -90,23 +88,28 @@ class FunctionCodegen(
        // `$$forInline` versions of suspend functions have the same bodies as the originals, but with different
        // name/flags/annotations and with no state machine.
        val notForInline = irFunction.suspendForInlineToOriginal()
-        if (!context.state.classBuilderMode.generateBodies || flags.and(Opcodes.ACC_ABSTRACT) != 0 || irFunction.isExternal) {
+        val smap = if (!context.state.classBuilderMode.generateBodies || flags.and(Opcodes.ACC_ABSTRACT) != 0 || irFunction.isExternal) {
            generateAnnotationDefaultValueIfNeeded(methodVisitor)
+            SMAP(listOf(FileMapping.SKIP))
        } else if (notForInline != null) {
-            classCodegen.generateMethodNode(notForInline).accept(MethodBodyVisitor(methodVisitor))
+            val (originalNode, smap) = classCodegen.generateMethodNode(notForInline)
+            originalNode.accept(MethodBodyVisitor(methodVisitor))
+            smap
        } else {
+            val sourceMapper = context.getSourceMapper(classCodegen.irClass)
            val frameMap = irFunction.createFrameMapWithReceivers()
            context.state.globalInlineContext.enterDeclaration(irFunction.suspendFunctionOriginal().descriptor)
            try {
                val adapter = InstructionAdapter(methodVisitor)
-                ExpressionCodegen(irFunction, signature, frameMap, adapter, classCodegen, inlinedInto, smapOverride).generate()
+                ExpressionCodegen(irFunction, signature, frameMap, adapter, classCodegen, inlinedInto, sourceMapper).generate()
            } finally {
                context.state.globalInlineContext.exitDeclaration()
            }
            methodVisitor.visitMaxs(-1, -1)
+            SMAP(sourceMapper.resultMappings)
        }
        methodVisitor.visitEnd()
-        return methodNode
+        return SMAPAndMethodNode(methodNode, smap)
    }

    // Since the only arguments to anonymous object constructors are captured variables and complex
@@ -86,13 +86,10 @@ class IrSourceCompilerForInline(
        }

    override val lazySourceMapper: DefaultSourceMapper
-        get() = codegen.smapOverride ?: codegen.classCodegen.getOrCreateSourceMapper()
+        get() = codegen.smap.also { codegen.classCodegen.writeSourceMap = true }

-    override fun generateLambdaBody(lambdaInfo: ExpressionLambda): SMAPAndMethodNode {
-        val smap = codegen.context.getSourceMapper(codegen.classCodegen.irClass)
-        val node = FunctionCodegen((lambdaInfo as IrExpressionLambdaImpl).function, codegen.classCodegen, codegen).generate(smap)
-        return SMAPAndMethodNode(node, SMAP(smap.resultMappings))
-    }
+    override fun generateLambdaBody(lambdaInfo: ExpressionLambda): SMAPAndMethodNode =
+        FunctionCodegen((lambdaInfo as IrExpressionLambdaImpl).function, codegen.classCodegen, codegen).generate()

    override fun doCreateMethodNodeFromSource(
        callableDescriptor: FunctionDescriptor,
@@ -101,9 +98,7 @@ class IrSourceCompilerForInline(
        asmMethod: Method
    ): SMAPAndMethodNode {
        assert(callableDescriptor == callee.symbol.descriptor.original) { "Expected $callableDescriptor got ${callee.descriptor.original}" }
-        val classCodegen = ClassCodegen.getOrCreate(callee.parentAsClass, codegen.context)
-        val node = classCodegen.generateMethodNode(callee)
-        return SMAPAndMethodNode(node, SMAP(classCodegen.getOrCreateSourceMapper().resultMappings))
+        return ClassCodegen.getOrCreate(callee.parentAsClass, codegen.context).generateMethodNode(callee)
    }

    override fun hasFinallyBlocks() = data.hasFinallyBlocks()
@@ -116,7 +111,7 @@ class IrSourceCompilerForInline(
    override fun createCodegenForExternalFinallyBlockGenerationOnNonLocalReturn(finallyNode: MethodNode, curFinallyDepth: Int) =
        ExpressionCodegen(
            codegen.irFunction, codegen.signature, codegen.frameMap, InstructionAdapter(finallyNode), codegen.classCodegen,
-            codegen.inlinedInto, codegen.smapOverride
+            codegen.inlinedInto, codegen.smap
        ).also {
            it.finallyDepth = curFinallyDepth
        }
@@ -83,11 +83,10 @@ internal val DeclarationDescriptorWithSource.psiElement: PsiElement?
 fun JvmBackendContext.getSourceMapper(declaration: IrClass): DefaultSourceMapper {
    val sourceManager = this.psiSourceManager
    val fileEntry = sourceManager.getFileEntry(declaration.fileParent)
-    check(fileEntry != null) { "No PSI file entry found for class: ${declaration.dump()}" }
    // NOTE: apparently inliner requires the source range to cover the
    //       whole file the class is declared in rather than the class only.
    // TODO: revise
-    val endLineNumber = fileEntry.getSourceRangeInfo(0, fileEntry.maxOffset).endLineNumber
+    val endLineNumber = fileEntry?.getSourceRangeInfo(0, fileEntry.maxOffset)?.endLineNumber ?: 0
    return DefaultSourceMapper(
        SourceInfo.createInfoForIr(
            endLineNumber + 1,
@@ -0,0 +1,3 @@
+import inline1.*
+
+fun f() = h()
@@ -0,0 +1,7 @@
+package inline1
+
+import inline2.*
+
+fun g() = h()
+
+inline fun h() = root { "OK" }
@@ -0,0 +1,7 @@
+package inline1
+
+import inline2.*
+
+fun g() = h() + "!"
+
+inline fun h() = root { "OK" }
@@ -0,0 +1,6 @@
+================ Step #1 =================
+Compiling files:
+  src/b.kt
+End of files
+Exit code: OK
+------------------------------------------
@@ -0,0 +1,5 @@
+package inline2
+
+inline fun root(crossinline x: () -> String) = object {
+    fun run() = x()
+}.run()
@@ -0,0 +1 @@
+// IGNORE_BACKEND: JVM
@@ -0,0 +1,3 @@
+import inline1.*
+
+fun f() = h()
@@ -0,0 +1,7 @@
+package inline1
+
+import inline2.*
+
+fun g() = root()
+
+inline fun h() = root()
@@ -0,0 +1,7 @@
+package inline1
+
+import inline2.*
+
+fun g() = root() + "!"
+
+inline fun h() = root()
@@ -0,0 +1,6 @@
+================ Step #1 =================
+Compiling files:
+  src/b.kt
+End of files
+Exit code: OK
+------------------------------------------
@@ -0,0 +1,3 @@
+package inline2
+
+inline fun root() = "OK"