case classes and varargs

[scabug]

This seems an unfortunate property for an immutable object.

scala> case class Foo(x: Int, xs: Int*)
defined class Foo

scala> val x = Foo(5, 10, 15)
x: Foo = Foo(5,WrappedArray(10, 15))

scala> x.xs.asInstanceOf[collection.mutable.WrappedArray[Int]].array(0) = 123

scala> x
res0: Foo = Foo(5,WrappedArray(123, 15))

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-4919?orig=1
Reporter: @paulp

[scabug]

@retronym said (edited on Aug 17, 2011 5:43:58 AM UTC):
The behaviour isn't any different from case class Foo(x: Int, xs: collection.Seq[Int]), I'm not so sure it's a big problem. Or if it is deemed a problem, it's not exclusive to case classes -- they don't have exclusive rights to immutability.

Maybe there is a place for a lint check on this.

[scabug]

@paulp said:
What would I say in a lint warning? "Don't use varargs" ? It's different because "case class Foo(x: Int, xs: collection.Seq[Int])" explicitly takes a value for which a mutable seq is legal, and the opportunity is to not supply one is visible. In this case, the existence of that value is hidden; the user supplies nothing but integers; the case class is supposed to be immutable; we are the ones wrapping it, and the wrapper being used has a public array field. I say it is dereliction of wrapping duty.

[scabug]

@retronym said (edited on Aug 17, 2011 1:19:20 PM UTC):
Okay, you're proposing to wrap in an immutable structure when varargs are provided. But the caller can still pass mutableSeq : _*, so you don't have any guarantee about immutability.

This reminds me a bit of the lead up to 2.8 when hashCode for case classes rejected mutable Seq-s.

http://www.scala-lang.org/node/2600

[scabug]

@paulp said:
It's kind of like how under the law, if one has a "reasonable expectation" of privacy then the rules are different. The creator of Foo(5, 10, 15) has a reasonable expectation of immutability. The creator of { val xs = Array(10, 15) ; Foo(5, xs: _*) } does not.

[scabug]

@adriaanm said:
we discussed this in the meeting and decided those who cast the first stone should not expect immutability

[scabug]

@adriaanm said:
I meant "those who cast [to mutable types]"

[scabug]

@paulp said:
If anyone could do this:

  def f(xs: List[Int]): List[Int] = { xs.tail = List(-1, -2, -3) ; xs }

would that be a problem? I suggest the answer is "yes, obviously." So what is the difference, other than that list is more widely used?

Think of f above as a library function (possibly the standard library) and the user of that library function as someone with a passing concern in security and data integrity. (Only a passing concern, not a serious concern, which is in any case de facto excluded for reasons beyond this one.)

[scabug]

@adriaanm said (edited on Feb 19, 2014 12:28:15 AM UTC):
To summarize the desired state: in general, Scala should lead you towards the path of correctness and immutability, so I think it makes sense to apply this logic to varargs as well, especially in case classes since they should be immutable (see their generated equals/hashCode combo).

If you care about performance use an Array, not varargs. Not sure it matters much, but varargs are the work horse of conveniently creating collections.

As usual, java interop suffers for it...

Lets revisit in 2.12. (See also: scala/scala#3489)

[scabug]

@paulp said:
And don't forget it's not just Array which compromises varargs but any mutable Seq.

scala> case class Immutable(xs: Int*)
defined class Immutable

scala> val xs = scala.collection.mutable.ListBuffer(1, 2, 3)
xs: scala.collection.mutable.ListBuffer[Int] = ListBuffer(1, 2, 3)

scala> def f(xs: Seq[Int]) = Immutable(xs: _*)
f: (xs: Seq[Int])Immutable

scala> val x = f(xs)
x: Immutable = Immutable(ListBuffer(1, 2, 3))

scala> xs ++= 1 to 10
res1: xs.type = ListBuffer(1, 2, 3, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10)

scala> x
res2: Immutable = Immutable(ListBuffer(1, 2, 3, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10))

[scabug]

@Ichoran said:
This is a different idea of mutability than Scala has. (Rust has this idea.)

That is, in Rust, mutability is a property of the context. If you are in a mutable context, you can mutate things. If you are in an immutable context, you can't. If you stick a mutable thing into an immutable context, you can't mutate it any more. (Conversely: if you stick an immutable thing into a mutable context, you can mutate it.) There are both advantages and disadvantages to this approach. It lowers surprise when you wield the system intentionally, but it makes it difficult for you to really protect something from mutation.

In Scala as it stands, mutability is a property of the field and therefore you get cases where immutable fields hold mutable objects and vice versa. This is just one example of this sort. Option(Array(1,2,3)) is another.

I do think it's bad that X* doesn't have a very clear type, so you can be thinking "immutable" and yet get a mutating version handed to you. Personally I would just disallow X* in case classes, or think of some more robust fix. If you want a vararg in your case class to make it seem like the case class can take an arbitrary number of parameters, you'll be have a bit of a rough ride:

scala> case class C(a: Int, xs: Int*)
defined class C

scala> C(1,2,3)  // toString doesn't match what we type
res0: C = C(1,WrappedArray(2, 3))

scala> C(1, List(2,3): _*)  // Wrapped type leaks through
res1: C = C(1,List(2, 3))

scala> res0 match { case C(a,b,c) => 1 + 2 + 3; case _ => 0 }  // Can match 3 args!
res2: Int = 6

scala> res0.productArity  // Wait, what?
res3: Int = 2

[scabug]

@paulp said:
Those are bugs in the synthetic toString and in the pattern matching semantics, both of which can be found in other open tickets. If you start throwing features out because there are a lot of bugs in the vicinity you won't have any language left. Varargs is the only way to write case classes and get a variable-arity synthetic apply and there will be plenty of people out there making use of it, so you probably want to focus more on fixing the obvious flaws (at least, obvious now, as we emerge from our eighteen month wontfix hiatus from the obvious) than on trying to dump a feature which goes back to the mid-2000s.

Varargs is built into the language syntax and there is no other way to do what it does. Things which can be described that way merit fixing.

[scabug]

@Ichoran said:
That would be the "more robust fix" I mentioned. What should copy do, though?

[scabug]

@paulp said:
If it's hardened such that varargs are enforced immutable, why does it matter what copy does?

[scabug]

@Ichoran said:
Not having a sensible copy behavior leaves you with partially incomplete case classes. Having a feature that adds behavior you can't get anywhere else, but which breaks your typical use cases, is less clear of a win than if you keep all typical use cases.
Here's what 2.10 does:

scala> case class C(a: Int, b: Int, c: Int)
defined class C

scala> C(1,2,3).copy(4,5)
res0: C = C(4,5,3)

scala> case class D(a: Int, xs: Int*)
defined class D

scala> D(1,2,3).copy(4,5)
<console>:10: error: value copy is not a member of D
              D(1,2,3).copy(4,5)

If this is something with a natural fix, then great. But if it doesn't have a natural fix, maybe the privileged position of varargs in case classes is less of a win than we think.

[scabug]

@Ichoran said (edited on Feb 20, 2014 1:19:40 AM UTC):
This sort of works, but I'm not sure it follows the principle of least surprise (and it can't completely get around the default args vs. varargs limitation):

scala> case class D(a: Int, bs: Int*) {
     |   def copy(a: Int = this.a): D = new D(a, bs: _*)
     |   def copy(a: Int, bs: Int*): D = new D(
     |     a,
     |     (if (bs.length < this.bs.length) bs ++ this.bs.drop(bs.length) else bs): _*
     |   )
     | }
defined class D

scala> D(1,2,3)
res11: D = D(1,WrappedArray(2, 3))

scala> res11.copy()
res12: D = D(1,WrappedArray(2, 3))

scala> res11.copy(4)
res13: D = D(4,WrappedArray(2, 3))

scala> res11.copy(4,5)
res14: D = D(4,ArrayBuffer(5, 3))

scala> res11.copy(4,5,6,7)
res15: D = D(4,WrappedArray(5, 6, 7))

[scabug]

@paulp said:
I didn't understand the question - I didn't know that copy doesn't even exist on varargs case classes. But it doesn't seem relevant. It's just another bug or missing feature. The copy method is a lot more recent entrant than varargs case classes. If you want to change something, change that - it should be a macro anyway.

[scabug]

@Ichoran said:
Regardless of history, I'd like all language features to be consistent and unsurprising. Since that won't/can't happen, I at least like things to go in that direction, preferably without dead-ends. I'm just trying to assess whether vararg case classes is a dead end. I'm pretty sure that copy is now used a lot more than varargs.

I agree that copy should be a macro, and one for an inlined extension method (effectively--just insert all the code at the call site). Having it as a method doesn't save any work over having it all at the call-site anyway, and requiring the type at the call site is fine since case classes are typically intended to be leaves in the inheritance hierarchy.

I'm still not entirely sure that the copy semantics can be coherent with varargs semantics, but I'm less doubtful than before.

[scabug]

@SethTisue said:
I wouldn't shed a tear if varargs case classes were deprecated.

[xuwei-k]

related to #11040

[lrytz]

This issue is fixed by the fact that scala.Seq is now immutable.Seq. The only way to provoke a modification of a _* class parameter is by using an immutable.Seq which is actually mutable, such as immutable.ArraySeq, but that's by design.

scala> case class Foo(x: Int, xs: Int*)

scala> val x = Foo(1,2,3)
x: Foo = Foo(1,ArraySeq(2, 3))

scala> x.xs.asInstanceOf[collection.immutable.ArraySeq[Int]].unsafeArray.asInstanceOf[Array[Int]].update(0, 33)

scala> x
res16: Foo = Foo(1,ArraySeq(33, 3))