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?