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 <jeff.dink...@oracle.com> > wrote: > >> > >> Adding Mike’s current email. > >> > >> -jeff > >> > >>> On Feb 9, 2017, at 10:18 AM, Volker Simonis > <volker.simo...@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 :) > >>
