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

kotlin.Lazy support #2353

Closed
sergeys-opera opened this issue Feb 3, 2021 · 17 comments
Closed

kotlin.Lazy support #2353

sergeys-opera opened this issue Feb 3, 2021 · 17 comments

Comments

@sergeys-opera
Copy link

It would be nice to use kotlin.Lazy for lazily instantiated dependencies instead of dagger.Lazy in Kotlin code and omit som .get() method calls.

Then instead of:

class AccountManager @Inject constructor(
    service: dagger.Lazy<AccountService>
) {
  service.get().getAccount()
}

it would be possible to write:

class AccountManager @Inject constructor(
    service: kotlin.Lazy<AccountService>
) {
  service.getAccount()
}
@bcorso
Copy link

bcorso commented Feb 3, 2021

Hi @sergeys-opera,

IIUC, kotlin.Lazy would still need to be referenced by service.value.getAccount().

In order to reference it without value I think you would still need to have something like:

class AccountManager @Inject constructor(
    lazyService: kotlin.Lazy<AccountService>
) {
  val service by lazyService
  ... service.getAccount()
}

I'm also not a Kotlin expert, so please let me know if I missed something.

@sergeys-opera
Copy link
Author

@bcorso I get your point but I think that

class AccountManager @Inject constructor(
    lazyService: kotlin.Lazy<AccountService>
) {
  val service by lazyService
  ... service.getAccount()
}

is simpler than

class AccountManager @Inject constructor(
    lazyService: dagger.Lazy<AccountService>
) {
  val service by lazy {
      lazyService.get()
  }
  ... service.getAccount()
}

I even wrote an extension method for dagger.Kotlin:

inline fun <reified T> Lazy<T>.me() = lazy { get() }

to write like this:

class AccountManager @Inject constructor(
    lazyService: dagger.Lazy<AccountService>
) {
  val service by lazyService.me()
  ... service.getAccount()
}

kotlin.Lazy is also a part of the Kotlin language so people are more used to it than to dagger.Lazy when they use Hilt on Android. I myself tried using kotlin.Lazy first (one of the reasons was that it doesn't need an import statement) but then switched to dagger.Lazy when the code didn't compile. It reminds me a bit of a situation with all these @NotNull/@NonNull annotations in Java when none of them was recognized globally (different tools assumed different package names for the annotations).

I'm quite satisfied with an extension method mentioned above but having native support would be even better. I also wonder if/why I'm the first one to request such feature.

@bcorso
Copy link

bcorso commented Feb 4, 2021

Ah yes, I just wanted to be clear that it wouldn't automatically allow you to write service.getAccount().

I do agree that it's probably worth supporting kotlin.Lazy.

@tbroyer
Copy link

tbroyer commented Feb 4, 2021

Fwiw, supporting val service by lazyService with a dagger.Lazy is possible today by providing the operator fun getValue as an extension function: https://kotlinlang.org/docs/reference/delegated-properties.html#property-delegate-requirements
Something like (totally untested)

operator fun <T> dagger.Lazy<T>.getValue(receiver: Any?, property: KProperty<*>): T = get()

much more idiomatic than your me() extension.

@sergeys-opera
Copy link
Author

@tbroyer this was the first thing I tried that but the code didn't compile ¯_(ツ)_/¯

AccountManager.kt: (72, 23): Type 'Lazy<Database>' has no method 'getValue(AccountManager, KProperty<*>)' and thus it cannot serve as a delegate

There must be some difference between how compiler treats kotlin.Lazy and other property delegates.

@tbroyer
Copy link

tbroyer commented Feb 4, 2021

🤔 https://pl.kotl.in/nTI70NSnt

Or an alternative with provideDelegate: https://pl.kotl.in/NBG5iyP3Q (but you'll have a kotlin.Lazy with its own synchronization around a dagger.Lazy that already synchronizes!)

@JakeWharton
Copy link

The Kotlin compiler does not special case kotlin.Lazy. It's entirely a library feature that neither the compiler nor language are aware of.

In general I don't see much advantage to adding support for Kotlin's lazy in Dagger, but there's also very little disadvantage I suppose. Dagger already has too many things that have more than one way of doing it. What's one more.

I would rather see time spent on parsing Kotlin metadata to avoid generic variance mismatches that require @JvmSuppressWildcard or wiring up @Provide suspend fun and injectable Deferred<T> if we're going to focus on Kotlin-specific features of Dagger.

@sergeys-opera
Copy link
Author

@tbroyer I realized that I put the property delegate declaration into a file located in a separate package and the mentioned AccountManager located in another package just couldn't find it. Importing x.y.z.getValue explicitly made the code compile but there is no way to force such import when using by with dagger.Lazy - people will stumble upon this obscure compilation error over and over again.

The Kotlin compiler does not special case kotlin.Lazy

@JakeWharton you're probably right about that but why don't you need to import getValue operator for kotiln.Lazy explicitly then?

I agree that this task is probably not that important compared to other Kotlin improvements that can be made. Adding a property delegate extension method for dagger.Lazy and importing it explicitly is good enough to me. Thanks for your time and the replies, I'm closing this feature request.

@tbroyer
Copy link

tbroyer commented Feb 4, 2021

you're probably right about that but why don't you need to import getValue operator for kotiln.Lazy explicitly then?

Because it's a Kotlin class with an operator fun getValue as a member, not an extension. That would work out of the box for Dagger too if it was written in Kotlin that way.

@sergeys-opera
Copy link
Author

@tbroyer no, it's an extension according to this:

@kotlin.internal.InlineOnly
public inline operator fun <T> Lazy<T>.getValue(thisRef: Any?, property: KProperty<*>): T = value

The actual reason why you don't need to import kotlin.getValue is that kotlin package is auto-imported into every source file:

A number of packages are imported into every Kotlin file by default:
kotlin.*
...

@sergeys-opera
Copy link
Author

FYI Inlining getValue triggers a NPE in kapt 1.4.30: #2388 and https://youtrack.jetbrains.com/issue/KT-45032

@ZacSweers
Copy link

ZacSweers commented Feb 28, 2021

I think orienting this discussion around use of dagger.Lazy as delegate buried the lede a bit. It would be nice, but the biggest value prop IMO would be avoiding the constant papercut that is Kotlin's implicit import of its own Lazy.

i.e. this code looks correct until dagger fails at runtime with a long trace about not providing a Lazy, something I've seen multiple colleagues (and myself) get burned by.

class TestClass @Inject constructor(val dependency: Lazy<Dependency>)

this looks right but fails to compile, and kotlin users reflexively will try to write it this way. This costs cycles and cognitive overhead

+import dagger.Lazy
+
class TestClass @Inject constructor(val dependency: Lazy<Dependency>)

I think the combination of this and the free delegate support make this a pretty compelling addition. If not, I'd want to probably sent a PR to add a lint check for this case instead since it comes up regularly.

@asthagarg2428
Copy link

Does dagger support lazy constructor injection?

class ABC @Inject constructor(
    private val obj: dagger.Lazy<B>){}

Does it work as expected like we have lazy field injection for B object?

@sergeys-opera
Copy link
Author

Does dagger support lazy constructor injection?

Yes

Does it work as expected like we have lazy field injection for B object?

Yes

@Clement-Jean
Copy link

Is there any plan to add this? Just want to share my use case, in case someone is interested. I have the following module structure:

├── app
└── login
    ├── api // Only interfaces
    ├── impl // Implementations of api
    ├── di // Dagger code
    └── ui // UI code

Now, the thing that I try to do is keeping the UI module away from dagger dependencies. I just want to have compileOnly 'javax.inject:javax.inject:1' in my gradle file. I do this because I do not want to link the view to dagger so that if I need to switch to other DI framework, I simply change code in di module.

If I need to inject something lazily in the ui module, adding dagger.Lazy simply violates that and make the view dependent on Dagger. Hope this makes sense.

@sergeys-opera
Copy link
Author

@Clement-Jean you can use javax.inject.Provider instead and wrap it in kotlin.Lazy in a val declaration:

class Foo @Inject constructor(barProvider: Provider<Bar>) {
  val bar by lazy { barProvider.get() }
}

@Clement-Jean
Copy link

Right, just more boilerplate I guess. Thank you for the solution though :)

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

7 participants