This is just one case of a generic problem with “wedge writing” in Kotlin.
class C {
var s: StringBuilder? = null
fun append(c: Char)
{
if (s == null) {
s = StringBuilder()
}
s!!.append(c)
}
}
In many cases I do not want to allocate object until it is really needed (usually in such cases it is not allocated at all except some corner cases so this optimization matters). The problem is that compiler for some reason enforces those !! (and real run-time check as well, which fortunately may be stripped off by ProGuard, but the code readability still suffers), even if the null check is performed just one line higher. Actually this is absolutely wrong design decision - there is no mean to check for null once more before actual access, if it assumes that the property can be changed from somewhere. In such case it still can be changed after the second check and before the actual access (typical race). It is my responsibility to ensure synchronization for mutable properties.
I propose to at least make a possibility to tell the compiler that the mutable property synchronization is handled by the code via some annotation or modifier. Ideal solution would be revising and removing such useless enforcements.
Object allocation in Java is not that bad. I never have any performance problem with lazy, not even on Android.
Anyway… you can use something like that:
fun append(c: Char) = (s ?: StringBuilder().also { s = it }).append(c)
Don’t know if it is better or worse, but is one alternative.
Thanks for the alternative. Indeed, this matters for performance critical applications only, when there are big data flows processing, etc. For Android, example could be 3D-game, where you have some work to do on each frame, and allocations count should be minimized in order to minimize GC calls (each one typically causes some FPS freeze).
It still would be nice to have some explanation for logical inconsistency in the language design:
The compiler assumes that the property can be changed by someone else (perhaps some other thread).
It enforces additional null check which is not atomic.
Then it accesses the property which still could be changed after step 2. This is an obvious race condition which usually does not pass code review in other languages in multi-threaded projects.
^ This is great. “Performance” is often the enemy of clean code. If the asymptotic growth isn’t obviously made worse, then go for it. You can always come back and improve the performance later if necessary. Most likely it won’t make a noticeable difference and there will be more important performance improvements to be made.
The !! is actually like a cast, where you say to the compiler: “Don’t bother me, I know what i’m doing!”
Yes, but it becomes annoying when you are forced to use it across all the code, where the code logic ensures that the value is not null. Besides that, it also really performs one more null check and exception throwing in run-time.
Yes, you still need the !!. There are many different ways you could go with this. Here’s one example:
fun main(args: Array<String>) {
val c = C()
println(c.s)
c.append('a')
println(c.s)
c.append('b')
println(c.s)
}
class C {
var s: StringBuilder? = null
fun append(c: Char)
{
val sb = s ?: StringBuilder()
sb.append(c)
s = sb
}
}
I don’t recommend doing that though.
I’d recommend using a Lazy delegate
Replacing the var with val
Using lateinit
Currently, all of the users must check for nullability instead of just giving an empty StringBuilder.
But in this case the code does not look clean because of this double check. If you would do this in some other language, your code would not pass review because if you make such double check, you probably do not understand what are you doing and your design is wrong (double check does not eliminate race). In Kotlin this “unclean” code is enforced by the language design (see question with the three steps above).
The !! is not necessarily a code-smell. It’s there for you to use and it should be used. Code should never fail a review just because of a !! alone. In some cases !! is the best option.
So why does the compiler insist that s could be null after you just checked it? Because it could be. Like you said, multithreaded applications can’t trust that s will have been initialized. Allowing you to leave out the !! would hide this issue, instead, you’re asked to confirm you acknowledge and accept it by using !!.
Everything can be solved with code that is a bit more verbose.
class C {
var s: StringBuilder? = null
fun append(c: Char)
{
guaranteedBuilder().append(c)
}
fun guaranteedBuilder(): StringBuilder {
val currentS = s
return if (currentS == null) {
val newS = StringBuilder()
s = newS
newS
} else {
currentS
}
}
}
fun main(args: Array<String>) {
val c = C()
c.append('a')
c.append('b')
c.append('c')
println(c.s)
}
I meant review in other language, if you would write equivalent double-check. The second check is not needed because the property either cannot be changed (ensured by some code logic or synchronization) or, if it still can be modified, the second check has no meaning because it still can be modified after the second check, but before the value used. So my complain is to this compiler feature which enforces to do !! after var property has been checked for null instead of smart cast as it is done for val. I see no case when this design decision would solve something because of the race. If the race does not exists (ensured by code logic) then it just adds inconvenience for writing the code (as well as useless run-time checks). This is the point.
^ These would be my concerns in a code review. The !! is fine, but both the var and the nullability could be fixed (which would make the !! unnecessary)
That’s why !! is often thought of as a code-smell when it isn’t. Because it’s often a result of other design choices, that when changed, get ride of the !!
That’s the thing, the compiler can’t ensure that “the race does not exist”. And the !! is not “a null check”. The !! says that you acknowledge the compiler’s complaint, but you can ensure, because of some design, that there won’t be a race condition–to show that, you use !!.
It seems like this is a perfect example of when !! would be used. Although I’d still revisit the var and nullability issues.
Addressing those two concerns, have you considered just initializing s to a StringBuilder like so (notice it doesn’t print null like my other example):
fun main(args: Array<String>) {
val c = C()
println(c.s)
c.append('a')
println(c.s)
c.append('b')
println(c.s)
}
class C {
val s: StringBuilder = StringBuilder()
fun append(c: Char)
{
s.append(c)
}
}
Another thread could set s back to null between the instantiation and the append(...). Not likely, and in this case you know for sure it won’t happen. But the compiler cannot guarantee it. And the Kotlin language guarantees null safety. (Yes, I know there are situations where null safety fails. I have seen the thread(s).)
Yes, StringBuilder can be allocated initially, but imagine a case when append() is called just in some exceptional case (1 per 1000000) but in most cases the result is empty. In such case the StringBuilder allocation is just a waste. This is just a particular example, the question is conceptual, about necessity of the !! in this and all similar cases and the race there.