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.