Is My java class Immutable

Question

Please see i am not asking what is immutability, i understand immutability but question is more how to make an immutable class when you are giving reference to a mutable object. Moreover my class failed the check with mutability detector project, hence requesting your view.

I have created a Immutable class EmpAndAddress.java and it has reference to a mutable class EmpAddress.java which is cloneable. I have followed the java rules and tried to test my class using mutability detector but my class is failing the immutable test. Just want to check if I am missing something. In my Immutable i am always creating new Object of type EmpAddress mutable to follow the rules.

http://mutabilitydetector.github.io/MutabilityDetector/ 1. Mutable EmpAddress.java

public class EmpAddress implements Cloneable{
    public String empCity;
    public int zipCode; 

    public EmpAddress(String empCity, int zipCode) {
        super();
        this.empCity = empCity;
        this.zipCode = zipCode;
    }

    public String getEmpCity() {
        return empCity;
    }

    public void setEmpCity(String empCity) {
        this.empCity = empCity;
    }

    public int getZipCode() {
        return zipCode;
    }

    public void setZipCode(int zipCode) {
        this.zipCode = zipCode;
    }

    protected Object clone() throws CloneNotSupportedException {         
        EmpAddress clone=(EmpAddress)super.clone();
        return clone;    
      } 
}


public final class EmpAndAddress implements Cloneable {
    private final int empId;
    private final String empName;   
    private final EmpAddress eAddr;

    public EmpAndAddress(int empId,String empName,EmpAddress eAddr){
        super();
        this.empId = empId;
        this.empName = empName;
        this.eAddr = new EmpAddress(" ", -1);       
    }

    public int getEmpId() {
        return empId;
    }


    public String getEmpName() {
        return empName;
    }

    public EmpAddress geteAddr() throws CloneNotSupportedException {
        return (EmpAddress) eAddr.clone();
    }

}

Answer 1

The only problem I see is that you are actually not using the EmpAddress instance being passed to the EmpAndAddress constructor. I doubt that was intentional on your part.

In any case, to ensure that your class is immutable despite a reference to a mutable object is by performing clones both when receiving the EmpAddress instance in the constructor, and when returning an instance from the geteAddr() method.

You are already doing that inside the geteAddr() method, so you're ok on that front.

All that you are missing is fixing your constructor, like this:

public EmpAndAddress(int empId,String empName,EmpAddress eAddr){
    this.empId = empId;
    this.empName = empName;
    this.eAddr = (EmpAddress) eAddr.clone();    
}

Answer 2

The MutabilityDetector code is checking that the class is transitively immutable. It is not sufficient for the class itself to be immutable. The types of all of the classes fields must also be immutable. The child objects referenced by the fields are assumed to be part of the state of the parent object, so changing a child object changes the parent.

In your case, the (supposedly) immutable class EmpAndAddress has a field whose type is mutable. Furthermore, the field in an EmpAndAddress object is initialized with a value that is passed as a constructor argument. If the caller of the constructor keeps the EmpAddress reference, it can change the state of the EmpAndAddress object.

Answer 3

Full disclosure: author of Mutability Detector here... with a loooong answer.

If we start out with the class you defined in your question, and assert on it being immutable, like so;

@Test
public void isImmutable() {
    assertImmutable(EmpAndAddress.class);
}

You get a test failure with the following message:

org.mutabilitydetector.unittesting.MutabilityAssertionError: 
Expected: org.mutabilitydetector.stackoverflow.Question_32020847$EmpAndAddress to be IMMUTABLE
  but: org.mutabilitydetector.stackoverflow.Question_32020847$EmpAndAddress is actually NOT_IMMUTABLE
  Reasons:
      Field can have a mutable type (org.mutabilitydetector.stackoverflow.Question_32020847$EmpAddress) assigned to it. [Field: eAddr, Class: org.mutabilitydetector.stackoverflow.Question_32020847$EmpAndAddress]
  Allowed reasons:
      None.

(I've defined both classes as a static inner class, hence showing up as $EmpAndAddress in the message, but ignore that for now.)

Another answer to this question is totally correct. EmpAndAddress is considered mutable because EmpAddress is considered mutable, and every field of an immutable object should also be immutable. EmpAddress is mutable for several reasons: can be subclassed; has public non-final fields; setter methods. The first question is, why does cloning the EmpAddress field in the getter not help make it immutable? Well, in this case it does happen to make it immutable, but Mutability Detector doesn't perform the kind of analysis required to be confident of it. Mutability Detector doesn't have any special analysis for safely cloning mutable objects when letting them "escape" to callers. This is because it's very easy to misuse .clone and introduce mutability. Imagine if EmpAddress had a mutable field, like List, you can observe mutation like so:

@Test
public void go_on_try_to_mutate_me_now_i_have_a_list_field() throws Exception {
    EmpAndAddress e = new EmpAndAddress(1234, "John Doe");

    assertThat(e.geteAddr().getMyList(), is(Collections.<String>emptyList()));

    e.geteAddr().getMyList().add("Haha, I'm mutating you");

    assertThat(e.geteAddr().getMyList(), is(Collections.<String>emptyList())); // Fails because list now has one element in it
}

This is because Object.clone does not perform deep copies. It's only safe in this case because the fields cloned in EmpAddress are immutable (String and primitive int). Mutability Detector could try to recognise safe usages of .clone, but it would probably be very fragile. Because it can't be confident that your class is immutable, Mutability Detector decides it is mutable.

You are correct that creating a new Object of type EmpAddress is helping to make the class immutable, that's because it is "protecting" the instance, keeping it private, where no other code can access it. If the constructor took an instance and assigned it to a field, whoever passed the parameter to the constructor could modify it, thus mutating any instance that used it for a field. Like in this example:

@Test
public void mutate_object_by_giving_it_a_parameter_then_modifying_it() throws Exception {
    EmpAddress empAddress = new EmpAddress("New York", 1234);
    EmpAndAddress e = new EmpAndAddress(1234, "John Doe", empAddress);

    assertThat(e.geteAddr().getCity, is("New York"));

    empAddress.setCity("Haha, I'm mutating you");

    assertThat(e.geteAddr().getCity(), is("New York")); // fails because city has been changed
}

---

So, what to do about it? There are a couple of options.

Method 1: Override Mutability Detector because you know better

Change your test to add an "allowed reason" to be mutable. That is, you are satisfied the failure is a false positive, you want to catch other potential errors which introduce mutability, but ignore this case. To do this, add code like the following:

@Test
public void isImmutable_withAllowedReason() {
    assertInstancesOf(EmpAndAddress.class, areImmutable(),
            AllowedReason.assumingFields("eAddr").areNotModifiedAndDoNotEscape());
}

You should be very sure about this before you take this option, and if you are new to immutable objects, I would recommend not doing this, so that you can learn to create immutable objects more safely.

Method 2: Make EmpAddress immutable as well, like so:

@Immutable
public static final class ImmutableEmpAddress {
    private final String empCity;
    private final int zipCode;

    public ImmutableEmpAddress(String empCity, int zipCode) {
        this.empCity = empCity;
        this.zipCode = zipCode;
    }

    public String getEmpCity() { return empCity; }

    public int getZipCode() { return zipCode; }
}

Then when you return it as a field from EmpAndAddress you don't need to clone it. This would be an ideal solution.

Method 3: Create an immutable adapter

However, in some cases you can't make EmpAddress immutable. Maybe the code is in a different library, or it's used by a framework that needs to set fields on it with reflection (such as Hibernate, or other JavaBean libraries). In this case you could create an immutable adapter like so:

@Immutable
public static final class ImmutableEmpAddressAdapter {
    public final String empCity;
    public final int zipCode;

    public ImmutableEmpAddressAdapter(EmpAddress mutableAddress) {
        // perform a deep copy of every field on EmpAddress that's required to re-construct another instance
        this.empCity = mutableAddress.getEmpCity();
        this.zipCode = mutableAddress.getZipCode();
    }

    public EmpAddress getEmpAddress() {
        return new EmpAddress(this.empCity, this.zipCode);
    }
}

Then in EmpAndAddress could look like this:

public static final class EmpAndAddress {
    // some code ommitted for brevity

    private final ImmutableEmpAddressAdapter immutableEmpAddressAdapter;

    public EmpAndAddress(int empId, String empName){
        this.immutableEmpAddressAdapter = new ImmutableEmpAddressAdapter(new EmpAddress(" ", -1));
    }

    public EmpAddress geteAddr() {
        return immutableEmpAddressAdapter.getEmpAddress();
    }
}

Although this technique would require more code, it makes it very explicit to other readers of this class that EmpAddress has an immutable field, and doesn't rely on fragile behaviour of Object.clone

This is also a good technique if you want to make a class immutable, but you have to make many code changes to do it. This allows you to gradually introduce an immutable version in more and more places in a codebase until eventually the original mutable class is only used at the margins of your system, or even disappears entirely.

I hope this anwser, and Mutability Detector have been useful in learning how to create immutable objects.