Skip to content

resolve status of scala/scala#8554 for 2.13.2 #671

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

Closed
SethTisue opened this issue Feb 23, 2020 · 7 comments
Closed

resolve status of scala/scala#8554 for 2.13.2 #671

SethTisue opened this issue Feb 23, 2020 · 7 comments
Assignees
Labels
Milestone

Comments

@SethTisue
Copy link
Member

it's breaking scala-collection-contrib; see scala/scala#8554 for details and discussion

@SethTisue SethTisue added this to the 2.13.2 milestone Feb 23, 2020
@SethTisue
Copy link
Member Author

SethTisue commented Feb 23, 2020

@szeiger can I ask you to find some resolution here, perhaps working with @joshlemer ?

for 2.13.2, I think it would be acceptable to simply revert the PR and then reconsider it for 2.13.3. but if a better solution can be found in time, great

@lrytz lrytz added the blocker label Feb 28, 2020
@SethTisue
Copy link
Member Author

ping @szeiger?

@joshlemer
Copy link

Just to lay out what I have been thinking:

It's true that my comment from the original issue suggested that perhaps a solution could involve moving the toString implementation to a StrictOptimized_______Ops trait, but later came around to thinking that that is breaking with the existing precedent that these StrictOptimized_Ops traits have until now only been about optimizing implementation, not about changing semantics. And that going forward with changing the semantics to be that only strict collections which inherit from StrictOptimized_Ops are correctly implemented constitutes a drastic change in the api of the collections.

Even though it's unsatisfying that a user-defined infinite Iterable has a broken toString unless they override it, it's always been like this:

2.12:

scala> new Iterable[Int] { def iterator = Iterator.continually(1) }.toString
java.lang.OutOfMemoryError: Java heap space

So it doesn't seem worth it to me, to break the api to fix it. Perhaps in a future major release, we can consider how to deal with toString in the collections in a more systematic way which may include just implementing toString as an override in StrictOptimizedIterableOps.

So my recommendation would be to revert the change.

@SethTisue
Copy link
Member Author

@eed3si9n opinion?

@eed3si9n
Copy link
Member

eed3si9n commented Mar 19, 2020

I think it depends on the philosophy / spirit of the Collection library. What do all the traits mean? Why do we need them, as opposed to just a bag of concrete datatypes?

Scala 2.8 collection when it came out was a shining example of what a Scala library can be, and I think it's one of the best things about Scala. Over time, we did find various warts both in its design and implementation; and one of them I think was the fact that traits were broken by default to accommodate various subclasses like parallel collections, mutable, and views. My impression of Scala 2.13 collection is that it tries to keep all the good parts while fixing the warts from the old collection.

Coming back to scala/bug#11785, there's no good answer here because Iterable#toString will be broken out-of-the-box one way or the other.

  • it's broken if Iterator.continually(42) can cause OutOfMemoryError.
  • it's broken if toString requires an override or mixin of StrictOptimizedIterableOps to get Foo(1, 2, 3).

In general, I think someone creating a custom collection and/or Iterable is rare use case, and I don't think it matters much compared to (for example) performance and UX of Vector etc. But having said that, I think we should embrace the possibility of infinite collection if that trait's contract is such, and avoid known OutOfMemoryError. Perhaps we could also add a note in Scaladoc to explain the scala/scala@dead51b behavior.

@SethTisue
Copy link
Member Author

I'll revert the PR in a few days (for 2.13.2) if nobody else has anything further they want to say.

@SethTisue
Copy link
Member Author

scala/scala#8847

hamzaremmal pushed a commit to hamzaremmal/scala3 that referenced this issue May 2, 2025
hamzaremmal pushed a commit to scala/scala3 that referenced this issue May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants