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.

Sign up for GitHub

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

Don't allow objects to be cloned #3764

Closed
scabug opened this issue Aug 15, 2010 · 8 comments · Fixed by scala/scala#10677
Closed

Don't allow objects to be cloned #3764

scabug opened this issue Aug 15, 2010 · 8 comments · Fixed by scala/scala#10677
Assignees
@som-snytt
Labels
enhancement typer
Milestone
2.13.13

Comments

@scabug
Copy link

scabug commented Aug 15, 2010

When an "object" is placed into a package or another such object, it is assumed to be a singleton and the compiler apparently optimizes "this" references in closures within methods that object to refer to the singleton.

This assumption is incorrect when the object is clone()d, as demonstrated by this snippet:

object ObjectIdentityTest {
   def main(args: Array[String]) { new ObjectIdentityTest() }

   class Base extends Cloneable {
     override def clone() = super.clone()
     def foo = { println("  hashCode = "+this.hashCode()); true }
   }

   object A extends Base { def bar = List(1).filter(_ => foo) }

   val B = new Base { def bar = List(1).filter(_ => foo) }
}

class ObjectIdentityTest {
   import ObjectIdentityTest._

   object C extends Base { def bar = List(1).filter(_ => foo) }

   println("Original A:")
   A.foo
   A.bar

   val clonedA = A.clone().asInstanceOf[A.type]
   println("Cloned A:")
   clonedA.foo
   clonedA.bar

   println("Original B:")
   B.foo
   B.bar

   val clonedB = B.clone().asInstanceOf[B.type]
   println("Cloned B:")
   clonedB.foo
   clonedB.bar

   println("Original C:")
   C.foo
   C.bar

   val clonedC = C.clone().asInstanceOf[C.type]
   println("Cloned C:")
   clonedC.foo
   clonedC.bar
}

The output looks like this:

Original A:
   hashCode = 837043581
   hashCode = 837043581
Cloned A:
   hashCode = 1125883825
   hashCode = 837043581 <-- still the original A!
Original B:
   hashCode = 1251033058
   hashCode = 1251033058
Cloned B:
   hashCode = 183062162
   hashCode = 183062162
Original C:
   hashCode = 1894479961
   hashCode = 1894479961
Cloned C:
   hashCode = 1613816448
   hashCode = 1613816448

For B and C this is as expected but for object A, calling bar() on a
clone calls foo() on the original object.

The compiler can recognize this situation because the object in question extends java.lang.Cloneable. In this case, the object should not be assumed to be a singleton (with regards to the optimizations the compiler may perform).
@scabug
Copy link
Author

scabug commented Aug 15, 2010

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

@scabug
Copy link
Author

scabug commented Aug 17, 2010

@lrytz said:
maybe we should disallow objects to extend Cloneable

@scabug
Copy link
Author

scabug commented Jan 26, 2011

@paulp said:
Or, it occurs to me now that there's a "scala.Serializable", maybe we should stop settling for all the brokenness that comes with clone() and create a scala.Cloneable. (And then not let objects extend it.)

@scabug
Copy link
Author

scabug commented Aug 6, 2012

Naftoli Gugenheim (naftoligug) said:
The reason why Stefan reported this is that ScalaQuery/Slick currently rely on cloning singletons.

@scabug
Copy link
Author

scabug commented May 2, 2013

@adriaanm said:
This was driven by a use case in Slick, but should probably be worked around there. We really want objects to stay singletons.

@scabug
Copy link
Author

scabug commented May 2, 2013

@adriaanm said:
Thus, the resolution for this ticket is as Paul suggested: have a separate notion of cloneable and don't let objects be cloneable.

@scabug
Copy link
Author

scabug commented May 3, 2013

@szeiger said:
After thinking about this some more, I think our conclusion from yesterday's discussion was wrong. The crucial point is that an object's singleton type should not be violated, and while Slick does that at the moment and it is also used in the code example above, it is only a tangent. I don't see a good reason why the implementation of an object should not be cloned as long as the clone does not pretend to have the same singleton type as the original. Here is a modified example without the unsoundness of the original one:

object ObjectIdentityTest {
   def main(args: Array[String]) { new ObjectIdentityTest() }

   abstract class Base extends Cloneable {
     override def clone(): Base = super.clone().asInstanceOf[Base]
     def foo = { println("  hashCode = "+this.hashCode()); true }
     def bar: Unit
   }

   object A extends Base { def bar = List(1).filter(_ => foo) }
}

class ObjectIdentityTest {
   import ObjectIdentityTest._

   println("Original A:")
   A.foo
   A.bar

   val clonedA = A.clone()
   println("Cloned A:")
   clonedA.foo
   clonedA.bar
}

@scabug scabug added the typer label Apr 7, 2017
@scabug scabug added this to the Backlog milestone Apr 7, 2017
@som-snytt som-snytt mentioned this issue Jan 27, 2024
Merged
@som-snytt
Copy link

Since the statute of limitations has run out, the linked PR just lints it.

Thus, the resolution of this ticket is as lrytz suggested: except it's just a lint.

Thankfully, there is no need to deprecate scala.Cloneable.

@SethTisue SethTisue modified the milestones: Backlog, 2.13.13 Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@som-snytt som-snytt

Labels
enhancement typer
Projects
None yet
Milestone
2.13.13
Development

Successfully merging a pull request may close this issue.

Lint cloneable modules som-snytt/scala
3 participants
@SethTisue @som-snytt @scabug