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 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

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 13, 2012

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 14, 2012

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

@scabug

This comment has been minimized.

Copy link
Author

@scabug 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

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 14, 2012

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

@scabug

This comment has been minimized.

Copy link
Author

@scabug 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

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 20, 2012

@lrytz said:
i'll have a look

@scabug

This comment has been minimized.

Copy link
Author

@scabug 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

This comment has been minimized.

Copy link
Author

@scabug 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

This comment has been minimized.

Copy link
Author

@scabug 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

This comment has been minimized.

Copy link
Author

@scabug 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

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 1, 2012

@paulp said:
#5009

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 5, 2012

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

@scabug scabug closed this 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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.