sbt / sbt-remote-control

Create and manage sbt process using unicorns and forks
Other
74 stars 14 forks source link

protocol/Keys.scala uses scala.Seq, not immutable.Seq. #57

Open skyluc opened 10 years ago

skyluc commented 10 years ago

In the wip/jsuereth-model-view-server branch, scala.Seq is used, like in Keys.scala. It would be better to use scala.collection.immutable.Seq.

jsuereth commented 10 years ago

Why is it better? Just curious....

skyluc commented 10 years ago

Really?

scala.Seq is a type alias to scala.collection.Seq, so it cannot be assumed to be immutable. It leads to some fun code:

  case class Foo(s: Seq[Int]) {
    def sum = s.sum
  }

  val s = ListBuffer(1, 2)                        //> s  : scala.collection.mutable.ListBuffer[Int] = ListBuffer(1, 2)
  val f = Foo(s)                                  //> f  : testa.Foo = Foo(ListBuffer(1, 2))

  f.sum                                           //> res0: Int = 3
  s.append(3)
  f.sum                                           //> res1: Int = 6

  val f2 = Foo(s.to[List])                        //> f2  : testa.Foo = Foo(List(1, 2, 3))

  f == f2                                         //> res2: Boolean = true
  s.append(4)
  f == f2                                         //> res3: Boolean = false
jsuereth commented 10 years ago

True, but it's ok to mutate something locally and expose an immutable interface to it as long as you give up control of the mutability once you pass the reference to someone. It's one of those nice optimisation tricks. Again, mutability should never escape a single method/object and then you're ok.

Practically, anyone could implement immutable.Seq with a mutable collection, it's just evil to do so. As is passing a mutable collection to something taking Seq[_] and then mutating later :)

In this case though, I may be being pedantic, because I don't expect users to construct any of the values in Keys. Keys represents a read-only view of data for clients. Personally I still prefer collection.Seq for flexibility in local implementations.