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 shouldn't subvert private constructor #7884

Open
scabug opened this Issue Sep 29, 2013 · 9 comments

Comments

Projects
None yet
4 participants
@scabug

scabug commented Sep 29, 2013

case class Foo private (num: Int)

object Foo {
  def apply(x: Double): Foo = x.toInt match {
    case x if x < 0 => throw new Exception(x + " < 0")
    case x          => new Foo(x)
  }
}

object Test {
  def main(args: Array[String]): Unit = {
    val x = Foo(5d)
    println(x -> x.copy(num = -1000))
  }
}

// % scala Test
// (Foo(5),Foo(-1000))
@scabug

This comment has been minimized.

scabug commented Sep 29, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7884?orig=1
Reporter: @paulp
Affected Versions: 2.10.4, 2.11.5

@scabug

This comment has been minimized.

scabug commented Feb 20, 2015

@retronym said:
Workaround for those in the know is to define a different method named copy to prevent the creation of the synthetic one.

I think the principled thing is to give the synthetic copy method the same access modifiers as the primary constructor. This isn't a source compatible change, however, so it will be some work to slowly put the genie back in the bottle.

@akauppi

This comment has been minimized.

akauppi commented Jun 22, 2018

I made a repo to exercise and study this behavior a bit.

Had been touched by it once or twice, last year, when I wanted to have a case class that only gets created internally. Had noticed that not defining the .apply allowed outside creation of instances, which I thought surprising.

I agree with @scabug that "principled thing is to give the synthetic copy method the same access modifiers as the primary constructor." I would extend that to the created .apply feature, as well.

How to suggest this for 2.13?

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jun 22, 2018

How to suggest this for 2.13?

I think a pull request on this would likely be accepted.

UPDATE: on the Discourse thread, @smarter suggests a SIP would be necessary. this is a modest enough change that involving the SIP committee seems like overkill to me, but, I'd bow to consensus on the subject

@akauppi

This comment has been minimized.

akauppi commented Jun 23, 2018

I'll be likely to work on this in July.

@akauppi

This comment has been minimized.

akauppi commented Jun 24, 2018

@SethTisue I'd be happy to make SIP, but plan to do so after I've got a PR with associated tests ready (hopefully this July). One reason to go SIP is that (to what I've seen so far) the current behaviour doesn't seem to be clearly defined, rather it "just is". I'll see that better when making changes to Scala, and if those changes don't break existing tests, that would useful information.

@adriaanm

This comment has been minimized.

Member

adriaanm commented Jun 25, 2018

The copy method is synthesized here: https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/typechecker/Unapplies.scala#L176-L233. Its signature is completed by https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/typechecker/Namers.scala#L593-L616

EDIT: By the way, this is a tricky part of the compiler. Case in point: scala/scala#5730, which needed scala/scala#5845. (Fixed in 2.11.11 and 2.12.2)

@SethTisue

This comment has been minimized.

Member

SethTisue commented Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment