Andreas Schaefer's Blog

Cloneable: How an old Bug can bite for a very long time

I cannot remember when I complained about the missing public Object clone() method in the java.lang.Cloneable interface for the first time but in the latest release of Java (1.5) this bug hurts us developers even more.

Assume we want to create our very own list that allows deep copies when cloned. In 1.5 the code could look like this (disclaimer: I am not a 1.5 expert so bare with me if the syntax if wrong):

ArrayList<Cloneable> myCloneableList = new ArrayList<Cloneable>();
ArrayList<Cloneable> myDeepCopiedList = (ArrayList<Cloneable>)
      myCLoneableList.clone(); // Assuming the method is overwritten and public

public Object clone() {
   // Create the list here
   lNewList = new ArrayList<Cloneable>();
   Iterator i = getIterator();
   while(i.hasNext()) {
      lNewList.add(((Cloneable) i.next()).clone());
   }
   return lNewList;
}

Comments

• Whoa!
  Cloning a container should NEVER do a deep-copy.

  Simple; if you want to do a deep-copy you should _know_ beforehand that each child is clonable. If not; don't clone it.

  You could naturally implement the inner while like this:

  while(i.hasNext()) {
  Object o = i.next();
  if(o instanceof Clonable)
  o = ((Clonable) o).clone();

  lNewList.add(o);
  }

  But I find that very distastefull to begin with..

  Posted by: zander on October 13, 2004 at 12:44 AM

• Hello,

  Maybe this will be helpfull:

  List cloneableLine = new ArrayList();
  Cloneable[] cloneableArray = cloneableLine.toArray();
  Cloneable[] otherArray = cloneableArray.clone();

  Thanks!

  Posted by: aefimov on October 13, 2004 at 02:26 AM

• Sorry, previous post filtered:

  List list = new ArrayList();
  Cloneable[] array1 = list.toArray();
  Cloneable[] array2 = array1.clone();

  
  Thanks!

  Posted by: aefimov on October 13, 2004 at 02:29 AM

• Whoa!
  Sorry, but if you read my post carefully then you have seen that exactly what you propose is not possible beause the Cloneable interface does not contain method clone().
  By the way why should a container not be allowed to do a deep copy? A shallow copy cannot localize the changes of objects in one list from being visible in the other.

  Posted by: schaefa on October 13, 2004 at 07:35 AM

• Also from JavaDocs for Object.clone(), with emphasis added:

  By convention, the object returned by this method should be independent of this object (which is being cloned). To achieve this independence, it may be necessary to modify one or more fields of the object returned by super.clone before returning it. Typically, this means copying any mutable objects that comprise the internal "deep structure" of the object being cloned and replacing the references to these objects with references to the copies. If a class contains only primitive fields or references to immutable objects, then it is usually the case that no fields in the object returned by super.clone need to be modified.

  So, the deep copy isn't necessarily bad when it's even a recommendation (although not exactly a requirement, it seems).

  The only real workaround I know is to use reflection and hope for the best.
  

  Posted by: tjpalmer on October 13, 2004 at 08:38 AM

• They should reopen http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4098033.

  According to the evaluation, adding public Object clone() to the interface would not break binary compatibility but rather source-compatibility where some classes implement Cloneable but don't actually define clone().

  I think their reasoning is flawed because any class that implements Cloneable but does not implement clone() is buggy to begin with and *should not* be supported! It breaks the Cloneable contract and as such we don't have to (nor should we) support it.

  How do we get Sun to reopen this issue?

  Posted by: cowwoc on October 13, 2004 at 10:30 AM

• Ok, I missed that the Clonable interface has no methods, which naturally means the Interface is useless.
  The bug then is that it should implement that method, like cowwoc wrote.

  To answer your question why a clone on a list should not do deep copies: you can't predict what will happen with it unless you KNOW that these objects actually allow deep copies for good reasons.
  Let me give an example; your object is a Person, which is persistent in one way or another (properties file, database I dont care). Cloning and changing that object will lead to wierd results since saving one copy of the Person and then later saving another copy of the cloned Person object will overwrite all changes of the first save. And thats in case everything goes correct in your persistance layer! Cloning of persons is a bad idea.

  Naturally there are loads of objects where cloning is a good idea; most of these objects are small and a good selection are immutable objects. For those objects its a good idea to let them actually implement the clonable interface anyway. But thats not the point. If you clone a Vector you get a new Vector which contains all the old objects. I cloned the Vector, not each objects inside. From an OO perspective that is exactly what I expect. I mean;
  what do you EXPECT to happen in this case:
  Iterator iter = myVector.clone().iterator();
  while(iter.hasNext() {
  Bill bill = (Bill) iter.next();
  if(bill.isPayed()) myVector.delete(bill);
  }
  What you expect is not what you get if you would do deep copying.

  Posted by: zander on October 13, 2004 at 10:56 AM

• I do not think that a deep copy is the problem in your code snippet:

  Iterator iter = myVector.clone().iterator();
  while(iter.hasNext() {
  Bill bill = (Bill) iter.next();
  if(bill.isPayed()) myVector.delete(bill);
  }

  but more the fact that the vector is cloned before the interator is obtained. You can make it even worse with:

  Iterator iter = myVector.clone().iterator();
  while(iter.hasNext() {
  Bill bill = (Bill) iter.next();
  if(bill.isPayed()) myVector.clone().delete(bill);
  }

  
  Nevertheless I am not advocating that suddenly all clone() methods on containers do a deep copy. I am advocating that the Clonable interface provides the means to create a deep copy if you like so that the developer can choose and pick.

  Currently it is not possible to create a deep copy without using serialization or reflection if I need a container that is completely independent of the source.

  Also your example of the cloned person is not valid in my opinion because this only applies to a single JVM. Most likely in the case of a persistent object you could have multiple users working on the same data object and then you need to make sure that a record based on old data is not overwritting new data. There you have to use a record timestamp (Oracle) to prevent this. This is an issue of persistence and not of a shallow or deep copy.

  Posted by: schaefa on October 13, 2004 at 11:35 AM

• Off topic, but fun anyway...
  In Java 5 you can now say:

  
  public Cloneable clone() {
  // ...
  }

  or, in a less trivial/more useful fashion:

  
  public class Bill {

  // ...

  public Bill clone() {
  // ...
  }
  }

  
  thanks to covariance support.

  
  R.J.

  Posted by: rjlorimer on October 13, 2004 at 02:52 PM

• There is a deeper problem with deep clones. Suppose you are cloning a list which has two or more references to a given object. Then in the cloned structure the same property should remain. This is trivial with shallow clones, but for deep clones we need to pass some additional information to the 'clone' method --- a map of original objects to clones.
  So rather then changing the Cloneable interface we need some new interfaces. There was a proposal for a Copyable interface which did declare a clone method, I don't know what happened to that. Supporting deep clones of non trivial structures would require a further interface.

  Posted by: mthornton on October 14, 2004 at 02:11 AM

This work is licensed under a Creative Commons License.