Suggestion for a more sensible implementation of EMPTY_MAP

Stuart Marks stuart.marks at oracle.com
Fri Jun 14 00:42:29 UTC 2019



On 6/10/19 7:34 AM, Abraham Marín Pérez wrote:
> I agree that it makes sense for EMPTY_MAP to have the same logic as Map.of() or unmodifiable(new HashMap()), which means that my suggestion cannot be applied to just EMPTY_MAP... but maybe that’s ok: maybe we can change all of them? If we keep the default implementation for Map.computeIfPresent in EMPTY_MAP, Map.of(), unmodifiableMap, etc., instead of overriding it to throw an exception, then:
> 
> - For cases where the key isn’t present (regardless of empty or non-empt map), the call will be a safe no-op.
> - For cases where the key is present, the call will still throw UOE via the implementation of put()
> 
> I believe this would still respect the spec for Map.computeIfPresent while providing a more forgiving implementation (in the sense that it will avoid throwing an exception when possible).

Hi Abraham,

The specification does not mandate one behavior or another. Either behavior is 
permitted, and in fact both behaviors are present in the JDK.

The second and more significant point is raised by your last statement 
suggesting a "more forgiving implementation." The question is, do we actually 
want a more forgiving implementation?

The collections specs define certain methods as "optional", but it's not clear 
whether this means that calling such a method should always throw an exception 
("strict" behavior) or whether the method should throw an exception only when it 
attempts to do something that's disallowed ("lenient" behavior).

Take for example the putAll() method. What happens if we do this?

     Map<String, String> map1 = Collections.emptyMap();
     map1.putAll(otherMap);

The answer is that it depends on the state of otherMap. If otherMap is empty, 
then the operation succeeds (and does nothing). However, if otherMap is 
non-empty, then the operation will throw UnsupportedOperationException. That's 
an example of lenient behavior. What's notable about this is that you can't 
predict the outcome unless you know the state of otherMap.

Now, how about this example?

     Map<String, String> map2 = Map.of();
     map2.putAll(otherMap);

This always throws UnsupportedOperationException, regardless of the state of 
otherMap. I'd call this strict behavior.

More recent implementations, including the Java 8 default methods, and the Java 
9 unmodifiable collections (Map.of et al), have all preferred strict behavior. 
This is because less information about the state of the system is necessary in 
order to reason about the code, which leads to fewer bugs, or at least earlier 
detection of bugs. It's unfortunate that emptyMap().putAll() has this lenient 
behavior, but we're stuck with it for compatibility reasons.

Now there is a slight wrinkle with computeIfPresent(). If we have

     Map<String, String> map = Collections.emptyMap();
     map.computeIfPresent(key, remappingFunction);

then as you point out, this can never modify the map, since the key is never 
present. Thus, there isn't any behavior that's dependent upon additional state.

But, as Michael observed, there is now an inconsistency with Map.of() and 
Collections.unmodifiableMap(). So, continuing with your suggestion, we might 
change those implementations to allow computeIfPresent() as well. Thus,

     Map<String, String> map1 = Collections.emptyMap();
     Map<String, String> map2 = Map.of();
     Map<String, String> map3 = Collections.unmodifiableMap(new HashMap<>());

and then

     {map1,map2,map3}.computeIfPresent(key, remappingFunction);

all are no-ops. Well, maybe. What if we had retained a reference to the HashMap 
wrapped by the unmodifiableMap, and then we added an element? Now it contains a 
mapping; what should happen then? Should computeIfPresent() throw an exception 
because it *might* attempt to modify the map? Or should it throw an exception 
*only* if it attempts to modify the map? State-dependent behavior has slipped 
back in.

We also need to consider the impact on the other compute* methods. Consider:

     Map<String, String> map = Collections.emptyMap();
     map.compute(key, (k, v) -> null);

This can never change the map, but it currently throws UOE. Should it be changed 
to a no-op as well?

**

What we have now is strict rule: calling mutator methods on unmodifiable 
collections throws an exception. It's simple to describe and reason about, and 
it's (mostly) consistent across various collections. However, it prohibits some 
operations that sometimes people want to do.

If we were to make the system more lenient, or more forgiving, then weird 
inconsistencies and conditional behaviors pop up in other places. This makes the 
system more complicated and harder to reason about, and that leads to more bugs.

s'marks



More information about the core-libs-dev mailing list