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

[Alan Bateman]

On 14/02/2017 07:36, Volker Simonis wrote:


:
Finally somebody who understands me :)
As this seems a very controversial topic, I'll wait for a second
positive review before I'll push this.

If you are going to push this change then best to do it with another 
JIRA issue as JDK-8033909 is a request for a spec change. That is what I 
was trying to explain in the other mail.


-Alan

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

[Volker Simonis]

On Tue, Feb 14, 2017 at 12:56 AM, David Holmes  wrote:
> On 14/02/2017 2:57 AM, Alan Bateman wrote:
>>
>> On 09/02/2017 16:18, Volker Simonis wrote:
>>
>>> :
>>>
>>> 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.
>>>
>> If I read JDK-8033909 correctly then it's a request for the javadoc to
>> specify the expected behavior when called with a messageSupplier of
>> null. I think Joe said at one point that he deliberately didn't specify
>> this. If that remains the position then the issue can be closed as WNF
>> so that we don't waste any more discussion on it.
>
>
> I think the fact this method is intended to throw a NPE if the target object
> is null, complicates the "normal" position of "if any arg is null we throw
> NPE", and so requires explicitly being addressed. I would address this by
> making the supplier be optional, so that it is only used if non-null and so
> can itself never be the source of the generated NPE. That would then lead
> naturally to the implementation Volker proposes as being the only correct
> implementation.
>

Thanks David!

Finally somebody who understands me :)
As this seems a very controversial topic, I'll wait for a second
positive review before I'll push this.

Regards,
Volker

> David
> -
>
>
>> If I read your mail and patch correctly then it's more about what the
>> NPE message is. I agree the exception you pasted in from the SAP build
>> is difficult to read, in which case your patch makes it easier to read.
>> That make sense and maybe just push that with a separate issue.
>>
>> -Alan

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

[David Holmes]

On 14/02/2017 2:57 AM, Alan Bateman wrote:

On 09/02/2017 16:18, Volker Simonis wrote:


:

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.


If I read JDK-8033909 correctly then it's a request for the javadoc to
specify the expected behavior when called with a messageSupplier of
null. I think Joe said at one point that he deliberately didn't specify
this. If that remains the position then the issue can be closed as WNF
so that we don't waste any more discussion on it.


I think the fact this method is intended to throw a NPE if the target 
object is null, complicates the "normal" position of "if any arg is null 
we throw NPE", and so requires explicitly being addressed. I would 
address this by making the supplier be optional, so that it is only used 
if non-null and so can itself never be the source of the generated NPE. 
That would then lead naturally to the implementation Volker proposes as 
being the only correct implementation.


David
-


If I read your mail and patch correctly then it's more about what the
NPE message is. I agree the exception you pasted in from the SAP build
is difficult to read, in which case your patch makes it easier to read.
That make sense and maybe just push that with a separate issue.

-Alan

Re: 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  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 
wrote:



Adding Mike’s current email.

-jeff


On Feb 9, 2017, at 10:18 AM, Volker Simonis 
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 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 :)

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

[Alan Bateman]

On 09/02/2017 16:18, Volker Simonis wrote:


:

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.

If I read JDK-8033909 correctly then it's a request for the javadoc to 
specify the expected behavior when called with a messageSupplier of 
null. I think Joe said at one point that he deliberately didn't specify 
this. If that remains the position then the issue can be closed as WNF 
so that we don't waste any more discussion on it.


If I read your mail and patch correctly then it's more about what the 
NPE message is. I agree the exception you pasted in from the SAP build 
is difficult to read, in which case your patch makes it easier to read. 
That make sense and maybe just push that with a separate issue.


-Alan

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

[Volker Simonis]

On Mon, Feb 13, 2017 at 9:27 AM, David Holmes  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')?

> David
>
>
> On 13/02/2017 6:04 PM, Volker Simonis wrote:
>>
>> Ping...
>>
>> On Thu, Feb 9, 2017 at 5:32 PM, Jeff Dinkins 
>> wrote:
>>>
>>>
>>> Adding Mike’s current email.
>>>
>>> -jeff
>>>
 On Feb 9, 2017, at 10:18 AM, Volker Simonis 
 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 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 :)
>>>
>>>
>

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

[Langer, Christoph]

Hi

> FWIW I think a null Supplier should simply be ignored.
...which is what Volker's change does.

After reading through all the discussion, I'm pro adding the null check as 
proposed in Volker's webrev.

Best regards
Christoph

> On 13/02/2017 6:04 PM, Volker Simonis wrote:
> > Ping...
> >
> > On Thu, Feb 9, 2017 at 5:32 PM, Jeff Dinkins 
> wrote:
> >>
> >> Adding Mike’s current email.
> >>
> >> -jeff
> >>
> >>> On Feb 9, 2017, at 10:18 AM, Volker Simonis
>  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 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 :)
> >>

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

[David Holmes]

FWIW I think a null Supplier should simply be ignored.

David

On 13/02/2017 6:04 PM, Volker Simonis wrote:

Ping...

On Thu, Feb 9, 2017 at 5:32 PM, Jeff Dinkins  wrote:


Adding Mike’s current email.

-jeff


On Feb 9, 2017, at 10:18 AM, Volker Simonis  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 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 :)

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

[Volker Simonis]

Ping...

On Thu, Feb 9, 2017 at 5:32 PM, Jeff Dinkins  wrote:
>
> Adding Mike’s current email.
>
> -jeff
>
>> On Feb 9, 2017, at 10:18 AM, Volker Simonis  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 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 :)
>

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

[Jeff Dinkins]

Adding Mike’s current email.

-jeff

> On Feb 9, 2017, at 10:18 AM, Volker Simonis  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 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 :)

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

[Volker Simonis]

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 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 :)

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

[Volker Simonis]

Hi Mike,

as described in the bug comments, I still think that correctly
handling a null Supplier is the cleanest and easiest solution. Without
the change our VM will throw the following NPE:

java.lang.NullPointerException: while trying to invoke the method
java.util.function.Supplier.get() of a null object loaded from the
second parameter of the method
   at java.util.Objects.requireNonNull(Objects.java:290)

which is more descriptive and perfectly legal, but it may be not what
the user expects (i.e. a NPE with a null message).

Anyhow, I don't insist on my change. If everybody agrees that it would
be better to change the documentation then please go ahead. But keep
in mind that Joe also had some objections against the documentation
changes in the bug comments.

Regards,
Volker


On Tue, Feb 11, 2014 at 12:24 AM, Mike Duigou mike.dui...@oracle.com wrote:
 I would prefer to leave the implementation as is and amend the documentation. 
 The difference in behaviour between the overloads seems OK since it will 
 still be valid for the Supplier to return null. The String overload 
 reasonably allows null since the Throwable(String) constructor allows null 
 message. (See Throwable.getMessage()).

 It seems like ignoring/masking the likely error of passing a null Supplier 
 isn't really being helpful.

 YMMV,

 Mike

 On Feb 10 2014, at 05:30 , Volker Simonis volker.simo...@gmail.com wrote:

 Hi,

 could you please review the following tiny change which fixes a
 problem in Objects.requireNonNull:

 http://cr.openjdk.java.net/~simonis/webrevs/8033909/
 https://bugs.openjdk.java.net/browse/JDK-8033909

 The problem is that 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 while trying
 to call .get on the Supplier argument during the creation of the
 explicit NullPointerException which is supposed to be thrown by
 Objects.requireNonNull(T, Supplier) for a null T argument.

 This makes the behavior of Objects.requireNonNull(T, Supplier)
 slightly different from the one of the overladed
 Objects.requireNonNull(T, String) version. The latter one always
 throws a NPE with a null message in the case where the String argument
 is null. For the first one, it depends on the implementation of the VM
 whether the trown NPE will have a null message or not (i.e. the VM is
 free to create a NPE with an arbitrary message).

 The bug report mentions that this behavior makes it hard to develop a
 conformance test for the scenario and suggests to tweak the JavaDoc of
 Objects.requireNonNull(T, Supplier) such that it allows NPE with an
 unspecified message.

 However, I think the easiest fix is to just check for the supplier
 object beeing null and explicitely creating a NPE with a null message
 in that case. This will align the behavior of
 Objects.requireNonNull(T, Supplier) with that of
 Objects.requireNonNull(T, String). Also notice that the extra
 null-check is only performed if the object argument T of
 Objects.requireNonNull(T, Supplier) is null, which should be the
 unusual case anyway. I therefore don't expect this change to have any
 negative performance impact.

 This webrev is for jdk9, but I think it should be also downported to jdk8.

 Thank you and best regards,
 Volker

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

[Volker Simonis]

Hi,

could you please review the following tiny change which fixes a
problem in Objects.requireNonNull:

http://cr.openjdk.java.net/~simonis/webrevs/8033909/
https://bugs.openjdk.java.net/browse/JDK-8033909

The problem is that 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 while trying
to call .get on the Supplier argument during the creation of the
explicit NullPointerException which is supposed to be thrown by
Objects.requireNonNull(T, Supplier) for a null T argument.

This makes the behavior of Objects.requireNonNull(T, Supplier)
slightly different from the one of the overladed
Objects.requireNonNull(T, String) version. The latter one always
throws a NPE with a null message in the case where the String argument
is null. For the first one, it depends on the implementation of the VM
whether the trown NPE will have a null message or not (i.e. the VM is
free to create a NPE with an arbitrary message).

The bug report mentions that this behavior makes it hard to develop a
conformance test for the scenario and suggests to tweak the JavaDoc of
Objects.requireNonNull(T, Supplier) such that it allows NPE with an
unspecified message.

However, I think the easiest fix is to just check for the supplier
object beeing null and explicitely creating a NPE with a null message
in that case. This will align the behavior of
Objects.requireNonNull(T, Supplier) with that of
Objects.requireNonNull(T, String). Also notice that the extra
null-check is only performed if the object argument T of
Objects.requireNonNull(T, Supplier) is null, which should be the
unusual case anyway. I therefore don't expect this change to have any
negative performance impact.

This webrev is for jdk9, but I think it should be also downported to jdk8.

Thank you and best regards,
Volker

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

[Mike Duigou]

I would prefer to leave the implementation as is and amend the documentation. 
The difference in behaviour between the overloads seems OK since it will still 
be valid for the Supplier to return null. The String overload reasonably allows 
null since the Throwable(String) constructor allows null message. (See 
Throwable.getMessage()).

It seems like ignoring/masking the likely error of passing a null Supplier 
isn't really being helpful.

YMMV,

Mike

On Feb 10 2014, at 05:30 , Volker Simonis volker.simo...@gmail.com wrote:

 Hi,
 
 could you please review the following tiny change which fixes a
 problem in Objects.requireNonNull:
 
 http://cr.openjdk.java.net/~simonis/webrevs/8033909/
 https://bugs.openjdk.java.net/browse/JDK-8033909
 
 The problem is that 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 while trying
 to call .get on the Supplier argument during the creation of the
 explicit NullPointerException which is supposed to be thrown by
 Objects.requireNonNull(T, Supplier) for a null T argument.
 
 This makes the behavior of Objects.requireNonNull(T, Supplier)
 slightly different from the one of the overladed
 Objects.requireNonNull(T, String) version. The latter one always
 throws a NPE with a null message in the case where the String argument
 is null. For the first one, it depends on the implementation of the VM
 whether the trown NPE will have a null message or not (i.e. the VM is
 free to create a NPE with an arbitrary message).
 
 The bug report mentions that this behavior makes it hard to develop a
 conformance test for the scenario and suggests to tweak the JavaDoc of
 Objects.requireNonNull(T, Supplier) such that it allows NPE with an
 unspecified message.
 
 However, I think the easiest fix is to just check for the supplier
 object beeing null and explicitely creating a NPE with a null message
 in that case. This will align the behavior of
 Objects.requireNonNull(T, Supplier) with that of
 Objects.requireNonNull(T, String). Also notice that the extra
 null-check is only performed if the object argument T of
 Objects.requireNonNull(T, Supplier) is null, which should be the
 unusual case anyway. I therefore don't expect this change to have any
 negative performance impact.
 
 This webrev is for jdk9, but I think it should be also downported to jdk8.
 
 Thank you and best regards,
 Volker