codingwell / scala-guice

Scala extensions for Google Guice
Apache License 2.0
341 stars 44 forks source link

Replace uses of Manifest with ClassTag / TypeTag #79

Closed ashleymercer closed 5 years ago

ashleymercer commented 5 years ago

If a type is owned by an object then it looks like binding is special-cased so, for example, the following works as expected:

object Foo {
  case class Bar(value: Int)
}

bind[Foo.Bar].toInstance(Foo.Bar(value = 1))

However, if the type Bar is instead used as a type parameter for some generic type, then the binding fails with ScalaReflectException because the enclosing type cannot be found in the Mirror, so this:

trait Factory[T] {
  def get(): T
}

class FactoryImpl[T] @Inject()(value: T) extends Factory[T] {
  def get(): T = value
}

bind[Factory[Foo.Bar]].to[FactoryImpl[Foo.Bar]]

results in

scala.ScalaReflectionException: class Foo in JavaMirror with DependencyClassLoader{...elided...}
    at scala.reflect.internal.Mirrors$RootsBase.staticClass(Mirrors.scala:129)
    at scala.reflect.internal.Mirrors$RootsBase.staticClass(Mirrors.scala:29)
    at net.codingwell.scalaguice.TypeConversions$.findOwnerOf(TypeConversions.scala:87)
    at net.codingwell.scalaguice.TypeConversions$.scalaTypeToJavaType(TypeConversions.scala:66)
    at net.codingwell.scalaguice.TypeConversions$.$anonfun$scalaTypeToJavaType$1(TypeConversions.scala:67)
    ...
nbauernfeind commented 5 years ago

What versions of scala and scala-guice are you using?

ashleymercer commented 5 years ago

Ahh, knew I'd forget something! As follows:

Scala: 2.12.8 Java: 1.8.0_202 scala-guice: 4.2.3

ashleymercer commented 5 years ago

I reproduced a minimal test case in a clean project and it works just fine. The only difference when I see the error is that I'm running Play in DEV mode: this is a well-known source of ClassLoader-related gotchas since Play does a lot of tricks to enable hot compile/reload during development.

The weird thing is that pretty much all the rest of scala-guice works as expected (certainly, I use it to bind a bunch of other types, and reflection finds them just fine) so I'll keep digging and see if this is something that can be fixed on the scala-guice side.

ashleymercer commented 5 years ago

Think I've found it: some of the methods in scala-guice use Manifest which is part of the older (pre-2.10) reflection API. Looks like this doesn't play nicely with Play's dev-mode ClassLoader shenanigans since I can see from debugging that it ends up with the wrong ClassLoader instance (it only has access to external dependencies, and not the project classes).

Since scala-guice no longer supports older versions of Scala I'm going to submit a PR which updates all the method signatures to use ClassTag or TypeTag (as appropriate) and these end up with the right ClassLoader.

For anyone else who hits this bug in the meantime, it looks like BindingExtensions.toType already has the correct signature, and is functionally equivalent to ScalaLinkedBindingBuilder.to so you can just rewrite the above example as:

import net.codingwell.scalaguice.BindingExtensions._

// This works, even in Play DEV mode!
bind[Factory[Foo.Bar]].toType[FactoryImpl[Foo.Bar]]