Skip to content
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

case class copy method should not be currying - Scala 2.10.0-M4 #5907

Closed
scabug opened this issue Jun 13, 2012 · 12 comments
Closed

case class copy method should not be currying - Scala 2.10.0-M4 #5907

scabug opened this issue Jun 13, 2012 · 12 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Jun 13, 2012

I believe the following code is wrong:

case class C(i: Int)(implicit m: Int) { def inc = copy(i = i+1) } 
>defined class C

implicit val n = 1
>n: Int = 1

scala> C(0)
res1: C = C(0)

scala> res1.inc
res2: Int => C = <function1>

res2 should be a new instance of C

@scabug
Copy link
Author

scabug commented Jun 13, 2012

Imported From: https://issues.scala-lang.org/browse/SI-5907?orig=1
Reporter: @etorreborre
See #5009, #6016

@scabug
Copy link
Author

scabug commented Jun 14, 2012

@lrytz said:
i'd say "not a bug"..

@scabug
Copy link
Author

scabug commented Jun 14, 2012

@paulp said:
I hadn't really thought about implicits when this was dreamed up, but now it seems like a big problem. There's just no way to see this as anything but a regression:

scala> case class Foo[T: Ordering](x: T)
defined class Foo

scala> Foo(5)
res0: Foo[Int] = Foo(5)

scala> res0.copy(x = 10)
res1: Ordering[Int] => Foo[Int] = <function1>

@scabug
Copy link
Author

scabug commented Jun 14, 2012

@adriaanm said:
I agree we should supply implicit arguments before considering the need for eta-expansion.

@scabug
Copy link
Author

scabug commented Jun 15, 2012

@paulp said (edited on Jun 15, 2012 10:28:49 PM UTC):
Here's another reason, my email to scala-internals about #5929.

Oh, the interesting things one learns trying to compile scalatest against
the right version of scalacheck.  It didn't compile, actually, which led
me to this little bundle of badness.

  case class Foo(x: Int)(implicit y: Foo) { }
  def f(args: List[Foo]) = if (false) args map (_.copy()) else args

What return type is to be inferred for f? In m3 it is List[Foo].  In m4
it is List[Object], because of https://issues.scala-lang.org/browse/SI-5907 .

In scalatest, the affected method is

  private def getArgsWithSpecifiedNames(argNames: Option[List[String]], scalaCheckArgs: Prop.Args) = {
    if (argNames.isDefined) {
      // length of scalaCheckArgs should equal length of argNames
      val zipped = argNames.get zip scalaCheckArgs
      zipped map { case (argName, arg) => arg.copy(label = argName) }
    }
    else
      scalaCheckArgs
  }

@scabug
Copy link
Author

scabug commented Jun 20, 2012

@lrytz said:
i'll have a look

@scabug
Copy link
Author

scabug commented Jun 30, 2012

Ryan Hendrickson (ryan.hendrickson_bwater) said (edited on Jun 30, 2012 12:38:17 AM UTC):
This isn't limited to just implicits; the regression affects any case class with more than one parameter list:

case class Sad(i: Int)(j: Int)
Sad(0)(0).copy()()

In 2.9, this yielded another Sad(0)(0). In 2.10.0-M4, it's error: not enough arguments for method apply, thanks to the curried Function1 not having a default argument.

@scabug
Copy link
Author

scabug commented Jun 30, 2012

@paulp said:
Well yeah, the non-implicit second parameter list is the whole reason we did it in the first place. It's an error on purpose.

@scabug
Copy link
Author

scabug commented Jun 30, 2012

@paulp said:
Injecting my comment from the scala-internals thread because it seems plausibly among the least bad ways to go from here.


Hmm, maybe we should let people declare the copy signature they want, and accommodate it if possible.

case class C(x: Int)(y: Int)(implicit z: Int) {
  def copy(x: Int = x)(y: Int = y): C  // this way z is fixed at construction, and gets a field
  def copy(x: Int = x)(y: Int = y)(implicit z: Int = z): C // this way z is an optional implicit, and gets a field
  def copy(x: Int = x)(y: Int = y)(implicit z: Int): C // this way z is a mandatory implicit, no field
  def copy(x: Int = x)(y: Int)(implicit z: Int): C // and this way neither y nor z gets a field
}

@scabug
Copy link
Author

scabug commented Jul 1, 2012

Ryan Hendrickson (ryan.hendrickson_bwater) said:
Ah, sorry, didn't realize there was an intended spec change here. Is there a thread or something to which someone could point me that explains why this is a desirable change?

@scabug
Copy link
Author

scabug commented Jul 1, 2012

@paulp said:
#5009

@scabug
Copy link
Author

scabug commented Jul 5, 2012

@lrytz said:
fixed in [https://github.com/scala/scala/commit/6c7f2b6460de1aa6d15a6005921ca50e98a54027]

@scabug scabug closed this as completed Jul 5, 2012
@scabug scabug added the critical label Apr 7, 2017
@scabug scabug added this to the 2.10.0-M4 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants