quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.84k stars 2.7k forks source link

CDI: support for suspend Kotlin's functions #29789

Open gonciarz opened 1 year ago

gonciarz commented 1 year ago

Description

Currently the only way to avoid AmbiguousResolutionException when injecting a function using generic type is to add @Named qualifier. That's an overhead. A CDI framework could do that for us.

Example code:

import arrow.core.Either

typealias AssignKeyboard = suspend (Computer) -> Either<KeyboardError, Unit>
typealias AssignMouse = suspend (Computer) -> Either<MouseError, Unit>

class ApiConfig {

    @ApplicationScoped
    @Named("AssignMouse")
    fun assignMouseBean(): AssignMouse = // ...

    @ApplicationScoped
    @Named("AssignKeyboard")
    fun assignKeyboardBean(): AssignKeyboard = // ...

}

@Path("/api")
class AssignMouseRestApiAdapter(
    @Named("AssignMouse")
    private val assignMouse: AssignMouse,
) {

    @Path("/assigment")
    @POST
    suspend fun foo() {
        // some code
    }

}

Implementation ideas

No response

quarkus-bot[bot] commented 1 year ago

/cc @manovotn, @mkouba

mkouba commented 1 year ago

I'm not a kotlin expert but I wonder how does kotlin translate the typealiases for AssignKeyboard and AssignMouse? I would expect something like Function<Computer, Either<KeyboardError, Unit>> but that's obviously not the case because then you would not get an AmbiguousResolutionException.

BTW an idiomatic way in CDI is to use a special qualifier per each bean that has a shared type in its set of bean types; i.e. something like @Keyboard and @Mouse in this particular case.

gonciarz commented 1 year ago

@mkouba that's a bit different bean:

kotlin.jvm.functions.Function2<? super rg.zerokvm.domain.model.Computer, ? super kotlin.coroutines.Continuation<? super arrow.core.Either<? extends rg.zerokvm.domain.port.api.BroadcastSwitchActiveComputerError, ? extends kotlin.Unit>>, ?>

They key thing in that definition is a suspended function, that introduces co-routines and state machine under the hood. So the second argument is translated into Continuation.

Having an alias for function simplifies the code, but it's just kotlin's function (Function2). I guess that the code that scans bean should be aware that this is generic type should register a bean using TypeLiteral. And the same for searching the right bean.

This snippet works perfectly:

        val typeLiteral = object : TypeLiteral<AssignMouse>() {}
        val beanInstance = CDI.current().select(typeLiteral)
mkouba commented 1 year ago

They key thing in that definition is a suspended function, that introduces co-routines and state machine under the hood. So the second argument is translated into Continuation.

I see. But this shouldn't be a problem if kotlin translates the typealias consistently (i.e. when used as a return type or when used a constructor parameter). Is BroadcastSwitchActiveComputerError extended by KeyboardError and MouseError? I would expect something like Function2<? super Computer, ? super Continuation<? super Either<? extends KeyboardError, ? extends Unit>>, ?> instead...

gonciarz commented 1 year ago

@mkouba

I see. But this shouldn't be a problem if kotlin translates the typealias consistently (i.e. when used as a return type or when used a constructor parameter).

That's also my assumption.

Is BroadcastSwitchActiveComputerError extended by KeyboardError and MouseError?

No, I changed some packages to simplify samples. Let me just to publish a small project that can help us.

gonciarz commented 1 year ago

@mkouba https://github.com/gonciarz/zero-kvm branch: init

Update: https://github.com/gonciarz/cdi-quarkus

mkouba commented 1 year ago

Hm, it looks fairly complex. What should I do to reproduce the problem?

I've tried to build the project but I get:

> Task :compileKotlin
w: /opt/source/protean/bugs/issue-29789/zero-kvm/src/main/kotlin/rg/zerokvm/adapter/spi/publisher/zmq/publisherZmqSpiAdapter.kt: (12, 5): Parameter 'computer' is never used
w: /opt/source/protean/bugs/issue-29789/zero-kvm/src/main/kotlin/rg/zerokvm/adapter/spi/publisher/zmq/publisherZmqSpiAdapter.kt: (16, 5): Parameter 'assignedDeviceSuccess' is never used
gonciarz commented 1 year ago

Hi, I'm sorry. You may try to remove @Named annotation from RestApiConfig's bean or and/or from BroadcastSwitchActiveComputerRestApiAdapter's injection or any other beans definitions and their injection points. And then just run quarkusDev gradle's target. I'll try to simplify the project so it contains even less code.

gonciarz commented 1 year ago

Sorry my mobile started clicking randomly on a screen and accidentally closed the ticket. I reopened it.

gonciarz commented 1 year ago

@mkouba

It contains two beans:

typealias AssignKeyboard = suspend (String) -> Either<KeyboardError, Unit>
typealias AssignMouse = suspend (String) -> Either<MouseError, Unit>

@ApplicationScoped
@Named(BeanName.ASSIGN_KEYBOARD)
fun assignLocalKeyboardBean(shellAdapter: ShellAdapter): AssignKeyboard = // code

@ApplicationScoped
@Named(BeanName.ASSIGN_MOUSE)
fun assignLocalMouseBean(shellAdapter: ShellAdapter): AssignMouse = // code

And one injection:

class KeyboardController(
    @Named(BeanName.ASSIGN_KEYBOARD)  private val assignKeyboard: AssignKeyboard,
) {
// code
}

Just remove @Named to reproduce AmbiguousResolutionException

You may also have a look at Quarkus Junit's test. Field injection doesn't work with Generic type:

@Inject
@Named(BeanName.ASSIGN_KEYBOARD)
private lateinit var assignKeyboard: AssignKeyboard

And InjectMock (mockk extension) as well:

@InjectMock
@Named(BeanName.ASSIGN_KEYBOARD)
private lateinit var assignKeyboard: AssignKeyboard

Update https://github.com/gonciarz/cdi-quarkus

mkouba commented 1 year ago

Thanks @gonciarz. I was able to reproduce the problem locally:

 Caused by: javax.enterprise.inject.AmbiguousResolutionException: Ambiguous dependencies for type kotlin.jvm.functions.Function2<? super java.lang.String, ? super kotlin.coroutines.Continuation<? super arrow.core.Either<? extends rg.zerokvm.KeyboardError, kotlin.Unit>>, ? extends java.lang.Object> and qualifiers [@Default]
        - java member: rg.zerokvm.KeyboardController():assignKeyboard
        - declared on CLASS bean [types=[rg.zerokvm.KeyboardController, java.lang.Object], qualifiers=[@Default, @Any], target=rg.zerokvm.KeyboardController]
        - available beans:
                - PRODUCER METHOD bean [types=[kotlin.Function<java.lang.Object>, kotlin.jvm.functions.Function2<java.lang.String, kotlin.coroutines.Continuation<? super arrow.core.Either<? extends rg.zerokvm.MouseError, kotlin.Unit>>, java.lang.Object>, java.lang.Object], qualifiers=[@Default, @Any], target=kotlin.jvm.functions.Function2<java.lang.String, kotlin.coroutines.Continuation<? super arrow.core.Either<? extends rg.zerokvm.MouseError, kotlin.Unit>>, java.lang.Object> assignLocalMouseBean(rg.zerokvm.ShellAdapter shellAdapter), declaringBean=rg.zerokvm.AppConfig]
                - PRODUCER METHOD bean [types=[kotlin.Function<java.lang.Object>, kotlin.jvm.functions.Function2<java.lang.String, kotlin.coroutines.Continuation<? super arrow.core.Either<? extends rg.zerokvm.KeyboardError, kotlin.Unit>>, java.lang.Object>, java.lang.Object], qualifiers=[@Default, @Any], target=kotlin.jvm.functions.Function2<java.lang.String, kotlin.coroutines.Continuation<? super arrow.core.Either<? extends rg.zerokvm.KeyboardError, kotlin.Unit>>, java.lang.Object> assignLocalKeyboardBean(rg.zerokvm.ShellAdapter shellAdapter), declaringBean=rg.zerokvm.AppConfig]

And the required type is: kotlin.jvm.functions.Function2<? super java.lang.String, ? super kotlin.coroutines.Continuation<? super arrow.core.Either<? extends rg.zerokvm.KeyboardError, kotlin.Unit>>, ? extends java.lang.Object>.

So first of all, those producers are illegal according to the spec, because "If a producer method return type contains a wildcard type parameter or is an array type whose component type contains a wildcard type parameter, the container automatically detects the problem and treats it as a definition error." (https://jakarta.ee/specifications/cdi/2.0/cdi-spec-2.0.html#producer_method). In fact, ArC should throw a DefinitionException during build. Unfortunately, we don't detect this problem correctly.

gonciarz commented 1 year ago

Hi @mkouba,

I have seen the spec and I see your point. But CDI spec refers to class definition and a wildcard comes directly from bytecode generation that is done under the hood. It's Any? type (Object) because Kotlin does not support union types.

But anyway I think that the problem is not so trivial.

Coroutines are implemented using Continuation functions. Compiler analyze the context in which they can be executed and do a transformation into a state machine with local variables representing a coroutine's state. Then depending on various factor the same Continuation object (related to function invocation) can be passed from one suspended function to another if they are bounded within the same state machine. The details can be found here:

https://kotlinlang.org/spec/asynchronous-programming-with-coroutines.html#asynchronous-programming-with-coroutines

My understanding is that Continuation is just a class (CPS) that is instantiated when the suspend function is called for the first time at runtime. Whenever we call the same function from different contexts we create different Continuation instances.

Update

CDI 2.0 says about producer's definition, so I guess we should focus on a class / function definition. The question is can we can create a smart analyzer that could detect that we deal with Kotlin's suspend function and we can exclude Continuation. It depends on a context that calls it and in theory you may inject producer to different contexts.

What do you think?

gonciarz commented 1 year ago

@geoand / @stuartwdouglas could you advice? I do would like to use Quarkus and I created a suspend function as a bean. At the moment ARC can create producers from them, but in order to inject them I need to use a @Named qualifier for both producer's definition and injection.

geoand commented 1 year ago

I really really don't see why would it's useful to create a CDI bean from a suspend function.

The whole is idea with suspend functions is to use them to make Async code easier - converting for example a Uni based API.

CDI is not an Async API - it allows you to wire together independent software components (classes). The invocation of the suspend functions themselves is not something CDI is concerned about, that is handled by whatever framework is being used - i.e. RESTEasy Reactive, Reactive Messaging etc.

mkouba commented 1 year ago

Well, the spec is pretty clear that _"A parameterized type that contains a wildcard type parameter is not a legal bean type"_. The fact that those producers work with @Named means that there is a bug that needs to be fixed (according to type-safe resolution rules).

geoand commented 1 year ago

Sure, I am not denying that there are some specific technicalities that need to be addressed. My point is more about the general approach

gonciarz commented 1 year ago

@geoand function is a first class citizen for Kotlin. It has it's representation as a: kotlin.jvm.functions.Function{0,1,2..} Of course I can replace it with a class with constructor arguments, but I can also apply "a default" argument with arrow framework (by partially1 extension). This is useful for functional programming style.

The whole is idea with suspend functions is to use them to make Async code easier

exactly suspend is a built-in mechanism to suggest the injected function will run in an async, non-blocking way

I've made an exercise and compared how Spring can handle the code almost the same as in the example: https://github.com/quarkusio/quarkus/issues/29789#issuecomment-1347419777 And I was able to not only register beans (suspend function) but also inject them without additional @Named qualifier. Maybe you may consider their approach?

gonciarz commented 1 year ago

And a mentioned function doesn't use wildcard definition as I mentioned.

typealias AssignKeyboard = suspend (String) -> Either<KeyboardError, Unit>

Wildcard is introduced to a bytecode representation. Can you explain me in two words how current ARC approach works producer registration/injection or can you point me to some doc. I guess that suspend function doesn't fit for your model in the current form, does it?

geoand commented 1 year ago

I assume @mkouba will look into the details he mentioned above.

mkouba commented 1 year ago

Wildcard is introduced to a bytecode representation.

The thing is that ArC, and Quarkus in general, works with the bytecode. Our extensions do not see your kotlin source but the generated bytecode.

Can you explain me in two words how current ARC approach works producer registration/injection or can you point me to some doc.

Not really in two words. A CDI producer is a method/field of a bean that serves as a source of another bean. A simple example is described here. The way it works is that we collect all producer methods/fields of a bean class, analyze the return type, etc. and register them as producer beans. You can read more details about producers in the spec.

I guess that suspend function doesn't fit for your model in the current form, does it?

I'm not quite sure.

FYI we've just merged a fix for detection of wildcards and your app will simply fail during build now.

gonciarz commented 1 year ago

Thank you @mkouba

I also raised a general question on stackoverflow about CDI and suspend functions: https://stackoverflow.com/questions/74827454/should-kotlins-suspend-function-be-allowed-for-valid-cdi-producers-beans

Not really in two words. A CDI producer is a method/field of a bean that serves as a source of another bean. A simple example is described here. The way it works is that we collect all producer methods/fields of a bean class, analyze the return type, etc. and register them as producer beans. You can read more details about producers in the spec.

Thanks, I might have a look at it more deeply if I only find some time.

FYI we've just merged https://github.com/quarkusio/quarkus/pull/29883 for detection of wildcards and your app will simply fail during build now.

It's not a good info for me but thanks. I may always switch to spring native or restructure the code.

Ladicek commented 1 year ago

Aside: I'd love to know why a parameterized type with a wildcard type argument is not a legal bean type. The following types are legal bean types:

On the other hand, List<? extends CharSequence> is not.

If I had a ReadonlyList class, I'd define its type parameter as covariant in a language with declaration-site variance (such as Kotlin), so that ReadonlyList<String> is a subtype of ReadonlyList<CharSequence>. That isn't possible in Java, but it directly translates as a wildcard with upper bound on the use-site (that's what the Kotlin compiler generates). So I'd often have ReadonlyList<? extends CharSequence>, and I don't really see why this can't be a legal bean type.

@mkouba @manovotn do you maybe remember the reasoning behind this? Are there perhaps types with wildcards that for good reason cannot be legal bean types, and there's no straightforward way to distinguish them from those that could be legal?

mkouba commented 1 year ago

do you maybe remember the reasoning behind this? Are there perhaps types with wildcards that for good reason cannot be legal bean types, and there's no straightforward way to distinguish them from those that could be legal?

I have no idea. But it was defined in CDI 1.0: https://docs.jboss.org/cdi/spec/1.0/html/concepts.html#legalbeantypes, i.e. a long time before I joined the expert group.

rgonciarz commented 1 year ago

Just for reference, I added that info to stack overflow question, but here are the repos with example code for all 3 frameworks:

manovotn commented 1 year ago

do you maybe remember the reasoning behind this? Are there perhaps types with wildcards that for good reason cannot be legal bean types, and there's no straightforward way to distinguish them from those that could be legal?

I have no idea. But it was defined in CDI 1.0: https://docs.jboss.org/cdi/spec/1.0/html/concepts.html#legalbeantypes, i.e. a long time before I joined the expert group.

I am in the same boat.

Since you can have wildcard in injection point, I would assume there is a scenario where having a wildcard also in a bean type would result in unexpected assignment. Either that or the inability to correctly deduce set of bean types due to wildcard not being specific enough. But I cannot think of a specific example which exhibits any issues with it and version 1.0 is so old that there is pretty much no way to dig up information on why it was designed in this particular way...

manovotn commented 1 year ago

FYI I also tried browsing through some TCKs, Weld tests and old JIRA issues but found no mention of this.

mkouba commented 1 year ago

FTR the typesafe resolution rules (Assignability of raw and parameterized types) are consistent and do not cover the case where the bean type parameter is a wildcard. So it was not an oversight...

Ladicek commented 1 year ago

Good point. On the other hand, the assignability rules cover the cases when the required type is a parameterized type with a wildcard type argument, so maybe making such type a legal bean type would lead to more cases of ambiguity...? 🤷

mkouba commented 1 year ago

the assignability rules cover the cases when the required type is a parameterized type with a wildcard type argument

Yes, that's a wilcard used in an injection point, i.e. perfectly legal according to https://jakarta.ee/specifications/cdi/2.0/cdi-spec-2.0.html#legal_injection_point_types.

mkouba commented 1 year ago

By the way, if we follow the Java assignability rules, then a producer method like this:

List<? extends CharSequence> list() {
    return List.of("foo");
}

would only be assignable to injection points like List<? extends CharSequence>, List<?> and List.

manovotn commented 1 year ago

BTW one other reason I can think of is plain old consistency - you couldn't declare class-based bean with wildcard (Java won't allow that) but producer method/field could do that - and also via CDI extensions. That's not a strong reason, just thinking aloud :)

Ladicek commented 1 year ago

By the way, if we follow the Java assignability rules, then a producer method like this:

List<? extends CharSequence> list() {
    return List.of("foo");
}

would only be assignable to injection points like List<? extends CharSequence>, List<?> and List.

Right. I think the point is that a bean with bean type List<? extends String> (as well as List<String>, but that is already allowed today) would be assignable to List<? extends CharSequence>.

BTW one other reason I can think of is plain old consistency - you couldn't declare class-based bean with wildcard (Java won't allow that) but producer method/field could do that - and also via CDI extensions. That's not a strong reason, just thinking aloud :)

I'm not 1000% sure, as I'm writing from top of my head, but I believe you can very easily do this:

public class MyBean implements List<? extends CharSequence> {
    ...
}

If parameterized types with wildcard type arguments were legal bean types, and the class had a BDA, then one of the bean types could be List<? extends CharSequence>.

mkouba commented 1 year ago

I'm not 1000% sure, as I'm writing from top of my head, but I believe you can very easily do this:

public class MyBean implements List<? extends CharSequence> {
    ...
}

You can't - a supertype may not specify a wildcard.

Ladicek commented 1 year ago

You're right, my bad: https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.1.5