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
