NPE throwing behavior of immutable collections

[Alan Snyder]

I tend to agree.

Presumably it is similar reasoning that led to Collection<E>.contains() to take Object rather than E.

> On Jan 29, 2023, at 7:28 AM, John Hendrikx <john.hendrikx at gmail.com> 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
> 
>