Builder DSL is prone to confusing implicit receiver bugs
I've run into a confusing, subtle bug in some code that uses the builder DSL. Here's a simplified example that demonstrates the bug.
Imagine a proto like this:
```proto
message UserProto {
string name = 1;
bool is_administrator = 2;
}
```
The current ProtoKt generates code like this:
```kotlin
data class UserProto(
val name: String? = null,
val isAdministrator: Boolean = false,
) {
data class Builder(
var name: String? = null,
var isAdministrator: Boolean = false,
) {
fun build(): UserProto = UserProto(name = name, isAdministrator = isAdministrator)
}
companion object {
operator fun invoke(dsl: Builder.() -> Unit): UserProto = Builder().apply(dsl).build()
}
}
```
Imagine there's a `UserEntity` defined in another code base:
```kotlin
data class UserEntity(
var name: String? = null,
var isAdmin: Boolean = false,
)
```
In a 3rd code base, we've written an extension function on `UserEntity` to convert to a `UserProto`:
```kotlin
fun UserEntity.toUserProto(): UserProto {
val userEntity = this
return UserProto {
name = userEntity.name
// This line is wrong! This is what it's doing:
// userEntity.isAdmin = userEntity.isAdmin
//
// ...but it looks like it's assigning `isAdmin` on the `UserProto` just like `name =` above.
// The correct line would be `isAdministrator = userEntity.isAdmin` but that's not obvious at all.
isAdmin = userEntity.isAdmin
}
}
```
Here's a unit test that fails, demonstrating the confusing behavior:
```kotlin
class ProtoDslTest {
@Test
fun `does not convert to a user proto as expected`() {
val userEntity = UserEntity("Bob", true)
val expectedUserProto = UserProto("Bob", true)
assertThat(userEntity.toUserProto()).isEqualTo(expectedUserProto)
}
}
```
Failure:
```
Expecting:
<UserProto(name=Bob, isAdministrator=false)>
to be equal to:
<UserProto(name=Bob, isAdministrator=true)>
but was not.
Expected :UserProto(name=Bob, isAdministrator=true)
Actual :UserProto(name=Bob, isAdministrator=false)
```
Kotlin has a feature designed to help prevent this kind of thing--[DslMarker](https://kotlinlang.org/docs/type-safe-builders.html#scope-control-dslmarker). I see that this protokt defines a DSL marker: [BuilderDsl](https://github.com/open-toast/protokt/blob/866094e4cb318ee3658401ce113b3d41201401b4/protokt-runtime/src/commonMain/kotlin/protokt/v1/BuilderDsl.kt#L19). However, I don't think the marker annotation is being applied correctly to prevent this situation. I've played around with it a bit and haven't yet found a working solution. Ideally, the `isAdmin = userEntity.isAdmin` line would result in a compile error instead of silently being a no-op.
0 条评论