Re: RFR(XS): 8033909: Objects.requireNonNull(T, Supplier) doesn't specify what happens if the passed supplier is null itself
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
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
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
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
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
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
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
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
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
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
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
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 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 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 >
Re: RFR(XS): 8033909: Objects.requireNonNull(T, Supplier) doesn't specify what happens if the passed supplier is null itself
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 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
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