RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]

[Nir Lisker]

On Sun, 1 Jan 2023 15:04:17 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalContentBinding.java line 81:
>> 
>>> 79:         if ((obj1 instanceof ObservableList<?> list1) && (obj2 instanceof ObservableList<?> list2)) {
>>> 80:             @SuppressWarnings("unchecked")
>>> 81:             final ListContentBinding<Object> binding = new ListContentBinding<>((ObservableList<Object>) list1, (ObservableList<Object>) list2);
>> 
>> Although the previous code has the same problem, this is sketchy. The two lists can be of different types while `ListContentBinding` requires the same type. This is a result of the `Bindings` public API that takes two `Objects`, so all type information is lost. Is it worth adding a comment about this since suppressing the warning can be understood as "trust me, this is fine".
>
> What would go wrong if they're not the same type?  `ListContentBinding` doesn't (and can't) enforce it and doesn't care either way.  The whole function fails silently if types don't match.  Also `ListContentBinding` is a private class and so I'd expect this code to be aware of how it works and what is/isn't safe to do. 
> 
> I personally think this entire class is unfinished.  It fails miserably in edge cases without so much as a warning to the user. Take this for example:
> 
>     public static void main(String[] args) {
>         ObservableList<String> a = FXCollections.observableArrayList();
>         ObservableList<String> b = FXCollections.observableArrayList();
>         Bindings.bindContentBidirectional(a, b);
>         Bindings.bindContentBidirectional(a, b);
>         a.add("A");
>         System.out.println(a + " : " + b);
>     }
> 
> Prints:
> 
>     [A, A, A, A] : [A, A, A]
> 
> No mention about this in the API docs at all.  It breaks even worse when you make circular bindings `[a,b], [b,c], [c,a]` (stack traces get logged to the console complaining about `IndexOutOfBoundsException`).
> 
> I've created a solution that rejects double bindings and circular bindings, but it's a bit out of scope for this.  I think however that it is worth adding, and considering that the current behavior is broken when doing any of such things, not a big API break if instead we throw an exception.

You are right, nothing would go wrong.

I agree that the behavior is undesired and should be fixed in another issue. I was thinking of adding more specific overloads that make sense to the public API and deprecating the method that takes everything, making it throw.

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ContentBinding.java line 89:
>> 
>>> 87:             ListContentBinding<Object> binding = new ListContentBinding<>((List<Object>) list1);
>>> 88: 
>>> 89:             list2.removeListener(binding);
>> 
>> Another problem inherited from the existing code. What if the `obj2` is a `List` and `obj1` is an `ObservableList`? The docs don't say anything about the order.
>> 
>> Same question as before about adding a comment addressing the case that the two lists are not of the same type.
>
> Yes, looks like this is quite broken. This wouldn't have gone unnoticed so long if unbind would just throw an exception when nothing could be unbound; silently failing is never a good idea.

Can you file an issue for this if it's not filed already?

-------------

PR: https://git.openjdk.org/jfx/pull/972