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

Tuesday, 13 July 2010

EscapedThisReferenceChecker - A Sketch Of A Checker To Come

In a previous entry I discussed some of the rules for immutability, and how they are enforced in Mutability Detector through a set of "checkers". So, for example, where a method can change the reference of an object's field (i.e. a setter method) there is a checker (named SetterMethodChecker) which can detect this rule and render a class definitely not immutable.

This blog entry is aimed at discussing another checker which doesn't exist, but should, before a 1.0 release can be considered.

EscapedThisReferenceChecker

This can be a bit tricky to explain, the first hit for "immutability escape constructor" explains it pretty well. Basically, if you're constructing an object, and someone can get hold of a reference to it before the constructor has completed, there are no guarantees about what that part-constructed object which actually consist of. A code example will help. We'll start with an immutable class, and to keep a theme going I'll call the class IAmImmutable. For those unfamiliar with java.lang.String, it's necessary to know that it is immutable too.


public final class IAmImmutable {
  public final String aField;

  public IAmImmutable(String aField) {
    this.aField = aField;
  }
}


A good example of an immutable class. To show how the reference escapes, we'll show a near identical class, MyThisReferenceEscapes.

public final class MyThisReferenceEscapes {
  public final String aField;

  public MyThisReferenceEscapes(String aField) {
    this.aField = aField;
    ReferencePublishingService.publish(this);
  }
}

In this example, the "this" reference escapes before the constructor is complete, as it is passed to a method in another class. Because of the semantics of the Java memory model, within the publish() method, the reference to the MyThisReferenceEscapes instance could have it's field set, or it might not, there's no guarantees of behaviour. Even if the "this" reference escapes in the last line, after all the fields have been assigned, it doesn't matter, it's still escaped in an incomplete state. Since the ReferencePublishingService could do just about anything with that reference, it could lead to the instance of MyThisReferenceEscapes changing across threads, at different times. With this vulnerability, there is no guarantee that the MyThisReferenceEscapes class is immutable.

I envisage that this kind of checker should be relatively straight forward - once the rules are decided. The "this" can't be passed as a parameter, but only to other classes? Static methods in the same class? Instance methods? It also can't be assigned to fields in another class, but again, how about as a static field in the same class? How about as an instance field?
I'm not entirely sure about these things, so I'll have a think about it, then pap the thought off to my subconscious, and see if it comes up with any bright ideas.

Monday, 5 July 2010

Immutability and subclassing: are they mutually exclusive?

In an earlier post I introduced the Mutability Detector, and how Josh Bloch's rules were used in its development. One point I mentioned was that Josh's rules are a bit on the strict side. I'm using this blog entry to clarify what I meant by that, and get some of the ol' wheels turning on the question: are immutability and subclassing mutually exclusive?

One of the immutability rules from Effective Java is:
2. Ensure that the class can’t be extended.


I'd like to make the case that yes, while preventing your class being extended is not going harm the immutability status it isn't always necessary. There's also the issue to consider that declaring your class final may have benefits, somewhere, at some point, in the hazy world of the Java memory model.

I'll start by explaining when it is necessary.

For example, given the following class:

public class IAmImmutable {
private String label;
public IAmImmutable(String label) {
this.label = label;
}
public String getLabel() {
return this.label;
}
}


I'm pretty sure everyone would consider this immutable - an instance of IAmImmutable cannot change after construction. However, it fails one of Josh's rules: the class can be extended. So you could have a situation like:

public class ImInUrClassRuiningUrImmutability extends IAmImmutable {
... // similar constructor
public void setLabel(String label) {
this.label = label;
}
}


So you could have a class which depends on IAmImmutable. Perhaps you're composing your big immutable objects out of smaller immutable ones. Like this:

public class ComposedOfImmutables {
public final IAmImmutable firstField;
public final SecondImmutableClass secondField;

public ComposedOfImmutables(IAmImmutable first, SecondImmutableClass second) {
firstField = first;
secondField = second;
}
}


All is well: ComposedOfImmutables, because it's being built on the solid foundation of immutable instances, is easier to understand, debug, reason about, and almost trivial to use in multithreaded applications. That is, however, until IAmImmutable is subclassed, and the menace that is the immutability-ruining LOLclass is free to pollute the otherwise immutable world around it. An instance of ImInUrClassRuiningUrImmutability, because it extends IAmImmutable, can be passed as a parameter to ComposedOfImmutables, corrupted it with mutability. Like so:

ImInUrClassRuiningUrImmutability mutableThing = new ImInUrClassRuiningUrImmutability("lol");
ComposedOfImmutables supposedlyImmutable = new ComposedOfImmutables(mutableThing, ...);

publishImmutableThingToManyThreads(supposedlyImmutable);

supposedlyImmutable.firstField.setLabel("hr i goez - ruinin ur immutability");


This is the case where it's important that IAmImmutable cannot be extended. It's a consumer thing. The existence of a mutable subclass does not mean IAmImmutable instantly becomes mutable. You can still new them up and pass them around multiple threads with joyful abandon. But... ComposedOfImmutables depends on the instance it receives in the constructor being immutable. The onus of immutability in this case does, and should lie, with ComposedOfImmutables.

You may be asking, what's the problem? IAmImmutable obviously wants to be immutable. It lives to be used immutably (is that a word?), why not just prompt the developer to prevent subclassing?. Well, it becomes an issue for static code checking tools, like Mutability Detector. In the past year I spent my honours year project steeped in the stew of static bug checkers, with their incomplete analysis, unsoundness, and swathes of false positives. My research in that area has convinced me that any static analysis tool has to be very, very careful of reporting errors that get in the users way. If it's not reporting real problems they want to fix, it's wasting their time.

[Edit: looking back after the comments made by Seb Charrot, this conclusion was really misleading. Hopefully this revision makes more sense.]

That's why it's my opinion that Josh's rule is too strict. And it's the reason that Mutability Detector will not emit errors that IAmImmutable is in fact mutable, it will instead emit warnings. Instead of an "omg teh buildz broken!", it says "You know, there's a danger here. Gather round to hear why...". However, Mutability Detector has, and will, tell you that ComposedOfImmutables is definitely not immutable. If guarantees can't be made about the runtime instances passed to a class, the foundations it is built on are too weak to support it, and immutability cannot be claimed.

So while most of the time immutability and subclassing are incompatible, hopefully this post has demonstrated that they are not always mutually exclusive.

Sunday, 4 July 2010

An Introduction To Mutability Detector (Part 1 of n)

(n still to be decided)

Mutability Detector is a tool which analyses Java code, and tells you if instances of a given class are immutable or mutable. Having been a google code project for around six months, some recent discussions (one on the issues page, and another in the mailing list for Project Lombok, in an unrelated discussion) have brought a little bit of attention to the project. I thought it would be useful to give an overview of the usage of Mutability Detector, and how it can be used to recognise mutability in your classes.

Mutability Detector works by applying a set of checkers to each class. Each checker looks for a particular way of rendering a class as mutable, and the results from each checker are combined to give the overall "mutability result" of the class. The checkers closely relate to a set of rules for achieving immutability. These began as the rules defined by Josh Bloch in the excellent Effective Java, 2nd Edition. Josh lists five rules (in Item 15), these are:

1. Don’t provide any methods that modify the object’s state (known as mutators).
2. Ensure that the class can’t be extended.
3. Make all fields final.
4. Make all fields private.
5. Ensure exclusive access to any mutable components.

These formed a good basis for an everyday tool for ensuring your classes are immutable. But I found them to be a bit on the strict side (more on that in a later blog entry). When first writing the tool, those rules were combined with the following:

1. It should not be possible to reassign any field. (this is almost implicit in Josh's rules 1, 3 & 4)
2. An immutable object should consist of immutable fields.
This implies three sub-rules. If a field is not constructed directly (i.e. passed as a parameter, or as the result of a method call):
i. It must not be possible to subclass the type.
ii. It should be declared as a concrete type.
iii. If the previous two cannot be achieved, make a copy of in the constructor and assign that to the field.

Some of these rules are pretty opaque for understanding what the tool actually detects (and in the past few days, I've found them to be a bit lacking). What might help understand Mutability Detector is some of the patterns of code which are detected and reported. Listed here is each checker, some example code and the results of applying the checker to that code.


FinalClassChecker
public class MutableByNotBeingFinalClass {

}
MutableByNotBeingFinalClass is MAYBE immutable
Is not declared final, and thus may be immutable.

PublishedNonFinalFieldChecker

public class MutableByHavingPublicNonFinalField {
public String name;

public MutableByHavingPublicNonFinalField(String name) {
this.name = name;
}
}
MutableByHavingPublicNonFinalField is DEFINITELY_NOT immutable
Field [name] is visible outwith this class, and is not declared final.

SetterMethodChecker

public final class MutableByHavingSetterMethod {
private String name;
public void setName(String name) {
this.name = name;
}
}
MutableByHavingSetterMethod is DEFINITELY_NOT immutable
Field [name] can be reassigned within method [setName]
AbstractTypeToFieldChecker
public final class MutableByAssigningAbstractTypeToField {
private AbstractStringContainer nameContainer;

public MutableByAssigningAbstractTypeToField(AbstractStringContainer abstractNameContainer) {
nameContainer = abstractNameContainer;
}

abstract class AbstractStringContainer {
protected final String name = "my name";
}
final class StringContainer extends AbstractStringContainer {
public String someMutableField;
}
}
MutableByAssigningAbstractTypeToField is DEFINITELY_NOT immutable
Field [nameContainer] can have an abstract type (AbstractStringContainer) assigned to it.


(Here the problem is, that while AbstractStringContainer is immutable, StringContainer is not, and either could be passed as a parameter to the constructor. Thus we can never be sure that the field we're assigning to references an immutable object or not. Though this could be relaxed to render types like these as MAYBE immutable.)

InherentTypeMutabilityChecker

This checker is used more to help decide the immutability status for client classes. For example, if a class has a field which is inherently mutable, this affects the containing class.

public class MutableByHavingArrayTypeAsField {
private final String names[];
}
MutableByHavingArrayTypeAsField is DEFINITELY_NOT immutable
Field [names] is a primitive array.

This currently quite limiting - the field 'names' may not actually be mutated, yet the containing class would be declared as mutable. This is definitely something to work on.


Some of these rules aren't complete. Sometimes they don't interact too well with certain classes (inheritance is hairy issue). Common patterns, such as lazily loaded fields, aren't handled at all (thus java.lang.String is supposedly mutable). So there's a lot to do, but hopefully this entry has provided a decent introduction to Mutability Detector.