Skip to content

stringOf eagerly prints infinite iterable #11785

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

Open
som-snytt opened this issue Oct 29, 2019 · 12 comments
Open

stringOf eagerly prints infinite iterable #11785

som-snytt opened this issue Oct 29, 2019 · 12 comments
Assignees
Milestone

Comments

@som-snytt
Copy link

Haven't tried dotr yet.

scala> val vs = new Iterable[Int] { def iterator = Iterator.continually(42) }
java.lang.OutOfMemoryError: Java heap space
  at java.base/java.util.Arrays.copyOf(Arrays.java:3745)
  at java.base/java.lang.AbstractStringBuilder.ensureCapacityInternal(AbstractStringBuilder.java:172)
  at java.base/java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:538)
  at java.base/java.lang.StringBuilder.append(StringBuilder.java:174)
  at java.base/java.lang.StringBuilder.append(StringBuilder.java:168)
  at scala.collection.IterableOnceOps.addString(IterableOnce.scala:1170)
  at scala.collection.IterableOnceOps.addString$(IterableOnce.scala:1162)
  at $anon$1.addString(<console>:1)
  at scala.collection.IterableOnceOps.mkString(IterableOnce.scala:1112)
  at scala.collection.IterableOnceOps.mkString$(IterableOnce.scala:1110)
  at $anon$1.mkString(<console>:1)
  at scala.collection.Iterable.toString(Iterable.scala:77)
  at scala.collection.Iterable.toString$(Iterable.scala:77)
  at $anon$1.toString(<console>:1)
  at scala.runtime.ScalaRunTime$.inner$1(ScalaRunTime.scala:232)
  at scala.runtime.ScalaRunTime$.stringOf(ScalaRunTime.scala:243)
  at scala.runtime.ScalaRunTime$.replStringOf(ScalaRunTime.scala:251)
  at .$print$lzycompute(<synthetic>:8)
  ... 14 elided
@som-snytt som-snytt added the repl label Oct 29, 2019
@Jasper-M
Copy link

knownSize should have more possible states.

sealed trait Size
case class Known(n: Int) extends Size
case object Unknown extends Size
case object UnknownFinite extends Size
case object Infinite extends Size

@joshlemer
Copy link

joshlemer commented Nov 12, 2019

Should the default Iterable#toString implementation be changed to not even force its elements? View and LazyList for instance override their toString to avoid forcing elements, maybe the forcing implementation here could be migrated to StrictOptimizedIterableOps or somewhere else.

eed3si9n added a commit to eed3si9n/scala that referenced this issue Nov 24, 2019
Fixes scala/bug#11785

This changes the implementation of `Iterable#toString` to not force the evaluation of its elements. To maintain the backward compatibility for most collections the old implementation is called from Set, Seq, and Map, which happen to override toString anyway to avoid the ambiguity with `Function1#toString`.

### notes

I considered pushing this into `StrictOptimizedIterableOps` trait as originally suggested in scala/bug#11785, but adding new methods to the trait would introduce bincompat breakage, so swapping out just the implementation would be safer.
@eed3si9n
Copy link
Member

Here's my PR - scala/scala#8554

@eed3si9n eed3si9n self-assigned this Nov 24, 2019
@SethTisue SethTisue modified the milestones: 2.13.2, 2.13.3 Feb 6, 2020
eed3si9n added a commit to eed3si9n/scala that referenced this issue Feb 13, 2020
Fixes scala/bug#11785

This changes the implementation of Iterable#toString to not force the evaluation of its elements unless it extends StrictOptimizedIterableOps.
@lrytz lrytz modified the milestones: 2.13.3, 2.13.2 Feb 19, 2020
@lrytz lrytz closed this as completed Feb 19, 2020
@SethTisue
Copy link
Member

fix was reverted; see discussion on scala/scala-dev#671

@som-snytt
Copy link
Author

@NthPortal was that unintentional?

@Jasper-M also KnownUnknown and UnknownUnknown.

@NthPortal
Copy link

NthPortal commented Apr 3, 2020

yes it was.

this is why you only put the "Fixes X" in the PR, and never in the commit message.

@NthPortal NthPortal reopened this Apr 3, 2020
@eed3si9n
Copy link
Member

eed3si9n commented Apr 3, 2020

this is why you only put the "Fixes X" in the PR, and never in the commit message.

I'm sorry. I didn't know about this etiquette since I've been working on projects with 1~2 members.

@NthPortal
Copy link

NthPortal commented Apr 4, 2020

@eed3si9n no worries. I've made the same mistake myself. honestly, it's less etiquette and more a questionable decision on github's part.

the issue is, whenever someone with write permissions for the repo containing the issue pushes a new commit containing a "fixes" clause (to the default branch?) to a repo, github evaluates the "fixes" clause. consequently, any time someone with write permissions to scala/bug pushes the commit for the first time to their fork of scala/scala (as I did when I was updating my long-out-of-date copy of the 2.13.x branch), it re-evaluates the clause. it's actually not normally an issue, unless a bug gets reopened.

one possible solution would be if the clause was only evaluated when a PR containing it was closed (configurable maybe, as it's still useful in commits for smaller projects?).

relatedly, it's not good to put a valid github reference to an issue or PR in a commit message (e.g. "workaround for scala/bug#XXX"), as any time that commit is rebased or pushed to a fork, the issue gets a message added saying that someone referenced it in a commit. if there are a lot of rebases or forks, it gets really spammy (it's not super spammy there, but you can see the sort of thing I'm talking about in #11872).

@som-snytt
Copy link
Author

Off-loading to the smart dotty people.

@som-snytt
Copy link
Author

I had forgotten the research over here; it's like catching an old re-run on tv.

A quick thought is maybe isTraversableAgain implies that I'm allowed to attempt printing. (Lazy or strict is not the test, but whether I must capture the values before printing tosses them away.)

Someone was recently wondering what is isTraversableAgain good for.

@som-snytt
Copy link
Author

som-snytt commented Oct 6, 2022

Reopening, as it is the responsibility of stringOf not to do something annoying. This is in the context of improved ellipses, which raises the bar.

scala/scala#10178 (Edit: that is the context, not the fix)

@som-snytt
Copy link
Author

The linked PR strikes a different balance between naively calling toString and printing pretty collections.

The user story is: "I'm trying stuff out in the REPL, I don't necessarily know what I'm doing, so please try not to blow up or hang."

@SethTisue SethTisue modified the milestones: 2.13.11, 2.13.12 Mar 28, 2023
@SethTisue SethTisue modified the milestones: 2.13.12, Backlog Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment