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.