Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java interop: no more static 'empty' for collections #11509

Open
raboof opened this issue Apr 30, 2019 · 25 comments

Comments

Projects
None yet
7 participants
@raboof
Copy link
Member

commented Apr 30, 2019

scala/scala@a864ae0 (added between 2.13.0-M5 and 2.13.0-RC1) introduces a def empty() on scala.collection.Iterable.

Many (immutable) implementations of Iterable, such as scala.collection.immutable.List, have a companion object with a val empty. On 2.13.0-M5 and earlier, Java users could use the List.empty() static forwarder to get the empty List instance. From 2.13.0-RC1, this static forwarder no longer exists, because the List class itself now has a non-static empty() method inherited from Iterable.

According to the commit message the change was intended to "facilitate the switch to dotty's for comprehensions" - I don't know anything about that, so it might well be that the change is inevitable or worth it for that reason - I just wanted to flag this consequence that might not have been intentional.

What would be the new way for a Java user to get easy access to an empty instance of such an immutable collection?

@hrhino

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

asScalaIterable(Collections.newArrayList()).toList(), of course. (They're used to the verbosity.)

We could slap @varargs on List.apply and friends, but then we'd have to fix #10683. (I did a bit of work on that some moons ago; it's not trivial but may be Java-interop-specific to be considered for 2.13).

That would also give me List.apply("foo", "bar"), which I've been missing since forever.

@szeiger

This comment has been minimized.

Copy link

commented Apr 30, 2019

I'm not familiar with changes in for comprehensions in Dotty so I'll leave that to @adriaanm. But if they require this method then the change is probably inevitable.

You can always get the "static" parts in Java via MODULE$: https://github.com/scala/scala/blob/2.13.x/test/files/pos/t1642/JavaCallingScalaHashMap.java

@hrhino

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

I think the change hasn't happened yet (or I didn't notice) but it's in pursuit of lampepfl/dotty#2573 as I understand it. scala/scala-dev#607 mentions that we could wait for 2.14, but in scala/scala#7929 @adriaanm expressed a desire to make no backwards-incompatible changes in 2.14.

So depending on Dotty's willingness to run off of 2.14.x, (or merge lampepfl/dotty#5813), maybe this could just be backed out. (Unless the for-comprehension improvement is already merged?)

@szeiger szeiger added this to the 2.13.0-RC2 milestone Apr 30, 2019

@szeiger

This comment has been minimized.

Copy link

commented Apr 30, 2019

Scheduling for RC2 in case we want to revert the empty() change.

@smarter

This comment has been minimized.

Copy link

commented Apr 30, 2019

Or add a static List.of (like https://docs.oracle.com/javase/9/docs/api/java/util/List.html#of-E...-) for the express purpose of Java interop ?

@smarter

This comment has been minimized.

Copy link

commented Apr 30, 2019

Also to clarify, dotty doesn't care about empty being there or not currently, but I'd love it if lampepfl/dotty#2573 got solved in one way or another.

@hrhino

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

of! I had forgotten about that in Java 9.

I like it, but does that mean we'd have two ways of creating the same List? Might be confusing.

@smarter

This comment has been minimized.

Copy link

commented Apr 30, 2019

evil suggestion: make it private[scala] so that it's public from Java's POV but not Scala's :).

@lrytz

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@adriaanm

This comment has been minimized.

Copy link
Member

commented May 3, 2019

I think it's quite likely an improved for-comprehension desugaring would avoid filter/withFilter and use empty instead, so I'd prefer to keep the method in place. We could provide some convenience methods for Java to create Scala collections outside of the stdlib.

@lrytz

This comment has been minimized.

Copy link
Member

commented May 6, 2019

provide some convenience methods for Java to create Scala collections outside of the stdlib

Or in scala.jdk.javaapi.CollectionConverters

@adriaanm

This comment has been minimized.

Copy link
Member

commented May 8, 2019

We're considering removing the empty method because it's weird to have on the instance side of things (it should only be on the companion, but then the for-comprehension desugaring couldn't get at it without us having a way to get to the companion from the instance, which is also weird).

@szeiger

This comment has been minimized.

Copy link

commented May 8, 2019

After trying to remove empty I think we should keep it. While it's a bit of a weird method to have on collection instances, we already have it for Map and Set in 2.12. There is no good reason why these should be special cases. We should either have it on all collection kinds or none of them.

The version in 2.13.x before scala/scala@a864ae0 was not ideal, either. empty only returned an IterableCC or MapCC in the generic case but you want it to be of type C. This was probably just an oversight as it is actually easier to implement with C.

Fixing those types and deprecating the existing methods would be an option but they are used pervasively in the collection implementations. We need at least a protected version of this method so we'd have to add it under a new name and deprecate empty so the name can potentially be freed up in 2.14, unless we discover that we need it for a new for comprehension desugaring.

Maybe we are just missing the right abstraction. We have different polymorphic factories in collection instances but there is no monomorphic Factory[A, C]. The new *FactoryDefaults traits have to implement several methods that could all be provided byFactory. But it could be difficult to add this extra layer of indirection without negatively affecting performance.

@adriaanm

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Ok, I’m glad you did the due diligence. Let’s leave this ticket open for some alternate fix.

@dwijnand

This comment has been minimized.

Copy link
Member

commented May 14, 2019

But it could be difficult to add this extra layer of indirection without negatively affecting performance.

Do you mean having to go via .iterableFactory, or something else?

@adriaanm

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@hrhino

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Could you spec it as .empty but add that to the collections with an extension method? Agreed that .companion.empty is kludgy.

@adriaanm

This comment has been minimized.

Copy link
Member

commented May 14, 2019

That seems cleaner, yes. The larger question, though, is: what should the spec be in the first place :-) I think we could consider working on this in 2.14 if there's demand. Will keep that in mind when I open the roadmap ticket for 2.14, as soon as 2.13.0 final is tagged :)

@hrhino

This comment has been minimized.

Copy link
Member

commented May 14, 2019

If we remove it now, and Dotty wants to play with .empty as part of the spec, they can put an extension method in scala.collection, right? Especially when they can add it as a top-level definition.

@adriaanm

This comment has been minimized.

Copy link
Member

commented May 14, 2019

True, but there are other reasons to keep it: #11509 (comment)

@lrytz

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Related: scala/scala-dev#573

Another hack we could do is adding an @staticForwarderName("empties") annotation, so people can provide a different name in case of a conflict.

@szeiger

This comment has been minimized.

Copy link

commented May 15, 2019

OK, the specific factory doesn't really work. newSpecificBuilder and fromSpecific have to use A @uncheckedVariance. Both methods are protected so it's not too bad, but you wouldn't want them accessible from the outside. The same goes for the factory: protected[this] def specificFactory: Factory[A, C] should work but it doesn't help (even if Factory had an empty method), you still have to add a forwarder for empty. You could expose it to users as a Factory[_ <: A, C] but that's not useful, either, because you couldn't call anything other than empty on it.

@smarter

This comment has been minimized.

Copy link

commented May 15, 2019

As a side-note, why do some collections have a package-privateemptyInstance method ? https://github.com/scala/scala/blob/06392a55749f34ece097863c50a2af3fd6b3a88b/src/library/scala/collection/immutable/Set.scala#L131

raboof added a commit to akka/akka that referenced this issue May 15, 2019

@dwijnand

This comment has been minimized.

Copy link
Member

commented May 15, 2019

As a side-note, why do some collections have a package-privateemptyInstance method ? scala/scala:src/library/scala/collection/immutable/Set.scala@06392a5#L131

#6457

@szeiger

This comment has been minimized.

Copy link

commented May 15, 2019

Probably and oversight when the old code was ported from 2.12. These methods are not used for anything.

raboof added a commit to akka/akka that referenced this issue May 16, 2019

Add ByteString.emptyByteString to Java API (#26931)
* Add ByteString.emptyByteString to Java API

To work around scala/bug#11509

* Add note to 2.6 migration document
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.