From b42a7be0dea814c5d444d7a28090489f27d090be Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Mon, 24 Oct 2022 23:39:52 +0200 Subject: [PATCH] Psi2ir: keep nullability when substituting function type for SAM type After we added "careful approximation of contravariant projections" in 584b70719e, some SAM conversions started to require an additional implicit cast of the functional value before it is converted to the SAM interface. The target type of this implicit cast was computed incorrectly because it didn't contain nullability of the SAM type. This could lead to a situation where a nullable value was incorrectly cast to a non-null type, which caused a missing null check and NPE at runtime. For example, let's consider the test `kt54600.kt`. SAM conversion happens in the constructor call `J(filter)`. Before 584b70719e, the IR for that argument was (irrelevant things are omitted for simplicity): TYPE_OP SAM_CONVERSION type=Condition! GET_VAR filter type=Function1? After 584b70719e, the IR became: TYPE_OP SAM_CONVERSION type=Condition! TYPE_OP IMPLICIT_CAST type=Function1 GET_VAR filter type=Function1? Note the two changes: * The resulting SAM type changed from `Condition` to `Condition`. This is exactly the point of the "careful approximation" change, because just erasing the "in" projection from the parameter type is incorrect, see the explanation for that change. * The value is now implicitly cast to the _non-null_ function type before it is SAM-converted. The presence of the cast is fine, but the fact that it's to a non-null type is an oversight. The target type for this cast is computed at `KotlinType.getSubstitutedFunctionTypeForSamType` in psi2ir. Now it extracts the nullability from the SAM type and retains it in the resulting function type. After this change, the IR for the argument becomes: TYPE_OP SAM_CONVERSION type=Condition! TYPE_OP IMPLICIT_CAST type=Function1! GET_VAR filter type=Function1? Note that the target type is now flexible, as the resulting SAM type. Another option would be to make it nullable, as the type of the functional value, but there doesn't seem to be any difference. #KT-54600 Fixed --- .../FirBlackBoxCodegenTestGenerated.java | 6 +++++ .../psi2ir/generators/samConversions.kt | 21 ++++++++++----- .../codegen/box/sam/kt54600.fir.ir.txt | 15 +++++++++++ .../testData/codegen/box/sam/kt54600.ir.txt | 16 +++++++++++ compiler/testData/codegen/box/sam/kt54600.kt | 27 +++++++++++++++++++ .../sam/samConversionsWithSmartCasts.ir.txt | 7 +++-- .../sam/samConversionsWithSmartCasts.kt.txt | 3 ++- .../codegen/BlackBoxCodegenTestGenerated.java | 6 +++++ .../IrBlackBoxCodegenTestGenerated.java | 6 +++++ .../LightAnalysisModeTestGenerated.java | 5 ++++ 10 files changed, 101 insertions(+), 11 deletions(-) create mode 100644 compiler/testData/codegen/box/sam/kt54600.fir.ir.txt create mode 100644 compiler/testData/codegen/box/sam/kt54600.ir.txt create mode 100644 compiler/testData/codegen/box/sam/kt54600.kt diff --git a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java index 9dc0d05f4fe..d62df9234e5 100644 --- a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java +++ b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java @@ -46523,6 +46523,12 @@ public class FirBlackBoxCodegenTestGenerated extends AbstractFirBlackBoxCodegenT runTest("compiler/testData/codegen/box/sam/kt52417.kt"); } + @Test + @TestMetadata("kt54600.kt") + public void testKt54600() throws Exception { + runTest("compiler/testData/codegen/box/sam/kt54600.kt"); + } + @Test @TestMetadata("nonInlinedSamWrapper.kt") public void testNonInlinedSamWrapper() throws Exception { diff --git a/compiler/ir/ir.psi2ir/src/org/jetbrains/kotlin/psi2ir/generators/samConversions.kt b/compiler/ir/ir.psi2ir/src/org/jetbrains/kotlin/psi2ir/generators/samConversions.kt index 3bb691075ac..657b5349fc0 100644 --- a/compiler/ir/ir.psi2ir/src/org/jetbrains/kotlin/psi2ir/generators/samConversions.kt +++ b/compiler/ir/ir.psi2ir/src/org/jetbrains/kotlin/psi2ir/generators/samConversions.kt @@ -13,9 +13,7 @@ import org.jetbrains.kotlin.descriptors.synthetic.FunctionInterfaceAdapterExtens import org.jetbrains.kotlin.descriptors.synthetic.FunctionInterfaceConstructorDescriptor import org.jetbrains.kotlin.resolve.sam.getFunctionTypeForAbstractMethod import org.jetbrains.kotlin.resolve.sam.getSingleAbstractMethodOrNull -import org.jetbrains.kotlin.types.KotlinType -import org.jetbrains.kotlin.types.TypeSubstitutor -import org.jetbrains.kotlin.types.Variance +import org.jetbrains.kotlin.types.* fun GeneratorExtensions.SamConversion.isSamType(kotlinType: KotlinType): Boolean { val descriptor = kotlinType.constructor.declarationDescriptor @@ -35,12 +33,23 @@ fun CallableDescriptor.getOriginalForFunctionInterfaceAdapter() = null } -fun KotlinType.getSubstitutedFunctionTypeForSamType(): KotlinType { +fun KotlinType.getSubstitutedFunctionTypeForSamType(): KotlinType = + when (val unwrapped = this.unwrap()) { + is SimpleType -> unwrapped.getSubstitutedFunctionTypeForSamType() + is FlexibleType -> KotlinTypeFactory.flexibleType( + unwrapped.lowerBound.getSubstitutedFunctionTypeForSamType(), + unwrapped.upperBound.getSubstitutedFunctionTypeForSamType(), + ) + } + +private fun SimpleType.getSubstitutedFunctionTypeForSamType(): SimpleType { val descriptor = constructor.declarationDescriptor as? ClassDescriptor ?: throw AssertionError("SAM should be represented by a class: $this") val singleAbstractMethod = getSingleAbstractMethodOrNull(descriptor) ?: throw AssertionError("$descriptor should have a single abstract method") - val unsubstitutedFunctionType = getFunctionTypeForAbstractMethod(singleAbstractMethod, false) - return TypeSubstitutor.create(this).substitute(unsubstitutedFunctionType, Variance.INVARIANT) + val unsubstitutedFunctionType = getFunctionTypeForAbstractMethod(singleAbstractMethod, false).makeNullableAsSpecified(isMarkedNullable) + val result = TypeSubstitutor.create(this).substitute(unsubstitutedFunctionType, Variance.INVARIANT) ?: throw AssertionError("Failed to substitute function type $unsubstitutedFunctionType corresponding to $this") + return result as? SimpleType + ?: throw AssertionError("SAM type substitution result is not a simple type: $this -> $result") } diff --git a/compiler/testData/codegen/box/sam/kt54600.fir.ir.txt b/compiler/testData/codegen/box/sam/kt54600.fir.ir.txt new file mode 100644 index 00000000000..d01302d38b1 --- /dev/null +++ b/compiler/testData/codegen/box/sam/kt54600.fir.ir.txt @@ -0,0 +1,15 @@ +FILE fqName: fileName:/box.kt + FUN name:box visibility:public modality:FINAL <> () returnType:kotlin.String + BLOCK_BODY + RETURN type=kotlin.Nothing from='public final fun box (): kotlin.String declared in ' + CALL 'public final fun foo (filter: kotlin.Function1?): kotlin.String declared in ' type=kotlin.String origin=null + filter: CONST Null type=kotlin.Nothing? value=null + FUN name:foo visibility:public modality:FINAL <> (filter:kotlin.Function1?) returnType:kotlin.String + VALUE_PARAMETER name:filter index:0 type:kotlin.Function1? + BLOCK_BODY + TYPE_OP type=kotlin.Unit origin=IMPLICIT_COERCION_TO_UNIT typeOperand=kotlin.Unit + CONSTRUCTOR_CALL 'public constructor (filter: @[FlexibleNullability] .Condition?) declared in .J' type=.J origin=null + filter: TYPE_OP type=@[FlexibleNullability] .Condition<@[FlexibleNullability] kotlin.String?>? origin=SAM_CONVERSION typeOperand=@[FlexibleNullability] .Condition<@[FlexibleNullability] kotlin.String?>? + GET_VAR 'filter: kotlin.Function1? declared in .foo' type=kotlin.Function1? origin=null + RETURN type=kotlin.Nothing from='public final fun foo (filter: kotlin.Function1?): kotlin.String declared in ' + CONST String type=kotlin.String value="OK" diff --git a/compiler/testData/codegen/box/sam/kt54600.ir.txt b/compiler/testData/codegen/box/sam/kt54600.ir.txt new file mode 100644 index 00000000000..97f44ae2dda --- /dev/null +++ b/compiler/testData/codegen/box/sam/kt54600.ir.txt @@ -0,0 +1,16 @@ +FILE fqName: fileName:/box.kt + FUN name:box visibility:public modality:FINAL <> () returnType:kotlin.String + BLOCK_BODY + RETURN type=kotlin.Nothing from='public final fun box (): kotlin.String declared in ' + CALL 'public final fun foo (filter: kotlin.Function1?): kotlin.String declared in ' type=kotlin.String origin=null + filter: CONST Null type=kotlin.Nothing? value=null + FUN name:foo visibility:public modality:FINAL <> (filter:kotlin.Function1?) returnType:kotlin.String + VALUE_PARAMETER name:filter index:0 type:kotlin.Function1? + BLOCK_BODY + TYPE_OP type=kotlin.Unit origin=IMPLICIT_COERCION_TO_UNIT typeOperand=kotlin.Unit + CONSTRUCTOR_CALL 'public constructor (filter: @[FlexibleNullability] .Condition?) declared in .J' type=.J origin=null + filter: TYPE_OP type=@[FlexibleNullability] .Condition? origin=SAM_CONVERSION typeOperand=@[FlexibleNullability] .Condition? + TYPE_OP type=@[FlexibleNullability] kotlin.Function1<@[ParameterName(name = 't')] kotlin.Any?, kotlin.Boolean>? origin=IMPLICIT_CAST typeOperand=@[FlexibleNullability] kotlin.Function1<@[ParameterName(name = 't')] kotlin.Any?, kotlin.Boolean>? + GET_VAR 'filter: kotlin.Function1? declared in .foo' type=kotlin.Function1? origin=null + RETURN type=kotlin.Nothing from='public final fun foo (filter: kotlin.Function1?): kotlin.String declared in ' + CONST String type=kotlin.String value="OK" diff --git a/compiler/testData/codegen/box/sam/kt54600.kt b/compiler/testData/codegen/box/sam/kt54600.kt new file mode 100644 index 00000000000..7b96d0af73c --- /dev/null +++ b/compiler/testData/codegen/box/sam/kt54600.kt @@ -0,0 +1,27 @@ +// TARGET_BACKEND: JVM +// DUMP_IR +// FILE: box.kt + +fun box(): String = + foo(null) + +fun foo(filter: ((String) -> Boolean)?): String { + J(filter) + return "OK" +} + +// FILE: J.java + +public class J { + public J(Condition filter) { + if (filter != null) { + filter.value(""); + } + } +} + +// FILE: Condition.java + +public interface Condition { + boolean value(T t); +} diff --git a/compiler/testData/ir/irText/expressions/sam/samConversionsWithSmartCasts.ir.txt b/compiler/testData/ir/irText/expressions/sam/samConversionsWithSmartCasts.ir.txt index 90f46390c80..21037d89b92 100644 --- a/compiler/testData/ir/irText/expressions/sam/samConversionsWithSmartCasts.ir.txt +++ b/compiler/testData/ir/irText/expressions/sam/samConversionsWithSmartCasts.ir.txt @@ -106,10 +106,9 @@ FILE fqName: fileName:/samConversionsWithSmartCasts.kt CALL 'public open fun run1 (r: @[FlexibleNullability] java.lang.Runnable?): kotlin.Unit declared in .J' type=kotlin.Unit origin=null $this: CONSTRUCTOR_CALL 'public constructor () [primary] declared in .J' type=.J origin=null r: TYPE_OP type=@[FlexibleNullability] java.lang.Runnable? origin=SAM_CONVERSION typeOperand=@[FlexibleNullability] java.lang.Runnable? - TYPE_OP type=kotlin.Function0 origin=IMPLICIT_NOTNULL typeOperand=kotlin.Function0 - CALL 'public open fun id (x: @[FlexibleNullability] T of .J.id?): @[FlexibleNullability] T of .J.id? declared in .J' type=@[FlexibleNullability] kotlin.Function0? origin=null - : @[FlexibleNullability] kotlin.Function0? - x: GET_VAR 'a: kotlin.Function0 declared in .test8' type=kotlin.Function0 origin=null + CALL 'public open fun id (x: @[FlexibleNullability] T of .J.id?): @[FlexibleNullability] T of .J.id? declared in .J' type=@[FlexibleNullability] kotlin.Function0? origin=null + : @[FlexibleNullability] kotlin.Function0? + x: GET_VAR 'a: kotlin.Function0 declared in .test8' type=kotlin.Function0 origin=null FUN name:test9 visibility:public modality:FINAL <> () returnType:kotlin.Unit BLOCK_BODY CALL 'public open fun run1 (r: @[FlexibleNullability] java.lang.Runnable?): kotlin.Unit declared in .J' type=kotlin.Unit origin=null diff --git a/compiler/testData/ir/irText/expressions/sam/samConversionsWithSmartCasts.kt.txt b/compiler/testData/ir/irText/expressions/sam/samConversionsWithSmartCasts.kt.txt index c0b64824632..a2b263c7356 100644 --- a/compiler/testData/ir/irText/expressions/sam/samConversionsWithSmartCasts.kt.txt +++ b/compiler/testData/ir/irText/expressions/sam/samConversionsWithSmartCasts.kt.txt @@ -58,9 +58,10 @@ fun test7(a: Function1) { } fun test8(a: Function0) { - J().run1(r = id<@FlexibleNullability Function0?>(x = a) /*!! Function0 */ /*-> @FlexibleNullability Runnable? */) + J().run1(r = id<@FlexibleNullability Function0?>(x = a) /*-> @FlexibleNullability Runnable? */) } fun test9() { J().run1(r = ::test9 /*-> @FlexibleNullability Runnable? */) } + diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java index bce4fc07ca5..31ba58d46c8 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java @@ -44909,6 +44909,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { runTest("compiler/testData/codegen/box/sam/kt52417.kt"); } + @Test + @TestMetadata("kt54600.kt") + public void testKt54600() throws Exception { + runTest("compiler/testData/codegen/box/sam/kt54600.kt"); + } + @Test @TestMetadata("nonInlinedSamWrapper.kt") public void testNonInlinedSamWrapper() throws Exception { diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java index 68a1a321b78..caf46188682 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java @@ -46523,6 +46523,12 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes runTest("compiler/testData/codegen/box/sam/kt52417.kt"); } + @Test + @TestMetadata("kt54600.kt") + public void testKt54600() throws Exception { + runTest("compiler/testData/codegen/box/sam/kt54600.kt"); + } + @Test @TestMetadata("nonInlinedSamWrapper.kt") public void testNonInlinedSamWrapper() throws Exception { diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java index 78fd52cc640..a109d77bcc2 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java @@ -36225,6 +36225,11 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes runTest("compiler/testData/codegen/box/sam/kt52417.kt"); } + @TestMetadata("kt54600.kt") + public void testKt54600() throws Exception { + runTest("compiler/testData/codegen/box/sam/kt54600.kt"); + } + @TestMetadata("nonInlinedSamWrapper.kt") public void testNonInlinedSamWrapper() throws Exception { runTest("compiler/testData/codegen/box/sam/nonInlinedSamWrapper.kt");