From 4a76ea6ecb088341c084b9c99fa270a96f8e4e51 Mon Sep 17 00:00:00 2001 From: pyos Date: Tue, 5 Jan 2021 13:09:10 +0100 Subject: [PATCH] JVM: regenerate objects if they have been regenerated in parent contexts This is a hack to work around the fact that type mappings should not be inherited by inlining contexts for lambdas called from anonymous objects. As the lambda can call the inline function again, this could produce a reference to the original object, which is remapped to a new type in the parent context. Unfortunately, there are many redundant `MethodRemapper`s between the lambda and the class file, so simply editing `TypeRemapper` does not work. Hence, this hack. For now. (Issue found by compiling IntelliJ IDEA BTW.) --- .../kotlin/codegen/inline/MethodInliner.kt | 35 +++++++++---------- .../codegen/inline/TransformationInfo.kt | 18 +++++----- ...FirBlackBoxInlineCodegenTestGenerated.java | 5 +++ .../constructOriginalInRegenerated.kt | 15 ++++++++ .../BlackBoxInlineCodegenTestGenerated.java | 5 +++ ...otlinAgainstInlineKotlinTestGenerated.java | 5 +++ .../IrBlackBoxInlineCodegenTestGenerated.java | 5 +++ ...otlinAgainstInlineKotlinTestGenerated.java | 5 +++ ...JvmIrAgainstOldBoxInlineTestGenerated.java | 5 +++ ...JvmOldAgainstIrBoxInlineTestGenerated.java | 5 +++ .../IrJsCodegenInlineES6TestGenerated.java | 5 +++ .../IrJsCodegenInlineTestGenerated.java | 5 +++ .../JsCodegenInlineTestGenerated.java | 5 +++ 13 files changed, 89 insertions(+), 29 deletions(-) create mode 100644 compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt index a1ffd4dbbdf..df48334c6e4 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/MethodInliner.kt @@ -157,13 +157,8 @@ class MethodInliner( // MethodRemapper doesn't extends LocalVariablesSorter, but RemappingMethodAdapter does. // So wrapping with LocalVariablesSorter to keep old behavior // TODO: investigate LocalVariablesSorter removing (see also same code in RemappingClassBuilder.java) - val remappingMethodAdapter = MethodRemapper( - LocalVariablesSorter( - resultNode.access, - resultNode.desc, - wrapWithMaxLocalCalc(resultNode) - ), AsmTypeRemapper(remapper, result) - ) + val localVariablesSorter = LocalVariablesSorter(resultNode.access, resultNode.desc, wrapWithMaxLocalCalc(resultNode)) + val remappingMethodAdapter = MethodRemapper(localVariablesSorter, AsmTypeRemapper(remapper, result)) val fakeContinuationName = CoroutineTransformer.findFakeContinuationConstructorClassName(node) val markerShift = calcMarkerShift(parameters, node) @@ -174,7 +169,9 @@ class MethodInliner( transformationInfo = iterator.next() val oldClassName = transformationInfo!!.oldClassName - if (transformationInfo!!.shouldRegenerate(isSameModule)) { + if (inliningContext.parent?.transformationInfo?.oldClassName == oldClassName) { + // Object constructs itself, don't enter an infinite recursion of regeneration. + } else if (transformationInfo!!.shouldRegenerate(isSameModule)) { //TODO: need poping of type but what to do with local funs??? val newClassName = transformationInfo!!.newClassName remapper.addMapping(oldClassName, newClassName) @@ -206,7 +203,7 @@ class MethodInliner( ReifiedTypeInliner.putNeedClassReificationMarker(mv) result.reifiedTypeParametersUsages.mergeAll(transformResult.reifiedTypeParametersUsages) } - } else if (!transformationInfo!!.wasAlreadyRegenerated) { + } else { result.addNotChangedClass(oldClassName) } } @@ -300,7 +297,7 @@ class MethodInliner( val varRemapper = LocalVarRemapper(lambdaParameters, valueParamShift) //TODO add skipped this and receiver - val lambdaResult = inliner.doInline(this.mv, varRemapper, true, info, invokeCall.finallyDepthShift) + val lambdaResult = inliner.doInline(localVariablesSorter, varRemapper, true, info, invokeCall.finallyDepthShift) result.mergeWithNotChangeInfo(lambdaResult) result.reifiedTypeParametersUsages.mergeAll(lambdaResult.reifiedTypeParametersUsages) @@ -312,19 +309,19 @@ class MethodInliner( inlineOnlySmapSkipper?.onInlineLambdaEnd(remappingMethodAdapter) } else if (isAnonymousConstructorCall(owner, name)) { //TODO add method //TODO add proper message - var info = transformationInfo as? AnonymousObjectTransformationInfo ?: throw AssertionError( + val newInfo = transformationInfo as? AnonymousObjectTransformationInfo ?: throw AssertionError( " call doesn't correspond to object transformation info for '$owner.$name': $transformationInfo" ) - val objectConstructsItself = inlineCallSiteInfo.ownerClassName == info.oldClassName - if (objectConstructsItself) { - // inline fun f() -> new f$1 -> fun something() in class f$1 -> new f$1 - // ^-- fetch the info that was created for this instruction - info = inliningContext.parent?.transformationInfo as? AnonymousObjectTransformationInfo - ?: throw AssertionError("anonymous object $owner constructs itself, but we have no info on in") - } + // inline fun f() -> new f$1 -> fun something() in class f$1 -> new f$1 + // ^-- fetch the info that was created for this instruction + // Currently, this self-reference pattern only happens in coroutine objects, which construct + // copies of themselves in `create` or `invoke` (not `invokeSuspend`). + val existingInfo = inliningContext.parent?.transformationInfo?.takeIf { it.oldClassName == newInfo.oldClassName } + as AnonymousObjectTransformationInfo? + val info = existingInfo ?: newInfo if (info.shouldRegenerate(isSameModule)) { for (capturedParamDesc in info.allRecapturedParameters) { - val realDesc = if (objectConstructsItself && capturedParamDesc.fieldName == AsmUtil.THIS) { + val realDesc = if (existingInfo != null && capturedParamDesc.fieldName == AsmUtil.THIS) { // The captures in `info` are relative to the parent context, so a normal `this` there // is a captured outer `this` here. CapturedParamDesc(Type.getObjectType(owner), AsmUtil.CAPTURED_THIS_FIELD, capturedParamDesc.type) diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/TransformationInfo.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/TransformationInfo.kt index ef093536e94..23b847547e9 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/TransformationInfo.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/TransformationInfo.kt @@ -26,10 +26,6 @@ interface TransformationInfo { val nameGenerator: NameGenerator - val wasAlreadyRegenerated: Boolean - get() = false - - fun shouldRegenerate(sameModule: Boolean): Boolean fun canRemoveAfterTransformation(): Boolean @@ -86,9 +82,6 @@ class AnonymousObjectTransformationInfo internal constructor( lateinit var capturedLambdasToInline: Map - override val wasAlreadyRegenerated: Boolean - get() = alreadyRegenerated - constructor( ownerInternalName: String, needReification: Boolean, @@ -97,9 +90,14 @@ class AnonymousObjectTransformationInfo internal constructor( nameGenerator: NameGenerator ) : this(ownerInternalName, needReification, hashMapOf(), false, alreadyRegenerated, null, isStaticOrigin, nameGenerator) - override fun shouldRegenerate(sameModule: Boolean): Boolean = !alreadyRegenerated && - (!sameModule || capturedOuterRegenerated || needReification || capturesAnonymousObjectThatMustBeRegenerated || - functionalArguments.values.any { it != NonInlineableArgumentForInlineableParameterCalledInSuspend }) + // TODO: unconditionally regenerating an object if it has previously been regenerated is a hack that works around + // the fact that TypeRemapper cannot differentiate between different references to the same object. See the test + // boxInline/anonymousObject/constructOriginalInRegenerated.kt for an example where a single anonymous object + // is referenced twice with otherwise different `shouldRegenerate` results and the inliner gets confused, trying + // to map the inner reference to the outer regenerated type and producing an infinite recursion. + override fun shouldRegenerate(sameModule: Boolean): Boolean = alreadyRegenerated || + !sameModule || capturedOuterRegenerated || needReification || capturesAnonymousObjectThatMustBeRegenerated || + functionalArguments.values.any { it != NonInlineableArgumentForInlineableParameterCalledInSuspend } override fun canRemoveAfterTransformation(): Boolean { // Note: It is unsafe to remove anonymous class that is referenced by GETSTATIC within lambda diff --git a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/codegen/ir/FirBlackBoxInlineCodegenTestGenerated.java b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/codegen/ir/FirBlackBoxInlineCodegenTestGenerated.java index 765f6d542d7..83d79e91a01 100644 --- a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/codegen/ir/FirBlackBoxInlineCodegenTestGenerated.java +++ b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/codegen/ir/FirBlackBoxInlineCodegenTestGenerated.java @@ -102,6 +102,11 @@ public class FirBlackBoxInlineCodegenTestGenerated extends AbstractFirBlackBoxIn runTest("compiler/testData/codegen/boxInline/anonymousObject/changingReturnType.kt"); } + @TestMetadata("constructOriginalInRegenerated.kt") + public void testConstructOriginalInRegenerated() throws Exception { + runTest("compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt"); + } + @TestMetadata("constructorVisibility.kt") public void testConstructorVisibility() throws Exception { runTest("compiler/testData/codegen/boxInline/anonymousObject/constructorVisibility.kt"); diff --git a/compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt b/compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt new file mode 100644 index 00000000000..ac3178a5c69 --- /dev/null +++ b/compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt @@ -0,0 +1,15 @@ +// FILE: 1.kt +// Intentionally in the same package, as objects in other packages are always regenerated. + +fun f(x: () -> String) = x() + +inline fun g(crossinline x: () -> String) = f { x() } + +// FILE: 2.kt +// _1Kt$g$1 not regenerated because the original already does the same thing (invoking a functional object) +// \-v +fun h(x: () -> String) = g { g(x) } +// /-^ +// _1Kt$g$1 regenerated to inline the lambda + +fun box() = h { "OK" } diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java index 8452fce8d1b..a1c375e4b7f 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/BlackBoxInlineCodegenTestGenerated.java @@ -102,6 +102,11 @@ public class BlackBoxInlineCodegenTestGenerated extends AbstractBlackBoxInlineCo runTest("compiler/testData/codegen/boxInline/anonymousObject/changingReturnType.kt"); } + @TestMetadata("constructOriginalInRegenerated.kt") + public void testConstructOriginalInRegenerated() throws Exception { + runTest("compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt"); + } + @TestMetadata("constructorVisibility.kt") public void testConstructorVisibility() throws Exception { runTest("compiler/testData/codegen/boxInline/anonymousObject/constructorVisibility.kt"); diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java index 84f33fa5110..d9bc3ce43a5 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/CompileKotlinAgainstInlineKotlinTestGenerated.java @@ -102,6 +102,11 @@ public class CompileKotlinAgainstInlineKotlinTestGenerated extends AbstractCompi runTest("compiler/testData/codegen/boxInline/anonymousObject/changingReturnType.kt"); } + @TestMetadata("constructOriginalInRegenerated.kt") + public void testConstructOriginalInRegenerated() throws Exception { + runTest("compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt"); + } + @TestMetadata("constructorVisibility.kt") public void testConstructorVisibility() throws Exception { runTest("compiler/testData/codegen/boxInline/anonymousObject/constructorVisibility.kt"); diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java index 64d54213a38..5db3b918dbf 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/IrBlackBoxInlineCodegenTestGenerated.java @@ -102,6 +102,11 @@ public class IrBlackBoxInlineCodegenTestGenerated extends AbstractIrBlackBoxInli runTest("compiler/testData/codegen/boxInline/anonymousObject/changingReturnType.kt"); } + @TestMetadata("constructOriginalInRegenerated.kt") + public void testConstructOriginalInRegenerated() throws Exception { + runTest("compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt"); + } + @TestMetadata("constructorVisibility.kt") public void testConstructorVisibility() throws Exception { runTest("compiler/testData/codegen/boxInline/anonymousObject/constructorVisibility.kt"); diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/IrCompileKotlinAgainstInlineKotlinTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/IrCompileKotlinAgainstInlineKotlinTestGenerated.java index e3e8f6f2da4..50ee263b67e 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/IrCompileKotlinAgainstInlineKotlinTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/IrCompileKotlinAgainstInlineKotlinTestGenerated.java @@ -102,6 +102,11 @@ public class IrCompileKotlinAgainstInlineKotlinTestGenerated extends AbstractIrC runTest("compiler/testData/codegen/boxInline/anonymousObject/changingReturnType.kt"); } + @TestMetadata("constructOriginalInRegenerated.kt") + public void testConstructOriginalInRegenerated() throws Exception { + runTest("compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt"); + } + @TestMetadata("constructorVisibility.kt") public void testConstructorVisibility() throws Exception { runTest("compiler/testData/codegen/boxInline/anonymousObject/constructorVisibility.kt"); diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/JvmIrAgainstOldBoxInlineTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/JvmIrAgainstOldBoxInlineTestGenerated.java index 9e053a8c5dc..7fbe2635784 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/JvmIrAgainstOldBoxInlineTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/JvmIrAgainstOldBoxInlineTestGenerated.java @@ -102,6 +102,11 @@ public class JvmIrAgainstOldBoxInlineTestGenerated extends AbstractJvmIrAgainstO runTest("compiler/testData/codegen/boxInline/anonymousObject/changingReturnType.kt"); } + @TestMetadata("constructOriginalInRegenerated.kt") + public void testConstructOriginalInRegenerated() throws Exception { + runTest("compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt"); + } + @TestMetadata("constructorVisibility.kt") public void testConstructorVisibility() throws Exception { runTest("compiler/testData/codegen/boxInline/anonymousObject/constructorVisibility.kt"); diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/JvmOldAgainstIrBoxInlineTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/JvmOldAgainstIrBoxInlineTestGenerated.java index b3298bf657d..f66d8be4f83 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/JvmOldAgainstIrBoxInlineTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/ir/JvmOldAgainstIrBoxInlineTestGenerated.java @@ -102,6 +102,11 @@ public class JvmOldAgainstIrBoxInlineTestGenerated extends AbstractJvmOldAgainst runTest("compiler/testData/codegen/boxInline/anonymousObject/changingReturnType.kt"); } + @TestMetadata("constructOriginalInRegenerated.kt") + public void testConstructOriginalInRegenerated() throws Exception { + runTest("compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt"); + } + @TestMetadata("constructorVisibility.kt") public void testConstructorVisibility() throws Exception { runTest("compiler/testData/codegen/boxInline/anonymousObject/constructorVisibility.kt"); diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/es6/semantics/IrJsCodegenInlineES6TestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/es6/semantics/IrJsCodegenInlineES6TestGenerated.java index a1d8520d3d9..11e204014bb 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/es6/semantics/IrJsCodegenInlineES6TestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/es6/semantics/IrJsCodegenInlineES6TestGenerated.java @@ -102,6 +102,11 @@ public class IrJsCodegenInlineES6TestGenerated extends AbstractIrJsCodegenInline runTest("compiler/testData/codegen/boxInline/anonymousObject/changingReturnType.kt"); } + @TestMetadata("constructOriginalInRegenerated.kt") + public void testConstructOriginalInRegenerated() throws Exception { + runTest("compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt"); + } + @TestMetadata("constructorVisibility.kt") public void testConstructorVisibility() throws Exception { runTest("compiler/testData/codegen/boxInline/anonymousObject/constructorVisibility.kt"); diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/semantics/IrJsCodegenInlineTestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/semantics/IrJsCodegenInlineTestGenerated.java index 8e2b513129f..0d6fbc8db9c 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/semantics/IrJsCodegenInlineTestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/ir/semantics/IrJsCodegenInlineTestGenerated.java @@ -102,6 +102,11 @@ public class IrJsCodegenInlineTestGenerated extends AbstractIrJsCodegenInlineTes runTest("compiler/testData/codegen/boxInline/anonymousObject/changingReturnType.kt"); } + @TestMetadata("constructOriginalInRegenerated.kt") + public void testConstructOriginalInRegenerated() throws Exception { + runTest("compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt"); + } + @TestMetadata("constructorVisibility.kt") public void testConstructorVisibility() throws Exception { runTest("compiler/testData/codegen/boxInline/anonymousObject/constructorVisibility.kt"); diff --git a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/semantics/JsCodegenInlineTestGenerated.java b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/semantics/JsCodegenInlineTestGenerated.java index 11821520f20..ac312ac3401 100644 --- a/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/semantics/JsCodegenInlineTestGenerated.java +++ b/js/js.tests/tests-gen/org/jetbrains/kotlin/js/test/semantics/JsCodegenInlineTestGenerated.java @@ -102,6 +102,11 @@ public class JsCodegenInlineTestGenerated extends AbstractJsCodegenInlineTest { runTest("compiler/testData/codegen/boxInline/anonymousObject/changingReturnType.kt"); } + @TestMetadata("constructOriginalInRegenerated.kt") + public void testConstructOriginalInRegenerated() throws Exception { + runTest("compiler/testData/codegen/boxInline/anonymousObject/constructOriginalInRegenerated.kt"); + } + @TestMetadata("constructorVisibility.kt") public void testConstructorVisibility() throws Exception { runTest("compiler/testData/codegen/boxInline/anonymousObject/constructorVisibility.kt");