RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v5]

[Stuart Marks]

On Fri, 26 Feb 2021 21:37:14 GMT, Stuart Marks <smarks at openjdk.org> wrote:

>> Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Test refactoring. Adding implNote to modified methods
>
> The `@implNote` additions are good, and the test rewrite looks good too.

Hm. I had thought of this previously but I was a bit suspicious, and it didn't seem like it would make much difference, so I didn't say anything. But thinking about this further, the following issues arose:

1. Suppose you have an UnmodifiableSortedSet and call unmodifiableSet() on it, it returns its argument, and then you hand it out. Its static type is Set but it's really a SortedSet, which means somebody can downcast it and get its comparator. This leaks some information. Offhand this doesn't look dangerous, but it's a bit of a surprise.

2. Thinking about this further, this allows heap pollution. (Ian, turns out our conversation from the other day wasn't just an idle digression.) If we have class X and class Y extends X, then `SortedSet<Y>` cannot safely be cast to an unmodifiable `SortedSet<X>`. That's because comparator() will return `Comparator<? super X>` which is incorrect, since the actual comparator might be of type `Comparator<Y>`. Actually the headSet(), subSet(), and tailSet() methods also cause problems, because they both consume and produce the collection element type E.

3. This can actually happen in practice with code in the JDK! PriorityBlockingQueue's copy constructor does exactly the above. It takes a Collection and does an instanceof check to see if it's a SortedSet; if it is, it does a downcast and uses its comparator. Thus if we do the following:

    SortedSet<Integer> set1 = new TreeSet<>(Integer::compare);
    Set<Number> set2 = Collections.unmodifiableSet(set1); // hypothetical version that returns its argument
    PriorityBlockingQueue<Number> pbq = new PriorityBlockingQueue<>(set2);
    pbq.addAll(Arrays.asList(1.0, 2.0));

this compiles without warnings, but it results in ClassCastException. The culprit is the new upcast that potentially allows `SortedSet<? extends T>` to be cast to `Set<T>`, which is slipped in under the already-existing warnings suppression.

In any case, the extra checking in the unmodifiableSortedSet and -Map methods needs to be taken out. Offhand I don't know if there's a similar issue between unmodifiableSortedSet and a NavigableSet (resp., Map), but on general principle I'd say to take it out too. It's likely not buying much anyway.

The UnmodifiableList and UnmodifiableRandomAccessList stuff should stay, since that's how the RandomAccess marker interface is preserved.

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

PR: https://git.openjdk.java.net/jdk/pull/2596