Why is Map.keys returning a Collection and not a Set ?

[davidfestal]

No description provided.

[gavinking]

Fixed.

[davidfestal]

Thanks !

[gavinking]

This fix has now been moved to the 6063 branch.

[ePaul]

This looks like a non-backwards-compatible change – not for consumers, but for implementers of the interface. Whoever did refine this method (e.g. because there was a more efficient implementation) declaring the return type as Collection<Key> now has a class which doesn't compile anymore.

I don't know enough about binary compatibility to be sure, but from afar it looks like it is compatible neither for consumers nor for implementors on the JVM (because the erased return type is part of the signature).

[gavinking]

So, given the version number upgrade, I would advocate that we put this on the table for 1.3.0.

It's a break to BC, but one that I still insist is pretty minor. WDYT?

[jvasileff]

My only thought would be that I was hoping we'd get more bang for the buck with the next compatibility breaking release. I know there are other things that we probably don't have time for.

Regarding the Compatibility

• This change would cause new modules calling keys on Maps produced by old modules to fail at runtime.

But also:

• This change would cause old modules calling keys on Maps produced by new modules to fail at runtime.

And even:

• This change would cause old modules calling keys on Maps produced by old modules to fail at runtime.

The second incompatibility is slightly surprising, since Correspondence.keys still has a return type of Collection, so any newly compiled class that satisfies Map will support both. But, it seems that bridge methods don't actually help here, since the bridge is on the class and not the interface, which doesn't know about the Collection returning alternative.

If we call the old binary below from within a new runtime, we get:

shared void callKeys(Map<Anything,Anything> map) {
    Correspondence<Nothing> corr = map;
    noop(corr.keys); // ok
    noop(map.keys);  // java.lang.NoSuchMethodError: ceylon.language.Map.getKeys()Lceylon/language/Collection;
}

and from the above, it follows that old binaries like this fail too:

noop(HashMap { 1 -> 1 }.keys); // java.lang.NoSuchMethodError: ceylon.language.Map.getKeys()Lceylon/language/Collection;

Finally, more surprisingly, even if the old binary were to first cast to Correspondence, it would still fail!

Correspondence<Nothing> corr = HashMap { 1 -> 1 };
noop(corr.keys); // java.lang.NoSuchMethodError: ceylon.language.Map$impl.getKeys()Lceylon/language/Collection;

The reason is that the old HashMap is using the new Map$impl at runtime, which doesn't have the method in question, since Map$impl doesn't implement Map, and therefore javac doesn't create a bridge for Collection's Object getKeys()!

public final class Map$impl<Key, Item> implements .java.io.Serializable { ... }

TL;DR: Pretty much anything involving Map.keys and old modules will fail.

[quintesse]

@jvasileff paints a bleak picture for such a seemingly small change. Personally I don't think the breakage is worth the relatively small advantage the change brings.
Perhaps we could create a new bc-incompatible tag and keep issues marked like that around until we really break compatibility (like the one we have planned for the mix-ins) and then apply them all in one go?

[gavinking]

Alright, fine, so not for 1.3.0. :-(

[gavinking]

Done, finally.