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?
Posts
