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

Jump to bottom

case class copy method shouldn't subvert private constructor #7884

Closed
scabug opened this issue Sep 29, 2013 · 9 comments
Closed

case class copy method shouldn't subvert private constructor #7884

scabug opened this issue Sep 29, 2013 · 9 comments
Labels
case class fixed in Scala 3 This issue does not exist in the Scala 3 compiler (https://github.com/lampepfl/dotty/) help wanted
Milestone
2.13.2

Comments

@scabug
Copy link

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
Copy link
Author

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
Copy link
Author

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.

@scabug scabug added this to the Backlog milestone Apr 7, 2017
@scabug scabug mentioned this issue Apr 7, 2017
Closed
@akauppi
Copy link

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?

@akauppi
Copy link

akauppi commented Jun 22, 2018

Have mentioned this in https://users.scala-lang.org/t/ending-the-confusion-of-private-case-class-constructor-in-scala-2-13-or-2-14/2915

@SethTisue
Copy link
Member

SethTisue commented Jun 22, 2018 •
edited

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

akauppi commented Jun 23, 2018

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

@akauppi
Copy link

akauppi commented Jun 24, 2018 •
edited

@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
Copy link
Contributor

adriaanm commented Jun 25, 2018 •
edited

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)

@gpampara gpampara mentioned this issue Jun 28, 2018
Closed
@adriaanm adriaanm modified the milestones: Backlog, 2.14.0-M1 Nov 20, 2018
@adriaanm adriaanm added the fixed in Scala 3 This issue does not exist in the Scala 3 compiler (https://github.com/lampepfl/dotty/) label Nov 20, 2018
@som-snytt som-snytt mentioned this issue Nov 27, 2018
Merged
@SethTisue
Copy link
Member

related: scala/scala3#5472

Jasper-M added a commit to Jasper-M/scala that referenced this issue Jan 31, 2019
@Jasper-M
Case class copy and apply inherit access modifiers from constructor
b4630df 
Fixes scala/bug#7884
Jasper-M added a commit to Jasper-M/scala that referenced this issue Feb 7, 2019
@Jasper-M
Case class copy and apply inherit access modifiers from constructor
9b3a2cb 
Fixes scala/bug#7884
Jasper-M added a commit to Jasper-M/scala that referenced this issue Sep 9, 2019
@Jasper-M
Case class copy and apply inherit access modifiers from constructor
7bf8736 
Fixes scala/bug#7884
@dwijnand dwijnand modified the milestones: 2.14.0-M1, 2.13.2 Feb 13, 2020
@jatcwang jatcwang mentioned this issue Mar 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
case class fixed in Scala 3 This issue does not exist in the Scala 3 compiler (https://github.com/lampepfl/dotty/) help wanted
Projects
None yet
Milestone
2.13.2
Development

No branches or pull requests
5 participants
@adriaanm @SethTisue @dwijnand @akauppi @scabug