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

cannot bind or create instance of scalaz reader (due to miss of Manifest) #42

Open
shengc opened this issue Jan 20, 2015 · 15 comments
Open
Assignees
Milestone

Comments

@shengc
Copy link

shengc commented Jan 20, 2015

import com.google.inject._
import net.codingwell.scalaguice._

import scalaz.Reader

class AModule extends AbstractModule with ScalaModule {
def configure() {
bind[Reader[Int, Int]].toInstance(Reader { (_: Int) * 2 })
}
}

gives me error: "Manifest of Reader[Int, Int] is not found"

so I have to resort to the legacy way of creating guice module

class AnotherModule extends AbstractModule {
def configure() {
bind(classOf[Reader[Int, Int]]).toInstance(Reader { (_: Int) * 2 })
}
}

but then when I tried to create instance using scalaguice, got the same error again

val injector = Guice.createInjector(new AnotherModule())

injector.instance[Reader[Int, Int]] // same error as above

@nbauernfeind
Copy link
Member

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

@shengc
Copy link
Author

shengc commented Jan 20, 2015

scala 2.10.4, 4.0-beta5 for google guice, and 4.0.0-beta5 for scala-guice

@nbauernfeind
Copy link
Member

The underlying issue is that scalaz.Reader is typedef'd to scalaz.Kleisli which has a type parameter that takes a type parameter.

Here's a shorter example that does not use scalaz or guice:

class A[T[_]]
manifest[A[List]] // No Manifest available

This leads me to this stackoverflow on the issue: http://stackoverflow.com/questions/26010800/why-does-getting-a-manifest-of-a-trait-with-a-higher-kinded-type-does-not-work

It seems like we won't be able to support this until we move to TypeTags. I'm going to try to squeeze it in as soon as I can.

@shengc
Copy link
Author

shengc commented Jan 20, 2015

That made a lot of sense. Looking forward to it.

@nbauernfeind
Copy link
Member

I've banged out the required changes to migrate from Manifest to TypeTag and they can be previewed here:
https://github.com/nbauernfeind/scala-guice/tree/TypeTag

There is a pretty big drawback to TypeTags, and that's that they are not thread safe in Scala 2.10.
See: https://issues.scala-lang.org/browse/SI-6240

I've, personally, coded around this for scala-guice library. However, it is basically impossible to coordinate across other libraries that might also be using TypeTags. That might be a big negative to other users of scala 2.10.

I still need to add a specific test case for your use, but I believe that this works for your use case.

@tsuckow, I could use your input here. Should we push forward with this change and begin to "deprecate" Scala 2.10?

@vkostyukov
Copy link

Thanks a lot @nbauernfeind for working on TypeTag support. This is what I was looking for. Really looking forward to it.

@nbauernfeind
Copy link
Member

You're welcome @vkostyukov.

I wrote those tests (haven't pushed them to my branch yet) and found a couple of edge-cases that are wrong related to Arrays. Arrays are tricky since there isn't a real class that supports them. JVM has a flag on the class instance to say whether or not the type is an array. With this particular use case the type tag doesn't conform to typeOf[Array[_]] so I'm trying to find another way to fix that.

@tsuckow
Copy link
Member

tsuckow commented Jan 22, 2015

I'll pull the sonatype stats and see what usage looks like. I would hate to maintain a separate branch, but that may be the best if we have to hack 2.10 or leave it broken.

nbauernfeind added a commit to nbauernfeind/scala-guice that referenced this issue Jan 22, 2015
@tsuckow
Copy link
Member

tsuckow commented Jan 23, 2015

Looks like 2.10 is curbing off. But still hundreds of users and 2.11 only has about half as many.

@shengc
Copy link
Author

shengc commented Jan 24, 2015

Guys found another case that is similar to this....

trait A { def a {} }
trait B

type AB = A with B

class Model extends AbstractModule with ScalaModule {
def configure() {
bind[A].toInstance(new A {})
bind[AB].toInstance(new A with B {})
}
}

Guice.createInjector(new Model()) // This fails at run time. Guice thinks binds are dups.

@nbauernfeind
Copy link
Member

@tsuckow I think that's probably fine. If someone has to care or runs into that problem we just direct them to the earlier versions of 2.10. Most of the time people don't upgrade to 2.11 because of other third party dependencies. Twitter is usually one of the last companies to upgrade and I see most of their popular projects have already upgraded. Let's just deprecate 2.10 and move on.

@shengc I've filed a separate issue for your second example since I'd prefer to deal with them separately. I haven't had a chance to find out why or if TypeTags fix them, yet. #43

@nbauernfeind
Copy link
Member

Also, it appears I didn't finish my changes related to Arrays. I'll add that to my todo list this weekend and send a proper pull-request.

@tsuckow
Copy link
Member

tsuckow commented May 28, 2018

So 4.2.1 has a conversion of typeLiteral[T] to use TypeTag. I started looking at replacing occurrences of Manifest with TypeTag. I'll see about adding this as a test case and see what happens.

I should also look into the compilation assertions, I would like to add some expected failures on some of the tickets with corner cases like this. But maybe with Typetags they will all be fixed. ¯_(ツ)_/¯

@halfninja
Copy link

Now that 2.10 support is gone, it's worth noting that Dotty (soon to become Scala 3) will no longer synthesize Manifest instances, so there are two reasons to move to type tags. Is there still a branch around somewhere?

@tsuckow
Copy link
Member

tsuckow commented Jan 28, 2019 via email

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

No branches or pull requests

5 participants