Hi,
I'm impressed you found that compatibility report. :-)
The most severe compatibility issues indeed occur if the keySet(), values(), and
entrySet() methods are overridden to have a covariant return type. Issues will arise
with subclasses that already override any of those methods, either with the return
types from Map, or with covariant overrides of their own.
It's true that bridge methods will be generated if this is done, and they take care
of the most common compatibility issues. However, there is a compatibility matrix to
be explored. My compatibility analysis considered old-binary, recompiled-source, and
modified-source cases for both a library that has an at-risk subclass as well as an
application that uses that library subclass. Some startling things emerged. What
pushed me over the edge to decide against covariant overrides (in this part of the
Sequenced Collections JEP) was that the behavior of a class could change silently
upon recompilation, even if the source code wasn't changed.
Aside from this issue, there's a more fundamental semantic issue with the object
design. If I have some ObservableMap implementation, presumably it provides a
keySet() implementation that's not observable. If ObservableMap is changed to have
the covariant overrides, this effectively imposes a new requirement that existing
implementations cannot possibly fulfill. Sure, you could tinker things around so
that things appear to work in some cases, but the semantics of doing this are highly
questionable.
Now JavaFX could decide go ahead with this anyway, for a couple reasons. One might
be, JavaFX doesn't care as much as the JDK about compatibility. It's (mostly) a
different project, and it might have different compatibility constraints. You folks
need to decide that. A second reason might be because there are vanishingly few or
zero implementations of ObservableMap "in the wild," so any incompatibility would
not cause any actual problems. This is difficult, but it's possible to get some
information by doing source code searches. (Unfortunately there appear to be several
libraries out there that have something called "ObservableMap" which will complicate
the analysis.)
The alternative is to add observableKeySet/Values/EntrySet() default methods instead
of covariant overrides. This is safer, but it does add some clutter to the API.
s'marks
On 10/26/22 1:24 PM, Nir Lisker wrote:
I'm CC'ing Stuart Marks who has recently dealt with a similar issue when working
on Sequenced Collections [1], and wrote a compatibility report [2] that includes
an item about covariant overrides ("Covariant Overrides of `SequencedMap` View
Collection Methods"), which is similar to what is discussed here. I contacted him
off list to get his insights into the risks involved here.
To recap, ObservableMap inherits keySet(), entrySet() and values() from Map, which
return the standard Set and Collection interfaces. ObservableMap should provide
ObservableSet and perhaps the not-yet-existing ObservableCollection. There are 2
options here: one is to add additional default methods to ObservableMap that
return observable collection, the second is to override the methods inherited from
Map and change the return value. The latter has some backwards compatibility
issues. It comes down to implementations of ObservableMap in the wild. I have yet
to see any, personally. JavaFX does not itself expose any of its implementations,
as ObservableMaps are obtained through FXCollections static methods.
I'd like to continue this discussion about the API side. I have already had some
advances on the implementation.
[1] https://openjdk.org/jeps/431
[2] https://bugs.openjdk.org/browse/JDK-8266572
On Tue, May 31, 2022 at 12:02 AM Nir Lisker <nlis...@gmail.com> wrote:
Then maybe a solution would be around adding new methods like
observableKeySet(). These will need to be default methods, and the
implementation could test if keySet() already returns an ObservableSet, in
which case it returns it, and if not it wraps the Set in an
ObservableSetWrapper or something like that.
On Mon, May 30, 2022 at 11:50 AM John Hendrikx <john.hendr...@gmail.com>
wrote:
Sorry, I misunderstood, I missed that the methods weren't already
defined in ObservableMap, so no existing signature is changed.
--John
On 30/05/2022 09:39, Tom Schindl wrote:
> Hi,
>
> Well the binary compat IMHO is not a problem. If your subtype
> overwrites the return type of a method the compiler will inserts a
> bridge method:
>
> Take this example
>
> package bla;
>
> import java.util.ArrayList;
> import java.util.Collection;
> import java.util.List;
>
> public class Test {
> public interface IB {
> public Collection<String> get();
> }
>
> public interface I extends IB {
> public List<String> get();
> }
>
> public class C implements I {
> public ArrayList<String> get() {
> return new ArrayList<String>();
> }
> }
> }
>
> if you look at C with javap you'll notice
>
> Compiled from "Test.java"
> public class bla.Test$C implements bla.Test$I {
> final bla.Test this$0;
> public bla.Test$C(bla.Test);
> public java.util.ArrayList<java.lang.String> get();
> public java.util.Collection get();
> public java.util.List get();
> }
>
>
> The problem is more that if someone directly implemented ObservableMap
> him/her self that it won't compile anymore. So it is a source
> incompatible change.
>
> Tom
>
> Am 30.05.22 um 08:58 schrieb John Hendrikx:
>> It's not binary compatible, as changing the return type results in a
>> new method that compiled code won't be able to find.
>>
>> See also "change result type (including void)" here:
>>
https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods
<https://urldefense.com/v3/__https://wiki.eclipse.org/Evolving_Java-based_APIs_2*Evolving_API_interfaces_-_API_methods__;Iw!!ACWV5N9M2RV99hQ!Jly4lMnm2UZQsTVKLfmhN7g0AHwp2nlUj4H4a-IfCIp4tqJXElDbEbDsVRhkL6Sa7l097FoQn8_Pi9YS$>
>>
>>
>> --John
>>
>> On 30/05/2022 03:22, Nir Lisker wrote:
>>> Hi,
>>>
>>> Picking up an old issue, JDK-8091393 [1], I went ahead and looked at
>>> the
>>> work needed to implement it.
>>>
>>> keySet() and entrySet() can both be made to return ObservableSet
rather
>>> easily. values() will probably require an ObservableCollection<E>
type.
>>>
>>> Before discussing these details, my question is: is it backwards
>>> compatible
>>> to require that these methods now return a more refined type? I
>>> think that
>>> it will break implementations of ObservableMap out in the wild if it
>>> overrides these methods in Map. What is the assessment here?
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8091393
