Override Java 8 default method Collection#removeIf

[aqsa505]

As mentioned in #500, the Collection#removeIf has not been overridden in Eclipse Collection yet.

I would like to work on that!

[donraab]

Thanks for volunteering @aqsa505. I assigned the issue to you.

[baliga04]

Can I try to solve the issue if it is open.

[itsdeekay]

Hi, there seems no progress on this. Can I work on this @donraab ?

[donraab]

The challenge with this issue is what exactly to do. I'm going to share some thoughts here @aqsa505 to hopefully make this easier to approach.

The MutableCollection class in Eclipse Collections defines removeIf as follows:

boolean removeIf(org.eclipse.collections.api.block.predicate.Predicate<? super T> predicate);

There is an additional removeIfWith method on MutableCollection. This method can remain as is.

<P> boolean removeIfWith(org.eclipse.collections.api.block.predicate.Predicate2<? super T, ? super P> predicate, P parameter);

The Collection class in JDK defines removeIf as follows:

default boolean removeIf(java.util.function.Predicate<? super E> filter) 

I believe Eclipse Collections has had removeIf since before it was open sourced in 2012. I was able to see we changed the return type of removeIf to return boolean in 2015 to be consistent with JDK method which was added in Java 8 in 2014.

Proposal:

I think we should just remove the EC overload of removeIf in MutableCollection and change the implementations to match the JDK signature with java.util.function.Predicate. The EC Predicate interface extends the JDK Predicate interface, so this should be a source compatible change. This is most likely not a binary compatible change, but I think that will be ok if this makes it in time for 12.0 release.

I would like to hear thoughts from the other committers @motlin @nikhilnanivadekar @prathasirisha @mohrezaei on this proposal.

[mohrezaei]

hmmm... Not a fan of the proposal. There are at least two other options:

1. Just override the JDK method. This is the maximal compatibility option.
2. Deprecate ec.Predicate. Leave the interface for backward compatibility, but search/replace with jdk.Predicate everywhere. This is the maximal cleanup option.

[donraab]

Thanks Moh. I think option 1 is fine, and should be easy enough to implement. I think option 2 would be hard to do. The reason I was suggesting removing overload on MutableCollections is that it might be confusing to developers having two removeIf methods defined, but then again this has been the case already for the past 8 years since Java 8 was released.

I tried the code below and was surprised there wasn't an ambiguity problem with using a lambda. I'm guessing it might be because EC Predicate extends JDK Predicate, and the more specific removeIf on MutableCollection is chosen.

@Test
public void removeIf()
{
    MutableList<Integer> list = Lists.mutable.with(1, 2, 3);
    list.removeIf(each -> each % 2 == 0);
    Assertions.assertEquals(Lists.mutable.with(1, 3), list);
}

I was surprised the lambda didn't require a cast, although casting does work and calls the implementation on Collection.

@Test
public void removeIf()
{
    MutableList<Integer> list = Lists.mutable.with(1, 2, 3);
    list.removeIf((java.util.function.Predicate<Integer>) (each -> each % 2 == 0));
    Assertions.assertEquals(Lists.mutable.with(1, 3), list);
}

[nikhilnanivadekar]

This blog: ***@***.***/eclipse-collections-10-4-0-released-7c5b3f43c0f0 And more specifically this blog: https://stuartmarks.wordpress.com/2020/09/22/incompatibilities-with-jdk-15-charsequence-isempty/ covers the reasons that can cause ambiguity thus clarifying why this is not ambiguous.

…

On Mon, Apr 17, 2023 at 7:22 AM Donald Raab ***@***.***> wrote: Thanks Moh. I think option 1 is fine, and should be easy enough to implement. I think option 2 would be hard to do. The reason I was suggesting removing ours is that it might be confusing to developers having two removeIfs defined, but then again this has been the case already for the past 8 years since Java 8 was released. I tried the code below and was surprised there wasn't an ambiguity problem with using a lambda. I'm guessing it might be because EC Predicate extends JDK Predicate, and the more specific removeIf on MutableCollection is chosen. @test public void removeIf() { MutableList<Integer> list = Lists.mutable.with(1, 2, 3); list.removeIf(each -> each % 2 == 0); Assertions.assertEquals(Lists.mutable.with(1, 3), list); } I was surprised the lambda didn't require a cast, although casting does work and calls the implementation on Collection. @test public void removeIf() { MutableList<Integer> list = Lists.mutable.with(1, 2, 3); list.removeIf((java.util.function.Predicate<Integer>) (each -> each % 2 == 0)); Assertions.assertEquals(Lists.mutable.with(1, 3), list); } — Reply to this email directly, view it on GitHub <#1432 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACLY4XITAC46DAO6Y2FVRCDXBSO7DANCNFSM6AAAAAAUS56E3Y> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mohrezaei]

I tried option 2. Documenting here for posterity.

1. Use IDE->rename ec.Predicate.accept to someUniqueString334455
2. replace in files, import ec.Predicate with import jdk.Predicate
3. replace in files, someUniqueString334455 with test. Rollback changes to ec.Predicate.
4. search accept in stg files. for files that are changed because of the above, replace usages of accept with test for Predicate (but not Predicate2, etc).
5. maven->test. One test required an extra import of jdk.Predicate to compile.
6. The biggest issue happens around serialization of block.function classes that have a Predicate member. jdk.Predicate is not serializable. One possibility would be to leave these classes alone. Gave up at this point.

[donraab]

Ok, @aqsa505 I think you can go ahead with option 1 from @mohrezaei . Just override the JDK method in MutableCollection. It should be doable via a default method that delegates to our current removeIf method using a method reference.

[aditya-32]

Hi @donraab I would like to take this up, if nobody is working on it now

[donraab]

Thanks for volunteering @aditya-32. It is assigned currently to @aqsa505. I'll wait for her to comment whether she is working on it currently.