JDK 8 code review request for 7140820 Add covariant overrides to Collections clone methods

[Joe Darcy]

Hello,

Follow-up below...

On 01/30/2012 09:10 AM, Joe Darcy wrote:
> On 01/29/2012 10:52 PM, Rémi Forax wrote:
>> On 01/30/2012 04:58 AM, Joe Darcy wrote:
>>> Hello,
>>>
>>> As an indirect outgrowth of warnings cleanup day, various categories 
>>> of warnings can be eliminated by in the use of Cloneable types by 
>>> overriding the
>>>
>>>     Object clone()
>>>
>>> method inherited from java.lang.Object with a covariant override 
>>> such as
>>>
>>>     MyType clone()
>>>
>>> Please review my changes for
>>>
>>>     7140820 Add covariant overrides to Collections clone methods
>>>     http://cr.openjdk.java.net/~darcy/7140820.0/
>>>
>>> which add such covariant override clone methods to collections and a 
>>> few other classes in java.util.  Doing a full JDK build with these 
>>> changes, I've also made alterations to other classes to remove now 
>>> superfuous casts (casts which are a javac lint warning!) and some 
>>> unneeded @SuppressWarnings annotations.  I also cleaned up a few 
>>> rawtypes warnings while in editing files in java.util.
>>>
>>> (Note that the old specListeners method in EventRequestSpecList.java 
>>> was much buggy; it cast an ArrayList from runtime.specListeners to a 
>>> Vector.)
>>>
>>> Thanks,
>>>
>>> -Joe
>>
>> WTF !
>>
>> while it's maybe a binary compatible change, I haven't take a look to 
>> all modified classes to be sure
>> it's not a source compatible change.
>
> Adding the covariant override is a binary compatible change because 
> there would be a bridge method providing the original "Object clone()" 
> signature.
>
>>
>> People had created class that inherits from ArrayList and override 
>> clone,
>> while it's not a good practice, there is a *lot* of code from pre-1.5 
>> days that does exactly that,
>> this change will simply break all those codes.
>>
>
> *sigh*
>
> Yes, I didn't fully consider the source compatibility implications 
> given that these types can be subclasses; I'll look this over some more.
>
> (This was meant to be part of a larger effort to review of potentially 
> overridable clone methods in the JDK.  I wrote an annotation processor 
> to look over the code base to find potential classes to upgrade; I can 
> refine the processor to look for classes that don't override clone and 
> are also final or effectively final, having no accessible constructors.)
>
> Thanks Remi,
>
> -Joe

I've looked into the source compatibility aspects of this change.  As a 
simplification, I considered two versions of the parent Cloneable class:

     Parent A: Object clone()
     Parent B: Parent clone()

and three versions of the Child with an explicit clone method:

     Child A: Object clone()
     Child B: Parent clone()
     Child C: Child clone()

The Child.clone implementations dutifully calls super.clone.  Using 
javac from JDK 7u2 I compiled each version of Child with the proper 
version of Parent on the classpath as a class file.  The results are:

Parent A    Child A    compiles
Parent B    Child A    doesn't compile // Cannot override the bridge method

Parent A    Child B    compiles [* see behavioral compatibility note below]
Parent B    Child B    compiles

Parent A    Child C    compiles
Parent B    Child C    compiles

All terms of binary compatibility, the continued ability to link, each 
version of Child when compiled against Parent A, linked against either 
Parent A or Parent B.  In other words, adding the covariant overrides in 
Parent is a binary compatible change.

In terms of behavioral compatibility, each version of Child when 
compiled against Parent A, ran against either Parent A or Parent B 
*except* for Child B compiled against Parent A and run against Parent B, 
which gave a stack overflow error.

In the problematic case, we have the new expected clone methods in the 
class file of Parent B:

   public Parent clone();
     flags: ACC_PUBLIC
     ...

   public java.lang.Object clone() throws 
java.lang.CloneNotSupportedException;
     flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
     Code:
       stack=1, locals=1, args_size=1
          0: aload_0
          1: invokevirtual #10                 // Method clone:()LParent;
          4: areturn
       LineNumberTable:
         line 1: 0
     Exceptions:
       throws java.lang.CloneNotSupportedException

and in the class file of Child B compiled against Parent A

   public Parent clone();
     flags: ACC_PUBLIC
     Code:
       stack=2, locals=2, args_size=1
          0: aload_0
          1: invokespecial #2                  // Method 
Parent.clone:()Ljava/lang/Object;
     ...

   public java.lang.Object clone();
     flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
     Code:
       stack=1, locals=1, args_size=1
          0: aload_0
          1: invokevirtual #8                  // Method clone:()LParent;
          4: areturn
       LineNumberTable:
         line 1: 0

So, what happens executing the seemingly innocent code

      Object o = (new Child()).clone();

that gets compiled to byte code

              7: invokevirtual #8                  // Method 
clone:()LParent;

is that the "Parent clone()" method on Child gets called.  This in turns 
calls the "Object clone()" method on Parent.  The "Object clone" method 
in Parent is a bridge method and does an invoke virtual on "Parent 
clone", which starts the cycle all over.  In the end the following stack 
trace is generated:

     at Parent.clone(Parent.java:1)
     at Child.clone(Child.java:12)
     at Parent.clone(Parent.java:1)
     at Child.clone(Child.java:12)
     ...

So, as long as the Child class either:

* Didn't override clone at all
* Overrode clone to return Child

then all is well.  There are different problems if Child overrode clone 
to return Object or to return Parent.

The JDK generally doesn't promise 100% source compatibility between 
major releases so the Parent B + Child A scenario doesn't necessarily 
disqualify this change.  (I think much more code would benefit from the 
covariant return on core collections than be harmed by it.)

Having Child override clone to return Parent would be weird and 
presumably rare.

So adding the covariant overrides of clone has a larger compatibility 
impact than I expected, but I'm still considering going cautiously 
forward with the work.  If it does go forward, I'd grudgingly 
acknowledge it might have to be backed out later in the release if there 
was too large a compatibility impact in practice.

-Joe