RFR(XS): 8033909: Objects.requireNonNull(T, Supplier) doesn't specify what happens if the passed supplier is null itself

[David Holmes]

On 13/02/2017 7:10 PM, Volker Simonis wrote:
> On Mon, Feb 13, 2017 at 9:27 AM, David Holmes <david.holmes at oracle.com> wrote:
>> FWIW I think a null Supplier should simply be ignored.
>>
>
> What do you mean by ignored? Do you mean "ignored" in the sense of  my
> proposed changes (i.e. throw a NPE with a a null message) or do you
> mean "ignored" in the sense that it is fine to generate a new,
> implicit NPE when accessing the null supplier (with the consequences
> detailed in the bug description')?

I mean "ignored" in the sense that no method should be called upon it if 
null. Which matches your patch.

David

>> David
>>
>>
>> On 13/02/2017 6:04 PM, Volker Simonis wrote:
>>>
>>> Ping...
>>>
>>> On Thu, Feb 9, 2017 at 5:32 PM, Jeff Dinkins <jeff.dinkins at oracle.com>
>>> wrote:
>>>>
>>>>
>>>> Adding Mike’s current email.
>>>>
>>>> -jeff
>>>>
>>>>> On Feb 9, 2017, at 10:18 AM, Volker Simonis <volker.simonis at gmail.com>
>>>>> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I want to finally resolve this long standing issue (or close it as
>>>>> "will not fix" if that's not possible):
>>>>>
>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8033909/
>>>>>
>>>>> This change has already been discussed in length on the mailing list:
>>>>>
>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-February/thread.html#24989
>>>>>
>>>>> and in the bug comments:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8033909
>>>>>
>>>>> So I'll give just a short summary here:
>>>>>
>>>>> - Objects.requireNonNull(T, Supplier) does not check for the Supplier
>>>>> argument being null. Instead it relies on the fact, that the VM will
>>>>> implicitly throw a NullPointerException when it calls .get on the
>>>>> Supplier argument during the creation of the explicit
>>>>> NullPointerException which it is supposed to throw.
>>>>>
>>>>> - this behavior slightly differs from Objects.requireNonNull(T,
>>>>> String) which simply creates a NullPointerException with a null
>>>>> message in the case where the String argument is null.
>>>>>
>>>>> - the difference is not evident in the OpenJDK, because the HotSpot VM
>>>>> creates a NPE with a null message by default if we call a method on a
>>>>> null object.
>>>>>
>>>>> - however creating such a NPE with a null message when invoking a
>>>>> method on a null object is not enforced by the standard, and other
>>>>> implementations can do better :) For the following trivial program:
>>>>>
>>>>> public class NonNull {
>>>>>  public static void main(String[] args) {
>>>>>    Supplier<String> ss = null;
>>>>>    Object o = Objects.requireNonNull(null, ss);
>>>>>  }
>>>>> }
>>>>>
>>>>> the current OpenJDK implementation returns:
>>>>>
>>>>> Exception in thread "main" java.lang.NullPointerException
>>>>>    at java.util.Objects.requireNonNull(Objects.java:290)
>>>>>    at NonNull.main(NonNull.java:8)
>>>>>
>>>>> but the SAP JVM will print:
>>>>>
>>>>> Exception in thread "main" java.lang.NullPointerException: while
>>>>> trying to invoke the method java.util.function.Supplier.get() of a
>>>>> null object loaded from local variable 'messageSupplier'
>>>>>    at java.util.Objects.requireNonNull(Objects.java:290)
>>>>>    at NonNull.main(NonNull.java:8)
>>>>>
>>>>> which is at least confusing for a call to Objects.requireNonNul() with
>>>>> a null Supplier. We think the exception should be the same like the
>>>>> one we get when invoking Objects.requireNonNul() with a null String.
>>>>>
>>>>> - because of this difference (and because we like our extended
>>>>> Exception messages :) the JCK test for Objects.requireNonNul(T,
>>>>> Supplier) (i.e.
>>>>> api/java_util/Objects/index.html#RequireNonNullMessageSupplier) was
>>>>> removed from TCK 8 and is still commented out in TCK 9
>>>>> (npe_checkingNullSupplier() in RequireNonNullMessageSupplier.java).
>>>>>
>>>>> I really think that the simplest and most natural fix for this problem
>>>>> is to simply check for a null Supplier argument and create the NPE
>>>>> with an explicit null message in that case (as outlined in the
>>>>> webrev). This:
>>>>> - makes the two requireNonNul() overloads for String and Supplier
>>>>> behave the same (which I think was the initial intention).
>>>>> - doesn't require documentation changes
>>>>> - makes it possible to write a simple and conforming TCK test
>>>>>
>>>>> If we can't agree on this quickly, I suggest to close the issue as
>>>>> "will-not-fix" and leave it untested by the TCK.
>>>>>
>>>>> Thank you and best regards,
>>>>> Volker
>>>>>
>>>>> PS: the 'XS' obviously only applies to the fix, not to this message :)
>>>>
>>>>
>>