-
Notifications
You must be signed in to change notification settings - Fork 21
case class optimisation #10659
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
Comments
Under what circumstances? What code do you expect to write, exactly, and what do you expect would be generated? |
The ticket has the expected input and output, what am I missing? |
Damn, we found that this optimisation breaks exhaustivity, see scalaz/scalaz#1579 (comment) |
I think @sjrd is looking for a more formalized version of the codegen (?). I agree this would be useful, I’ve seen this pattern in many places - it’s quite common when you define tree-like data structures and it’s not clear for beginners what they should do to have a singleton empty. |
Then it's not sound in general: final case class Empty[A]() extends Maybe[A] {
var oops: A = _
}
val e1 = Empty[Int]()
e1.oops = 5
val e2 = Empty[String]()
e2.oops = "foo"
println(e1.oops) // prints "foo"
val s = e1.oops // throws ClassCastException
println(s) And anyway, people could rely on the identity of the |
We may be willing to accept the loss of exhaustiveness checking in scalaz, which might make this a reasonable optimisation still to make. I feel like there should be some form of epic to capture optimisations that can be made when code is pure. Your I feel like I could code this up, but I don't understand what you're asking in terms of a formal spec... so anybody who feels they know how to write formal specs can comment here. I shan't be adding one. |
the pattern is flawed. |
it is quite common when writing invariant ADTs that have an "empty" element to use something along the lines of the following encoding
but this incurs an object allocation overhead that must be mitigated with the following (ugly) pattern
it would be fantastic if the compiler automatically added the
apply()
andunapply()
under these circumstances.The text was updated successfully, but these errors were encountered: