Checking for null _creates_ a null reference problem

[jbrains]

My apologies if you've already discussed this to death; just let me know.

I ran into this today.

var fallback = ...;
myOption.fold(fallback::g, null);

I was in a test where I knew myOption is none(), so I didn't bother supplying a function for the "some" case---instead I left it null. I reasoned that since only fallback was being called in this path, the fold() would silently work. Instead, it blew up with a failed null check.

Admittedly, I haven't thought this through for more than a few moments, but I infer that fold() is eagerly checking for null. I think I would prefer if it didn't. Is fold() checking for null as a defensive habit (for the sake of consistency, I imagine) or to avoid a genuine problem?

[danieldietrich]

Hi, I would stick to eager evaluation here because generally it is a good idea to fail early. Lazy evaluation might lead to unexpected errors. Your specific problem could be solved by just using () -> null instead of null.

[jbrains]

Thank you. I believe I understand your position, but I use the ability to pass null for a collaborator as a design tool to alert me to when code might better be split into two smaller parts. The idea is to document examples (tests) that can safely pass null, which this behavior now artificially prevents.

As you say, I could merely pass a collaborator that intentionally does nothing or I could use a Crash Test Dummy, which would blow up spectacularly if accidentally used. That would provide a similar benefit, although perhaps not be quite as easy to spot when skimming the code.

I think this comes down to personal preference. I don't think I care enough about the difference to cling to my preference. As you point out, lazy evaluation might cause more headache for others than it causes for me in this one case.

Thank you.

[danieldietrich]

I thought about it a bit..

I agree that it is artificial. In the early days of Vavr (Javaslang) having explicit null-checks all over the place was questioned. Especially when called internally (where I am sure that function refs are not null) it is an overhead of performing null-checks n times.

Others asked for adding (non-)nullability annotations to method parameters in order to be able to check it at compile-time (which would not help you because you want to set a non-nullable param to null).

To be honest, I would be okay with it to completely remove all the null-checks in favor of performance. Personally, the concrete message does not matter to me if a NPE occurs at runtime. Removing all the checks would change the semantics a bit (eager vs lazy eval) but hopefully no one relies on such exceptions for the control flow of their program (fingers crossed 🤞).

[jbrains]

To be honest, I would be okay with it to completely remove all the null-checks in favor of performance. Personally, the concrete message does not matter to me if a NPE occurs at runtime.

I agree strongly. I'm glad you reconsidered.

I still think I can easily leave with either choice, but I'm glad to see the change considered. Thank you.

[danieldietrich]

Note: I marked this issue with help wanted.

Task: Search all files for the string Objects.requireNonNull and remove the lines.

(Don't forget run the generator by rebuilding the project before creating the PR.)

[jbrains]

Simply blindly removing all the requireNonNull() statements causes 438 tests to fail. I presume, but haven't carefully verified, that these are all tests that explicitly check for the specific requireNonNull() exception due to null arguments. In your opinion, how safe is it merely to remove all these tests compared to trying to decide whether instead to weaken the assumption about which exception is thrown? I haven't looked at them yet, but I imagine that many of them merely overspecify the expected exception.

[danieldietrich]

It would be nice if you could weaken the assumption (ignoring messages, just check if a NPE is thrown). I would start with the generated code and the Abstract* collection tests.
Keeping them would give a hint if the behavior changed (no exception is thrown where previously was one).
There is no time pressure but I guess drinking a glass of wine 🍷while doing and finishing the stupid work would work best 🙃

[jbrains]

Indeed.

I suppose, if nothing else, I might practise some nice sed-related tricks.

[jbrains]

Quick update: 242 tests passing, 196 to go.

[achinaou]

I think there is a strong argument in favor of leaving the null checks, at least for arguments that will be dereferenced in the body of the function.

Unfortunately, Java's type-system doesn't give us tools to express the nullability of the arguments, so these null-checks, are mentally part of the argument's type information that the IDE too can access, and prevent us from passing null somewhere where we shouldn't.

Also, if for some reason a null has been passed to a Vavr's function and a NullPointerException is thrown, the stacktrace will point us to Vavr's internal code, where in some cases it won't be so straightforward which of the arguments cased it and we'll have to dive into the implementation details of a given Vavr's function.

@jbrains, in your example the NullPointerException is thrown before the Option#fold method is called.

[jbrains]

I ran out of steam on this one. I don't know whether I'll get back to it. Whatever happens, happens.

[patrickguenther]

I'd love to take this issue up and continue the work. Is it possible for me to continue building on your state, @jbrains ?

[patrickguenther]

There might be some null checks, that should be kept in place to avoid confusion. For example: Try::run would return Failure with NPE inside which might confuse a user, expecting the NPE originating from within his runnable. What should the correct behavior be on Try.run(null)? Throwing NPE immediately or returning Failure with NPE inside.