Hi, I’ve distilled a strange case I found in our code base to a simple example of some Kotlin code which the compiler seems happy with but results in unsafe code:
class Foo(val i: Bar = bar)
class Bar()
val foo: Foo = Foo()
val bar: Bar = Bar()
fun main(args: Array<String>) {
println("hi")
}
Running this main method results in:
Exception in thread "main" java.lang.ExceptionInInitializerError
Caused by: java.lang.IllegalArgumentException: Parameter specified as non-null is null: method com.noumenadigital.util.Foo.<init>, parameter i
at Foo.<init>(ScratchPad.kt)
at Foo.<init>(ScratchPad.kt:3)
at ScratchPadKt.<clinit>(ScratchPad.kt:6)
I’m relatively new to Kotlin so may have missed discussion on this already but googling didn’t produce anything interesting.
I can understand that compromises are required to provide smooth interoperability with Java and that runtime NPEs are possible when crossing the boundary between Kotlin and Java. But it’s disconcerting if the null-safety aspect of the (pure) Kotlin type system is unsound.
Just to be clear, I understand the mechanism causing this to happen and I understand how to fix it in this trivial case (by re-ordering the declarations) and I understand that the above code may not be good Kotlin style. But a hole in the type system is a hole in the type system regardless?
Does this work if you change your code slightly to
class Foo(val i: Bar = Bar())
class Bar()
fun main(args: Array<String>) {
println("hi")
}
and does it work if you swap the order of bar and foo vars
class Foo(val i: Bar = bar)
class Bar()
val bar: Bar = Bar()
val foo: Foo = Foo()
fun main(args: Array<String>) {
println("hi")
}
I think this might be an ordering problem. I think Kotlin initializes in order. So on var foo: Foo = Foo() it goes to use var bar which hasn’t been initialized yet. You could probably find out pretty easy by decompiling the byte code to java and looking at what it does.
Yes I can easily get it to work as I described originally - it is indeed an ordering problem.
But my question really boils down to: should this be considered a compiler bug?
My initial reaction was to think that it surely must be.
However I’ve re-read the online documentation (see https://kotlinlang.org/docs/reference/null-safety.html) and noticed that “some data inconsistency with regard to initialization” is listed as a possible cause of NPEs in Kotlin. I guess you could interpret my situation as being covered by this although neither of the situations described in the documentation match my case.
I guess as a relative new-comer to the language, I was a bit surprised to encounter a failure of the type system to guarantee null-safety so soon.
Some data inconsistency with regard to initialization, such as when:
An uninitialized this available in a constructor is passed and used somewhere (“leaking this”)
As far as I understand it, this part of the documentation you linked refers to your problem.
Every kotlin file gets converted into a class and each property and function gets converted to static functions and fields. So therefor this is not really covered by the part of the documentation but let’s pretend we have this code:
class Test {
inner class Foo(val i: Bar = bar)
class Bar()
val foo: Foo = Foo()
val bar: Bar = Bar()
}
The result is the same, we get an IllegalArgumentException
Parameter specified as non-null is null: method test.Test$Foo.
You notice I had to change Foo to an inner class but that is just so I could access the value of bar. The problem is basically the same. When we call the constructor of Test the first thing the program does is create a instance of Foo therefor accessing the value of bar. Bar has not yet been initialized so we get a NPE.
The instance of Foo is uninitialized therefor NPEs can occur.
In your and in my example you use a value from a not yet fully initialized class therefor the NPE is not a bug. This is a quirk of Kotlin I also stumbled across when I started to use Kotlin, but I don’t consider it a bug although I would love to see some improvement in the way NPEs get handled when inside of a constructor. I think it should be possible to throw a more descriptive exception, but than I don’t know how Kotlin works internally that well.
Indeed - in fact I first encountered the problem with an inner class. I transformed it into something simpler just to emphasise the point.
I’m coming around to agreeing with your position although I think the compiler could do a lot better in warning the user in this case. It looks like the normal initialization checking isn’t done for default values.
For example the same code without the default argument in Foo’s constructor, fails to compile:
class Foo(val i: Bar)
class Bar()
val foo: Foo = Foo(bar)
val bar: Bar = Bar()
The declaration of foo is rejected by the compiler with the message “Variable ‘bar’ must be initialized”. The compiler could surely issue the same error if you try to use “bar” as a default value before it’s initialized.
When you use bar in a default value the compiler doesn’t know whether it would be initialized or not at the point when the constructor is actually called.
And when you invoke the constructor without that parameter later it’s already too late to do the analysis. I mean how deep do you expect the compiler should delve into the implementation of the constructor, to find if all that it references is already initialized? This may seriously slow down the compilation.
I don’t understand, Ilya. I’m not suggesting the compiler perform any complex or computationally expensive analysis.
The compiler rejects this snippet of code even if “foo” isn’t accessed anywhere in the execution path:
class Foo(val i: Bar)
class Bar()
val foo: Foo = Foo(bar)
val bar: Bar = Bar()
fun main(args: Array<String>) = println("hi")
The compiler doesn’t have to do anything tricky or time consuming to reject this code that I can see?
I don’t see why the simple declaration order check that’s done with this code cannot be performed to also reject:
class Foo(val i: Bar = bar)
class Bar()
val bar: Bar = Bar()
val foo: Foo = Foo()
fun main(args: Array<String>) = println("hi")
If it’s reasonable for the compiler to reject the first example, then surely it is reasonable for it to reject the second one? Particularly if this would improve the soundness of Kotlin’s type system and reduce the number of wtfs users are likely to experience.
Yes that was a mistake - for the second example, I meant to post the foo/bar order which results in the runtime NPE.
But do you consider that the compiler rejects the first case a false positive also? The program can be safely run after all. I guess this is probably a difference of opinion but I do not consider it a false positive. My feeling is that once you accept static typing, you accept that some programs which can run without blowing up will be rejected by the type system.
And if you believe that the first one is not a false positive, then I would argue the second one should also be rejected both for consistency and to remove some of the loopholes in the null-safety aspects of the language (i.e. you could remove the “some data inconsistency with regard to initialization” exception from the null-safety section of the language reference).
I also think languages should have few surprises as possible and while surprise is a subjective notion, the other Kotlin users in my team were also surprised when I showed it to them.
Consider your example a little bit more complicated:
class Foo(val i: Bar = bar)
class Bar()
val foo: Foo = createFoo()
val bar: Bar = Bar()
fun createFoo(): Foo {
return Foo()
}
fun main(args: Array<String>) {
println("hi")
}
It still produces exception in initializer, but it’s already harder to detect. Inter-procedural analysis in compiler can be very time-consuming.
Also, to make it clear: it’s not enough just to check that bar is declared before class Foo, moreover, this fact has no importance at all. Consider this example:
val foo: Foo = Foo()
val bar: Bar = Bar()
class Foo(val i: Bar = bar)
class Bar()
fun main(args: Array<String>) {
println("hi")
}
The thing with what you’re suggesting - that the compiler should be warning you - is that it requires checking each call of the constructor individually. And, yeah, it can get very complex to identify the issue like with the createFoo() example, which would be a top of the iceberg.
There are more instances where the jetbrains team opted to leave out a check for compiler performance. However, it would be interesting to have a separate tool that can be considered a unit test, that does perform this expensive code analysis. It just doesn’t run each time the compiler does. On the other hand, if people start running it as part of the compilation cycle, there’s not much point separating this into another tool. Maybe that would be okay since they could be run in parallel…
If it is hard to detect for the compiler, it’s probably hard to detect for many users too.
Are there any best practices to avoid these kind of complexity?
For example one class per file and to have initializes ordered in terms of dependency within a file.
Just be very careful when calling methods while the object isn’t initialized completely. In particular, avoid calling methods of your own class while initializing fields or in the constructor, especially open methods. If you must call methods, make sure that everything they access is already initialized.
This example code violates that, because you call the Foo constructor (which acts like a method) during initialization, which accesses the bar property, which hasn’t been initialized, yet.
Another kinda important practices is not to call open functions before initialization is done as you have no control which fields they access and also not to pass a this reference around.
