On Wed, 17 Feb 2021 14:21:37 GMT, Claes Redestad <redes...@openjdk.org> wrote:

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

2 remarks:
1. MapN's entry set extends abstract set, whose `contains` is null-friendly 
like 
https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/AbstractCollection.java#L104
2. The problem of unmodifiable map's entry set not always delegating everything 
to the backing entry set still exists. 
https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/Collections.java#L1724
 This will bypass the underlying logic when the argument is `null` or not an 
entry.

The behavior pointed out by michaelhixson is the conglomeration of these 2 
unspecified behaviors.

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

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

Reply via email to