New EscapedThisReferenceChecker due for next release

In a previous post I discussed a new check that Mutability Detector should make to provide a more accurate result. The implementation of that checker is now complete, and will be included in an upcoming 0.7 release.

As with any other checks the tool does, the analysis is unsound, that is, it doesn't guarantee correctness. It makes as best a guess as it can, and I hope that empirical study of its use would show that it's good enough to rely on, but it is possible that the tool can be fooled into emitting false positives or false negatives. I thought it would be useful to talk about the checker does and doesn't do.

The checker analyses every constructor of your class, and checks for the following problems.


Assigning 'this' to a field
The tool will render your class mutable if it does any of the following:

1. assigns 'this' to static field 
• SomeOtherClass.staticField = this;
2. assigns 'this' to instance of other object
• someOtherObject.instanceField = this;
3. assigns 'this' to field of this instance
• this.myField = this;
4. assigns 'this' to static field of this class
• ThisClass.staticField = this;

Some of these will be a false positive, e.g. with #2, if you construct an object in the constructor, and assign 'this' to one of its fields, before it's published, it'll be safe.  Also assigning 'this' to an instance field is safe as well (or at least, I can't think why it is unsafe), but it's weird enough that I don't want to lose too much sleep trying to be correct about it.


Passing 'this' as a parameter

The tool will render your class mutable if it does any of the following:

1. passes 'this' as parameter to another method
• new SomeOtherClass(this);
• SomeOtherClass.staticMethod(this);
• someOtherObject.instanceMethod(this);
2. constructs an inner class (which has an implicit reference to 'this')
   
• new InnerClass()

A false positive in this case could be, with #1, passing 'this' to somewhere that only saves the reference, without trying to read any values from it, or with #2, the instance of the inner class might not escape, meaning the 'this' reference hasn't escaped.

Unexpected false positives
The last examples of false positives are ones that can occur with a correct implementation. However, there's one false positive that I didn't expect to create. While the tool is trying to check for 'this' being passed to other methods, it also accidentally raises an issue when instance fields are passed as well.


Here's an example from java.util.Vector:

public Vector(Collection c) {
    elementData = c.toArray();
    elementCount = elementData.length;

    if (elementData.getClass() != Object[].class)
        elementData = Arrays.copyOf(elementData, elementCount, Object[].class);
    }

}



The issue being the call to Arrays.copyOf() with two instance fields. This piece of code will cause Vector to be reported as mutable (among many other reasons), and quite nicely demonstrates the issues involved. Here, the method is single threaded, but it hypothetically could change at any point to publish the references it receives to multiple threads. Without further (expensive) analysis into what Arrays.copyOf() does, I can't conclude whether Vector is immutable or not. I'm choosing to err on the side of caution. There is the scope for a potential whitelist of safe methods, but that's not on the cards just yet.


Another interesting point is that one field is a primitive int, the other is an array. Since the value of the int will be copied to the method, it's probably safe anyway, no matter whether threads are involved or not. However, for the array, the reference to the field will be passed, not the contents. If the array is not completely populated and made visible when it's passed, the receiving method could observe it to change, breaking immutability. This leads on to the wider question of any mutable fields escaping at any point in the object's life cycle, not just in the constructor. For now, I'm embracing the happy accident that the checker will warn you of some potentially dodgy code, and will deliberately leave that particular false positive active.


I don't expect any of these false positives to really cause an issue If you disagree, get in touch, email me, leave a comment, or raise a bug (that may require sign in).



I expect to release a new version within the next week, stay tuned for more news!