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

akka.util.Reflect.findConstructor doesn't consider value classes #16444

Closed
ivanyu opened this Issue Dec 2, 2014 · 10 comments

Comments

Projects
None yet
4 participants
@ivanyu
Contributor

ivanyu commented Dec 2, 2014

Hello.

Suppose we have a value class and an actor that receives an object of this value class as a constructor parameter:

case class WrappedInt(v: Int) extends AnyVal

class TheActor(w: WrappedInt) extends Actor { ... }

Then we do:

val props = Props(classOf[TheActor], WrappedInt(42))
system.actorOf(props)

and will get the java.lang.IllegalArgumentException: no matching constructor found on class TheActor for arguments [class me.ivanyu.luscinia.WrappedInt]. But

val props = Props(classOf[TheActor], 42)
system.actorOf(props)

works well. I'm sure the problem is in akka.util.Reflect.findConstructor implementation. If we do

println(classOf[TheActor].getConstructors.head)

we'll get public TheActor(int) (probably, it's because value classes looks like types they wrap in runtime). Possibly, findConstructor should have additional check for such a situation, or, if it's considered unnecessary, this should be documented.

@ivanyu ivanyu changed the title from akka.util.Reflect.findConstructor doesn't considers value classes to akka.util.Reflect.findConstructor doesn't consider value classes Dec 2, 2014

@ktoso ktoso added t:core bug labels Dec 2, 2014

@ktoso ktoso added this to the 2.4-M1 milestone Dec 2, 2014

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Dec 2, 2014

Member

Thanks for reporting @ivanyu!
Not sure if this is to be considered a bug or just extending Props to support value classes, but we should investigate it anyway.

Member

ktoso commented Dec 2, 2014

Thanks for reporting @ivanyu!
Not sure if this is to be considered a bug or just extending Props to support value classes, but we should investigate it anyway.

@ktoso ktoso modified the milestones: 2.4.0, 2.4-M1 Dec 2, 2014

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Dec 2, 2014

Member

Initial research shows this to be rather hard to implement nicely - either jumping around and looking for $extension fields in "places" or implicit things.

Member

ktoso commented Dec 2, 2014

Initial research shows this to be rather hard to implement nicely - either jumping around and looking for $extension fields in "places" or implicit things.

@ktoso ktoso added the 1 - triaged label Dec 3, 2014

@ivanyu

This comment has been minimized.

Show comment
Hide comment
@ivanyu

ivanyu Dec 3, 2014

Contributor

@ktoso couldn't it be done by adding something like

def checkForPossibleValueClass(found: Class[_], required: Any): Boolean = {
  if (classOf[AnyVal].isInstance(required)) {
    // For descendants of AnyVal, we're sure that there is only one constructor
    // with one parameter
    required.getClass.getConstructors.head.getParameterTypes.head == found
  } else
    false
}

and

case (found, required) ⇒
  found.isInstance(required) || BoxedType(found).isInstance(required) ||
    (required == null && !found.isPrimitive) || checkForPossibleValueClass(found, required)

in findConstructor (here)?

Contributor

ivanyu commented Dec 3, 2014

@ktoso couldn't it be done by adding something like

def checkForPossibleValueClass(found: Class[_], required: Any): Boolean = {
  if (classOf[AnyVal].isInstance(required)) {
    // For descendants of AnyVal, we're sure that there is only one constructor
    // with one parameter
    required.getClass.getConstructors.head.getParameterTypes.head == found
  } else
    false
}

and

case (found, required) ⇒
  found.isInstance(required) || BoxedType(found).isInstance(required) ||
    (required == null && !found.isPrimitive) || checkForPossibleValueClass(found, required)

in findConstructor (here)?

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Dec 3, 2014

Member

Thanks for digging into this! So it's possible to adjust your snippet to get tests passing but it's not exactly doing what you'd expect it to:

I'll have to think a bit more about it, but the classOf[AnyVal].isInstance() check is not real / valid, see here:

   case class X(i: Int) extends AnyVal
   class Nope

   val x = X(42)

scala> classOf[AnyVal].isInstance(x)
res0: Boolean = true

scala> classOf[AnyVal].isInstance(new Nope)
res1: Boolean = true       // whoop! This is no value class mate!

This gives false positives, so we have to think if we'd compromise safety when adding this feature in this way... I'll look into it and ping the issue later, thanks!

Member

ktoso commented Dec 3, 2014

Thanks for digging into this! So it's possible to adjust your snippet to get tests passing but it's not exactly doing what you'd expect it to:

I'll have to think a bit more about it, but the classOf[AnyVal].isInstance() check is not real / valid, see here:

   case class X(i: Int) extends AnyVal
   class Nope

   val x = X(42)

scala> classOf[AnyVal].isInstance(x)
res0: Boolean = true

scala> classOf[AnyVal].isInstance(new Nope)
res1: Boolean = true       // whoop! This is no value class mate!

This gives false positives, so we have to think if we'd compromise safety when adding this feature in this way... I'll look into it and ping the issue later, thanks!

@ivanyu

This comment has been minimized.

Show comment
Hide comment
@ivanyu

ivanyu Dec 3, 2014

Contributor

@ktoso
Oops, you're totally right. I haven't checked: classOf[AnyVal].isInstance(x) returns true for any x (except null). Actually, classOf[AnyVal] is just java.lang.Object. I'm very sorry for misleading.

I've asked about this on scala-user. There was a proposition to check if something is a value class in the following way:

typeOf[WrappedInt].typeSymbol.asInstanceOf[scala.reflect.internal.Symbols#Symbol]
    .isDerivedValueClass

But here, I lack for Scala internals knowledge to tell whether this is a reliable approach.

UPD: Also, I think, such runtime checks might cause actual instantiation of the a value class as a regular class.

Contributor

ivanyu commented Dec 3, 2014

@ktoso
Oops, you're totally right. I haven't checked: classOf[AnyVal].isInstance(x) returns true for any x (except null). Actually, classOf[AnyVal] is just java.lang.Object. I'm very sorry for misleading.

I've asked about this on scala-user. There was a proposition to check if something is a value class in the following way:

typeOf[WrappedInt].typeSymbol.asInstanceOf[scala.reflect.internal.Symbols#Symbol]
    .isDerivedValueClass

But here, I lack for Scala internals knowledge to tell whether this is a reliable approach.

UPD: Also, I think, such runtime checks might cause actual instantiation of the a value class as a regular class.

@viktorklang

This comment has been minimized.

Show comment
Hide comment
@viktorklang

viktorklang Dec 6, 2014

Member

There is nothing to give to typeOf, all we have is a Class[_].

I think this one is likely to have to be closed as a wontfix, unless someone can come up with a solution that doesn't degrade performance.

Member

viktorklang commented Dec 6, 2014

There is nothing to give to typeOf, all we have is a Class[_].

I think this one is likely to have to be closed as a wontfix, unless someone can come up with a solution that doesn't degrade performance.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Dec 8, 2014

Member

Correct, as Viktor points out that solution is not applicable – it's "too late" to use that – the type information was lost already (for the runtime it's just an int for example).

The only solution I came up with is an un-safe one, since "any class that has one param with the matching type" would be able to be applied here, and that's too magic and more harm than good.

Instead when using value classes you'll have tor resort to using Props(new MyActor(ValueThing(42))

We talked a bit about it last week and consensus was to close as won't fix.

Member

ktoso commented Dec 8, 2014

Correct, as Viktor points out that solution is not applicable – it's "too late" to use that – the type information was lost already (for the runtime it's just an int for example).

The only solution I came up with is an un-safe one, since "any class that has one param with the matching type" would be able to be applied here, and that's too magic and more harm than good.

Instead when using value classes you'll have tor resort to using Props(new MyActor(ValueThing(42))

We talked a bit about it last week and consensus was to close as won't fix.

@ktoso ktoso modified the milestones: will not fix, 2.4.0 Dec 8, 2014

@ktoso ktoso removed bug 1 - triaged labels Dec 8, 2014

@ktoso ktoso closed this Dec 8, 2014

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Dec 11, 2014

Member

Pasting one more example which may help (unpacking manually works) if someone has this problem again:

case class Value(value: Int) extends AnyVal
// defined class Value

val v = Value(1337)
// v: Value = Value(1337)

class Juliet(x: Value) extends Actor {
  def receive = { case x => println(x) }
}
// defined class Juliet

system.actorOf(Props(classOf[Juliet], v)) // v: Value won't match, constructor wants int
// java.lang.IllegalArgumentException: no matching constructor found on class Juliet for arguments [class Value]
//    at akka.util.Reflect$.error$1(Reflect.scala:82)
//    at akka.util.Reflect$.findConstructor(Reflect.scala:106)

system.actorOf(Props(classOf[Juliet], v.value)) // unpack manually - THIS WORKS
// res2: akka.actor.ActorRef = Actor[akka://default/user/$a#-1022804200]

class Romeo(x: Value) extends Actor {
   def this(x: Int) { this(Value(x)) } // you can't do this, the default constructor already desugars to this (Int)
   def receive = { case x => println(x) }
}
//<console>:29: error: double definition:
//constructor Romeo:(x: Int)Romeo and
//constructor Romeo:(x: Value)Romeo at line 28
//have same type after erasure: (x: Int)Romeo
//         def this(x: Int) { this(Value(x)) }
Member

ktoso commented Dec 11, 2014

Pasting one more example which may help (unpacking manually works) if someone has this problem again:

case class Value(value: Int) extends AnyVal
// defined class Value

val v = Value(1337)
// v: Value = Value(1337)

class Juliet(x: Value) extends Actor {
  def receive = { case x => println(x) }
}
// defined class Juliet

system.actorOf(Props(classOf[Juliet], v)) // v: Value won't match, constructor wants int
// java.lang.IllegalArgumentException: no matching constructor found on class Juliet for arguments [class Value]
//    at akka.util.Reflect$.error$1(Reflect.scala:82)
//    at akka.util.Reflect$.findConstructor(Reflect.scala:106)

system.actorOf(Props(classOf[Juliet], v.value)) // unpack manually - THIS WORKS
// res2: akka.actor.ActorRef = Actor[akka://default/user/$a#-1022804200]

class Romeo(x: Value) extends Actor {
   def this(x: Int) { this(Value(x)) } // you can't do this, the default constructor already desugars to this (Int)
   def receive = { case x => println(x) }
}
//<console>:29: error: double definition:
//constructor Romeo:(x: Int)Romeo and
//constructor Romeo:(x: Value)Romeo at line 28
//have same type after erasure: (x: Int)Romeo
//         def this(x: Int) { this(Value(x)) }
@viktorklang

This comment has been minimized.

Show comment
Hide comment
@viktorklang

viktorklang Dec 11, 2014

Member

Nice one, Konrad

Cheers,

On 11 Dec 2014 10:53, "Konrad Malawski" notifications@github.com wrote:

Pasting more an example which may help (unpacking manually works) if
someone has this problem again:

case class Value(value: Int) extends AnyVal
// defined class Value

val v = Value(1337)
// v: Value = Value(1337)

class Juliet(x: Value) extends Actor {
def receive = { case x => println(x) }
}
// defined class Juliet

system.actorOf(Props(classOf[Juliet], v)) // v: Value won't match, constructor wants int
// java.lang.IllegalArgumentException: no matching constructor found on class Juliet for arguments [class Value]
// at akka.util.Reflect$.error$1(Reflect.scala:82)
// at akka.util.Reflect$.findConstructor(Reflect.scala:106)

system.actorOf(Props(classOf[Juliet], v.value)) // unpack manually
// res2: akka.actor.ActorRef = Actor[akka://default/user/$a#-1022804200]

class Romeo(x: Value) extends Actor {
def this(x: Int) { this(Value(x)) } // you can't do this, the default constructor already desugars to this (Int)
def receive = { case x => println(x) }
}
//:29: error: double definition:
//constructor Romeo:(x: Int)Romeo and
//constructor Romeo:(x: Value)Romeo at line 28
//have same type after erasure: (x: Int)Romeo
// def this(x: Int) { this(Value(x)) }


Reply to this email directly or view it on GitHub
#16444 (comment).

Member

viktorklang commented Dec 11, 2014

Nice one, Konrad

Cheers,

On 11 Dec 2014 10:53, "Konrad Malawski" notifications@github.com wrote:

Pasting more an example which may help (unpacking manually works) if
someone has this problem again:

case class Value(value: Int) extends AnyVal
// defined class Value

val v = Value(1337)
// v: Value = Value(1337)

class Juliet(x: Value) extends Actor {
def receive = { case x => println(x) }
}
// defined class Juliet

system.actorOf(Props(classOf[Juliet], v)) // v: Value won't match, constructor wants int
// java.lang.IllegalArgumentException: no matching constructor found on class Juliet for arguments [class Value]
// at akka.util.Reflect$.error$1(Reflect.scala:82)
// at akka.util.Reflect$.findConstructor(Reflect.scala:106)

system.actorOf(Props(classOf[Juliet], v.value)) // unpack manually
// res2: akka.actor.ActorRef = Actor[akka://default/user/$a#-1022804200]

class Romeo(x: Value) extends Actor {
def this(x: Int) { this(Value(x)) } // you can't do this, the default constructor already desugars to this (Int)
def receive = { case x => println(x) }
}
//:29: error: double definition:
//constructor Romeo:(x: Int)Romeo and
//constructor Romeo:(x: Value)Romeo at line 28
//have same type after erasure: (x: Int)Romeo
// def this(x: Int) { this(Value(x)) }


Reply to this email directly or view it on GitHub
#16444 (comment).

denisrosca added a commit to denisrosca/akka that referenced this issue Jun 10, 2016

Warning for actors with value class arguments
Update documentation to specify that value class arguments are not
supported for Prop creation using the recommended classOf[] approach.

Issue: #20735, #16444
@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Jul 5, 2016

Member

docs were updated

Member

patriknw commented Jul 5, 2016

docs were updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment