Add default overrides for getFirst and getLast in MutableList and Mut…#1461
Add default overrides for getFirst and getLast in MutableList and Mut…#1461donraab merged 1 commit intoeclipse-collections:masterfrom donraab:master
Conversation
...pse/collections/impl/map/ordered/mutable/UnmodifiableMutableOrderedMapSerializationTest.java
Outdated
Show resolved
Hide resolved
eclipse-collections-api/src/main/java/org/eclipse/collections/api/list/MutableList.java
Show resolved
Hide resolved
...e-collections-api/src/main/java/org/eclipse/collections/api/set/sorted/MutableSortedSet.java
Show resolved
Hide resolved
| + "SGFzaE1hcAUH2sHDFmDRAwACRgAKbG9hZEZhY3RvckkACXRocmVzaG9sZHhwP0AAAAAAAAB3CAAA\n" | ||
| + "ABAAAAAAeAA=", | ||
| UnmodifiableMutableOrderedMap.of(OrderedMaps.adapt(new LinkedHashMap<>()))); | ||
| } |
There was a problem hiding this comment.
The fact that there's no else block makes me wonder what happens in the new version. Is the serialized form changed? Does serialization now throw? Why?
There was a problem hiding this comment.
Yes the serialized form has changed. Something has changed in regards to serialization for LinkedHashMap but I do not have an easy way of determining what changed. I wanted to fix the builds for now and decide amongst the committers how best to handle the else situation, or remove the jdk version test altogether. We are not responsible for guaranteeing LinkedHashMap serialization, so we may want to rethink this test to guarantee our own serialization, not the JDKs.
I reported the issue to the OpenJDK Quality Outreach Program.
There was a problem hiding this comment.
@motlin I'm going to add a temporary serialization test just for LinkedHashMap so we can see the build failure and confirm it is unrelated to our types.
There was a problem hiding this comment.
@motlin As I expected, the JDK 21-ea build fails on a new serialization test for an empty LinkedHashMap.
Error: 9:951 [ERROR] UnmodifiableMutableOrderedMapSerializationTest.serializedFormLinkedHashMap:43 Serialization was broken. expected:<...zaE1hcDTATlwQbMD7AgA[BWgALYWNjZXNzT3JkZXJ4
cgARamF2YS51dGlsLkhhc2hNYXAFB9rBwxZg0QMAAkYACmxvYWRGYWN0b3JJAAl0aHJlc2hvbGR4
cD9AAAAAAAAAdwgAAAAQAAAAAHgA]> but was:<...zaE1hcDTATlwQbMD7AgA[CWgALYWNjZXNzT3JkZXJJ
AAdwdXRNb2RleHIAEWphdmEudXRpbC5IYXNoTWFwBQfawcMWYNEDAAJGAApsb2FkRmFjdG9ySQAJ
dGhyZXNob2xkeHA/QAAAAAAAAHcIAAAAEAAAAAB4AAAAAAA=]>
There was a problem hiding this comment.
Interesting! The only recent change to LinkedHashMap was to make it a SequencedCollection, and they did not change its serialVersionUID, so it's possible this is a bug upstream.
There was a problem hiding this comment.
it's possible this is a bug upstream.
I agree. Someone should report this.
There was a problem hiding this comment.
Hi all, JDK Sequenced Collections guy here. First, I hope I'm not causing you folks too much trouble. :-) @donraab brought this to my attention. Thanks Don!
Regarding this serialized form change, this was a mistake; there was no intent to change the serialized form. I've filed bug https://bugs.openjdk.org/browse/JDK-8309882 to restore to previous serialized form. It will likely be fixed in the next build of JDK 21.
That said, adding (and removing) fields from an object's serialized form is a compatible change; the behavior of "extra" and "missing" fields is well-defined in the serialization specification. We add or remove fields from time to time without changing the object's serialVersionUID. (Changing the serialVersionUID is incompatible and is almost impossible for clients to cope with.) That might mean this test is overly brittle, depending on what it's trying to do.
There was a problem hiding this comment.
Thanks for everything @stuart-marks! In the tests of the serialized forms of our wrapper classes, we're often wrapping JDK types. I agree with you that our tests are overly brittle. These tests have been quietly asserting that the serialized form doesn't change for empty ArrayLists, LinkedLists, HashSets, HashMaps, LinkedHashMaps, etc. for years. This is the first time I remember seeing a change to the serialized form, and I see that the addition of transient will bring back the old serialized form. I bet the serialized forms of these classes will never change. It might be helpful to add tests like these to the JDK! But if not, you can rely on us to ask about it when the form changes :)
While I have your attention, I have an unrelated question. Would it make sense to check for RandomAccess in List.getLast()?
default E getLast() {
if (this.isEmpty()) {
throw new NoSuchElementException();
} else if (this instanceof RandomAccess) {
return this.get(this.size() - 1);
} else {
return this.reversed().iterator().next();
}
}There was a problem hiding this comment.
Hi @motlin! Haven't talked in a while. Yeah, the serialized forms of collections haven't changed much if at all over time. Other classes like BigInteger have changed some though. And their internal representations have changed too, so there are several layers of compatibility code that handle mapping of different serialized forms to the current internal representation.
And yes, I'm surprised that nothing in the JDK tests caught this change. I know we've added checks to prevent some obvious mistakes (such as forgetting to add a serialVersionUID declaration) but apparently not accidental changes to serialized forms. I'll have to check into this.
On the getLast() default implementation... the JDK's LinkedList implementation -- which is the most common List implementation I'm aware of that doesn't implement RandomAccess -- will traverse from the end if the requested index is closer to the end. Thus access to size() - 1 will still be pretty much constant time. I suppose there might be some sequential list out there that doesn't know its size and that has to traverse from the front to find a particular index, but that would seem pretty unusual. I'm hard pressed to think of cases where doing a RandomAccess check will make a significant difference. If you're aware of any examples though, by all means let me know.
motlin
left a comment
There was a problem hiding this comment.
If you have the energy for it, I think we uncovered one or two issues worth reporting upstream. Either way, I think this looks like a good way to get compiling with all versions of Java again.
…ableSortedSet to Fixes #1460. Signed-off-by: Donald Raab <Donald.Raab@bnymellon.com>
…ableSortedSet to fix JDK 21 EA build failure