On Mon, 19 Oct 2026 19:07:38 GMT, Srikanth Palutla <[email protected]> wrote:
>> Problem >> new PriorityQueue<>(List.of(new Object())) succeeds today when it should >> throw ClassCastException. initFromCollection relies on heapify() to surface >> incomparable elements, but heapify performs no comparisons on a singleton, >> so the lone element is never validated. Collections with two or more >> non-Comparable elements already throw. >> >> Fix >> In initFromCollection, when the collection holds exactly one element and no >> comparator is in play, self-compare the element before heapifying. This >> mirrors what TreeMap does for its first key (see JDK-5045147). Behavior with >> an explicit comparator is unchanged: non-Comparable elements are still >> accepted when a comparator is provided. >> >> Testing >> Added a copyConstructorSingleton regression test to AddNonComparable >> covering PriorityQueue(Collection) and PriorityQueue(Collection, Comparator) >> with a null comparator, plus the positive cases (Comparable singleton, >> explicit comparator). Verified the new test passes against the patched class >> and the existing queues() test still passes. >> >> >> Please let me know for further questions >> >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Srikanth Palutla has updated the pull request incrementally with one > additional commit since the last revision: > > Simplify singleton check per review: instanceof + explicit CCE I have had a look at the JBS issue and the issue is reproducible even on Java 8. So I've updated the affects version of it. I've also updated the JBS title to remove the mention of "copy constructor". Please update the title of this PR to match the current JBS issue title. Finally, given the area of this change it would be good to have Stuart's input about this proposed change. I think a CSR will also be needed (but that can be done later once the changes and discussion here have settled down). test/jdk/java/util/PriorityQueue/AddNonComparable.java line 86: > 84: > 85: @Test > 86: public void copyConstructorSingleton() { As far as I know, "copy constructor" has a different meaning. I have updated the JBS issue to remove the mention of "copy constructor". I think this test method should also be renamed to something like "testSingleElementConstruction". test/jdk/java/util/PriorityQueue/AddNonComparable.java line 87: > 85: @Test > 86: public void copyConstructorSingleton() { > 87: // 8390157: copy constructors must reject a non-Comparable element Hello @sripal2013, we don't add comment referencing to JBS issues. The `@bug` usage in the test definition is the only place we refer to those bug ids. You can remove this code comment and instead add a brieft comment to the test method itself. The test method comment can state what it is testing. test/jdk/java/util/PriorityQueue/AddNonComparable.java line 91: > 89: List<NonComparable> singleton = List.of(new NonComparable()); > 90: assertThrowsCCE(() -> new > PriorityQueue<NonComparable>(singleton)); > 91: assertThrowsCCE(() -> new PriorityQueue<NonComparable>(singleton, > null)); There's also a constructor which takes a `SortedSet` as input. Can you also add test here for that constructor? ------------- PR Comment: https://git.openjdk.org/jdk/pull/32966#issuecomment-6766015476 PR Review Comment: https://git.openjdk.org/jdk/pull/32966#discussion_r4524206977 PR Review Comment: https://git.openjdk.org/jdk/pull/32966#discussion_r4524186462 PR Review Comment: https://git.openjdk.org/jdk/pull/32966#discussion_r4524222188
