2711 remove not null assertions #2744
Conversation
This change removed all the explicit null checks using Objects.requireNonNull(). It does not yet address the changes in tests. I am waiting for feedback from the project maintainers regarding their preferred strategy for changing the newly-failing tests.
…sulting function.
…, as long as it never needs to invoke that operator.
…d to compare anything.
…n't need to invoke the operator.
…over an empty collection.
…t need to invoke it on an empty collection.
…d to compare anything.
…to invoke it during fold().
…to invoke it during collect().
| default <R> Iterator<R> collect(PartialFunction<? super T, ? extends R> partialFunction) { | ||
| if (!hasNext()) { |
There was a problem hiding this comment.
I'm not sure if this is adheres to coding guidelines.
If partialFunction == null should not throw when empty, this additional check is required, because the expression partialFunction::isDefinedAt will otherwise throw immediately.
|
What is the reasoning behind this change? At first glance it seems to cloak a lot of programming mistakes by shifting away from fail-early-at-root-cause to fail-at-random-point-in-time. |
|
@szarnekow see discussion in #2711 The other argument, you brought up: "Throwing close to the source" is maybe only valid when the potential throwing of the NPE and the injection point of the null value are in completely different execution stacks. For that, I would support placing some null checks in strategic places like Futures, where an uncaught NPE might even lead it to never terminate. |
See the discussion of this in #2711. Evidently the reasoning is that it is for performance reasons that the null checks are being removed. Ideally null checks should not be needed in a functional library as nulls have no business being in functional code—but unfortunately in Java that minefield always lurks under the surface. Personally I think it’s ok to remove the null checks because 1) there shouldn’t be any nulls when your code properly protects against them coming from 3rd party libraries, and 2) a resulting NPE almost always provides enough locality info to quickly determine where the null came from. For me the compelling reason is to keep the code cleaner by removing the null-check noise; I am skeptical about performance being a compelling reason to remove them. |
tests fixed