codingwell / scala-guice

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

SingletonType bindings not working since 4.2.2 #83

Closed mvdstam closed 5 years ago

mvdstam commented 5 years ago

Since version 4.2.2 and upwards, this binding results in an exception:

// A sealed domain
sealed trait Root
final case object Foo extends Root
final case object Bar extends Root
final case object Baz extends Root

// A case class using said domain
case class MyContainerClass[T <: Root](inner: T)

// In a ScalaModule:
bind[MyContainerClass[Foo.type]].toProvider[FooProvider]
bind[MyContainerClass[Bar.type]].toProvider[BarProvider]
bind[MyContainerClass[Baz.type]].toProvider[BazProvider]

This results in the following exception:

[Error] Caused by: java.lang.UnsupportedOperationException: Could not convert scalaType Foo.type to a javaType: scala.reflect.internal.Types$UniqueSingleType at net.codingwell.scalaguice.TypeConversions$.scalaTypeToJavaType(TypeConversions.scala:78)

With 4.2.1, this works without a problem. Am I doing something wrong, or is this type of binding simply not supported (anymore), @tsuckow? Thanks in advance for the assistance. :+1:

tsuckow commented 5 years ago

It's definitely a side effect of having to transition to Typetags because Manifests are deprecated.

I'm busy due to the holiday but I'll try and add a unit test that at least captures what you are seeing. We are likely just not handling UniqueSingleType. The TypeConversions file is black magic even to me. It might be straight forward or I might need to reach out to the file author for advice. They could present at a conference on that thing, what it is trying to do is simple but documentation in Scala is lacking.

You could even look into it yourself. I don't Scala that much these days so I'm at a disadvantage.

mvdstam commented 5 years ago

Thanks a lot for your quick response @tsuckow. I've looked into it briefly and we agree on one thing; the TypeConversions.scala file is indeed black magic. However, I'll try to get something working and get back to you.

mvdstam commented 5 years ago

@tsuckow After some trial-and-error, I may have found a working solution:

diff --git a/src/main/scala/net/codingwell/scalaguice/TypeConversions.scala b/src/main/scala/net/codingwell/scalaguice/TypeConversions.scala
index 1ed78bd..26bfc18 100644
--- a/src/main/scala/net/codingwell/scalaguice/TypeConversions.scala
+++ b/src/main/scala/net/codingwell/scalaguice/TypeConversions.scala
@@ -6,8 +6,7 @@ import com.google.inject.internal.MoreTypes.WildcardTypeImpl
 import com.google.inject.util.Types._

 import scala.reflect.runtime.universe
-import scala.reflect.runtime.universe._
-import scala.reflect.runtime.universe.{Type => ScalaType, WildcardType => ScalaWildCardType}
+import scala.reflect.runtime.universe.{Type => ScalaType, _}

 /**
   * Copyright (C) 22/04/2018 - REstore NV
@@ -75,6 +74,14 @@ private [scalaguice] object TypeConversions {
         val mappedLowerBounds = lowerBounds.map(bound => scalaTypeToJavaType(bound, mirror)).toArray
         new WildcardTypeImpl(mappedUpperBounds, mappedLowerBounds)
       }
+      case SingleType(_, symbol) if symbol.isModule => {
+        val rm = universe.runtimeMirror(getClass.getClassLoader)
+        val moduleReflection = rm.reflectModule(symbol.asModule)
+        val instanceMirror = mirror.reflect(moduleReflection.instance)
+        val classMirror = rm.reflectClass(instanceMirror.symbol)
+
+        scalaTypeToJavaType(classMirror.symbol.toType, mirror)
+      }
       case _ => throw new UnsupportedOperationException(s"Could not convert scalaType $scalaType to a javaType: " + scalaType.dealias.getClass.getName)
     }
   }

This fixes the issue of not being able to bind SingletonType bindings and allows my application to run once more. What do you think?

tsuckow commented 5 years ago

Do you have an example of FooProvider?

Nevermind, I figured out my typo in making FooProvider

mvdstam commented 5 years ago

Ah, FooProvider was just a theoretical example anyway. It's basically just a javax.inject.Provider<MyContainerClass<Foo.type>>.

I created #84 yesterday with my fix. This compiles successfully with Scala 2.11, 2.12 and 2.13, and since the new case is put at the end of the pattern match, it won't break or change existing behaviour.

tsuckow commented 5 years ago

I mostly got things handled, I'll see about a release next week after I verify the unit test I created is good.

On Wed, Jul 3, 2019, 23:22 Max van der Stam notifications@github.com wrote:

Ah, FooProvider was just a theoretical example anyway. It's basically just a javax.inject.Provider<MyContainerClass>.

I created #84 https://github.com/codingwell/scala-guice/pull/84 yesterday with my fix. This compiles successfully with Scala 2.11, 2.12 and 2.13, and since the new case is put at the end of the pattern match, it won't break or change existing behaviour.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/codingwell/scala-guice/issues/83?email_source=notifications&email_token=AACACJ5FGLOMV6ZPBO4J6HDP5WJI3A5CNFSM4H436M6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZGOCHA#issuecomment-508354844, or mute the thread https://github.com/notifications/unsubscribe-auth/AACACJ6LRZCLLMP45DNXRW3P5WJI3ANCNFSM4H436M6A .

mvdstam commented 5 years ago

Cheers @tsuckow, let me know if you need anything else from my end. Looking forward to the formal release so I can upgrade my projects.

mvdstam commented 5 years ago

Hi @tsuckow, what is the status on this on your end? Thanks :)

tsuckow commented 5 years ago

Thanks for the reminder, works been crazy this week after the holiday. Hoping to get back to it this weekend.

On Thu, Jul 11, 2019, 22:47 Max van der Stam notifications@github.com wrote:

Hi @tsuckow https://github.com/tsuckow, what is the status on this on your end? Thanks :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/codingwell/scala-guice/issues/83?email_source=notifications&email_token=AACACJ2SCT6F62YCOQJAWLLP7ALGTA5CNFSM4H436M6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZYX6WY#issuecomment-510754651, or mute the thread https://github.com/notifications/unsubscribe-auth/AACACJ3BC52S5S3P34V5SKLP7ALGTANCNFSM4H436M6A .

tsuckow commented 5 years ago

4.2.6 has shipped and is pending sync to Maven Central.

Thanks for your patience.

mvdstam commented 5 years ago

Thanks @tsuckow!