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

ClassTag instances can cause runtime exceptions #25

Closed
carymrobbins opened this issue Apr 21, 2018 · 2 comments
Closed

ClassTag instances can cause runtime exceptions #25

carymrobbins opened this issue Apr 21, 2018 · 2 comments

Comments

@carymrobbins
Copy link
Member

ClassTags are used by scalac during type-based pattern matching. This is a problem for newtypes (but not newsubtypes) since their ClassTag runtime class is Object. As such, we are permitting users via our "safe" API to do unsafe things without it being obvious that what they are doing will definitely result in a runtime exception.

scala> import io.estatico.newtype.macros._, io.estatico.newtype.ops._

scala> @newtype case class Foo(x: String)

scala> (1: Any) match { case x: Foo => x ; case _ => Foo("nope") }
res0: Foo = 1

scala> res0.coerce[String]
java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String
  ... 38 elided

I think the solution for now is to remove ClassTag instances altogether. The only reason I think they're useful is for constructing Arrays, but I think this is the wrong solution. I have an experimental (and incomplete) branch for a different way of handling Arrays here (via an AsArray type class). I'm looking to try to use this to resolve #10 in a type safe way.

In the meantime, I think it's best to remove the ClassTag instances to prevent unsafe code from happening by accident.

Also see my original point about this with regard to the unapply method here.

carymrobbins added a commit that referenced this issue Apr 21, 2018
refs #25: Removing ClassTag instances
@carymrobbins
Copy link
Member Author

/cc @joroKr21 See the above comment regarding the removal of ClassTag instances. I think the work being done for #10 should eliminate the need for them, but feel free to let me know if I'm missing something.

@joroKr21
Copy link
Contributor

Sounds good 👍

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