ThreeTen Case Study: A Time (And Date) To Reflect

We started this series of blogs with 55 test cases, each using Mutability Detector to assert immutability of classes within ThreeTen. 6 of those test cases were failing, and worth investigating. We saw how Mutability Detector can prompt a developer to carefully analyse those classes, and if those classes are actually immutable, how those warnings can be easily and flexibly suppressed. 4/6 of those failures were false positives, and the tests were adapted to suppress those warnings. 2/6 were considered real problems, and have since been fixed in the master branch of ThreeTen.

From this case study, there are several things I have learned about Mutability Detector.

A high pass rate is possible
32 tests passed without suppressing warnings, 23 required suppressions. I am particularly pleased with this success rate, given the strictness of the tool.

The String Problem is both surprising and annoying
An issue that has consistently suprised users is that java.lang.String is found to be mutable. Users don't particularly care that String has a benign data race where its hash field is lazily calculated and assigned outside the constructor. They only care that String is so often held up as an example of an immutable Java class.

As well as being surprising, it causes problems even when you understand what causes it. For example, ThreeTen's ZoneOffset has a String field which requires a suppression like "provided(String.class).isAlsoImmutable()" in the test case. But more annoying than that, ZoneOffset's supposed mutability flows back to the other classes which have a ZoneOffset field. That causes an extra 5 suppressions in the tests for classes which have a ZoneOffset field.

I am now fairly convinced that automatically detecting that String is immutable, which involves correctly deciding if a data race is benign, is just too damn difficult. I have started work to allow hardcoding String as immutable in Mutability Detector. Since ThreeTen has shown that benign data races/internal caching is not limited to String, there should also be an out-of-the-box suppression for this particular use case. 

Panicking at the sight of a mutable type is simplistic
Mutability Detector does not yet allow the safe, encapsulated use of a mutable type. When your class has a field which is a mutable type (or an array) it will raise an error. Checking for non-mutating calls is non-trivial, but I believe, achievable. For arrays it would have to check for element mutation. For mutable objects, it would involve building up a dictionary of mutating methods, and ensuring you don't call them. So your class can have java.util.Date field, as long as it doesn't call, e.g. setYear. With this, I would have to be very wary of the performance implication of such an analysis.

Also, we saw that there is the potential for false negatives if your mutable field escapes. As in, the class never invokes setYear, but it passes the instance as a parameter to a method in another class which does.

Warnings about visibility in Java Memory Model are naive
We saw warnings about non-final fields not receiving the guarantee that they will be visible, even if assigned in the constructor. This ignores that it is possibly to obtain this guarantee in other ways: if your non-final field is volatile, or is of a particular type, e.g. AtomicReference; and if there are other memory barriers e.g. assignment to a volatile field coming last in a constructor where non-final fields are assigned.

The former should be an easy enough change to make. The latter might be more tricky, but still doable.

Analysis is implementation-focussed
The team behind ThreeTen's implementation previously discussed the possibility of using Mutability Detector or other tools for enforcing immutability. The idea being that the test could be part of JSR310's Technology Compatibility Kit (TCK). However, being on the strict side, it would be difficult to meaningfully test for immutability, while also allowing internal caching, in implementations other than the OpenJDK's. This limitation may also apply to anyone wishing to use the analysis at runtime. For now, Mutability Detector is best suited for testing specific implementations of your own classes.

Can help experts
Following the blog entries which exposed real mistakes in immutable implementations, the problems were deemed important enough to warrant fixing. ThreeTen is being developed by experts in their field, for inclusion in the reference implementation, lead by a developer who created one of the most popular examples of using immutability in Java. With Mutability Detector, I am aiming to serve both beginners learning how to write immutable classes, and experienced developers familiar with the technique who have made a "typo". This case demonstrated the latter does happen, and that the tool can help, with little cost in terms of setup and maintenance.

API for allowing reasons for mutability is flexible and powerful
In several of the examples, we saw how we could implement a custom Hamcrest matcher to suppress a false positive. I used Mutability Detector in the way that users are intended to, in that I did not make any changes to the source code. In each case, we could suppress false positives, in a custom way, without any modifications to Mutability Detector itself. Over time I would expect to incorporate some of the more common matchers into the out-of-the-box offerings.

In conclusion, I'm pleased with the results from analysing ThreeTen, but there's lots of room for improvement.

Have you tried Mutability Detector? Loved it? Hated it? I want to hear from you! Any kind of feedback is welcome, either here, on the discussion group, or in email to grundlefleck at gmail dot com.