Vavr: NPE in max() and min()

[eirikm]

Using min or max on a list containing a null value will either produce a NPE or Option.none:

    @Test
    public void nullPointerExceptionInMinAndMax() {
        Integer one = 1;
        Integer zero = 0;
        Integer nullInteger = null;

        try {
            List<Integer> nullSecond = List.of(zero, nullInteger, one);
            nullSecond.min();
            nullSecond.max();
        } catch (NullPointerException e) {
            System.out.println("Boooo. Gives NPE but should give min == zero && max == one. Due to o1.compareTo(o2) in max and min");
        }

        List<Integer> nullAsHead = List.of(nullInteger, zero, one);
        System.out.println("nullAsHead.min() = " + nullAsHead.min());
        System.out.println("nullAsHead.max() = " + nullAsHead.max());
        System.out.println("Boo gives None in both situations. should give min == zero && max = one. Due to the fact: !(list.head() instanceof Comparable))");

        try {
            List<Integer> nullAtEnd = List.of(zero, one, nullInteger);
            nullAtEnd.min();
            nullAtEnd.max();
        } catch (NullPointerException e) {
            System.out.println("Boooo. Gives NPE but should give min == zero && max == one. Due to o1.compareTo(o2) in max and min");
        }
    }

[danieldietrich]

I'm not sure about allowing null values. An alternative is to prohibit null as part of the interface documentation.

In native Java the min and max functions are only available in primitive Streams, like IntStream. My vision is to further restrict the types when future Java versions allow us to do so. Currently an extension of the type system is investigated as part of project Valhalla:

For now I suggest to _filter_ the values in order to get sure null values are handled properly:

List.of(null, 0, null, 1, null) . filter(Predicates::isNotNull) . min();

I will update the min/max documentation accordingly.

[paplorinc]

I would ban null values everywhere, muhahahaha :trollface:
But if that's not possible, we should at least do something more meaningful than an NPE :/

[danieldietrich]

Arithmetic operations should only process on defined element.

E.g. 1 + null is not 1. I would say it is undefined because our arithmetic has no notion for adding an element outside the set of ints to an int.

So why max(1, null) should be 1?

A healthy codebase does not implement additional logical branches for every special case and his dog :-) We should do it as straight forward as possible.

Therefore we will not include special null handling here.

[danieldietrich]

I would ban null values everywhere, muhahahaha :trollface:

YEAH - and the user has to SUFFER if he uses null values, MUHAHAHAHA 👹

[eirikm]

Even if max(1, null) shouldn’t be 1 it would be reasonable to assume that
max(1, null) would provide the same result/error as max(null, 1), wouldn’t it?

Eirik

On 14 Aug 2016, at 16:28, Daniel Dietrich [email protected] wrote:

Arithmetic operations should only process on defined element.

E.g. 1 + null is not 1. I would say it is undefined because our arithmetic has no notion for adding an element outside the set of ints to an int.

So why max(1, null) should be 1?

A healthy codebase does not implement additional logical branches for every special case and his dog :-) We should do it as straight forward as possible.

Therefore we will not include special null handling here.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/javaslang/javaslang/issues/1482#issuecomment-239676496, or mute the thread https://github.com/notifications/unsubscribe-auth/AAezuzLM9W5N1CQ4oDvIrJiwA8VAkNXsks5qfyYjgaJpZM4Jh7WK.

[danieldietrich]

@eirikm yes, that's right. We should throw something like an ArithmeticException in such cases. Thanks for the hint!

[danieldietrich]

We can't easily intercept a min/max computation and map a NPE to an ArithmeticException. This is because min/max internally call minBy/maxBy, which take a custom Comparator. A custom Comparator can throw a NPE for another reason, so we can't catch the NPE and rethrow an ArtithmeticException.

Therefore we don't do it more complicated than it should be. Plain Java acts like this:

// throws NullPointerException
Stream.of(null, 1).min(Integer::compare);

// throws NullPointerException
Stream.of(1, null).min(Integer::compare);

We will act the same way. This is just one extra check in min()/max(), if the head element is null.

[danieldietrich]

The simple solution works for all collections but TreeSet/RedBlackTree. Needs further investigation.

Btw - all SortedMap and SortedSet implementations should at least override min() because that is the head() element (if present).