Discussing the design, decisions and discovery involved in developing Mutability Detector, an open-source analysis tool for Java.

Monday, 16 July 2012

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.


No comments:

Post a Comment