LinkedHashMap.clone()

[Irfan Adilovic]

[According to svn logs, this email would best be addressed to
"gwt.team.ecc" and "jat"]

I ran into a nasty problem where my code would run in development mode
but would die in production. I pinpointed the problem to a cast to
LinkedHashMap on the result of cloning an existing LinkedHashMap, with
the assumption that LinkedHashMap.clone() will be of the same type.
However, LinkedHashMap.clone() returnes a HashMap instance in
production.

Now, according to Java's Object.clone() javadoc, .clone() is not
obliged to return anything specific, there are only guidelines and
conventions, and because of this, strictly speaking, this is not a
bug. However, it is counter-intuitive and it is different from what
the Java implementation of LinkedHashMap.clone() does.

I went to the gwt sources (user/core/super/com/google/gwt/emul/java/
util) to see what causes the problem, and saw that LinkedHashMap
inherits HashMap's implementation of .clone() which --correctly in the
case of a HashMap-- returns a HashMap instance. The fix would be
trivial, by adding the equivalent code from HashMap with adaptation
for LinkedHashMap:

@Override
public Object clone() {
return new LinkedHashMap<K, V>(this);
}

I did this, and went to write a test, and found the following in trunk/
user/test/com/google/gwt/emultest/java/util/LinkedHashMapTest.java:

/*
* Test method for 'java.util.LinkedHashMap.clone()'
*/
// public void donttestClone() {
// LinkedHashMap srcMap = new LinkedHashMap();
// checkEmptyLinkedHashMapAssumptions(srcMap);
//
// // Check empty clone behavior
// LinkedHashMap dstMap = (LinkedHashMap) srcMap.clone();
// ... [5 commented-out lines of test code]
// // Check non-empty clone behavior
// ... [10 commented-out lines of test code]
// }

This seems to test the LinkedHashMap clone behavior, as if it were
implemented, but it is disabled. By analyzing the (luckily short) svn
log of the file with some svn diff -r bisecting, I identified that the
disabled code is there from the very first revision of the file:

gwt/trunk/user/test/com/google/gwt/emultest/java/util$ svn log
LinkedHashMapTest.java | tail
------------------------------------------------------------------------
r2609 | gwt.team.ecc | 2008-04-29 20:01:50 +0200 (Tue, 29 Apr 2008) |
2 lines

Adding LinkedHashMap and supporting tests and RPC serializers
Review by:jat (desk review)

gwt/trunk/user/test/com/google/gwt/emultest/java/util$ svn cat -r 2609
LinkedHashMapTest.java | grep -C2 donttest
* Test method for 'java.util.LinkedHashMap.clone()'
*/
// public void donttestClone() {
// LinkedHashMap srcMap = new LinkedHashMap();
// checkEmptyLinkedHashMapAssumptions(srcMap);

I have a feeling the code was added with the intention to be enabled,
but never was, as nobody came to actually "implementing"
LinkedHashMap.clone(). What's the official situation on this? Should I
file a bug and submit a patch?

-- Irfan Adilovic

[John Tamplin]

On Mon, Jan 23, 2012 at 10:51 PM, Irfan Adilovic <irfana...@gmail.com> wrote:

I have a feeling the code was added with the intention to be enabled,
but never was, as nobody came to actually "implementing"
LinkedHashMap.clone(). What's the official situation on this? Should I
file a bug and submit a patch?

I no longer remember the details of that change, but in general clone() is not supported in GWT, which is why there is no Object.clone() method (see http://code.google.com/p/google-web-toolkit/issues/detail?id=1843 for more details).  Particularly types can and do have Cloneable where it makes sense, so I think it is reasonable to implement it correctly here.

If you want to file a patch for LinkedHashMap, see http://code.google.com/webtoolkit/makinggwtbetter.html#contributingcode

--
John A. Tamplin
Software Engineer (GWT), Google

[Irfan Adilovic]

I would like to submit unit tests that test the fixes, but I cannot
bring the tests to fail with the unfixed version of LinkedHashMap (and
LinkedHashSet, which is affected in the same way, btw).

try {
dstMap = (LinkedHashMap<String, String>) srcMap.clone();
// fail("LinkedHashMap clone is castable to LinkedHashMap");
} catch (ClassCastException cce) {
fail("LinkedHashMap clone is not castable to LinkedHashMap");
return; /* dstMap wasn't cloned successfully */
}

By uncommenting the commented fail() above, I can verify that I am
running the correct test, as it fails there, but it should actually
fail on the second fail() in this snippet. The tested code acts as if
the .clone() implementation is already correct in LinkedHashMap/Set.
Because of this I cannot prove with a unit test, that my fix has an
effect.

However, with a very simple GWT application I can prove that the gwt-
user.jar built with my fix makes a difference.

private static final String PROD_UNMOD = "Production mode with
unmodified gwt-user.jar";
private static final String PROD_FIXED = "Production mode with fixed
gwt-user.jar";
private static final String DEV = "Development mode.";

public void onModuleLoad() {
LinkedHashSet<Integer> lhs = new LinkedHashSet<Integer>();
try {
lhs = (LinkedHashSet<Integer>) lhs.clone();
Window.alert("Clone succeeded. " + (GWT.isScript() ?
PROD_FIXED : DEV));
} catch (ClassCastException e) {
Window.alert("Clone failed. " + (GWT.isScript() ? PROD_UNMOD :
DEV));
}
}

When run with the unmodified gwt-user.jar, this code shows the
PROD_UNMOD string as expected. When running with the fixed gwt-
user.jar, it shows the PROD_FIXED string, as expected.

So I know that my fix works, and I know that my test code is being
run. What am I doing wrong? My ant line for running the tests is 'ant
build && ant test.emma.htmlunit' from within the trunk/user directory.

[John Tamplin]

On Sat, Jan 28, 2012 at 12:07 PM, Irfan Adilovic <irfana...@gmail.com> wrote:

I would like to submit unit tests that test the fixes, but I cannot
bring the tests to fail with the unfixed version of LinkedHashMap (and
LinkedHashSet, which is affected in the same way, btw).

   try {
     dstMap = (LinkedHashMap<String, String>) srcMap.clone();
     // fail("LinkedHashMap clone is castable to LinkedHashMap");
   } catch (ClassCastException cce) {
     fail("LinkedHashMap clone is not castable to LinkedHashMap");
     return; /* dstMap wasn't cloned successfully */
   }

By uncommenting the commented fail() above, I can verify that I am
running the correct test, as it fails there, but it should actually
fail on the second fail() in this snippet. The tested code acts as if
the .clone() implementation is already correct in LinkedHashMap/Set.
Because of this I cannot prove with a unit test, that my fix has an
effect.

Are you running your test in DevMode?  In DevMode, you are using your JVM's real JRE classes, not GWT's emulation.  So, you need to run in prod mode (ie, compiled to JS) to test GWT's JRE emulation.