On Fri, 26 Feb 2021 21:37:14 GMT, Stuart Marks <sma...@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
