Add keySet to MapIterable and/or make API more consistent

[victornoel]

I noticed that the primitive Maps have a keySet method, but the generic ones don't.

The immutable ones return an unmodifiable set (because a MutableSet is expected to be returned by the interfaces).

I think it would make sense to add it also to the generic Maps because there is only a keyView available and sometimes, it's useful to have a Set (or a SetIterable maybe?).

[johnjstuart]

I can take up this issue if no one has started work on it yet!

[donraab]

Thank you @johnjstuart for volunteering. I can assign the issue to you but thought it would be good to kick off some discussion here first.

This seems like a tricky problem to me, that was solved more easily by having keysView. I took a look at IntIntMap and indeed as you said @victornoel was surprised to see keySet return a MutableIntSet. I would have preferred this to return IntSet so that IntIntMap presents a purely readable interface. Then MutableIntIntMap could have returned MutableIntSet. This seems like a separate issue we could create, or we could just leave it as is and replicate something similar as @victornoel suggests.

The problem is with a diamond hierarchy. MutableMap extends both Map, which has keySet returning Set, and MapIterable which currently does not have keySet. If we add keySet returning Set on MapIterable, things will more or less just work, but with the downside of introducing a mutable interface that should allow you to mutate the underlying Map. Ideally, if we returned SetIterable for keySet in MapIterable, then MutableMap would have to return a MutableSet to address the diamond hierarchy problem of extending both Map and MapIterable.

But maybe it is ok to just return a Set. I agree that having a Collection available for keySet is often more useful than having an Iterable.

Thoughts? @nikhilnanivadekar @motlin

[motlin]

Ideally, MapIterable.keySet() would return SetIterable. That's not easy to do until MutableMapIterable.keySet() and overrides return MutableSetIterables.

[nikhilnanivadekar]

I have tried this in the past only in vain. Let’s just return a Set, for ease

[donraab]

I agree with @nikhilnanivadekar and @victornoel it's just more practical at this stage to return a Set, even if SetIterable is the ideal return value.

[victornoel]

I'm working on a PR for this.

I created #1274 for the issue with primitive maps you uncovered @donraab.