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

[Ian Graves]

On Thu, 4 Mar 2021 03:57:35 GMT, Stuart Marks <smarks at openjdk.org> wrote:

>> 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.

Good thought on the heap pollution. I had only been considering point 1. I was thinking that perhaps if we could catch some of the wrapping of subclasses we might be able to guard against situations where rewrapping could occur if we were interleaving calls between subclass and superclass wrapper methods. That seems like a bit of a reach and in light of your point about heap pollution I think it makes sense to walk back the changes and stay with the original.

Likewise agree on the point about Lists.

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

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