vigna / fastutil

fastutil extends the Java™ Collections Framework by providing type-specific maps, sets, lists and queues.
Apache License 2.0
1.8k stars 197 forks source link

Resolve ambiguous overload warnings when using lambdas #197

Closed techsy730 closed 3 years ago

techsy730 commented 3 years ago

An umbrella bug for discussion of overloads that can cause ambiguous calls if used with a lambda, making using them tricky.

The primary cause of this in fastutil is are a pair of Overloads that take a type-specific and an object based functor type.

Examples include (list will be updated as they are found)

vigna commented 3 years ago

I have just checked in type-specific predicates and binary operators to help with this.

I think the path you set up should be followed uniformly: there's a type-specific class which extends the type-generic version and the JDK type-specific version, possibly by widening. Then everything is deprecated and has a default implementation, except for the fastutil type-specific call.

Type-specific callers must implement and deprecate the generic call, as usual, and must implement and deprecate the JDK type-specific call, if it is available, so lambdas are not ambiguous.

techsy730 commented 3 years ago

OK, I have started with fixing removeIf by doing the disambiguation overload thing that forEach and forEachRemaining does.

vigna commented 3 years ago

Before we start to bang heavily on this, let us discuss for a second why we want to support JDK's primitive operators. The main reason I see is that there might be ready-made implementations of that interface that are actually useful. Am I right?

vigna commented 3 years ago

The next question is: why do we want widened apply(int) in, say, CharConsumer? It is not useful for a lambda, and I don't see why there should be so many ready-made character-based consumers with an integer interface. But I might be wrong. And indeed the cost of having the widened apply() is fairly low.

techsy730 commented 3 years ago

For binary API compatibility if nothing else. If we just straight remove the Java library primitive predicate operations, then that will cause a NoSuchMethodError on already compiled code already calling the old one (as the overload is determined at compile time, even if they used lambdas). removeIf(IntPredicate) is already released API in the shorter data type collections. For the shortened data types though I agree we should at least deprecate them.

For IntPredicate, LongPredicate, and DoublePredicate, IMO they don't really value add over the Java library's version and are mostly here for consistency. So I see no reason not to make the Java library's overloads the "primary" ones, and just have our versions extend the library's versions.

vigna commented 3 years ago

No, that's another issue. That's the issue of "do we want methods that use predicates to have calls using JDK primitive operators, maybe widened?". I agree that's a good idea.

My question is whether it makes sense to have our own fastutil interfaces implementing widened calls.

techsy730 commented 3 years ago

Oh wait are you asking why CharConsumer and CharPredicate extend java...IntConsumer and java...IntPredicate? In that case, I do think that is a bit awkward and they probably shouldn't. Sort of a breaking change but the methods we would lose are already marked deprecated so we should be fine to remove that.

vigna commented 3 years ago

OK, so we agree on that. So in general that would mean:

Alternatively:

The first solution has much less #if'ing. The second solution passes seamlessly through the JDK type-specific call, gaining some performance in case one uses a ready-made, say, Consumer, instead of a lambda.

Am I right?

techsy730 commented 3 years ago

I'm a little unclear on your two proposals? What I was in the process of doing was: byte/short/char/float: The canonical functional methods take fastutil's ByteConsumer/BytePredicate, deprecate IntConsumer/IntPredicate overloads, and make those forward to the ByteConsumer/Predicate ones using a lambda does a widening cast. Those implementing functors for these primitive types explicitly should use the fastutil version.

int/long/double: The canonical functional methods take Java library IntConsumer/IntPredicates, implement the fastutils overload as final (as much as possible) as it only disambiguates. Explicit implementations of functors are free to choose whether they want to use the fastutil's or Java library's primitive functor interfaces.

vigna commented 3 years ago

Is it correct that since the disambiguation method delegates, all lambdas will occur into an additional method call?

techsy730 commented 3 years ago

The disambiguation does not introduce another lambda in the mix. Since fastutil IntConsumer is also a Java library IntConsumer, it just casts the functor straight to a Java IntConsumer. One time cost not each functor call. The reason we need it is also because IntConsumer is also an object based Consumer (taking Integers), so giving a fastutil IntConsumer would have otherwise been ambiguous (did you want Object Consumer or Java's IntConsumer?)

For the smaller types the Java functor -> fastutil functor method does introduce a new lambda layer to perform the widening cast. But we are deprecating it anyways.

techsy730 commented 3 years ago

I think I have a PR ready. It basically does to removeIf what I did with forEach. That may be clearer then trying to describe it. 😆

vigna commented 3 years ago

Yeah the necessity is clear to me. I think it's a very reasonable approach, what worries me is the amount of conditionals. For example, the andThen()/compose() method I implemented, following this logic, should take an IntUnaryOperator, not an Int2IntFunction. The conditionals blow out my mind tho.

But this has the advantage of extending the JDK, rather than replace it, which seems the right approach.

This is the second approached I tried to describe above.

In all this, technically having various kinds of widened calls is not necessary but it might be useful. I'm sure of that for predicates and consumers, as there's no typed output. A bit less for operators. But it's already done, so the right choice is to keep it.

I'll try to rewrite the second proposal in a clearer way. What you write above covers the implementation of functional interfaces, but I'd like to clarify the rules for the users of such interfaces.

vigna commented 3 years ago

Ha ha I feel you but I need to write something I'll understand in a year without looking at the code...