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

#469: Add parameterized new support #476

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Nek-12
Copy link

@Nek-12 Nek-12 commented Feb 9, 2025

Here's the implementation of the feature proposed in #469.

It introduces new() overload that takes the parameter provided by the factory / multiton binding into the resolution scope.

The function tries to resolve the parameter and inject it once and:

  1. If not used, injects it.
  2. If used for previous parameter, tries to inject an instance from the DI graph
  3. If neither are found, throws Di.NotFoundException

This implementation has some quirks that I will mention in comments.

Before merging, they need to be evaluated to align with the maintainers' vision and/or best practices.

closes #469

gradle/wrapper/gradle-wrapper.properties Show resolved Hide resolved
@@ -35,6 +35,8 @@ public inline fun <reified T: Any> DI.Builder.bindProvider(
noinline creator: DirectDI.() -> T
): Unit = Bind(tag = tag, overrides = overrides, binding = provider(creator = creator))

// region bindProviderOf overloads

// TODO This fails with Kotlin/JS Legacy
// Re-enable this with IR only target.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this was referring to, but I removed the noinline call as it actually was not needed and was affecting performance negatively

val subclass = S()
val b: B by di.instance(arg = subclass)

assertEquals(b.p, subclass, "injected parameter ${b.p} is not the expected subclass")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, this test passes if the parameters are subclasses of the intended type.

For example, you can see that we injected S, a child of A and they were injected successfully. This can lead to problems if the client (B) didn't actually intend to receive subclasses, but I believe this is reliant on LSP. If the developer follows LSP, then class B won't notice the manipulation.

This implementation only allows subclasses of A (using isAssignableFrom), but a more performant solution would leverage instanceOf, allowing any implementation of A, even if an interface. This would reduce safety, but improve construction performance.
I'm not sure how well reflection like this will work here, because the reflection is abstracted under another kosi-libs library.

Suggestion: Evaluate whether isAssignableFrom is actually needed or if it can be replaced with a single is check. Choose the more performant option if subclass usage is approved, and opt for ::class check if not.

bind { factory { p: A -> new(p, ::D) } }
}

val param = A() // injected class does not use the param
Copy link
Author

@Nek-12 Nek-12 Feb 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, this test passes even though we never used the parameter at all. It was not used in the injected dependency (D) constructor.

So the parameter A that the client provided is wasted.

This can lead to unintended behavior if the developer made a mistake of providing parameters, but accidentally used another class in the target dependency, which happened to also be present in the DI graph.

However, it's not possible to fix this in compile time, and the only solution I see is to throw something like DI.UnusedParameterException after creating an instance.

Suggestion: Choose one option:

  1. Leave as is : more convenient and lenient, but can cause runtime bugs
  2. Change to add verification code and throw: Strict, but will crash in runtime at instance's first usage.

@Test
fun test_03_new_bindMultitonOf_param() {
val di = DI {
bindMultitonOf<A, _, _>(::B)
Copy link
Author

@Nek-12 Nek-12 Feb 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see that even though we added overloads bindMultitonOf and bindFactoryOf, we cannot not provide the type of the argument, as there is nothing indicating what it is.

This leads to us writing boilerplate like <ParameterType, _, _, _, _, _, _, _>.

Kotlin does not yet have syntax for partial type arg specification and there is no lambda to define the type of the param. This also leads to compilation errors each time a parameter is added or removed.

Suggestion: Leave, but document that using
bind* { it: ParameterType -> new(it, ::Class) } is a more convenient solution,

OR

Remove all bindMultitonOf / bindFactoryOf overloads from this PR.

* @see new
*/
public inline fun <
Copy link
Author

@Nek-12 Nek-12 Feb 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added more overloads of new and other functions because 22 is the maximum value actually supported by Kotlin, with 2 proofs:

  1. 22 parameters is the maximum number for which Kotlin generates componentN() functions on data classes.
  2. Kotlin supports referring to functions and creating their instances. Function22 is the highest supported number of parameters for function interface references.

I did this because 10+ parameters are actually relatively common in classes like ViewModels, which can inject a lot of use-cases etc. When the new function breaks at 10 params, it quickly becomes annoying to completely rewrite the declaration to use instance() everywhere.

I get it's a huge pile of boilerplate, but there is no other way to do this at the moment as vararg type arguments are not implemented in kotlin.

@Nek-12
Copy link
Author

Nek-12 commented Feb 9, 2025

Additionally, looks like isAssignableFrom does not properly work and fails the test on macOS.

test_02_new_subclass[macosArm64]: 

org.kodein.di.DI.NotFoundException: No binding found for B, with argument S

This is likely a bug in org.kodein.type related to how native code gets executed. I've seen some tickets on YT re: usage of kotlin.reflect.typeOf on Native.

@romainbsl
Copy link
Member

romainbsl commented Feb 10, 2025

Thanks a lot for your contribution!
I'll need few days to validate, but sounds great.

Looks like test_02_new_subclass is failing on all NON JVM targets.
[EDIT]
This is because isAssignableFrom does not work with inheritance, always because reflection is not as advanced on the Native/JS than on the JVM. I'd need to investigate more.

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

Successfully merging this pull request may close these issues.

Implement new DSL for Factories instead of Providers only.
2 participants