Skip to content

Support java collections & streams in for-do loops#15379

Closed
AugustNagro wants to merge 1 commit into
scala:mainfrom
AugustNagro:foreach-extension-for-java-collections
Closed

Support java collections & streams in for-do loops#15379
AugustNagro wants to merge 1 commit into
scala:mainfrom
AugustNagro:foreach-extension-for-java-collections

Conversation

@AugustNagro

Copy link
Copy Markdown
Contributor

Support java collections & streams in for-do loops by defining foreach extension methods in scala.Predef

Alternative to #15368

@AugustNagro

Copy link
Copy Markdown
Contributor Author

I can reproduce the failure of tests/neg/i12640.scala on current main; it doesn't seem related to this PR

@TheElectronWill

TheElectronWill commented Jun 5, 2022

Copy link
Copy Markdown
Contributor

Yes this test seems to be flaky, see #15371 (comment)

@dwijnand

dwijnand commented Jun 7, 2022

Copy link
Copy Markdown
Member

Isn't this fixed with import scala.jdk.CollectionConverters.* and .asScala?

@AugustNagro

Copy link
Copy Markdown
Contributor Author

It is certainly a hassle and hard for beginners.

Java collections already work fine with for-yield, but refactor to for-do and it suddenly requires an import and .asScala call.

@som-snytt

Copy link
Copy Markdown
Contributor
scala> for x <- List.of(1,2,3) do println(x)
-- [E008] Not Found Error: ---------------------------------------------------------------------------------------------
1 |for x <- List.of(1,2,3) do println(x)
  |         ^^^^^^^^^^^^^^
  |         value foreach is not a member of java.util.List[Int] - did you mean java.util.List[Int].forEach?
1 error found

scala>

should be

scala> for x <- List.of(1,2,3) do println(x)
-- [E008] Not Found Error: ---------------------------------------------------------------------------------------------
1 |for x <- List.of(1,2,3) do println(x)
  |         ^^^^^^^^^^^^^^
  |         value foreach is not a member of java.util.List[Int] - did you mean import scala.jdk.CollectionConverters.*; List.of(1,2,3).asScala?
1 error found

scala>

@dwijnand

dwijnand commented Jun 8, 2022

Copy link
Copy Markdown
Member

It is certainly a hassle and hard for beginners.

Java collections already work fine with for-yield, but refactor to for-do and it suddenly requires an import and .asScala call.

Work fine without, how?

Welcome to Scala 3.2.0-RC1-bin-20220606-cec9aa3-NIGHTLY-git-cec9aa3 (11.0.15, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> for x <- java.util.List.of(1, 2, 3) yield x
-- [E008] Not Found Error: -----------------------------------------------------
1 |for x <- java.util.List.of(1, 2, 3) yield x
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
  |         value map is not a member of java.util.List[Int]
1 error found

scala> for x <- java.util.List.of(1, 2, 3); y <- List(x) yield y
-- [E008] Not Found Error: -----------------------------------------------------
1 |for x <- java.util.List.of(1, 2, 3); y <- List(x) yield y
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
  |value flatMap is not a member of java.util.List[Int], but could be made available as an extension method.
  |
  |One of the following imports might fix the problem:
  |
  |  import collection.convert.ImplicitConversions.list asScalaBuffer
  |  import collection.convert.ImplicitConversionsToScala.list asScalaBuffer
  |  import collection.convert.ImplicitConversions.collection AsScalaIterable
  |  import collection.convert.ImplicitConversionsToScala.collection AsScalaIterable
  |  import collection.convert.ImplicitConversions.iterable AsScalaIterable
  |  import collection.convert.ImplicitConversionsToScala.iterable AsScalaIterable
  |
1 error found

@TheElectronWill

TheElectronWill commented Jun 8, 2022

Copy link
Copy Markdown
Contributor

It does work with Stream because it provides map and flatMap.

scala> for x <- java.util.List.of(1).stream yield x
val res0: java.util.stream.Stream[Int] = java.util.stream.ReferencePipeline$3@6ac9b66b

scala> for x <- java.util.List.of(1).stream; y <- java.util.List.of(2).stream yield y
val res1: java.util.stream.Stream[Int] = java.util.stream.ReferencePipeline$7@129b0ed

@AugustNagro

Copy link
Copy Markdown
Contributor Author

That's a good point @dwijnand

@bishabosha

Copy link
Copy Markdown
Member

I guess that extensions could be added for map and flatMap by converting to a Scala collection and then back to a Java collection.

def foreach(f: java.util.function.Consumer[T]): Unit =
it.forEach(f)

extension [T](s: java.util.stream.Stream[T])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also support IntStream and the other primitive streams?

@TheElectronWill TheElectronWill Jun 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It we could support BaseStream (javadoc), which is the common interface above Stream, IntStream and all, it would be even better :)
edit: actually I got carried away: BaseStream doesn't provide any meaningful operation on its elements...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to also support IntStream and the other primitive streams.

@sjrd

sjrd commented Jun 10, 2022

Copy link
Copy Markdown
Member

I really don't think this has its place in Predef, of all things! Predef is for:

  • things that are essential to the language,
  • things for which requiring an import would make them totally useless (that's ???, and this one got in after a very long discussion of its merits)
  • some historical mistakes (manifests, printf).

There is no precedent for putting things in Predef that make using the JDK more convenient. There would need to be a very strong motivation to do so. Avoiding one import to use Java Streams in for do is far from making the cut, as far as I'm concerned.

@AugustNagro

Copy link
Copy Markdown
Contributor Author

What if we make a type class like

trait ForLoopSyntax[F[_]]:
  def foreach[A](fa: F[A])(f: A => Unit): Unit
  def map[A, B](fa: F[A])(f: A => B): F[B]
  def flatMap[A, B](fa: F[A])(f: A => F[B]): F[B]

And if the compiler can't resolve the desugared foreach, map, or flatMap call, then it tries to summon ForLoopSyntax.

The standard library could define some common givens, like for CompletableFuture and j.u.List.

@TheElectronWill

Copy link
Copy Markdown
Contributor

Isn't that a bit "overkill"?
The extensions methods could just be moved somewhere else, like in the package scala.jdk. With an import, you'll be able to for-do and for-yield on java collections, without converting the collections with asScala.

@odersky odersky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it's probably better to move this to some module that can be explicitly imported. Putting things in Predef is a big step. For instance, what should happen on the JS platform?

def foreach(f: java.util.function.Consumer[T]): Unit =
it.forEach(f)

extension [T](s: java.util.stream.Stream[T])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to also support IntStream and the other primitive streams.

it.forEach(f)

extension [T](s: java.util.stream.Stream[T])
def foreach(f: java.util.function.Consumer[T]): Unit =

@odersky odersky Jun 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is in Predef then foreach needs to be inline, since stdLibPatches/Predef.scala has no runtime equivalent. Or else we need a PR against 2.13 Predef instead.

@dwijnand dwijnand closed this Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants