From 9b853edf09f59a9ecf589fbcb6061546c990f17b Mon Sep 17 00:00:00 2001 From: Nikita Klimenko Date: Fri, 15 Dec 2023 21:07:14 +0200 Subject: [PATCH 1/2] fix conversion of value classes to dataframe --- .../kotlinx/dataframe/impl/api/toDataFrame.kt | 36 ++++++++++- .../kotlinx/dataframe/api/toDataFrame.kt | 60 +++++++++++++++++++ .../kotlinx/dataframe/impl/api/toDataFrame.kt | 35 ++++++++++- .../kotlinx/dataframe/api/toDataFrame.kt | 60 +++++++++++++++++++ 4 files changed, 189 insertions(+), 2 deletions(-) diff --git a/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt b/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt index 93ecd6682b..0207ff280a 100644 --- a/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt +++ b/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt @@ -21,13 +21,16 @@ import org.jetbrains.kotlinx.dataframe.impl.getListType import org.jetbrains.kotlinx.dataframe.impl.projectUpTo import org.jetbrains.kotlinx.dataframe.impl.schema.getPropertiesOrder import java.lang.reflect.InvocationTargetException +import java.lang.reflect.Method import java.time.temporal.Temporal import kotlin.reflect.KClass import kotlin.reflect.KProperty import kotlin.reflect.KVisibility import kotlin.reflect.full.isSubclassOf import kotlin.reflect.full.memberProperties +import kotlin.reflect.full.primaryConstructor import kotlin.reflect.full.withNullability +import kotlin.reflect.jvm.isAccessible import kotlin.reflect.jvm.javaField import kotlin.reflect.typeOf @@ -141,7 +144,26 @@ internal fun convertToDataFrame( val property = it if (excludes.contains(property)) return@mapNotNull null + class ValueClassConverter(val unbox: Method, val box: Method) + + val valueClassConverter = (it.returnType.classifier as? KClass<*>)?.let { kClass -> + if (!kClass.isValue) null else { + val constructor = + requireNotNull(kClass.primaryConstructor) { "value class $kClass is expected to have primary constructor, but couldn't obtain it" } + val parameter = constructor.parameters.singleOrNull() + ?: error("conversion of value class $kClass with multiple parameters in constructor is not yet supported") + // there's no need to unwrap if underlying field is nullable + if (parameter.type.isMarkedNullable) return@let null + // box and unbox impl methods are part of binary API of value classes + // https://youtrack.jetbrains.com/issue/KT-50518/Boxing-Unboxing-methods-for-JvmInline-value-classes-should-be-public-accessible + val unbox = kClass.java.getMethod("unbox-impl") + val box = kClass.java.methods.single { it.name == "box-impl" } + val valueClassConverter = ValueClassConverter(unbox, box) + valueClassConverter + } + } property.javaField?.isAccessible = true +// property.isAccessible = true var nullable = false var hasExceptions = false @@ -151,7 +173,19 @@ internal fun convertToDataFrame( null } else { val value = try { - it.call(obj) + val value = it.call(obj) + /** + * here we do what compiler does + * @see org.jetbrains.kotlinx.dataframe.api.CreateDataFrameTests.testKPropertyGetLibrary + */ + if (valueClassConverter != null) { + val var1 = value?.let { + valueClassConverter.unbox.invoke(it) + } + var1?.let { valueClassConverter.box.invoke(null, var1) } + } else { + value + } } catch (e: InvocationTargetException) { hasExceptions = true e.targetException diff --git a/core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt b/core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt index 5861975a9a..737ca98fff 100644 --- a/core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt +++ b/core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt @@ -10,6 +10,7 @@ import org.jetbrains.kotlinx.dataframe.kind import org.jetbrains.kotlinx.dataframe.type import org.junit.Ignore import org.junit.Test +import kotlin.reflect.KProperty import kotlin.reflect.typeOf class CreateDataFrameTests { @@ -224,4 +225,63 @@ class CreateDataFrameTests { println() } } + + // nullable field here - no generated unwrapping code + @JvmInline + internal value class Speed(val kmh: Number?) + + internal class PathSegment( + val id: String, + val speedLimit: Speed? = null, + ) + + @Test + fun valueClassNullableField() { + val segments = listOf(PathSegment("foo", Speed(2.3)), PathSegment("bar")) + + val df = segments.toDataFrame() + df["speedLimit"].values() shouldBe listOf(Speed(2.3), null) + } + + @Test + fun valueClassNullableField1() { + val segments = listOf(PathSegment("foo", Speed(2.3)), PathSegment("bar", Speed(null))) + + val df = segments.toDataFrame() + df["speedLimit"].values() shouldBe listOf(Speed(2.3), Speed(null)) + } + + @JvmInline + internal value class Speed1(val kmh: Number) + + internal class PathSegment1( + val id: String, + val speedLimit: Speed1? = null, + ) + + @Test + fun valueClass() { + val segments = listOf(PathSegment1("foo", Speed1(2.3)), PathSegment1("bar")) + + val df = segments.toDataFrame() + df["speedLimit"].values() shouldBe listOf(Speed1(2.3), null) + } + + @Test + fun testKPropertyGet() { + val segment = PathSegment("bar") + val result = PathSegment::speedLimit.call(segment) + result shouldBe null + } + + fun call(kProperty0: KProperty<*>, obj: Any) = kProperty0.call(obj) + + @Test + fun testKPropertyCallLibrary() { + val segment = PathSegment1("bar") + val result = call(PathSegment1::speedLimit, segment) + // Sudden result! I cannot create this value, so toString. + // In the test above you can see decompiled code that "fixes" this strange wrapping + result.toString() shouldBe "Speed1(kmh=null)" + } } diff --git a/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt b/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt index 93ecd6682b..6680112fce 100644 --- a/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt +++ b/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt @@ -21,13 +21,16 @@ import org.jetbrains.kotlinx.dataframe.impl.getListType import org.jetbrains.kotlinx.dataframe.impl.projectUpTo import org.jetbrains.kotlinx.dataframe.impl.schema.getPropertiesOrder import java.lang.reflect.InvocationTargetException +import java.lang.reflect.Method import java.time.temporal.Temporal import kotlin.reflect.KClass import kotlin.reflect.KProperty import kotlin.reflect.KVisibility import kotlin.reflect.full.isSubclassOf import kotlin.reflect.full.memberProperties +import kotlin.reflect.full.primaryConstructor import kotlin.reflect.full.withNullability +import kotlin.reflect.jvm.isAccessible import kotlin.reflect.jvm.javaField import kotlin.reflect.typeOf @@ -141,6 +144,24 @@ internal fun convertToDataFrame( val property = it if (excludes.contains(property)) return@mapNotNull null + class ValueClassConverter(val unbox: Method, val box: Method) + + val valueClassConverter = (it.returnType.classifier as? KClass<*>)?.let { kClass -> + if (!kClass.isValue) null else { + val constructor = + requireNotNull(kClass.primaryConstructor) { "value class $kClass is expected to have primary constructor, but couldn't obtain it" } + val parameter = constructor.parameters.singleOrNull() + ?: error("conversion of value class $kClass with multiple parameters in constructor is not yet supported") + // there's no need to unwrap if underlying field is nullable + if (parameter.type.isMarkedNullable) return@let null + // box and unbox impl methods are part of binary API of value classes + // https://youtrack.jetbrains.com/issue/KT-50518/Boxing-Unboxing-methods-for-JvmInline-value-classes-should-be-public-accessible + val unbox = kClass.java.getMethod("unbox-impl") + val box = kClass.java.methods.single { it.name == "box-impl" } + val valueClassConverter = ValueClassConverter(unbox, box) + valueClassConverter + } + } property.javaField?.isAccessible = true var nullable = false @@ -151,7 +172,19 @@ internal fun convertToDataFrame( null } else { val value = try { - it.call(obj) + val value = it.call(obj) + /** + * here we do what compiler does + * @see org.jetbrains.kotlinx.dataframe.api.CreateDataFrameTests.testKPropertyGetLibrary + */ + if (valueClassConverter != null) { + val var1 = value?.let { + valueClassConverter.unbox.invoke(it) + } + var1?.let { valueClassConverter.box.invoke(null, var1) } + } else { + value + } } catch (e: InvocationTargetException) { hasExceptions = true e.targetException diff --git a/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt b/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt index 5861975a9a..737ca98fff 100644 --- a/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt +++ b/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt @@ -10,6 +10,7 @@ import org.jetbrains.kotlinx.dataframe.kind import org.jetbrains.kotlinx.dataframe.type import org.junit.Ignore import org.junit.Test +import kotlin.reflect.KProperty import kotlin.reflect.typeOf class CreateDataFrameTests { @@ -224,4 +225,63 @@ class CreateDataFrameTests { println() } } + + // nullable field here - no generated unwrapping code + @JvmInline + internal value class Speed(val kmh: Number?) + + internal class PathSegment( + val id: String, + val speedLimit: Speed? = null, + ) + + @Test + fun valueClassNullableField() { + val segments = listOf(PathSegment("foo", Speed(2.3)), PathSegment("bar")) + + val df = segments.toDataFrame() + df["speedLimit"].values() shouldBe listOf(Speed(2.3), null) + } + + @Test + fun valueClassNullableField1() { + val segments = listOf(PathSegment("foo", Speed(2.3)), PathSegment("bar", Speed(null))) + + val df = segments.toDataFrame() + df["speedLimit"].values() shouldBe listOf(Speed(2.3), Speed(null)) + } + + @JvmInline + internal value class Speed1(val kmh: Number) + + internal class PathSegment1( + val id: String, + val speedLimit: Speed1? = null, + ) + + @Test + fun valueClass() { + val segments = listOf(PathSegment1("foo", Speed1(2.3)), PathSegment1("bar")) + + val df = segments.toDataFrame() + df["speedLimit"].values() shouldBe listOf(Speed1(2.3), null) + } + + @Test + fun testKPropertyGet() { + val segment = PathSegment("bar") + val result = PathSegment::speedLimit.call(segment) + result shouldBe null + } + + fun call(kProperty0: KProperty<*>, obj: Any) = kProperty0.call(obj) + + @Test + fun testKPropertyCallLibrary() { + val segment = PathSegment1("bar") + val result = call(PathSegment1::speedLimit, segment) + // Sudden result! I cannot create this value, so toString. + // In the test above you can see decompiled code that "fixes" this strange wrapping + result.toString() shouldBe "Speed1(kmh=null)" + } } From 7ac540a43443859b0e42a6c5141ee7e6bae4e3c5 Mon Sep 17 00:00:00 2001 From: Nikita Klimenko Date: Fri, 15 Dec 2023 21:08:38 +0200 Subject: [PATCH 2/2] fix the conversion of public properties within private classes --- .../jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt | 2 +- .../org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt | 7 +++++++ .../jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt | 1 + .../org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt | 7 +++++++ 4 files changed, 16 insertions(+), 1 deletion(-) diff --git a/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt b/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt index 0207ff280a..56f99abd02 100644 --- a/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt +++ b/core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt @@ -163,7 +163,7 @@ internal fun convertToDataFrame( } } property.javaField?.isAccessible = true -// property.isAccessible = true + property.isAccessible = true var nullable = false var hasExceptions = false diff --git a/core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt b/core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt index 737ca98fff..d29ae4ef8a 100644 --- a/core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt +++ b/core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt @@ -284,4 +284,11 @@ class CreateDataFrameTests { // In the test above you can see decompiled code that "fixes" this strange wrapping result.toString() shouldBe "Speed1(kmh=null)" } + + private class PrivateClass(val a: Int) + + @Test + fun `convert private class with public members`() { + listOf(PrivateClass(1)).toDataFrame() shouldBe dataFrameOf("a")(1) + } } diff --git a/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt b/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt index 6680112fce..56f99abd02 100644 --- a/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt +++ b/core/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/api/toDataFrame.kt @@ -163,6 +163,7 @@ internal fun convertToDataFrame( } } property.javaField?.isAccessible = true + property.isAccessible = true var nullable = false var hasExceptions = false diff --git a/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt b/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt index 737ca98fff..d29ae4ef8a 100644 --- a/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt +++ b/core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/api/toDataFrame.kt @@ -284,4 +284,11 @@ class CreateDataFrameTests { // In the test above you can see decompiled code that "fixes" this strange wrapping result.toString() shouldBe "Speed1(kmh=null)" } + + private class PrivateClass(val a: Int) + + @Test + fun `convert private class with public members`() { + listOf(PrivateClass(1)).toDataFrame() shouldBe dataFrameOf("a")(1) + } }