Handling generic data classes when determining identifier stability #4007
Conversation
By inspecting the stability of the actual values, we can see beyond the generics of the data class. Fixes #3947
| Pair::class, | ||
| Triple::class, |
There was a problem hiding this comment.
I noticed that these classes were sometimes incorrectly marked as stable.
| @Deprecated("Internal class, will not be exposed in the future. Deprecated since 5.9") | ||
| fun isStable(kclass: KClass<*>, t: Any? = null): Boolean { |
There was a problem hiding this comment.
This function is probably public to allow usage from kotest-framework-datatest even though this function is in kotest-common module.
- isStable seems to be used by only
kotest-framework-datatest. Could we move these functions there and mark them asinternal? - Otherwise, we could mark the classes as
@KotestInternal?
There was a problem hiding this comment.
I would if in 6.0 we should just merge datatest into the core module.
Are we gaining anything from having the two ?
| return when { | ||
| allPlatformStableTypes.contains(kclass) -> true | ||
| reflection.isEnumClass(kclass) -> true | ||
| reflection.isDataClass(kclass) && hasStableMembers(kclass) -> true | ||
| reflection.isDataClass(kclass) && hasStableMembers(kclass, t) -> true |
There was a problem hiding this comment.
because the intent here is to "have a stable toString()", should we also verify that this class does not override toString()?
There was a problem hiding this comment.
How does overriding toString() affect stability?
There was a problem hiding this comment.
A contrived example:
class SampleTest: StringSpec() {
init {
"unstable toString" {
val something = WithContrivedToString("something")
something.toString() shouldNotBe something.toString()
}
}
}
data class WithContrivedToString(
var name: String
) {
var accessCount: Int = 0
override fun toString(): String {
return "${super.toString()}, accessed ${++accessCount} times."
}
}There was a problem hiding this comment.
Fair enough, that might be. However, we haven't made that distinction before, and I haven't seen any complaints about it so I think we could just leave it as-is for now.
There was a problem hiding this comment.
It's perfectly possible for someone to have a data class with a random string implementation, but I think that's getting quite edge casey as @Kantis says.
There's always going to be avenues for this to fail. The isStable method is meant to be a quick check to rule out the majority of use cases.
By inspecting the stability of the actual values, we can see beyond the generics of the data class.
Fixes #3947