Better error message for Collectors.toMap

[zbynek]

Fixes #9495 by changing the error message.

[niloc132]

Lambdas and method references are pretty expensive to generate, and while the new error message will be shorter, the total generated code is probably going to increase for this change.

Is there another approach we can use to avoid this, composing the toMap() call with some other collision check? At a glance, I'd either say composing something into the other lambdas we have to produce (likely parameterizing them, which isn't all that great), or maybe a call like mapping(collisionCheckHere, toMap(...))?

[zbynek]

The only way I was able to reduce the number of lambdas was parametrizing the private toMap 🤔
The extra parameter could be avoided if we say that mergeFunction == null should be interpreted as unique == true in the private method (and possibly add a null check to the public one).

I don't think this can be easily solved with composition since either collisionCheck would have to return both T and K wrapped in an Entry or keyMapper would have to be called twice (in collision checking and in accumulator).

[niloc132]

The null check is surprising - it isn't specified at https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Collectors.html#toMap(java.util.function.Function,java.util.function.Function), but it does seem to be part of the current JDK at least:

jshell> Stream.of(1).collect(Collectors.toMap(ignore -> "", ignore -> null))
|  Exception java.lang.NullPointerException
|        at Objects.requireNonNull (Objects.java:209)
|        at Collectors.lambda$uniqKeysMapAccumulator$1 (Collectors.java:180)
|        at ReduceOps$3ReducingSink.accept (ReduceOps.java:169)
|        at Streams$StreamBuilderImpl.forEachRemaining (Streams.java:411)
|        at AbstractPipeline.copyInto (AbstractPipeline.java:509)
|        at AbstractPipeline.wrapAndCopyInto (AbstractPipeline.java:499)
|        at ReduceOps$ReduceOp.evaluateSequential (ReduceOps.java:921)
|        at AbstractPipeline.evaluate (AbstractPipeline.java:234)
|        at ReferencePipeline.collect (ReferencePipeline.java:682)
|        at (#9:1)

You could consider composing the existing function with disallowNulls(...).

[zbynek]

composing the existing function with disallowNulls

Wouldn't that instantiate more lambdas?

[niloc132]

Instantiating lambdas isn't awful, its declaring them - any time you write -> or ::, you can imagine you just made a new class instead. Not just an anonymous inner class, but build the rest of the machinery in your head for there to be a constructor to pass in the outer variables/instances (fields, etc), potentially the getClass, the type info to know the supertype/interfaces of the lambda, etc.

I wouldn't bat an eye at someone adding lambdas in application code, but core JRE emulation code for something as commonly used as toMap seems worth working a little harder on - even if it means the error message might be a little subpar. Library code I'd want to work slightly harder where its easy, but not worry about it (e.g. prefer if (optional.isPresent()) {...} over optional.map(...) sorts of things).

Instantiating them is only as bad as instantiating anything else, new Constructor(args, to, close, over), since at the end of the day (non-JsFunction) lambdas are just implementations of SAM interfaces. We could probably do better (...the JVM does), but it both adds complexity to the compiler, and could risk making something like incremental builds stop working. Instantiating them also only happens once, at the declaration site - but if you pull that out into a util method (like disallowNulls()), you just pay all that once, and then get to do a(b) to wrap the existing lambda in another lambda each time it is needed.

JsFunction lambdas are a little different in how they are built, but are actually slightly cheaper to call in GWT-generated Java than non-JsFunction lambdas - rather than instance.apply(args)/a.b(c) when you invoke, its just instance(args)/a(c), savings of 2ish bytes per callsite.