Java's clone mechanism is teh suq

Doing cloning properly in Java is way harder than it should be. I thought I knew all the traps, but recently found yet another. In a nutshell, making inner classes cloneable is almost always a bad idea. The reason is that inner classes have an internal reference to an instance of their enclosing class, which is implicitly final and can't be changed as part of a clone. For example:

class Outer implements Cloneable
{
    int i = 0;
    Inner inner = new Inner();

    public Object clone()
    {
        try
        {
            Outer clone = (Outer)super.clone();
            clone.inner = (Inner)inner.clone(); // A.
                 Cloned inner still refers to this Outer instance
            return clone;
        }
        catch (CloneNotSupportedException e)
        {
        }
    }

    class Inner implements Cloneable
    {
        int j = 0;
        
        public Object clone()
        {
            try
            {
                return super.clone();  // B. WARNING!
                      - this does the Wrong Thing
            }
            catch (CloneNotSupportedException e)
            {
            }
        }

        void inc()
        {
            i++;
            j++;
        }
    }
}

This code is attempting to do a deep clone of an Outer instance, which requires a deep clone of the Inner it references. The problem occurs at B, because the object returned by super.clone refers to the same Outer instance as the original. So if you do this:

Outer outer1 = new Outer();
Outer outer2 = (Outer)outer1.clone();
outer2.inner.inc();  // will change i in outer1 instead of outer2
System.out.println(outer1.i);
System.out.println(outer2.i);

You get:

1
0

Because the cloned outer2.inner has an internal reference to the original outer1 instance. There may be cases where you want it to work that way, but it seems very unlikely. So what to do? Calling super.clone() from an inner class is basically broken, and there's no way to fix it. If you're able to make the inner class final, this can be avoided relatively easily by using a copy constructor to clone the inner class instead of Object.clone:

class Outer implements Cloneable
{
    int i = 0;
    Inner inner = new Inner();

    public Object clone()
    {
        try
        {
            Outer clone = (Outer)super.clone();
            clone.inner = clone.new Inner(inner);  // C.
                  Use "qualified" copy constructor
            return clone;
        }
        catch (CloneNotSupportedException e)
        {
        }
    }

    final class Inner  // no longer Cloneable
    {
        int j = 0;
        
        Inner(Inner inner)
        {
            j = inner.j;
        }

        void inc()
        {
            i++;
            j++;
        }
    }
}

This gives the desired output of:

0
1

Check out the change at line C, it's actually valid syntax. It's called a "qualified" new, where you explicitly specify the enclosing instance of the new inner class instance.

Using copy constructors for cloning is only OK for final classes, because they aren't polymorphic. If we had sublcasses of Inner involved here, then it gets harder - Object.clone is the only easy way to get the right class instantiated. Idea will actually give a warning on line C for this reason.

Another, possibly better, solution is to make Inner a standard (non-inner) class with an explicit, non-final reference to its Outer instance. Basically, do the inner class stuff by hand. Because the reference to the outer class isn't final, it can be fixed up before returning from clone(). Of course, this lacks to convenience that was the motivation for inner classes in the first place.

More info on Java's cloning woes: http://www.adtmag.com/java/articleold.asp?id=364.