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

Support @newtype case classes if not inside object #11

Closed
oleg-py opened this issue Mar 3, 2018 · 11 comments
Closed

Support @newtype case classes if not inside object #11

oleg-py opened this issue Mar 3, 2018 · 11 comments

Comments

@oleg-py
Copy link

oleg-py commented Mar 3, 2018

I'm making a web app and using newtypes to for type-safe IDs for different model types.

As there are quite a few model types, I tried to move type code into a trait that can be mixed into companion object:

trait OwnIdType {
  @newtype case class ID(toInt: Int)

  object ID {
    // Circe things
    implicit val toJson: Encoder[ID] = deriving
    implicit val fromJson: Decoder[ID] = deriving
  }
}

That didn't work, however, telling me that fields can only be used if the newtype is defined inside an object

Ultimately I wrote case class boilerplate myself:

trait OwnIdType {
  @newtype class ID(underlying: Int)

  object ID {
    def apply(value: Int): ID = value.coerce

    implicit class Ops(val id: ID) {
      def toInt: Int = id.coerce
    }

    implicit val toJson: Encoder[ID] = deriving
    implicit val fromJson: Decoder[ID] = deriving
  }
}

It compiles and works, and the only significant difference is that Ops class cannot extend AnyVal.

Is it possible to support @newtype case class inside traits by dropping extends AnyVal on ops class?

@carymrobbins
Copy link
Member

@oleg-py Maybe you can use a private val ? So long as fields aren't defined for your newtype, you can define them in traits/classes -

trait OwnIdType {
  @newtype case class ID(private val toInt: Int)

  object ID {
    // Circe things
    implicit val toJson: Encoder[ID] = deriving
    implicit val fromJson: Decoder[ID] = deriving
  }
}

What I would then do is just use .coerce where you need to convert from one to the other.

If you really want an extension method on it, you could maybe just write a ToInt type class and have your ID newtype derive the instance for it, but that seems a bit overkill when you could just do .coerce

@carymrobbins
Copy link
Member

@oleg-py I went ahead and tested out the idea, here's a basic example of using this strategy with the private val, coerce, and a ToInt type class. It still seems to me that just using .coerce would be much simpler; ideally, you shouldn't have to swap between Int and ID too often.

package example

import io.estatico.newtype.macros.newtype
import io.estatico.newtype.ops._

object Example {
  def main(args: Array[String]): Unit = {
    val zebra = Zebra(Zebra.ID(1), "zeb")
    // Using coerce, no need for the ToInt type class
    val zebraIntId1 = zebra.id.coerce[Int]
    // Using the ToInt type class' ops
    import ToInt.ops._
    val zebraIntId2 = zebra.id.toInt
  }
}

trait OwnIdType {
  @newtype case class ID(private val toInt: Int)
  object ID {
    implicit val toInt: ToInt[ID] = deriving
  }
}

case class Zebra(id: Zebra.ID, name: String)
object Zebra extends OwnIdType

// ToInt type class example
trait ToInt[A] {
  def toInt(a: A): Int
}

object ToInt {

  implicit val intToInt: ToInt[Int] = new ToInt[Int] {
    override def toInt(a: Int): Int = a
  }

  final class Ops[A](val repr: A) extends AnyVal {
    def toInt(implicit ev: ToInt[A]): Int = ev.toInt(repr)
  }

  trait ToOps {
    implicit def toToIntOps[A](x: A): Ops[A] = new Ops(x)
  }

  val ops = new ToOps {}
}

@oleg-py
Copy link
Author

oleg-py commented Mar 10, 2018

@carymrobbins not sure what problem ToInt typeclass solves.

My point is that it is possible to have extension methods on @newtypes defined inside traits, using the same code as @newtype macro would generate, only without extends AnyVal part. It would be nice if the library supported such use-case directly.

@carymrobbins
Copy link
Member

@oleg-py The problem is that it becomes less efficient than just using a case class in place of the newtype. If our Ops does not extend AnyVal, we'll get a new allocation of the Ops class every time an extension method is invoked. The ToInt type class avoids that by still defining the Ops in an object, permitting the use of AnyVal.

@carymrobbins
Copy link
Member

To clarify, the reason I'm not comfortable with that feature is that it seems to undermine the point of using newtype in the first place.

@oleg-py
Copy link
Author

oleg-py commented Mar 15, 2018

@carymrobbins sorry for the long reply.

I use newtypes to avoid extra boxing, giving me easy support for H2 and Postgres arrays.

As far as extension methods go, I expect performance overhead of not having AnyVal to be zero / negligible (well, on HotSpot JVM). So I would quite like to have it, maybe as an option on annotation.

@carymrobbins
Copy link
Member

@oleg-py While working on @newsubtype I discovered that Scala 2.10 can't handle AnyVal Ops for something like Int with Tag, so Scala 2.10 users won't get AnyVal Ops in those cases. As such, I don't think it's too much of a compromise to allow users to opt-out via a flag.

What about something like this?

@newtype(boxedExtensions = true) case class ID(toInt: Int)

Naming things is hard, I'm open to suggestions. Also considering -

@newtype(anyValOps = false) case class ID(toInt: Int)

I think I can include this change in the next release, along with @newsubtype.

@oleg-py
Copy link
Author

oleg-py commented Mar 19, 2018

@carymrobbins sounds amazing 👍

I would name it something along the lines of disableOpsOptimization = true, seems more explicit w.r.t. what it actually is doing. But yeah, naming hard 😅

@carymrobbins
Copy link
Member

Yeah that seems slightly better, I'd change the negative = true though, e.g.

@newtype(optimizeOps = false) case class ID(toInt: Int)

I like this also because it doesn't really guarantee that things are optimized at any particular level, just that they will be to the extent that we can (and I can even disallow optimizeOps = true).

@carymrobbins
Copy link
Member

@oleg-py PR which provides support for this - #17

@carymrobbins
Copy link
Member

This has been merged to master and will be available in the 0.4.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants