Add specialized forEachKey(ValueType) methods to Multimap leaf interfaces

[donraab]

There is a method forEachKeyMultiValues on Multimap with the following signature:

void forEachKeyMultiValues(Procedure2<? super K, ? super RichIterable<V>> procedure);

I would like to propose that we add the following methods with specific types to the leaf interfaces in the Multimap hierarchy.

MutableListMultimap -> void forEachKeyMutableList(Procedure2<? super K, ? super MutableList<V>> procedure); ImmutableListMultimap -> void forEachKeyImmutableList(Procedure2<? super K, ? super ImmutableList<V>> procedure);

And so on and so forth for the other leaf interfaces. For the MutableMultimap types, the procedure should be passed an unmodifiable wrapper to the collection as is done today with get().

[donraab]

Thoughts @motlin @prathasirisha @nikhilnanivadekar @mohrezaei? I've started implementing these, and I think it has been a type specific method we have been missing for a long time. It is also safer than forEachKeyMultiValues because that method does not wrap the underlying collection in an unmodifiable wrapper (because it doesn't know the type), which means folks can modify the List or Set directly if they cast it, which we know folks do.

[nikhilnanivadekar]

IMO this might be a bit of an overkill. Casting is generally fine. It’s annoying but I think it’s ok. Curious to hear what @motlin , @mohrezaei , @prathasirisha think.

[mohrezaei]

What do the implementations look like? do they have to re-implement every time?

[donraab]

AbstractMutableListMultimap:

    @Override
    public void forEachKeyMutableList(Procedure2<? super K, ? super MutableList<V>> procedure)
    {
        this.getMap().forEachKeyValue((key, value) -> procedure.value(key, value.asUnmodifiable()));
    }

ImmutableListMultimapImpl:

    @Override
    public void forEachKeyImmutableList(Procedure2<? super K, ? super ImmutableList<V>> procedure)
    {
        this.getMap().forEachKeyValue(procedure);
    }

I'll need to have code like this for each leaf Multimap type.

Currently, this is the code in AbstractMultimap which is unsafe for both casting and mutating the collection inside of the Multimaps outside.

    @Override
    public void forEachKeyMultiValues(Procedure2<? super K, ? super RichIterable<V>> procedure)
    {
        this.getMap().forEachKeyValue(procedure);
    }

[mohrezaei]

Since the impls are simple (hey, that rhymes :smile:), I think it's fine.

[motlin]

I like the idea overall. I don't see much point in using the Mutable interfaces specifically, when we wrap them in unmodifiable wrappers. We could use ListIerable instead of MutableList for example, and I don't think we give up much, or any, api.

About the implementation, should we move asUnmodifiable() higher up in the hierarchy? If it's not already on MutableCollection, it probably should be.

[donraab]

It's not the api that we give up, it is the compatibility with java.util.List when we use ListIterable.

I checked on asUnmodifiable. It is defined on MutableCollection.