35

In Kotlin if you have an open class which refers to this in its constructor or init block, you (quite rightly) get a compiler warning:

Leaking 'this' in constructor of non-final class

The reason for this is explained here.

My question is: why is this not reported when the class is final? If this is used in the init block before that block has completed, the object is still not in a fully constructed state, so shouldn't the warning apply there too?

This can even lead to a situation where a val property seems to change at runtime. Take this code as an example:

class Listener {
    fun onCreated(leaker: Leaker) = println("Listener hears that leaker created with a value of ${leaker.myVal}")
}

class Leaker(listener: Listener) {
    val myVal: Int

    init {
        listener.onCreated(this)
        myVal = 1
        println("Leaker knows that it's been created with a value of $myVal")
    }
}

Using these objects as follows:

Leaker(Listener())

will result in the following output:

Listener hears that leaker created with a value of 0
Leaker knows that it's been created with a value of 1

Notice that myVal is initially reported as being 0, then as being 1.

As can be seen, Leaker passes an instance of itself to Listener before Leaker has been fully constructed. Listener can then access the myVal property before it's been initialized, so it'll have the default value (0 in this case as it's an integer). Later on Listener then changes the value of this property (to 1 in this example). This means that the program behaves as if a val has changed.

Should the compiler warn you about this?

Nahuel Varela
  • 1,022
  • 7
  • 17
Yoni Gibbs
  • 6,518
  • 2
  • 24
  • 37

1 Answers1

26

tl;dr: https://youtrack.jetbrains.com/issue/KT-22044 is a good fit regarding this issue.

I will cite what Intellij IDEAs inspection called "Leaking 'this' in constructor" says about this:

This inspection reports dangerous operations inside constructors including:

  • Accessing a non-final property in constructor
  • Calling a non-final function in constructor
  • Using this as a function argument in a constructor of a non-final class

These operations are dangerous because your class can be inherited, and a derived class is not yet initialized at this moment. Typical example:

abstract class Base {
    val code = calculate()
    abstract fun calculate(): Int
}

class Derived(private val x: Int) : Base() {
    override fun calculate() = x
}

fun testIt() {
    println(Derived(42).code) // Expected: 42, actual: 0
}

I think that there should be warning nonetheless as you were able to access a non-initialized variable. The reason: the compiler already disallows direct access to uninitialized variables, i.e. the following will not compile:

class Demo {
  val some : Int
  init {
    println(some) // Variable 'some' must be initialized

but accessing it indirectly compiles and shows the default value of the variable type:

class Demo2 {
  val some : Int
  val someString : String
  init {
    fun Demo2.indirectSome() = some
    fun Demo2.indirectSomeString() = someString
    println(indirectSome()) // prints 0
    println(indirectSomeString()) // prints null; and yes.. this can lead to NullPointerExceptions

and there we also have a "leak", basically accessing some before it should ;-)

Roland
  • 22,259
  • 4
  • 57
  • 84
  • There's also the issue of multi threading to consider. If you leak an instance before the constructor is finished even if it is immutable, you lose all the visibility guarantees given by the JVM. – Voo May 02 '19 at 15:12
  • yes... but in this case, i.e. having a `final` class, the init/constructor of that class is the only place were you allow "leaking" anything... if you put all the initialization before your actual `listener.onCreated(this)`-call you are safe (at least I don't see where this could fail)... that's why I would rather warn about the potential uninitialized-variable access then on anything else... – Roland May 02 '19 at 15:15
  • 1
    Thanks @Roland and Voo. I wonder if this existing issue covers the same problem: https://youtrack.jetbrains.com/issue/KT-20834. Also related to this: https://youtrack.jetbrains.com/issue/KT-22044. Looks to me like JetBrains have a bunch of similar issues already logged, so probably no point logging another one, unless you disagree? – Yoni Gibbs May 02 '19 at 15:27
  • Yes, KT-22044 seems a perfect fit for this as it even has the same sample in the comment. However I wouldn't have put that in the same category as "leaking 'this'"... so my understanding of "leak" is probably another one ;-) – Roland May 02 '19 at 15:33
  • @Roland "if you put all the initialization before your actual listener.onCreated(this)-call you are safe". A common misunderstanding because that's how it works for single threaded code, but you absolutely do **not** have that guarantee in multi threaded code if you leak `this` from the constructor. The JMM only guarantees safe publishing of final objects if this is not leaked in the constructor. In practice writes can easily be reordered even if the writes are above the method call in the source code. – Voo May 02 '19 at 15:55
  • I wonder why this inspection isn't available for Java... isn't this constellation actually rather a Java/JVM problem? well... if I ever return to Java... and ... if I then will ever use such constructs... – Roland May 02 '19 at 16:01
  • @Voo wait... don't ignore that part in brackets please ;-) I just can't yet imagine what will basically really leak... even if I imagine a multi-threaded scenario.. which part exactly will be reordered? do you have a source or article for that? So maybe I see it clearer then... – Roland May 02 '19 at 16:03
  • 1
    @Roland Well [JSR-113](http://www.cs.umd.edu/~pugh/java/memoryModel/CommunityReview.pdf) is the nominal source for everything java memory model related, but it's pretty dense. Basically it comes down to that if you have some code such as `int a = 1; int b = 2;` the compiler (and CPU) are perfectly in their right to reorder the writes to `b = 2; a = 1`. In your case just because `listener.onCreated(this)` is the last line does not mean that it can't be reordered *before* some of the writes. The compiler just has to prove that there's not difference in single-threaded behavior. – Voo May 02 '19 at 16:08
  • 1
    The whole "publish instances correctly in case of concurrent access" was one of the main reasons for creating the JSR in Java 5. [Here's](http://www.cs.umd.edu/users/pugh/java/memoryModel/DoubleCheckedLocking.html) a good summary of that one in particular. Edit: Aleksey Shipilev has also a pretty good primer [on the whole topic](https://shipilev.net/blog/2014/jmm-pragmatics/) even with practical examples - although for some examples you might need the right hardware. – Voo May 02 '19 at 16:10
  • ok, I see... ([JLS link regarding JMM happens-before order](https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.4.5)) so summarizing then the general rule about "not doing anything fancy in the constructor" still applies... as I never expose `this` nor do I call anything overridable within the constructor, that problem doesn't really apply for me though :-) (and I am glad for it, if i see what that basically implies) – Roland May 02 '19 at 16:16
  • 1
    @Roland "not doing anything fancy in the constructor" amen to that. Keep it simple is just always the best. – Voo May 02 '19 at 16:20
  • And uninitialized fields are even more dangerous in Kotlin, as any field that's a reference in the underlying JVM, defaults to null, which may lead to NPEs later on (which actually may be easier to debug than uninitialized `Int`s, now that I think about it). – Gaming32 Nov 27 '22 at 12:41