Skip to content

Commit 71ab27a

Browse files
JolanrensenJolanrensen
authored
Merge pull request #467 from Kotlin/better-rendering
Improved rendering of types with arguments, like `Comparable<*>`
2 parents 835de4b + 4646f6a commit 71ab27a

File tree

147 files changed

+2159
-1795
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

147 files changed

+2159
-1795
lines changed

‎core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/api/filter.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import org.jetbrains.kotlinx.dataframe.DataFrame
66
import org.jetbrains.kotlinx.dataframe.Predicate
77
import org.jetbrains.kotlinx.dataframe.RowFilter
88
import org.jetbrains.kotlinx.dataframe.columns.ColumnReference
9-
import org.jetbrains.kotlinx.dataframe.impl.toIndices
9+
import org.jetbrains.kotlinx.dataframe.impl.getTrueIndices
1010
import org.jetbrains.kotlinx.dataframe.indices
1111
import kotlin.reflect.KProperty
1212

@@ -27,7 +27,7 @@ public fun <T> DataFrame<T>.filter(predicate: RowFilter<T>): DataFrame<T> =
2727
}.let { get(it) }
2828

2929
public fun <T> DataFrame<T>.filterBy(column: ColumnSelector<T, Boolean>): DataFrame<T> =
30-
getRows(getColumn(column).toList().toIndices())
30+
getRows(getColumn(column).toList().getTrueIndices())
3131

3232
public fun <T> DataFrame<T>.filterBy(column: String): DataFrame<T> = filterBy { column.toColumnOf() }
3333

‎core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/Rendering.kt

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -48,26 +48,48 @@ internal fun renderType(column: ColumnSchema) =
4848
else -> throw NotImplementedError()
4949
}
5050

51-
internal val classesToBeRenderedShort = setOf(URL::class, LocalDateTime::class, LocalTime::class)
51+
internal fun renderType(type: KType?): String {
52+
return when (type?.classifier) {
53+
null -> "*"
5254

53-
internal fun renderType(type: KType): String =
54-
when (type.classifier) {
5555
Nothing::class -> "Nothing" + if (type.isMarkedNullable) "?" else ""
56-
List::class -> {
57-
val argument = type.arguments[0].type?.let { renderType(it) } ?: "*"
58-
"List<$argument>"
59-
}
6056

6157
else -> {
62-
val result = type.toString()
63-
if (classesToBeRenderedShort.contains(type.classifier) ||
64-
result.startsWith("kotlin.") ||
65-
result.startsWith("org.jetbrains.kotlinx.dataframe.")
66-
) {
67-
(type.jvmErasure.simpleName?.let { if (type.isMarkedNullable) "$it?" else it }) ?: result
68-
} else result
58+
val fullName = type.jvmErasure.qualifiedName ?: return type.toString()
59+
val name = when {
60+
type.classifier == URL::class ->
61+
fullName.removePrefix("java.net.")
62+
63+
type.classifier in listOf(LocalDateTime::class, LocalTime::class) ->
64+
fullName.removePrefix("java.time.")
65+
66+
fullName.startsWith("kotlin.collections.") ->
67+
fullName.removePrefix("kotlin.collections.")
68+
69+
fullName.startsWith("kotlin.") ->
70+
fullName.removePrefix("kotlin.")
71+
72+
fullName.startsWith("org.jetbrains.kotlinx.dataframe.") ->
73+
fullName.removePrefix("org.jetbrains.kotlinx.dataframe.")
74+
75+
else -> fullName
76+
}
77+
78+
buildString {
79+
append(name)
80+
if (type.arguments.isNotEmpty()) {
81+
val arguments = type.arguments.joinToString {
82+
renderType(it.type)
83+
}
84+
append("<$arguments>")
85+
}
86+
if (type.isMarkedNullable) {
87+
append("?")
88+
}
89+
}
6990
}
7091
}
92+
}
7193

7294
internal fun renderType(column: AnyCol) =
7395
when (column.kind()) {

‎core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/TypeUtils.kt

Lines changed: 69 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import kotlin.reflect.KClass
99
import kotlin.reflect.KType
1010
import kotlin.reflect.KTypeParameter
1111
import kotlin.reflect.KTypeProjection
12+
import kotlin.reflect.KVariance
13+
import kotlin.reflect.KVariance.*
1214
import kotlin.reflect.KVisibility
1315
import kotlin.reflect.full.allSuperclasses
1416
import kotlin.reflect.full.createType
@@ -50,37 +52,47 @@ internal fun KType.projectUpTo(superClass: KClass<*>): KType {
5052
}
5153

5254
/**
53-
* Changes generic type parameters to `Any?`, like `List<T> -> List<Any?>`.
55+
* Changes generic type parameters to its upperbound, like `List<T> -> List<Any?>` and
56+
* `Comparable<T> -> Comparable<Nothing>`.
5457
* Works recursively as well.
5558
*/
5659
@PublishedApi
57-
internal fun KType.eraseGenericTypeParameters(): KType {
58-
fun KType.eraseRecursively(): Pair<Boolean, KType> {
59-
var replaced = false
60-
val arguments = arguments.map {
61-
val type = it.type
62-
val (replacedDownwards, newType) = when {
63-
type == null -> typeOf<Any?>()
64-
65-
type.classifier is KTypeParameter -> {
66-
replaced = true
67-
(type.classifier as KTypeParameter).upperBounds.firstOrNull() ?: typeOf<Any?>()
60+
internal fun KType.replaceGenericTypeParametersWithUpperbound(): KType {
61+
fun KType.replaceRecursively(): Pair<Boolean, KType> {
62+
var replacedAnyArgument = false
63+
val arguments = arguments.mapIndexed { i, it ->
64+
val oldType = it.type ?: return@mapIndexed it // is * if null, we'll leave those be
65+
66+
val type = when {
67+
oldType.classifier is KTypeParameter -> { // e.g. T
68+
replacedAnyArgument = true
69+
70+
// resolve the variance (`in`/`out`/``) of the argument in the original class
71+
val varianceInClass = (classifier as? KClass<*>)?.typeParameters?.getOrNull(i)?.variance
72+
when (varianceInClass) {
73+
INVARIANT, OUT, null ->
74+
(oldType.classifier as KTypeParameter).upperBounds.firstOrNull() ?: typeOf<Any?>()
75+
76+
// Type<in T> cannot be replaced with Type<Any?>, instead it should be replaced with Type<Nothing>
77+
// TODO: issue #471
78+
IN -> nothingType(false)
79+
}
6880
}
6981

70-
else -> type
71-
}.eraseRecursively()
72-
73-
if (replacedDownwards) replaced = true
82+
else -> oldType
83+
}
84+
val (replacedDownwards, newType) = type.replaceRecursively()
85+
if (replacedDownwards) replacedAnyArgument = true
7486

7587
KTypeProjection.invariant(newType)
7688
}
7789
return Pair(
78-
first = replaced,
79-
second = if (replaced) jvmErasure.createType(arguments, isMarkedNullable) else this,
90+
first = replacedAnyArgument,
91+
second = if (replacedAnyArgument) jvmErasure.createType(arguments, isMarkedNullable) else this,
8092
)
8193
}
8294

83-
return eraseRecursively().second
95+
return replaceRecursively().second
8496
}
8597

8698
internal fun inheritanceChain(subClass: KClass<*>, superClass: KClass<*>): List<Pair<KClass<*>, KType>> {
@@ -221,12 +233,16 @@ internal fun commonParents(classes: Iterable<KClass<*>>): List<KClass<*>> =
221233
* @receiver the types to find the common type for
222234
* @return the common type including listify behaviour
223235
*
224-
* @see commonType
236+
* @param useStar if true, `*` will be used to fill in generic type parameters instead of `Any?`
237+
* (for invariant/out variance) or `Nothing` (for in variance)
238+
*
239+
* @see Iterable.commonType
225240
*/
226-
internal fun Iterable<KType>.commonTypeListifyValues(): KType {
241+
internal fun Iterable<KType>.commonTypeListifyValues(useStar: Boolean = true): KType {
227242
val distinct = distinct()
243+
val nullable = distinct.any { it.isMarkedNullable }
228244
return when {
229-
distinct.isEmpty() -> Any::class.createStarProjectedType(distinct.any { it.isMarkedNullable })
245+
distinct.isEmpty() -> typeOf<Any>().withNullability(nullable)
230246
distinct.size == 1 -> distinct.single()
231247
else -> {
232248
val classes = distinct.map {
@@ -269,20 +285,43 @@ internal fun Iterable<KType>.commonTypeListifyValues(): KType {
269285
}
270286

271287
else -> {
272-
val kclass = commonParent(distinct.map { it.jvmErasure }) ?: return typeOf<Any>()
273-
val projections = distinct.map { it.projectUpTo(kclass).eraseGenericTypeParameters() }
274-
require(projections.all { it.jvmErasure == kclass })
275-
val arguments = List(kclass.typeParameters.size) { i ->
288+
val kClass = commonParent(distinct.map { it.jvmErasure })
289+
?: return typeOf<Any>().withNullability(nullable)
290+
val projections = distinct
291+
.map { it.projectUpTo(kClass).replaceGenericTypeParametersWithUpperbound() }
292+
require(projections.all { it.jvmErasure == kClass })
293+
294+
val arguments = List(kClass.typeParameters.size) { i ->
295+
val typeParameter = kClass.typeParameters[i]
276296
val projectionTypes = projections
277297
.map { it.arguments[i].type }
278298
.filterNot { it in distinct } // avoid infinite recursion
279299

280-
val type = projectionTypes.filterNotNull().commonTypeListifyValues()
281-
KTypeProjection.invariant(type)
300+
when {
301+
projectionTypes.isEmpty() && typeParameter.variance == KVariance.IN -> {
302+
if (useStar) {
303+
KTypeProjection.STAR
304+
} else {
305+
KTypeProjection.invariant(nothingType(false))
306+
}
307+
}
308+
309+
else -> {
310+
val commonType = projectionTypes.filterNotNull().commonTypeListifyValues(useStar)
311+
if (commonType == typeOf<Any?>() && useStar) {
312+
KTypeProjection.STAR
313+
} else {
314+
KTypeProjection(typeParameter.variance, commonType)
315+
}
316+
}
317+
}
282318
}
283319

284-
if (kclass == Nothing::class) nothingType(nullable = distinct.any { it.isMarkedNullable })
285-
else kclass.createType(arguments, distinct.any { it.isMarkedNullable })
320+
if (kClass == Nothing::class) {
321+
nothingType(nullable)
322+
} else {
323+
kClass.createType(arguments, nullable)
324+
}
286325
}
287326
}
288327
}

‎core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/Utils.kt

Lines changed: 43 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import kotlin.reflect.KClass
1818
import kotlin.reflect.KProperty
1919
import kotlin.reflect.KType
2020
import kotlin.reflect.KTypeProjection
21+
import kotlin.reflect.KVariance
2122
import kotlin.reflect.full.createType
2223
import kotlin.reflect.full.findAnnotation
2324
import kotlin.reflect.full.isSubtypeOf
@@ -67,16 +68,16 @@ internal fun <T : Any> convert(src: Int, tartypeOf: KClass<T>): T = when (tartyp
6768
else -> throw NotImplementedError("Casting int to $tartypeOf is not supported")
6869
}
6970

70-
internal fun BooleanArray.toIndices(): List<Int> {
71+
internal fun BooleanArray.getTrueIndices(): List<Int> {
7172
val res = ArrayList<Int>(size)
72-
for (i in 0 until size)
73+
for (i in indices)
7374
if (this[i]) res.add(i)
7475
return res
7576
}
7677

77-
internal fun List<Boolean>.toIndices(): List<Int> {
78+
internal fun List<Boolean>.getTrueIndices(): List<Int> {
7879
val res = ArrayList<Int>(size)
79-
for (i in 0 until size)
80+
for (i in indices)
8081
if (this[i]) res.add(i)
8182
return res
8283
}
@@ -136,27 +137,55 @@ internal fun Iterable<KClass<*>>.commonType(nullable: Boolean, upperBound: KType
136137
/**
137138
* Returns the common supertype of the given types.
138139
*
139-
* @see commonTypeListifyValues
140+
* @param useStar if true, `*` will be used to fill in generic type parameters instead of `Any?`
141+
* (for invariant/out variance) or `Nothing` (for in variance)
142+
*
143+
* @see Iterable.commonTypeListifyValues
140144
*/
141-
internal fun Iterable<KType?>.commonType(): KType {
145+
internal fun Iterable<KType?>.commonType(useStar: Boolean = true): KType {
142146
val distinct = distinct()
143147
val nullable = distinct.any { it?.isMarkedNullable ?: true }
144148
return when {
145-
distinct.isEmpty() || distinct.contains(null) -> Any::class.createStarProjectedType(nullable)
149+
distinct.isEmpty() || distinct.contains(null) -> typeOf<Any>().withNullability(nullable)
146150
distinct.size == 1 -> distinct.single()!!
151+
147152
else -> {
148-
val kclass = commonParent(distinct.map { it!!.jvmErasure }) ?: return typeOf<Any>()
149-
val projections = distinct.map { it!!.projectUpTo(kclass).eraseGenericTypeParameters() }
150-
require(projections.all { it.jvmErasure == kclass })
151-
val arguments = List(kclass.typeParameters.size) { i ->
153+
// common parent class of all KTypes
154+
val kClass = commonParent(distinct.map { it!!.jvmErasure })
155+
?: return typeOf<Any>().withNullability(nullable)
156+
157+
// all KTypes projected to the common parent class with filled-in generic type parameters (no <T>, but <UpperBound>)
158+
val projections = distinct
159+
.map { it!!.projectUpTo(kClass).replaceGenericTypeParametersWithUpperbound() }
160+
require(projections.all { it.jvmErasure == kClass })
161+
162+
// make new type arguments for the common parent class
163+
val arguments = List(kClass.typeParameters.size) { i ->
164+
val typeParameter = kClass.typeParameters[i]
152165
val projectionTypes = projections
153166
.map { it.arguments[i].type }
154167
.filterNot { it in distinct } // avoid infinite recursion
155168

156-
val type = projectionTypes.commonType()
157-
KTypeProjection.invariant(type)
169+
when {
170+
projectionTypes.isEmpty() && typeParameter.variance == KVariance.IN -> {
171+
if (useStar) {
172+
KTypeProjection.STAR
173+
} else {
174+
KTypeProjection.invariant(nothingType(false))
175+
}
176+
}
177+
178+
else -> {
179+
val commonType = projectionTypes.commonType(useStar)
180+
if (commonType == typeOf<Any?>() && useStar) {
181+
KTypeProjection.STAR
182+
} else {
183+
KTypeProjection(typeParameter.variance, commonType)
184+
}
185+
}
186+
}
158187
}
159-
kclass.createType(arguments, nullable)
188+
kClass.createType(arguments, nullable)
160189
}
161190
}
162191
}

‎core/generated-sources/src/main/kotlin/org/jetbrains/kotlinx/dataframe/impl/columns/constructors.kt

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,15 @@ import org.jetbrains.kotlinx.dataframe.api.dataFrameOf
2424
import org.jetbrains.kotlinx.dataframe.api.indices
2525
import org.jetbrains.kotlinx.dataframe.api.toColumnOf
2626
import org.jetbrains.kotlinx.dataframe.api.toDataFrame
27-
import org.jetbrains.kotlinx.dataframe.columns.*
27+
import org.jetbrains.kotlinx.dataframe.columns.ColumnResolutionContext
28+
import org.jetbrains.kotlinx.dataframe.columns.ColumnSet
29+
import org.jetbrains.kotlinx.dataframe.columns.ColumnWithPath
30+
import org.jetbrains.kotlinx.dataframe.columns.toColumnsSetOf
2831
import org.jetbrains.kotlinx.dataframe.impl.DataFrameReceiver
2932
import org.jetbrains.kotlinx.dataframe.impl.DataRowImpl
3033
import org.jetbrains.kotlinx.dataframe.impl.asList
31-
import org.jetbrains.kotlinx.dataframe.impl.eraseGenericTypeParameters
3234
import org.jetbrains.kotlinx.dataframe.impl.guessValueType
35+
import org.jetbrains.kotlinx.dataframe.impl.replaceGenericTypeParametersWithUpperbound
3336
import org.jetbrains.kotlinx.dataframe.index
3437
import org.jetbrains.kotlinx.dataframe.nrow
3538
import kotlin.reflect.KClass
@@ -58,7 +61,7 @@ internal fun <T, R> ColumnsContainer<T>.newColumn(
5861
Infer.Nulls -> DataColumn.create(
5962
name = name,
6063
values = values,
61-
type = type.withNullability(nullable).eraseGenericTypeParameters(),
64+
type = type.withNullability(nullable).replaceGenericTypeParametersWithUpperbound(),
6265
infer = Infer.None,
6366
)
6467

@@ -71,7 +74,7 @@ internal fun <T, R> ColumnsContainer<T>.newColumn(
7174
Infer.None -> DataColumn.create(
7275
name = name,
7376
values = values,
74-
type = type.eraseGenericTypeParameters(),
77+
type = type.replaceGenericTypeParametersWithUpperbound(),
7578
infer = Infer.None,
7679
)
7780
}

‎core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/testSets/person/PivotTests.kt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ class PivotTests {
9595

9696
data["age"].type() shouldBe typeOf<List<Int>>()
9797
data["city"].type() shouldBe typeOf<String>()
98-
data["weight"].type() shouldBe typeOf<Comparable<Any>>()
98+
data["weight"].type() shouldBe typeOf<Comparable<*>>() // Comparable<String + Int> -> Comparable<Nothing | *>
9999

100100
res.renderToString(columnTypes = true, title = true) shouldBe
101101
defaultExpected.group { drop(1) }.into("key").renderToString(columnTypes = true, title = true)
@@ -136,7 +136,9 @@ class PivotTests {
136136

137137
@Test
138138
fun `pivot two values`() {
139-
val pivoted = typed.pivot(inward = false) { key }.groupBy { name }
139+
val pivoted = typed
140+
.pivot(inward = false) { key }
141+
.groupBy { name }
140142
.values { value and (expr { value?.toString() } into "str") default "-" }
141143

142144
val expected = defaultExpected.replace("age", "city", "weight").with {

‎core/generated-sources/src/test/kotlin/org/jetbrains/kotlinx/dataframe/types/TypeProjectionTests.kt

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,10 @@ class TypeProjectionTests {
5151

5252
@Test
5353
fun `common type tests`() {
54-
listOf(typeOf<List<Int>>(), typeOf<Set<Double?>>()).commonType() shouldBe typeOf<Collection<Number?>>()
55-
listOf(typeOf<List<Int>>(), typeOf<Set<*>>()).commonType() shouldBe typeOf<Collection<Any?>>()
56-
listOf(typeOf<List<Int>>(), typeOf<Set<*>?>()).commonType() shouldBe typeOf<Collection<Any?>?>()
54+
listOf(typeOf<List<Int>>(), typeOf<Set<Double?>>()).commonType() shouldBe typeOf<Collection<out Number?>>()
55+
listOf(typeOf<List<Int>>(), typeOf<Set<*>>()).commonType(false) shouldBe typeOf<Collection<out Any?>>()
56+
listOf(typeOf<List<Int>>(), typeOf<Set<*>>()).commonType() shouldBe typeOf<Collection<*>>()
57+
listOf(typeOf<List<Int>>(), typeOf<Set<*>?>()).commonType(false) shouldBe typeOf<Collection<out Any?>?>()
58+
listOf(typeOf<List<Int>>(), typeOf<Set<*>?>()).commonType() shouldBe typeOf<Collection<*>?>()
5759
}
5860
}

0 commit comments

Comments
 (0)