Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Make scala.Seq = strawman.collection.immutable.Seq #149

Closed
julienrf opened this issue Jul 4, 2017 · 14 comments
Closed

Make scala.Seq = strawman.collection.immutable.Seq #149

julienrf opened this issue Jul 4, 2017 · 14 comments

Comments

@julienrf
Copy link
Contributor

julienrf commented Jul 4, 2017

No description provided.

@EPronovost
Copy link
Contributor

Is this a step towards #179 ?

@julienrf julienrf added this to the 0.11.0 milestone Feb 19, 2018
@szeiger
Copy link
Member

szeiger commented Mar 1, 2018

Other non-immutable collection types in package object scala:

  • type IterableOnce[+A] = scala.collection.IterableOnce[A]: New, replaces TraversableOnce, no immutable version

  • type Iterator[+A] = scala.collection.Iterator[A]: Like IterableOnce this has no immutable version

  • type Iterable[+A] = scala.collection.Iterable[A]: There is an immutable.Iterable but I suggest we keep this one as the lowest common denominator for abstracting over collection types.

  • type IndexedSeq[+A] = scala.collection.IndexedSeq[A]: Since we're changing Seq we should also change IndexedSeq

  • type BufferedIterator[+A] = scala.collection.BufferedIterator[A]: Why is this even here? BufferedIterator is not a very common type. I suggest deprecating the alias.

  • type StringBuilder = scala.collection.mutable.StringBuilder: Probably quite common, I'd keep it for now

@Ichoran
Copy link
Contributor

Ichoran commented Mar 1, 2018

Incidentally, if we're going to add immutable iterators, now would be the time to do it. They're annoying to use because the signature works out to be equivalent to

def advance: Option[(Immuterator[A], A)]

which imposes an additional burden of keeping track of the advanced state of the immutable iterator.

But since we're thinking about an enhanced immutable interface, I thought it was worth raising.

Note for those wondering about usage: immutable iterators are best consumed in tail-recursive methods, e.g.

def sum(ii: Immuterator[Int], accumulator: Int = 0) = ii.advance match {
  case Some((ij, n)) => sum(ij, accumulator + n)
  case None => accumulator
}

and similar constructs.

@SethTisue
Copy link
Member

@szeiger agreed on all points

@julienrf
Copy link
Contributor Author

julienrf commented Mar 1, 2018

@Ichoran what’s the advantage over pattern matching on case n :: ij?

@SethTisue
Copy link
Member

could we move the immutable-iterator discussion to a new ticket?

@szeiger
Copy link
Member

szeiger commented Mar 2, 2018

Further issues:

  • Implicit conversions from Array to Seq are very common (at least in the compiler codebase). I'll add a deprecated even-lower-priority-than-WrappedArray conversion from Array to immutable.IndexedSeq that copies the array.

  • Should toSeq and toIndexedSeq always return the immutable version? It would be nicer if toSeq actually gave you a Seq (and not just a collection.Seq). When used on an existing mutable collection they would have copy it anyway to avoid accidental sharing so it shouldn't be much of a performance issue. Never mind, they already do

@szeiger
Copy link
Member

szeiger commented Mar 2, 2018

This "immutable iterator" looks like a Stream or LazyList to me, assuming advance doesn't really advance anything. If it does, it is no longer immutable, just a less efficient mutable iterator.

@Ichoran
Copy link
Contributor

Ichoran commented Mar 2, 2018

In brief: immutable iterators don't cache anything (unlike LazyList), and are a generalization of head-tail decomposition to collections where tail isn't terribly efficient. But, yeah, if we wanted to do this we should talk about it on another ticket.

@dwijnand
Copy link
Member

dwijnand commented Mar 6, 2018

type Iterable[+A] = scala.collection.Iterable[A]: There is an immutable.Iterable but I suggest we keep this one as the lowest common denominator for abstracting over collection types.

I'd bias the other way: if you want to abstract over immutable and mutable collections you should have to use scala.collection.Iterable[A].

@sjrd
Copy link
Member

sjrd commented Mar 6, 2018

I wholeheartedly agree with @dwijnand. Everything else in Predef is now immutable by default. We should keep Iterable immutable as well. When you write code that takes an Iterable in Scala, one would first assume that it is immutable. Handling possible mutations under one's feet is tricky, and should be explicit.

@szeiger szeiger removed this from the 0.11.0 milestone Apr 19, 2018
@julienrf julienrf reopened this Apr 20, 2018
@julienrf
Copy link
Contributor Author

Reopening because we might want to make scala.Iterable[+A] = scala.collection.immutable.Iterable[A] as well…

@dwijnand
Copy link
Member

(given the description of this ticket, maybe it's better to create a new one to track that)

@julienrf
Copy link
Contributor Author

@dwijnand done!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants