google / dagger

A fast dependency injector for Android and Java.
https://dagger.dev
Apache License 2.0
17.45k stars 2.02k forks source link

@BindsOptionalOf is not compilant with the Java 7 compiler #1544

Open VincentMasselis opened 5 years ago

VincentMasselis commented 5 years ago

Hi everyone,

I'm facing to a compiler bug with @BindsOptionalOf when compiling with the Java 7 source compatibility. I've created a module RootModule with this binding :

@Module
interface RootModule {
    @BindsOptionalOf
    fun string(): String
}

and the RootComponent :

@Component(modules = [RootModule::class])
interface RootComponent {
    @Component.Factory
    interface Factory {
        fun build(): RootComponent
    }
    fun inject(application: Application)
}

Inside my class Application, I create an injectable field:

class Application : Application() {

    @Inject
    lateinit var string: Optional<String>

    override fun onCreate() {
        super.onCreate()
        DaggerRootComponent.factory().build().also { it.inject(this) }
    }
}

With this previous setup, I have a compiler exception into the generated DaggerRootComponent at the line 28:

private Application injectApplication(Application instance) {
    Application_MembersInjector.injectString(instance, Optional.empty());
    return instance;
  }

It fails because because the generic type of Optional.empty() should be explicitly specified in Java 7 (not required in Java8), so, when using a Java 7 compiler, this code should be replaced by Optional.<String>empty().

You can clone a working example in my repository HERE. Warning ! It's an Android app using Kotlin, not a plain Java app, I'm less familiar with pure java developement than Android :). On this repository, you can enable java 8 compilation at this location to see the difference between and face the bug.

Sevastyan commented 5 years ago

I faced the same issue.

VincentMasselis commented 5 years ago

Important notice for Android Developers only regarding to this bug, you can put this code into your gradle file :

android {
   // Your config
    compileOptions {
        sourceCompatibility JavaVersion.VERSION_1_8
        targetCompatibility JavaVersion.VERSION_1_8
    }
}

It's going to compile with the Java 8 syntax to avoid the bug, the Android Gradle Plugin keeps the compatibility with < 24 APIs by calling the desugar task. More informations about desugar https://developer.android.com/studio/write/java8-support.

Plastix commented 3 years ago

I wish Dagger would provide its own Optional<T> type just like its Lazy<T>. This way Android wouldn't have to rely on desugaring.

JakeWharton commented 3 years ago

I wish Android actually cared about not fragmenting the Java ecosystem. There is no reason to embed your own optional anymore. You're just doing what core library desugar is doing, but worse, since your version is inoperable with the standard versions.

ronshapiro commented 3 years ago

Is the fact that it's Kotlin causing the problem? Dagger has some conditional code for Java <7 here: https://github.com/google/dagger/blob/b5990a0641a7860b760aa9055b90a99d06186af6/java/dagger/internal/codegen/writing/OptionalBindingExpression.java#L65-L68

Perhaps the wrong source version isn't correct?

ronshapiro commented 3 years ago

It doesn't seem like the issue is the type of Optional, but rather a javac type inference discrepancy

VincentMasselis commented 3 years ago

@Plastix You don't have to create your own implementation, Dagger2 is already compatible with the Optional from Guava.

Plastix commented 3 years ago

Thanks @JakeWharton and @VincentMasselis. My project currently doesn't use Guava or Core Library desugaring which is why I asked. I think we'll adopt the core library desugaring.