Add withFilter

[julienrf]

This started nicely. But then… things got worse when it was time to support sorted and Map collections.

I used Views to implement WithFilter. I noticed that in the current collections withFilter calls are fused, so I implemented filter fusion on Views too.

That’s it for the nice part.

Now, what’s the problem with Map and sorted collections? The implementation of WithFilter delegates to iterableFactory to build the collection resulting of a map or flatMap call subsequent to withFilter. However, in the case of Map, iterableFactory builds an Iterable instead of the expected concrete Map, and in the case of SortedSet, iterableFactory builds a Set instead of the expected concrete SortedSet. Consequently, I created subclasses of WithFilter for each case (SortedWithFilter, MapWithFilter and SortedMapWithFilter). Then, I refined the withFilter member in Map, SortedSet and SortedMap to return the corresponding specialized XxxWithFilter. Since this is a refinement, it means that all of these XxxWithFilter classes have to extend the initial WithFilter class (in the same way that all collection types extend Iterable). I’m not happy with the result because there are lots of parameters and type parameters. However, I don’t have a better solution :(

Fixes #145.

[julienrf]

This test fails on Dotty. This is probably something related to hashcodes. We observed a weird behavior here too. I’ll create an issue in Dotty.

[julienrf]

On Dotty xs1 == TreeSet().

[julienrf]

Actually that’s not related to hashcodes, but maybe to inlining in Dotty. I added a commit that un-inlines some methods and now the tests pass on Dotty. I’m trying to minimize the problem.

[Ichoran]

I agree this looks pretty bad but I haven't had time to come up with anything better, if it's even possible to come up with something better. Is it possible to simply not provide a default implementation for the difficult cases? If we were worrying about CBF, something with five type parameters is not going to go over well.

[julienrf]

What do you mean by “not provide a default implementation”? If we remove the complex subclasses of WithFilter (e.g. the one in SortedMap) then people using an if in a for expression whose generator is a SortedMap will get back a Map. I think that would be a bit ridiculous: for without if would return back the sorted Map, but as soon as you introduce an if you loose sortedness.

In my opinion, the PR is fine to merge, these complex classes are not aimed to be manipulated by end-users.

[Ichoran]

I mean that we manually write the WithFilter for each leaf class with no abstraction at all (beyond the element type). It gives us a bit more work to do but it makes the idea of implementing your own class more approachable. (More work to actually do, but less work to understand, and usually the understanding is more of a barrier than the doing.)

[julienrf]

I mean that we manually write the WithFilter for each leaf class with no abstraction at all (beyond the element type).

For Map collection types that would mean implement 6 methods (two overloads of map, two overloads of flatMap, foreach and withFilter.

[Ichoran]

Yes, but we don't have that many map collection types. Anyway, it's not a very satisfying answer, but it's the best idea I have right now (aside from the scheme you already came up with).

[odersky]

I wonder, why is the implementation in the existing collections so much simpler? What do they do that we cannot? CBF?

[Ichoran]

I haven't had time to work this all the way through, but it seems to me if the WithFilter hierarchy exactly parallels the Ops hierarchy, you "only" need exactly the same type arguments (no extra ones). This is a little more bulky than what you've already got, but it's pretty close.

I haven't yet had time to play with making WithFilter an inner class of the Ops trait. In that case it would simply adopt all the type parameters, and I think that would be enough, but I haven't worked out whether it's possible or desirable to provide alternate implementations. If nobody else gets to it, I'll play with this next time I have a chance. Please let me know if you have, though, so I don't duplicate work.

[julienrf]

@Ichoran I’m trying something with BuildFrom. So far it works great (only one implementation of WithFilter) but I have issues related to #134 / #78. Maybe I’ll fix all of them at the same time.

Without BuildFrom, even with an inner class I’m afraid that you still need 4 kinds of WithFilter because we currently have no way to abstract over the arity of the collection type constructor and over the implicit constraints.

[Ichoran]

I don't think there's anything wrong with four kinds of WithFilter, just with five generic type parameters. If instead it has zero type parameters because it adopts everything it needs from its parent class, it presents a much friendlier interface. Yes, it does make abstracting over WithFilters awkward, but WithFilter is strange anyway so I don't think we lose much.

[julienrf]

OK, I see. I’ll experiment with that tomorrow.

[julienrf]

I moved WithFilter to be inner classes. The resulting classes have only one parameter (the filtered collection).

However now the API doc shows this: