8281301: Map.of() calls with null argument should throw more contextual information.

[Stuart Marks]

Sorry, it appears that you have fallen into a thicket of issues. [*]

I'm usually fairly skeptical of these kinds of changes, as they often have more 
overhead than is apparent. Java requires all arguments to be /evaluated/ before the 
method is invoked. Evaluating a lambda includes loading and running the lambda 
metafactory, spinning bytecodes, and loading another class into the JVM. This is 
done even if the lambda is never /invoked/.

The unmodifiable collections are used a lot at startup time, so this means this 
extra work will be done on critical code paths even before main() is called. Also, 
most of this is run in interpretive mode, so we can't rely on the benefit of JIT 
compilation.

But there are other reasons to avoid using lambdas in critical startup code like 
this. Just for grins I applied your patch at ran it, and the result was this:


$ java -version
Error occurred during initialization of VM
java.lang.ExceptionInInitializerError
	at 
java.lang.invoke.MethodHandleNatives.findMethodHandleType(java.base/MethodHandleNatives.java:411)
	at java.util.ImmutableCollections$MapN.<init>(java.base/ImmutableCollections.java:1186)
	at java.util.Map.of(java.base/Map.java:1584)
	at jdk.internal.reflect.Reflection.<clinit>(java.base/Reflection.java:55)
	at java.security.AccessController.doPrivileged(java.base/AccessController.java:319)
	at java.lang.reflect.AccessibleObject.<clinit>(java.base/AccessibleObject.java:524)
Caused by: java.lang.NullPointerException
	at 
java.lang.invoke.MethodType$ConcurrentWeakInternSet.<init>(java.base/MethodType.java:1406)
	at java.lang.invoke.MethodType.<clinit>(java.base/MethodType.java:231)
	at 
java.lang.invoke.MethodHandleNatives.findMethodHandleType(java.base/MethodHandleNatives.java:411)
	at java.util.ImmutableCollections$MapN.<init>(java.base/ImmutableCollections.java:1186)
	at java.util.Map.of(java.base/Map.java:1584)
	at jdk.internal.reflect.Reflection.<clinit>(java.base/Reflection.java:55)
	at java.security.AccessController.doPrivileged(java.base/AccessController.java:319)
	at java.lang.reflect.AccessibleObject.<clinit>(java.base/AccessibleObject.java:524)


Essentially the JVM crashed even before it got to print out the version message. I 
don't know exactly what happened, but this kind of error often occurs if lambdas are 
used "too early" during initialization. The reason is that lambdas use a bunch of 
runtime infrastructure in java.lang.invoke that's written in Java. This Java code in 
turn uses a variety of collections. If collections code were to use lambdas that are 
evaluated at startup time, this would cause problems like initialization 
circularities, references to uninitialized data, or other inexplicable errors. 
That's what's happening here.

We could try to work around this by using explicit classes instead of lambdas for 
the messageSupplier, but this would still mean more classes loaded and initialized 
and bytecodes executed at startup time, which I'm fairly uncomfortable with given 
the reasons above.

s'marks

[*] Fortunately, it seems unlikely that you will be eaten by a grue. :-)


On 2/28/23 1:14 PM, Sage Vaillancourt wrote:
> Hi there. It's my first time posting to the development list, so
> please forgive any formatting errors.
> 
> My proposed change is pretty small. Basically, a pattern I see with
> some frequency is calling Objects.requireNonNull(object, "objectName")
> before (or within) Map.of(), because otherwise there's not much of a
> way to tell _which_ parameter caused the exception. Even just an index
> and key/value hint would make tracking down these types of errors
> easier without that bit of boilerplate.
> 
> I'd expect that any performance impact to be negligible, already being
> off the happy path, here.
> 
> Diff as I'm imagining it, but very open to suggestions:
> 
> diff --git a/src/java.base/share/classes/java/util/ImmutableCollections.java
> b/src/java.base/share/classes/java/util/ImmutableCollections.java
> index 3de7e1d5eae..814c1a9ec1b 100644
> --- a/src/java.base/share/classes/java/util/ImmutableCollections.java
> +++ b/src/java.base/share/classes/java/util/ImmutableCollections.java
> @@ -1103,8 +1103,8 @@ class ImmutableCollections {
>           private final V v0;
> 
>           Map1(K k0, V v0) {
> -            this.k0 = Objects.requireNonNull(k0);
> -            this.v0 = Objects.requireNonNull(v0);
> +            this.k0 = Objects.requireNonNull(k0, () -> "Key in map
> entry was null");
> +            this.v0 = Objects.requireNonNull(v0, () -> "Value in map
> entry was null");
>           }
> 
>           @Override
> @@ -1183,9 +1183,9 @@ class ImmutableCollections {
> 
>               for (int i = 0; i < input.length; i += 2) {
>                   @SuppressWarnings("unchecked")
> -                    K k = Objects.requireNonNull((K)input[i]);
> +                    K k = Objects.requireNonNull((K)input[i], () ->
> "Key for map entry " + (i / 2) + " was null");
>                   @SuppressWarnings("unchecked")
> -                    V v = Objects.requireNonNull((V)input[i+1]);
> +                    V v = Objects.requireNonNull((V)input[i+1], () ->
> "Value for map entry " + (i / 2) + " was null");
>                   int idx = probe(k);
>                   if (idx >= 0) {
>                       throw new IllegalArgumentException("duplicate key: " + k);
>