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.

Checking for Mutability in JSR 310: StandardZoneRules

Here we have the final failing test case, out of the 55 intended immutable classes in ThreeTen.

The assertion:

assertImmutable(Class.forName("javax.time.zone.StandardZoneRules"));

Fails with:

org.mutabilitydetector.unittesting.MutabilityAssertionError: 
Expected: javax.time.zone.StandardZoneRules to be IMMUTABLE
     but: javax.time.zone.StandardZoneRules is actually NOT_IMMUTABLE
    Reasons:
        Field can have a mutable type (java.util.concurrent.ConcurrentHashMap) assigned to it. [Field: lastRulesCache, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: standardTransitions, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: standardOffsets, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: savingsLocalTransitions, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: wallOffsets, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: savingsInstantTransitions, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: lastRules, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (java.util.concurrent.ConcurrentHashMap) assigned to it. [Field: lastRulesCache, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: standardTransitions, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: standardOffsets, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: savingsInstantTransitions, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: wallOffsets, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: lastRules, Class: javax.time.zone.StandardZoneRules]
        Field can have a mutable type (a primitive array) assigned to it. [Field: savingsLocalTransitions, Class: javax.time.zone.StandardZoneRules]
        Field is a primitive array. [Field: standardTransitions, Class: javax.time.zone.StandardZoneRules]
        Field is a primitive array. [Field: standardOffsets, Class: javax.time.zone.StandardZoneRules]
        Field is a primitive array. [Field: savingsInstantTransitions, Class: javax.time.zone.StandardZoneRules]
        Field is a primitive array. [Field: savingsLocalTransitions, Class: javax.time.zone.StandardZoneRules]
        Field is a primitive array. [Field: wallOffsets, Class: javax.time.zone.StandardZoneRules]
        Field is a primitive array. [Field: lastRules, Class: javax.time.zone.StandardZoneRules]
    Allowed reasons:
        None.

This time, a huge 21 reasons why StandardZoneRules is mutable. But it's not, it's immutable. These 21 reasons fall into roughly the same category though. The class has both a mutable type (ConcurrentHashMap) and array types as fields. This causes Mutability Detector to basically panic without further analysis to see if the mutable things are actually modified. However, all the array fields are final, encapsulated, and contain elements which we consider to be immutable. The ConcurrentHashMap is modified, but it's another example of an unobservable mutation, an internal caching strategy.

VERDICT: False positive.
WHAT TO DO: Using the allowed reason matcher implementations from previous examples, we can alter the test to look like this:

assertInstancesOf(Class.forName("javax.time.zone.StandardZoneRules"),
          areImmutable(),
          AssumingTheFields.named("lastRulesCache").areModifiedAsPartAsAnUnobservableCachingStrategy(),
          AssumingArrayFields.named("standardTransitions", 
                        "standardOffsets", 
                        "savingsLocalTransitions",
                        "wallOffsets", 
                        "savingsInstantTransitions", 
                        "lastRules")
              .areNotModifiedAndDoNotEscape());

Which passes. For the initial number of failure reasons, the passing test doesn't end up too bad. A bit refactoring-unfriendly, in that changing field names will cause the test to break. Perhaps a fuzzier matcher would avoid having to change the test whenever a field name changes. Overall I'm fairly happy with that assertion.

Final result: ThreeTen: 4, Mutability Detector 2. Convincing win from ThreeTen.

Stay tuned for a conclusion and my overall assessment and takeaways from this case study into immutability in ThreeTen.

Update: concluding post now available.

Checking for Mutability in JSR 310: ResourceZoneRulesVersion

This post will cover the penultimate failing test case, in a test for immutability in ThreeTen.

The assertion:

assertImmutable(Class.forName("javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion"));

Fails with the message:

org.mutabilitydetector.unittesting.MutabilityAssertionError: 
Expected: javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion to be IMMUTABLE
     but: javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion is actually NOT_IMMUTABLE
    Reasons:
        Can be subclassed, therefore parameters declared to be this type could be mutable subclasses at runtime. [Class: javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion]
        Field can have a mutable type (javax.time.zone.ResourceZoneRulesDataProvider) assigned to it. [Field: provider, Class: javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion]
        Field can have a mutable type (java.lang.String) assigned to it. [Field: versionID, Class: javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion]
        Field can have a mutable type (a primitive array) assigned to it. [Field: regionArray, Class: javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion]
        Field can have a mutable type (a primitive array) assigned to it. [Field: ruleIndices, Class: javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion]
        Field is a primitive array. [Field: regionArray, Class: javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion]
        Field is a primitive array. [Field: ruleIndices, Class: javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion]
    Allowed reasons:
        None.

As usual, like in the previous examples, we can suppress the String problem related to the "versionID" field. There is also the "provider" field, of type ResourceZoneRulesDataProvider. We covered this case in an earlier blog, and decided this class is immutable. Thus our assertion can now look like:

assertInstancesOf(Class.forName("javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion"),
                  areImmutable(),
                  provided(String.class).isAlsoImmutable(),
                  provided("javax.time.zone.ResourceZoneRulesDataProvider").isAlsoImmutable());

Next up is the failure reason that ResourceZoneRulesVersion can be subclassed. Mutability Detector complains about this because if you are a consumer of a type Foo (i.e. you have a method which takes a Foo) but Foo can be subclassed, then callers can pass you a subclass, like MutableFoo. This issue could manifest itself in subtle, hard to detect ways. For example, if you have been adding Foo's as keys in a HashMap, but they are actually MutableFoos which have changed, you could see that the same instance cannot be used to correctly retrieve the relevant value. To allow consumers of your immutable class to have faith in it, so to speak, you should prevent subclassing. The situation is slightly different when you are the producer of the class. As with this example, the ResourceZoneRulesVersion class is an internal class, constructed and used by ResourceZoneRulesDataProvider. Since the producer is explicitly using the new keyword, there is no doubt that instances can be swapped out for mutable subclasses. Here we can do one of two things: we can add the final keyword, which should be a non-breaking change, since scope to extend the class is limited; or we can add suppress the warning so our assertion would look like:

assertInstancesOf(Class.forName("javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion"),
                  areImmutable(),
                  provided(String.class).isAlsoImmutable(),
                  provided("javax.time.zone.ResourceZoneRulesDataProvider").isAlsoImmutable(),
                  allowingForSubclassing());

This would allow the class to be extended. Which leads us to the remaining failures, that "regionArray" and "rulesIndices" are mutable array fields. The element types of these arrays are String and short, respectively. Thus we need not worry that the elements themselves can be mutated. We need only worry that the array is protected from mutation. In this case they are: regionArray is defensively copied into an unmodifiable Set, to provide a getter; and rulesIndices is never exposed. Thus, as in previous examples, we can get the test to pass by assuming these fields are handled safely, and creating an appropriate allowed reason:

public class AssumingArrayFields {
    private ImmutableSet fieldNames;

    public AssumingArrayFields(ImmutableSet fieldNames) {
        this.fieldNames = fieldNames;
    }

    public static AssumingArrayFields named(String first, String... rest) {
        return new AssumingArrayFields(ImmutableSet.copyOf(Iterables.concat(asList(first), asList(rest))));
    }
    
    public Matcher areNotModifiedAndDoNotEscape() {
        return new TypeSafeDiagnosingMatcher() {

            @Override public void describeTo(Description description) { }

            @Override
            protected boolean matchesSafely(MutableReasonDetail reasonDetail, Description mismatchDescription) {
                if (reasonDetail.codeLocation() instanceof FieldLocation) {
                    return reasonDetail.reason().isOneOf(MUTABLE_TYPE_TO_FIELD, ARRAY_TYPE_INHERENTLY_MUTABLE)
                            && fieldNames.contains(((FieldLocation)reasonDetail.codeLocation()).fieldName());
                } else {
                    return false;
                }
            }
            
        };
    }
}

Which allows us to write a passing test:

assertInstancesOf(Class.forName("javax.time.zone.ResourceZoneRulesDataProvider$ResourceZoneRulesVersion"),
                  areImmutable(),
                  provided(String.class).isAlsoImmutable(),
                  provided("javax.time.zone.ResourceZoneRulesDataProvider").isAlsoImmutable(),
                  allowingForSubclassing(),
                  AssumingArrayFields.named("regionArray", "ruleIndices").areNotModifiedAndDoNotEscape());

We finally have a passing test. But it's taken a lot of suppressions to get there. I'll leave it as an exercise to the reader to decide if the test, with all its allowed reasons, is pulling its weight.

An interesting issue is brought up by this class. Although the array fields do not escape through, for example, getter methods, they do escape to method invoked by the class itself. These include java.util.Arrays#binarySearch, and java.util.Arrays#asList. In this case, we can trust these methods are safe havens, just from knowing the implementation. However, Mutability Detector does not know anything about these methods. For all Mutability Detector knows, the class could be invoking com.smelly.Evil#mutateYourArray and it would not complain. This is a false negative which, unfortunately, users need to be wary of.

VERDICT: False positive.
WHAT TO DO: In this case, I would make the class final, and allow the other reasons. Personally I feel the test is still worth the cost. It's still preventing setter methods and non-final fields, for me that's good enough, but I understand the number of allowed reasons is getting rather annoying.

Score so far, ThreeTen 3, Mutability Detector 2.

One class left to go, StandardZoneRules. Can Mutability Detector pull out a draw in extra time?
               

Checking for Mutability in JSR 310: ZoneOffsetTransitionRule

Another example of a failing test case from ThreeTen. In this case, the assertion:

assertImmutable(javax.time.zone.ZoneOffsetTransitionRule.class);

Fails with the message:

org.mutabilitydetector.unittesting.MutabilityAssertionError: 
Expected: javax.time.zone.ZoneOffsetTransitionRule to be IMMUTABLE
     but: javax.time.zone.ZoneOffsetTransitionRule is actually NOT_IMMUTABLE
    Reasons:
        Field is not final, if shared across threads the Java Memory Model will not guarantee it is initialised before it is read. [Field: timeEndOfDay, Class: javax.time.zone.ZoneOffsetTransitionRule]
        Field can have a mutable type (javax.time.ZoneOffset) assigned to it. [Field: standardOffset, Class: javax.time.zone.ZoneOffsetTransitionRule]
        Field can have a mutable type (javax.time.ZoneOffset) assigned to it. [Field: offsetBefore, Class: javax.time.zone.ZoneOffsetTransitionRule]
        Field can have a mutable type (javax.time.ZoneOffset) assigned to it. [Field: offsetAfter, Class: javax.time.zone.ZoneOffsetTransitionRule]
    Allowed reasons:
        None.

Once again, the String Problem is biting us. In this case, mutability is seen to flow back from the type javax.time.ZoneOffset. There is a separate test case for ZoneOffset, which is only considered mutable because it has a String field. Thus, we can change the assertion to look like this:

assertInstancesOf(javax.time.zone.ZoneOffsetTransitionRule.class, 
                  areImmutable(),
                  provided(ZoneOffset.class).isAlsoImmutable());

Which leaves us with the single error, that the boolean field timeEndOfDay is not final. In this case, I think Mutability Detector has found a real problem. The field is never reassigned, so callers cannot change the object. However, instances of ZoneOffsetTransitionRule could be published to other threads, without extra synchronisation around the timeEndOfDay field, Without guarantees of visibility from the Java Memory Model, users could see it change from its default value (false) to its real value (true or false) once it's been made visible correctly.

VERDICT: True positive (yay)
WHAT TO DO: Change the field to be final. Making it volatile would also prevent the aforementioned problem, but Mutability Detector wouldn't take account of it (see the previous post). All the other fields in the class are final anyway, so it makes sense to have this one be final as well.

ThreeTen: 2, Mutability Detector 2

Next: ResourceZoneRulesVersion

Checking for Mutability in JSR 310: ZoneRulesGroup

In seeing how Mutability Detector fared on ThreeTen, the reference implementation for JSR 310, the previous two blogs described an in depth analysis of classes which cause failing immutability tests. Hopefully the following examples will be a bit more simple, and clear cut. 

The assertion:
      assertImmutable(ZoneRulesGroup.class); 

Fails with the message:

org.mutabilitydetector.unittesting.MutabilityAssertionError: 
Expected: javax.time.zone.ZoneRulesGroup to be IMMUTABLE
     but: javax.time.zone.ZoneRulesGroup is actually NOT_IMMUTABLE
    Reasons:
        Field is not final, if shared across threads the Java Memory Model will not guarantee it is initialised before it is read. [Field: versions, Class: javax.time.zone.ZoneRulesGroup]
        Field can have a mutable type (java.util.concurrent.atomic.AtomicReference) assigned to it. [Field: versions, Class: javax.time.zone.ZoneRulesGroup]
        Field can have a mutable type (java.lang.String) assigned to it. [Field: groupID, Class: javax.time.zone.ZoneRulesGroup]
    Allowed reasons:
        None.

It's becomming a common and irritating theme, but we can suppress the String Problem as per the previous examples. That leaves us with two issues: that ZoneRulesGroup has a non-final field, and that same field is a mutable type (AtomicReference).

The declaration of the field in question looks like this:

private AtomicReference<TreeMap<String, ZoneRulesVersion>> versions =
            new AtomicReference<TreeMap<String, ZoneRulesVersion>>(new TreeMap<String, ZoneRulesVersion>(Collections.reverseOrder()));

The field itself contains a mutable type, but it's another example of an unobservable caching stretegy. There is also extra synchronization around modification, but thread safety != immutability, and Mutability Detector isn't intended to analyse this. The other complaint from Mutability Detector is that the field may not be visible if published to other threads. This however, is a special case that Mutability Detector does not account for. As described in Java Concurrency In Practice (p52, ch3.5.3 Safe Publication Idioms) 

"A properly constructed object can be safely published by [...] Storing a reference to it into a volatile field or AtomicReference". 

The aforementioned synchronization would also guarantee visibility.


 VERDICT: False positive.

 WHAT TO DO?: I don't see any reason to not make the field final, since it is never reassigned anyway. Thus we could add the final modifier to the declaration. You could say that it's overkill, and just noise, but I think the semantics of how the field is used matches the final keyword, so I'd be happy to add it. To solve the mutable field problem, we could borrow the same allowed reason from an earlier example. The assertion would look like this:

assertInstancesOf(javax.time.zone.ZoneRulesGroup.class,
                  areImmutable(),
                  provided(String.class).isAlsoImmutable(),
                  assumingTheFields("versions").areModifiedAsPartAsAnUnobservableCachingStrategy());

If there's some reason that the field cannot be final that I don't understand, Mutability Detector ships with an allowed reason implementation which can help here. Keeping the field non-final, an assertion could be:

assertInstancesOf(javax.time.zone.ZoneRulesGroup.class,
                  areImmutable(),
                  provided(String.class).isAlsoImmutable(),
                 assumingTheFields("versions").areModifiedAsPartAsAnUnobservableCachingStrategy(),
                  allowingNonFinalFields());

But, "allowingNonFinalFields()" can be overly permissive, allowing new non-final fields to be added in such a way that they don't guarantee safe publication. Perhaps it would be better to specify the field by name, as with the other allowed reason.
For me, this false positive is a good find. In terms of visibility in the Java Memory Model, Mutability Detector could fairly easily permit a field if it is either volatile or an instance of AtomicReference.

Since I'm keeping score, it's ThreeTen: 2, Mutability Detector 1. Next up, ZoneOffsetTransitionRule.

Checking for Mutability in JSR 310: ResourceZoneRulesDataProvider

Checking for Mutability in JSR 310: ResourceZoneRulesDataProvider

Carrying on in my checking of mutability in the reference implementation of JSR 310, ThreeTen, I'm going to look at the next failing test case.

The assertion[0]:

 assertImmutable(Class.forName("javax.time.zone.ResourceZoneRulesDataProvider"));

Fails with:

  org.mutabilitydetector.unittesting.MutabilityAssertionError: 
  Expected: javax.time.zone.ResourceZoneRulesDataProvider to be IMMUTABLE
      but: javax.time.zone.ResourceZoneRulesDataProvider is actually NOT_IMMUTABLE
      Reasons:
   Field can have a mutable type (java.lang.String) assigned to it. [Field: groupID, Class: javax.time.zone.ResourceZoneRulesDataProvider]
   Field can have a mutable type (java.util.HashSet) assigned to it. [Field: regions, Class: javax.time.zone.ResourceZoneRulesDataProvider]
   Field can have a mutable type (java.util.HashSet) assigned to it. [Field: versions, Class: javax.time.zone.ResourceZoneRulesDataProvider]
   Field can have a mutable type (java.util.concurrent.atomic.AtomicReferenceArray) assigned to it. [Field: rules, Class: javax.time.zone.ResourceZoneRulesDataProvider]
      Allowed reasons:
   None.

The first failure reason, exemplifies The String Problem. Which I can suppress by changing the assertion to:

  assertInstancesOf(Class.forName("javax.time.zone.ResourceZoneRulesDataProvider"), 
   areImmutable(),
   provided(String.class).isAlsoImmutable());
 
Which leaves us with the three remaining failure reasons. The first two are quite similar, that the class has mutable fields of type java.util.Set (the assignment is from the concrete java.util.HashSet). Although we can't tell from the bytecode, the generic type of these Sets are String, and a ThreeTen type, ZoneRulesVersion, which is expected to be immutable.Thus, element types of both fields can be considered immutable. This means that in terms of these fields, we only have to worry about the collection type, not the element type.

At first glance, this appears to be a problem. Here's a snippet which represents a subset of the code in the ResourceZoneRulesDataProvider class:

final class ResourceZoneRulesDataProvider {
  private final Set<ZoneRulesVersion> versions;
  private final Set<String> regions;
  
  private ResourceZoneRulesDataProvider(URL url) {
    // constructor code reading from an input stream
    this.regions = new HashSet<String>(Arrays.asList(aLocallyPopulatedArrayVariable));
    this.versions = aLocallyPopulatedHashSet;
  }
  
  public Set<ZoneRulesVersion> getVersions() {
    return versions;
  }

  public Set<String> getRegionIDs() {
    return regions;
  }
}

Taken at face value, this code represents a problem. If a user could get a hold of an instance of ResourceZoneRulesDataProvider, they could for instance, execute:

 resourceZoneRulesDataProvider.getRegionIDs().remove("Europe/London");

This represents a trade off in Mutability Detector's design, and the choice to not perform whole program analysis. Those methods to retrieve the mutable sets are only invoked from one place, the class ZoneRulesGroup. This class is in the same package, taking advantage of the package-scoped methods in ResourceZoneRulesDataProvider. Tracing up those calls, it appears they are only triggered from the static initialisation block from ZoneRulesGroup, from which neither the ResourceZoneRulesDataProvider instances, or their mutable fields escape. Such a situation would take much more analysis than Mutability Detector currently performs, increasing the time taken to analyse, and greatly complicate the implementation. That's not likely to be an avenue I'd be willing to take, so the next best thing is to offer users the ability to overrule Mutability Detector, and tell the tool that this particular example is safe.

Fortunately, an API for doing so already exists in Mutability Detector, it just takes a bit more effort from the user. It involves implementing a Hamcrest matcher to allow mutability reasons. In the assertion example above, the call "provided(String.class).isAlsoImmutable()" is nothing more than just a Hamcrest matcher. An example of how that could look is:

static final class AssumingTheFields  {
    
    private Set<String> fieldNames;

    private AssumingTheFields(Set<String> fieldNames) {
 this.fieldNames = fieldNames;
    }
    
    public static AssumingTheFields assumingTheFields(String first, String... rest) {
 return new AssumingTheFields(Sets.newHashSet(concat(asList(first), asList(rest))));
    }
    
    Matcher<MutableReasonDetail> areNotModifiedByCallers() {
 return new TypeSafeDiagnosingMatcher<MutableReasonDetail>() {
     @Override public void describeTo(Description description) { }

     @Override
     protected boolean matchesSafely(MutableReasonDetail item, Description mismatchDescription) {
  CodeLocation<?> locationOfMutability = item.codeLocation();
  if (locationOfMutability instanceof FieldLocation) {
      return item.reason().isOneOf(MUTABLE_TYPE_TO_FIELD)
       && fieldNames.contains(((FieldLocation)locationOfMutability).fieldName());
  } else {
      return false;
  }
     }
 };
    }
}

This allows us to write an assertion like this:

    assertInstancesOf(Class.forName("javax.time.zone.ResourceZoneRulesDataProvider"), 
                      areImmutable(),
                      provided(String.class).isAlsoImmutable(),
                      assumingTheFields("regions", "versions").areNotModifiedByCallers());
                   
("regions" and "versions" being the Sets which escape from ResourceZoneRulesDataProvider). The result of this assertion now looks like this:

org.mutabilitydetector.unittesting.MutabilityAssertionError: 
Expected: javax.time.zone.ResourceZoneRulesDataProvider to be IMMUTABLE
     but: javax.time.zone.ResourceZoneRulesDataProvider is actually NOT_IMMUTABLE
    Reasons:
        Field can have a mutable type (java.util.concurrent.atomic.AtomicReferenceArray) assigned to it. [Field: rules, Class: javax.time.zone.ResourceZoneRulesDataProvider]
    Allowed reasons:
        Field can have a mutable type (java.lang.String) assigned to it. [Field: groupID, Class: javax.time.zone.ResourceZoneRulesDataProvider]
        Field can have a mutable type (java.util.HashSet) assigned to it. [Field: regions, Class: javax.time.zone.ResourceZoneRulesDataProvider]
        Field can have a mutable type (java.util.HashSet) assigned to it. [Field: versions, Class: javax.time.zone.ResourceZoneRulesDataProvider]

Allowing these fields to be mutable leaves us with just one failure. In this case the "rules" field, which is of type AtomicReferenceArray. This is a very similar problem to the previous one. In this case, the collection does not escape, but its contents are modified in one of the instance methods. This appears to be implementation detail of a caching strategy, not an observable change. Again, the scope to invoke this method is limited, and with the current state of the code, it looks to be used safely. As such, we can just treat this field like the escaping Sets, so our assertion can look like this:

  assertInstancesOf(Class.forName("javax.time.zone.ResourceZoneRulesDataProvider"), 
                    areImmutable(),
                    provided(String.class).isAlsoImmutable(),
                    assumingTheFields("regions", "versions").areNotModifiedByCallers(),
                    assumingTheFields("rules").areModifiedAsPartAsAnUnobservableCachingStrategy());
                 
Which passes. Note that the implementation for both suppressions are the same, the difference in name is to communicate to other developers.

VERDICT: This case is tricky. It's a false positive in the context of whole program analysis. However, the class in isolation is definitely mutable. Classes within the same package have the necessary access to create and mutate instances of this class.
WHAT TO DO?: It is possible to suppress the reasons to get a passing test. Personally, I would either: stop claiming the class is immutable in JavaDoc, and document that instances "depend on the kindness of strangers" as it were, to be effectively immutable; or be more strict, and wrap mutable collection types in unmodifiable versions[1].


In conclusion, in a blatant show of bias, I'm going to consider this a very narrow win for Mutability Detector.That put's us at ThreeTen: 1, Mutability Detector: 1.

Click here for the next entry, ZoneRulesGroup.


[0] This assertion demonstrates using the String version of the method, which is useful when your test is for a class which is out of scope (e.g. private, protected, etc).
[1] Though there are two issues with that: the overhead in creating an unmodifiable wrapper; and that Mutability Detector doesn't yet correctly analyse defensive copies into unmodifiable wrappers, so users would still have to add a suppression anyway.

Times Have Changed, But An Instant Stays The Same: Checking for Mutability in JSR 310

I recently attended a London Java Community meetup to try out the new date and time API scheduled for Java 8[1]. JSR 310 (or "ThreeTen" as named on GitHub) provides an upgrade to the standard date and time libraries available in the JDK. At first use, ThreeTen looks to have addressed the mistakes of old JDK Date API, and draws influence from the popular, defacto standard date/time library, JodaTime[2].

Overall I was very impressed with the design and discoverability of the API. And of course, I'm a big fan of it's design choice to use immutability wherever possible.

Since JodaTime is somewhat of a poster-child for immutability in Java, and it's unsurprising the same design principle has transferred to ThreeTen. I was curious to see how effective Mutability Detector could be in helping to enforce this principle.

Using the highly rigourous and scientific method of grep'ing the codebase for the term " immutable ", as included in the JavaDoc for such types, I found 55 immutable classes in ThreeTen. For each of those 55, I added a unit test case to assert the class is immutable. Out of those 55, 6 tests are failing. Among the passing tests are some of ThreeTen's core classes: LocalDate, LocalDateTime, Duration, Period, and more. I'm really impressed by how 'cleanly' these immutable implementations are. I'm also happy with the apparent success rate of Mutability Detector on this real-world codebase (though I know that the results assume zero false negatives).

There's two ways to look at 6 failing tests from 55. Firstly, assuming that when a ThreeTen author documents a class as immutable, it is, Mutability Detector produces 6 false positives. Secondly, assuming that Mutability Detector correctly analyzes, ThreeTen has unexpected mutability in 6 classes. Since these conditions are mutually exclusive, over the next few blog entries, I'm going to take a look at the classes in question. First up is DateTimeFormatter.

The assertion:

  assertImmutable(javax.time.format.DateTimeFormatter.class);

Fails with:

  org.mutabilitydetector.unittesting.MutabilityAssertionError: 
  Expected: javax.time.format.DateTimeFormatter to be IMMUTABLE
      but: javax.time.format.DateTimeFormatter is actually NOT_IMMUTABLE
      Reasons:
 Field can have a mutable type (java.util.Locale) assigned to it. [Field: locale, Class: javax.time.format.DateTimeFormatter]
 Field can have a mutable type (javax.time.format.DateTimeFormatterBuilder$CompositePrinterParser) assigned to it. [Field: printerParser, Class: javax.time.format.DateTimeFormatter]
      Allowed reasons:
 None.

DateTimeFormatter is considered mutable because it has two fields of mutable types. The types in question being java.util.Locale, and a ThreeTen internal type, CompositePrinterParser.  Since the result for Locale (for the JDK 1.6u25 incarnation at least) is a false positive relating to The String Problem, which I can suppress in the unit test, I'll take a more detailed look at CompositePrinterParser.

The problem that Mutability Detector has with this class is that it contains a field which is an array type. There are several ways of (ab)using an array field that breaks immutability. The class can: (A) modify the contents of the array field after construction; (B) let a mutable element of the array escape to where it can be modified; (C) let the array escape so that callers can modify the contents; (D) take the array passed to the constructor without performing a defensive copy. Note that Mutability Detector isn't yet capable of this kind of analysis, so it just panics at the sight of an array field, and leaves it up to us, the developer, to investigate.

Out of those potential problems described:

(A) doesn't happen. The class only iterates over the array, it doesn't modify the contents.
(B) isn't applicable here. The element type of the array is an interface which is contracted to be immutable.
(C) and (D) are the interesting parts. There is no getter for the array field, and it's private. The only way the array escapes, is that it is passed as the constructor argument to another instance of the same class. Since we know that same class doesn't modify the array, the destination for the escaping array is a "safe haven". That particular constructor is also only ever called by the same class, so it doesn't need to copy the passed in array. There is a second constructor, but that takes a List, and performs a defensive copy of it, so the class really is in control of that array field.

VERDICT: False Positive

WHAT TO DO?:
For both Locale, and CompositePrinterParser, we can add suppressions, or "allowed reasons" for mutability. Thus the test case can look like this:

assertInstancesOf(javax.time.format.DateTimeFormatter.class, 
                  areImmutable(), 
                  provided(Locale.class).isAlsoImmutable(),
                  provided("javax.time.format.DateTimeFormatterBuilder$CompositePrinterParser").isAlsoImmutable());
               
This will allow the test to pass, ignoring the false positives, but failing again if someone introduces mutability another way.
               
Score so far, ThreeTen 1, Mutability Detector 0. Next up, ResourceZoneRulesDataProvider. (Click here for the next blog entry).

[1] Thanks to OpenGamma, Stephen Colebourne, Richard Warburton, James Gough et al for organising.
[2] This is unsurprising, since one of the technical spec leads of JSR 310, Stephen Colebourne is the creator of JodaTime.