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

[Claes Redestad]

On Wed, 17 Feb 2021 00:30:09 GMT, Michael Hixson <github.com+297150+michaelhixson at openjdk.org> wrote:

>> Modify the `unmodifiable*` methods in `java.util.Collections` to be idempotent. That is, when given an immutable collection from `java.util.ImmutableCollections` or `java.util.Collections`, these methods will return the reference instead of creating a new immutable collection that wraps the existing one.
>
> src/java.base/share/classes/java/util/Collections.java line 1473:
> 
>> 1471:     public static <K,V> Map<K,V> unmodifiableMap(Map<? extends K, ? extends V> m) {
>> 1472:         if(m.getClass() == UnmodifiableMap.class ||
>> 1473:            m.getClass() == ImmutableCollections.Map1.class ||
> 
> (I'm not a reviewer.)
> 
> I think this causes a change in behavior to this silly program.
> 
> var map1 = Map.of(1, 2);
> var map2 = Collections.unmodifiableMap(map1);
> 
> try {
>   System.out.println("map1 returned " + map1.entrySet().contains(null));
> } catch (NullPointerException e) {
>   System.out.println("map1 threw");
> }
> 
> try {
>   System.out.println("map2 returned " + map2.entrySet().contains(null));
> } catch (NullPointerException e) {
>   System.out.println("map2 threw");
> }
> 
> With JDK 15 the output is:
>> map1 threw
>> map2 returned false
> 
> With this change I think the output will be:
>> map1 threw
>> map2 threw
> 
> It seems unlikely that anyone will be bit by this, but it is a change to behavior and it wasn't called out in the Jira issue, so I felt it was worth mentioning.
> 
> I think it is just this one specific case that changes -- only `Map1`, and only `entrySet().contains(null)`.  Other sub-collections like `keySet()` and `values()` and `subList(...)` already throw on `contains(null)` for both the `ImmutableCollections.*` implementation and the `Collections.umodifiable*` wrapper.  `MapN`'s `entrySet().contains(null)` already returns `false` for both.

This sounds like an inconsistency between `Map1` and `MapN` that should perhaps be considered a bug that needs fixing. /ping @stuart-marks

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

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