Why does ImmutableCollection.contains(null) fail?
Asked Answered
P

3

8

Question ahead: why does in Java the call coll.contains(null) fail for ImmutableCollections?

I know, that immutable collections cannot contain null-elements, and I do not want to discuss whether that's good or bad.

But when I write a Function, that takes a (general, not explicit immutable) Collection, it fails upon checking for nulls. Why does the implementation not return false (which is actually the 'correct' answer)?

And how can I properly check for nulls in a Collection in general?

Edit: with some discussions (thanks to the commenters!) I realized, that I mixed up two things: ImmutableCollection from the guava library, and the List returned by java.util.List.of, being some class from ImmutableCollections. However, both classes throw an NPE on .contains(null).

My problem was with the List.of result, but technically the same would happen with guaves implementation. [edit: It does not]

Punk answered 5/2, 2021 at 15:43 Comment(4)
Needs code. What did you try? Can you show us how you produce the problem?Fly
Yes, that collection violates the Liskov substitution principle for multiple reasons and you have encountered one of those cases and the reason for why classes should strive NOT to violate that principle. There is no clean way around this specific issue, you could check the actual object type and then do different checks depending on it, but that is a code smell as well. Btw: you can add items to collections but this class throws exceptions instead, bad idea as well but it is the way it is.Swallow
@Fly You reproduce just how I said: call .contains(null) on any ImmutableCollection (e.g. ImmutableList)Leigha
@Swallow thanks for the Principles. That was my Problem, but I did not have a name for it. I also know that it throws an error when you insert (that's why it's immutable), but I find it strange for the contains function...Leigha
O
15

I am distressed by this discussion!

Collections that do this have been a pet peeve of mine since before I wrote the first collections that eventually became Guava. If you find any Guava collection that throws NPE just because you asked it a perfectly innocent question like .contains(null), please file a bug! We hate that crap.

EDIT: I was so distressed that I had to go back to look at my 2007 changelist that first created ImmutableSet and saw literally this:

  @Override public boolean contains(@Nullable Object target) {
    if (target == null) {
      return false;
    }

ahhhhh.

Overwind answered 13/6, 2021 at 7:59 Comment(4)
So, immutableList.contains(null) should, as per the principles of Guava's spec/API design, resolve to false instead of to throwing an NPE? I can see how the vast majority of folks who ran into this didn't think to file a bug and assumed it was intended behaviour (I certainly did!)Shocker
I'm sorry: who ran into what? Is there a Guava collection that's throwing in this case?Overwind
@KevinBourillion brainfart. The eyes see return false; and the brain parsed throw new NullPointerException. Brain has been duly castigated.Shocker
I'm relieved. But since that comment.... we found one! EvictingQueue has been doing this. So we're fixing it.Overwind
Q
1

The problem gets worse, if calling containsAll() on ImmutableCollections:

@Override
public boolean contains(Object o) {
    return o.equals(e0) || e1.equals(o); // implicit nullcheck of o
}

which uses AbstractCollection's super implementation:

public boolean containsAll(Collection<?> c) {
    for (Object e : c)
        if (!contains(e))
            return false;
    return true;
}

This provokes a NullPointerException, as soon as the (disallowing null) ImmutableCollection's check against another Collection (that allows null) encounters a null reference.

So, when trying something like this, where an ImmutableCollection is compared against a HashSet (in OpenJDK 15):

Set.of("").containsAll(Stream.of((Object)null).collect(Collectors.toSet()));

A NullPointerException is thrown. Things like that can make it very unpleasant to work with the 'modern' Collection API in projects, that have a way back history.

You'd have to do a potentially absurd explicit check of the compared 'legacy' Collection beforehand like this:

Collection coll = Stream.of((Object)null).collect(Collectors.toSet());
boolean hasNull = coll == null || coll.contains(null);
if (!hasNull) {
    Set.of("").containsAll(coll);
}
Quality answered 30/3, 2023 at 11:5 Comment(0)
S
0

why does in Java the call coll.contains(null) fail for ImmutableCollections?

Because the design team (the ones who have created guava) decided that, for their collections, null is unwanted, and therefore any interaction between their collections and a null check, even in this case, should just throw to highlight to the programmer, at the earliest possible opportunity, that there is a mismatch. Even where the established behaviour (as per the existing implementations in the core runtime itself, such as ArrayList and friends, as well as the javadoc), rather explicitly go the other way and say that a non-sequitur check (is this pear part of this list of apples?) strongly suggests that the right move is to just return false and not throw.

In other words, guava messed up. But now that they have done so, going back is potentially backwards compatibility breaking. It really isn't very - you are replacing an exception thrown with a false return value; presumably code could be out there that relies on the NPE (catching it and doing something different from what the code would do had contains(null) returned false instead of throwing) - but that's a rare case, and guava breaks backwards compatibility all the time.

And how can I properly check for nulls in a Collection in general?

By calling .contains(null), just as you are. The fact that guava doesn't do it right doesn't change the answer. You might as well ask 'how do I add elements to a list', and counter the answer of "well, you call list.add(item) to do that" with: Well, I have this implementation of the List interface that plays Rick Astley over the speaker instead of adding to the list, so, I reject your answer.

That's.. how java and interfaces work: You can have implementations of them, and the only guardianship that they do what the interface dictates they must, is that the author understands there is a contract that needs to be followed.

Now, normally a library so badly written they break contract for no good reason*, isn't popular. But guava IS popular. Very popular. That gets at a simple truth: No library is perfect. Guava's API design is generally quite good (in my opinion, vastly superior to e.g. Apache commons libraries), and the team actively spends a lot of time debating proper API design, in the sense that the code that one would write using guava is nice (as defined by: Easy to understand, has few surprises, easy to maintain, easy to test, and probably easy to mutate to deal with changing requirements - the only useful definition for nebulous terms like 'nice' or 'elegant' code - it's code that does those things, anything else is pointless aesthetic drivel). In other words, they are actively trying, and they usually get it right.

Just, not in this case. Work around it: return item != null && coll.contains(item); will get the job done.

There is one major argument in favour of guava's choice: They 'contract break' is an implicit break - one would expect that .contains(null) works, and always returns false, but it's not explicitly stated in the javadoc that one must do this. Contrast to e.g. IdentityHashMap, which uses identity equivalence (a==b) and not value equality (a.equals(b)) in its .containsKey etc implementations, which explicitly goes against the javadoc contract as stated in the j.u.Map interface. IHM has an excellent reason for it, and highlights the discrepancy, plus explains the reason, in the javadoc. Guava isn't nearly as clear about their bizarre null behaviour, but, here's a crucial thing about null in java:

Its meaning is nebulous. Sometimes it means 'empty', which is bad design: You should never write if (x == null || x.isEmpty()) - that implies some API is badly coded. If null is semantically equivalent to some value (such as "" or List.of()), then you should just return "" or List.of(), and not null. However, in such a design, list.contains(null) == false) would make sense.

But sometimes null means not found, irrelevant, not applicable, or unknown (for example, if map.get(k) returns null, that's what it means: Not found. Not 'I found an empty value for you'). This matches with what NULL means in e.g. SQL. In all those cases, .contains(null) should be returning neither true nor false. If I hand you a bag of marbles and ask you if there is a marble in there that is grue, and you have no idea what grue means, you shouldn't answer either yes or no to my query: Either answer is a meaningless guess. You should tell me that the question cannot be answered. Which is best represented in java by throwing, which is precisely what guava does. This also matches with what NULL does in SQL. In SQL, v IN (x) returns one of 3 values, not 2 values: It can resolve to true, false, or null. v IN (NULL) would resolve to NULL and not false. It is answering a question that can't be answered with the NULL value, which is to be read as: Don't know.

In other words, guava made a call on what null implies which evidently does not match with your definitions, as you expect .contains(null) to return false. I think your viewpoint is more idiomatic, but the point is, guava's viewpoint is different but also consistent, and the javadoc merely insinuates, but does not explicitly demand, that .contains(null) returns false.

That's not useful whatsoever in fixing your code, but hopefully it gives you a mental model, and answers your question of "why does it work like this?".

Shocker answered 5/2, 2021 at 16:20 Comment(9)
It helps, thanks. A few Comments: IHM has 'strange' behaviour, but it states it in a very clear manner. On the other hand, it took me a while to even find ImmutableCollection as the root of my problem... I guess I did not expect such behaviour in the java.util package. Also, your code x == null || x.isEmpty() is not enough, i really need to know weather null is in the collection (if it's not immutable, of course). Guess I need an instanceof..Leigha
and For your idea with marbles: if you hand me grue I expect you to hand me an equality function, and I just check areEqual(e, grue) for each of my elements. If any of them returns true I return true, else falseLeigha
Wait, @DánielSomogyi - what are you on about? ImmutableCollection is not in java.util. It is part of the guava library: Surely we are talking about this ImmutableCollection, right? null cannot be in the collection. Assuming it's guava ImmutableCollection It's impossible. add throws. The contains method in the j.u.Collection interface does not allow for an equality function, so you can't cop out with that answer.Shocker
yeah, first I mixed things up. I thought javas ImmutableCollections (e.g. returned from java.util.List.of()) and guaves ImmutableCollection are the same. (mixed things up while searching for a solution). I also know that you can't handle an equality to any of the contains functions, that was just my thought on how to handle 'grue'. imho It is Javas flaw, that the general equality-function depends on one of the two objects being non-null AND you even have to know which one (e.g. null.equals(a) fails).Leigha
edit: of course you also can use Objects.equals. And I just found the detail in the Java documentation: "More formally, returns true if and only if this list contains at least one element e such that Objects.equals(o, e)." (and I also found, that NPE is allowed for lists, that prohibit the use of null)Leigha
So uhh... no one here thought of checking whether this allegation against Guava was at any time true? :-)Overwind
@KevinBourrillion The 'allegation' of guava's take on null being somewhat similar to SQLs take (namely: That it means unknown / unexpected, and as a consequence, usually there is no way to answer any questions when null is involved, thus, an exception is better than an arbitrary answer' bit? I actually quite like that take on null so I'm not entirely certain 'allegation' is the best choice of words. If you're referring to something else, please. I'm always interested in API design debates with someone whose track record contains quite a few APIs I'm fond of.Shocker
Note: I use "allegation" a little tongue-in-cheek just because of how much I dislike this design choice. Sure, it's fine to "like that take on null" in general, but we're talking about Collection in particular. Here, contains(null) has a single unambiguous well-defined meaning shared by every subtype you can think of except these throwing ones. And these throwing ones are perfectly able to adhere to that same specification, and no one who called it could reasonably claim surprise at what it did.Overwind
@KevinBourrillion Collection.contains is allowed to throw a NullPointerException and always has been. And there are collections doing so since day one, e.g. TreeSet or the keySet() and values() of the legacy Hashtable class. The concurrent collections added in Java 5 also throw on contains(null).Fewell

© 2022 - 2024 — McMap. All rights reserved.