That seems like mutually exclusive sentences, and that is what my concern is about. The compiler cannot guarantee the value using will not end up with NPE. At the same time it checks it for null and throws its own NPE (why?) on “!!” (yes, it is not just acknowledge in code, it is additional run-time check, every time you write “!!”), and enforces you to write “!!” on each access. That seems to be illogical for me. There is no added value for “!!” in this case, but there are drawbacks.
It adds the null check so it can fail before anything else. Like a cast.
It is not guaranteed to fail there, the value can be modified after the check. This is called undefined behavior, and thus design flaw.
These statements are not mutually exclusive:
- Kotlin will guarantee that if a value cannot be
null
, then you can use it without?
or!!
. - In this case the compiler sees that it cannot guarantee that
s
is notnull
, so it forces you to use?
or!!
.
You are checking on a variable that is accessible to multiple threads, so its value can change between the check and the use on any thread.
If you are looking for a language for extremely performance-sensitive code and that helps you prevent common programming mistakes, it will be hard to find one. I guess you can try Rust. Kotlin provides a lot of safety and more than adequate performance for most situations.
There is no undefined behavior here whatsoever. Here is a clear definition: The value of s
at append(...)
does not have to be the same value s
had when checked for null
.
No, it can’t be modified after the null check, the code becomes something like that:
public final StringBuilder append(char c) {
StringBuilder var10000 = this.s;
if (this.s == null) {
Intrinsics.throwNpe();
}
return var10000;
}
Actually like this:
StringBuilder var10000 = this.s;
if (var10000 == null) {
Intrinsics.throwNpe();
}
Yes, you are right, in such case we do not have race.
I actually copied that from a decompiled code in intellij.
I wonder if it is a bug.
@vagran Ah, so are you saying you’d prefer a warning on the s!!.appeand()
line instead of a required !!
? Something that you could optionally suppress using @SuppressWarnings("couldBeNull")
?
The problem with that is that it would effectively disable Kotlin’s null safety and make it behave similar to C#.
The concern:
I see. Let’s also imagine that, instead of StringBuiler, we’re allocating a very memory hungry object (maybe a huge array?). In that case, it’d be even more important to only allocate the object as needed.
What you want is a “lazy” behavior.
There are several ways to do this. You could use a backing field (good), you could use a Lazy Delegate (better). You mentioned you didn’t want to allocate a LazyDelegate object for performance reasons. This shouldn’t be a concern as the delegate object is small and may be optimized by the compiler, After some profiling, if it turns up causing issues then I’d recommend going with a backing field.
The real issue
I think the real issue is that it’s a nullable var. Which means,
val c = C()
// 1) Users must always check for null.
c.s?.length
c.s!!.length
// 2) Users can reset it to null or another instance.
c.s = null
c.s = StringBuilder()
// This is possible even if it was a non-nullable var since it's still public.
// Users may just directly access the object and avoid the c.append() method.
c.s?.append('a')
// Users may use multiple threads to change the property and cause NPE
thread {
while (true) {
c.s = null // Well, that messes up anyone who uses this object.
}
}
Non of the above issues would be possible with:
private val s: StringBuilder by lazy { StringBuilder() }
Here’s a full runnable example for those who are interested (it doesn’t work on try.kotlin.org so you have to copy/past it into your local IDE). Notice how it sometimes prints “a”, other times “null”, and other times throws a NullPointerException when calling the c.append
. Your results may vary depending on your computer
multithreading example
import kotlin.concurrent.*
fun main(args: Array<String>) {
val c = C()
// Users may use multiple threads to change the property
thread {
while (true) {
c.s = null // Well, that messes up anyone who uses this object.
}
}
repeat(10) {
Thread.sleep(1000)
c.append('a') // Could throw NPE
println("Printing: " + c.s) // Could print null even though we just appended 'a'
}
}
class C {
var s: StringBuilder? = null
fun append(c: Char)
{
if (s == null) {
s = StringBuilder()
}
s!!.append(c) // Could throw NPE even though we just checked for null
}
}
That’s strange. I also checked the decompiled code and I have the local variable in the condition like it should be.
This proves what I want to tell. If you know that your code is subject for concurrency, you should explicitly ensure atomicity for variable access/modification using corresponding synchronisation primitives. Or otherwise assume it is already synchronised or used in a single thread. In any case you don’t need the approach currently provided by the Kotlin.
Assume only one thread
I see. You would prefer the compiler assume the code is always run by one thread.
I’m not sure the compiler could make that assumption without being wrong.
Even if you don’t use concurrent access to the property, maybe a dependency will, maybe someone who depends on your code will. Even if your code is private, maybe it will get called indirectly.
The compiler can’t know all of this.
Luckily, there’s !!
If somehow you do know that no one will ever use concurrent access, there’s !!
. A small price to pay for telling the compiler you know something so difficult to find out (and extremely unlikely).
The !!
also has the added benefit of telling other coders that you “meant to do that”. In a code review, someone would rightly point out that s
is mutable and could throw an NPE, the !!
makes it clear that you know that.
Th alternatives are safer and cleaner
If you somehow know there will never be more than one thread running, then why would you still want to change the code?
- It’s not as clean as
val s: StringBuilder by lazy { StringBuilder() }
- It’s forced to be mutable
- It’s forced to be nullable
Even if you assume the code is run in a single thread this does not ensure that the value is never set to null.
val foo: Boolean? = true
get() {
when(field) {
true -> field = false; return true
false -> field = null; return false
null -> field = true; return null
}
}
}
if(foo != null) {
// foo can be null here
}
Same is true for special setters. A setter may check if the value is valid and set it to null otherwise. You never know. Checking this is too much work for the compiler to do in a reasonable time if at all possible. That’s why kotlin does not even try in this case.
That’s awesome!
I totally forgot about custom getters/setters.
EDIT: I just had to make a runnable example:
fun main(args: Array<String>) {
//sampleStart
foo = true
if (foo != null) {
println("foo: $foo")
} //sampleEnd
}
var foo: Boolean? = true
get() = field.also {field = null}
And
fun main(args: Array<String>) {
//sampleStart
foo = true
if (foo != null) {
println("foo: $foo")
println("foo: $foo")
println("foo: $foo")
} //sampleEnd
}
var foo: Boolean? = true
get() = when(field) {
true -> { field = false; true }
false -> { field = null; false }
null -> { field = true; null }
}
^ Of course, any normal method, not just custom getters/setters, that has a side-effect of changing that property could also make the var
null after the null check:
fun main(args: Array<String>) {
//sampleStart
if (foo != null) {
doSomething()
print("foo: $foo")
} //sampleEnd
}
var foo: Boolean? = true
fun doSomething() {
// doSomething ...
foo = null
}
I am pretty sure that any overhead for lazy property initialization in this case is optimized by JVM to zero if you turn off thread safety by LazyThreadSafetyMode
to NONE
. Also, it happens only once for each class instance. Instantiating the class will be much more expensive. Of course one can’t be sure that the same level of optimization is available on other platforms, but I think you should test it first. Mindless sacrifice of code simplicity in favor of unknown performance boost is the road to damnation.