In this reply I'll focus on the null handling issues in collections.
As you've noted, there are really (at least) two distinct issues here: whether a
collection permits nulls, and the behavior of contains(null) queries. There have
been continual complaints about both, and the issues are somewhat distinct.
The collection implementations in the JDK are all over the place with regard to
nulls. I believe this is because the original collections and the concurrent
collections up through JDK 1.6 or so were mostly done by Josh Bloch and Doug Lea,
who disagreed about null handling. They had this exchange in 2006:
Doug Lea wrote: [1]
The main reason that nulls aren't allowed in ConcurrentMaps
(ConcurrentHashMaps, ConcurrentSkipListMaps) is that
ambiguities that may be just barely tolerable in non-concurrent
maps can't be accommodated. The main one is that if
map.get(key) returns null, you can't detect whether the
key explicitly maps to null vs the key isn't mapped.
In a non-concurrent map, you can check this via map.contains(key),
but in a concurrent one, the map might have changed between calls.
Further digressing: I personally think that allowing
nulls in Maps (also Sets) is an open invitation for programs
to contain errors that remain undetected until
they break at just the wrong time. (Whether to allow nulls even
in non-concurrent Maps/Sets is one of the few design issues surrounding
Collections that Josh Bloch and I have long disagreed about.)
Josh Bloch wrote: [2]
I have moved towards your position over the years. It was probably a
mistake to allow null keys in Maps and null elements in Sets. I'm
still not sure about Map values and List elements.
In other words, Doug hates null more than I do, but over the years
I've come to see it as quite troublesome.
(I haven't talked to Josh or Doug about this particular issue recently, so I suppose
it's possible their opinions have changed in the intervening time.)
I had to decide what to do about nulls when I added the unmodifiable collections in
JDK 9. Given that Doug "hates" nulls, and Josh finds them at least quite
troublesome, I started from a position of disallowing null members in all the new
collections. Most of the unmodifiable collections are still this way (but see below).
What about contains(null)? Surely it should be ok to query for a null, and return
false, even if the collection itself can't contain null. However, a bunch of
collections and collection views in the JDK throw NPE on contains(null), so the
unmodifiable collections don't set a precedent here.
If you're dealing with all non-null values, and a null creeps in somehow, then
calling contains(null) might be an error, and it would be good for the library to
inform you of that instead of just returning false and letting the program continue
until it fails later (fail-fast). A similar issue arises with the non-generic
contains() method. If you have a Collection<Integer>, then shouldn't contains("abc")
be an error? It's not, and it simply returns false, because the signature is
contains(Object). But there have been complaints that this should have been an error
because a Collection of Integer cannot possibly contain a String. Similar reasoning
applies to contains(null) on a collection that can't contain null. (Yes, the error
occurs at runtime instead of compile time, but better late than never.)
A meta-issue is: should a new thing correct a perceived design flaw in the older
thing, or should it be consistent with the old thing? Either way, you lose.
Another factor in my original decision was that it's much easier to start with a
restriction and relax it later than it is to go the other way. We have some
flexibility to allow nulls and contains(null) if we want; but it's essentially a
non-starter to disallow nulls once they've been allowed somewhere.
That's why my starting position for the design prohibited nulls everywhere.
Over time we've relaxed some restrictions around null handling in the unmodifiable
collections. Streams permit nulls, and the List returned from Stream.toList() also
permits nulls, and it also allows contains(null). So there's a different
unmodifiable List implementation under there, accessible only via streams. (Note
that the null-permitting unmodifiable lists lose the optimizations for sizes 0, 1,
and 2, which are in part based on nulls being disallowed.)
I'm fairly sympathetic to the case for changing the unmodifiable collections to
allow contains(null), given that the current NPE-throwing behavior has generated a
fair amount of complaints over the years. And the situation with having two flavors
of unmodifiable list is quite odd and is an uncomfortable point in the design space.
I should point out however that even if the unmodifiable collections were changed to
allow contains(null), it's still kind of unclear as to whether this code is safe:
public void addShoppingItems(Collection<String> shoppingItems) {
if (shoppingItems.contains(null)) { // this looks quite reasonable and
safe...
throw new IllegalArgumentException("shoppingItems should not contain
nulls");
}
...
}
If you're only ever passing ArrayList, Arrays.asList(), and List.of() lists here,
then sure, this would work great. If the source collection is of unknown provenance,
you can still run into trouble:
1. contains(null) calls on the keySet and values collections of ConcurrentHashMap
and Hashtable all throw NPE. So does contains(null) on a set from
Collections.newSetFromMap(), which is built from those maps' keySets. (I suppose
Doug Lea might be convinced to relax the CHM behavior.)
2. Speaking of concurrency, if the source collection is being modified concurrently,
then a null could be introduced after the check (TOCTOU) so you'd need to make a
defensive copy before checking it.
3. NavigableSet and friends delegate contains() to the comparison method, which
might or might not accept nulls. There's no way the collection implementation can
tell in advance.
You might consider List.copyOf() as an alternative, as that makes a null-free
defensive copy in one shot, and avoids copying if the source is an unmodifiable
list. If you don't care about concurrency, then
shoppingItems.forEach(Objects::requireNonNull)
might be suitable.
If unmodifiable collections were to change to allow contains(null), this probably
would make them somewhat less annoying, but it wouldn't necessarily solve yours or
everyone's problems. There's simply no way at this point to make the collections'
treatment of nulls uniform.
s'marks
[1]
https://web.archive.org/web/20210510190135/https://cs.oswego.edu/pipermail/concurrency-interest/2006-May/002485.html
[2]
https://web.archive.org/web/20200930213909/http://cs.oswego.edu/pipermail/concurrency-interest/2006-May/002486.html
On 1/29/23 7:28 AM, John Hendrikx wrote:
TLDR; why does contains(null) not just return false for collections that don't allow
nulls. Just because the interface allows it, does not mean it should do it as it
devalues the usefulness of the abstraction provided by the interface.
Background:
I'm someone that likes to check correctness of any constructor or method parameter
before allowing an object to be constructed or to be modified, in order to maintain
invariants that are provided by the class to its users.
This ranges from simple null checks, to range checks on numeric values, to complete
checks like "is a collection sorted" or is a list of nodes acyclic. Anything I can
check in the constructor that may avoid problems further down the line or that may
affect what I can guarantee on its own API methods. For example, if I check in the
constructor that something is not null, then the associated getter will guarantee
that the returned value is not null. If I check that a List doesn't contain nulls,
then the associated getter will reflect that as well (assuming it is immutable or
defensivily copied).
For collections, this is currently becoming harder and harder because more and more
new collections are written to be null hostile. It is fine if a collection doesn't
accept nulls, but throwing NPE when I ask such a collection if it contains null
seems to be counter productive, and reduces the usefulness of the collection
interfaces.
This strict interpretation makes the collection interfaces harder to use than
necessary. Interfaces are only useful when their contract is well defined. The more
things an interface allows or leaves unspecified, the less useful it is for its users.
I know that the collection interfaces allow this, but you have to ask yourself this
important question: how useful is an interface that makes almost no guarantees about
what its methods will do? Interfaces like `List`, `Map` and `Set` are passed as
method parameters a lot, and to make these useful, implementations of these
interfaces should do their best to provide as consistent an experience as is
reasonably possible. The alternative is that these interfaces will slowly decline in
usefulness as methods will start asking for `ArrayList` instead of `List` to avoid
having to deal with a too lenient specification.
With the collection interfaces I get the impression that recently there has been too
much focus on what would be easy for the collection implementation instead of what
would be easy for the users of said interfaces. In my opinion, the concerns of the
user of interfaces almost always far outweigh the concerns of the implementors of
said interfaces.
In the past, immutable collections were rare, but they get are getting more and more
common all the time. For example, in unit testing. Unfortunately, these immutable
collections differ quite a bit from their mutable counterparts. Some parts are only
midly annoying (not accepting `null` as the **value** in a `Map` for example), but
other parts require code to be rewritten for it to be able to work as a drop-in
replacement for the mutable variant. A simple example is this:
public void addShoppingItems(Collection<String> shoppingItems) {
if (shoppingItems.contains(null)) { // this looks quite reasonable and
safe...
throw new IllegalArgumentException("shoppingItems should not contain
nulls");
}
this.shoppingItems.addAll(shoppingItems);
}
Testing this code is annoying:
x.addShoppingItems(List.of("a", null")); // can't construct immutable
collection with null
x.addShoppingItems(Arrays.asList("a", null")); // fine, go back to what we
did before then...
The above problems, I suppose we can live with it; immutable collections don't want
`null` although I don't see any reason to not allow it as I can write a similar
`List.of` that returns immutable collections that do allow `null`. For JDK code this
is a bit disconcerting, as it is supposed to be as flexible as is reasonable without
having too much of an opinion about what is good or bad.
This next one however:
assertNoExceptionThrown(() -> x.addShoppingItems(List.of("a", "b"))); // not
allowed, contains(null) in application code throws NPE
This is much more insidious; the `contains(null)` check has been a very practical
way to check if collections contain null, and this works for almost all collections
in common use, so there is no problem. But now, more and more collections are
starting to just throw NPE immediately even just to **check** if a null element is
present. This only serves to distinguish themselves loudly from other similar
collections that will simply return `false`.
I think this behavior is detrimental to the java collection interfaces in general,
especially since there is a clear answer to the question if such a collection
contains null or not. In fact, why `contains` was ever allowed to throw an exception
aside from "UnsupportedOperationException" is a mystery to me, and throwing one when
one could just as easily return the correct and expected answer seems very opiniated
and almost malicious -- not behavior I'd expect from JDK core libraries.
Also note that there is no way to know in advance if `contains(null)` is going to be
throwing the NPE. If interfaces had a method `allowsNulls()` that would already be
an improvement.
--John
