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

Allow Kotlin Collections.List for @All qualifier #24766

Closed
Dudeplayz opened this issue Apr 5, 2022 · 13 comments · Fixed by #24795
Closed

Allow Kotlin Collections.List for @All qualifier #24766

Dudeplayz opened this issue Apr 5, 2022 · 13 comments · Fixed by #24795
Labels
area/kotlin kind/enhancement New feature or request
Milestone

Comments

@Dudeplayz
Copy link

Dudeplayz commented Apr 5, 2022

Describe the bug

I was trying to inject different implementations of the same interface at the same time. Following the docs I have done it with:

@Inject
@All
protected lateinit var processors: List<IProcessor>

But on compilation it throws:

Build step io.quarkus.arc.deployment.ArcProcessor#generateResources threw an exception: javax.enterprise.inject.spi.DefinitionException: Wildcard is not a legal type argument for ...Resource#processors

As I am using Kotlin, I changed the type of the list to MutableList, which works.
I think List is the more appropriate type following the docs: The injected instance is an immutable list of the contextual references of the disambiguated beans.. So either both or only List should be supported. The later one could break existing code.

I am also unsure, if the wildcard exception is the correct exception in this case?

Expected behavior

An immutable list is injected for List<T>.

Actual behavior

An exception is thrown.

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.8.0.Final

Build tool (ie. output of mvnw --version or gradlew --version)

mvn

Additional information

No response

@Dudeplayz Dudeplayz added the kind/bug Something isn't working label Apr 5, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 5, 2022

/cc @evanchooly

@gsmet
Copy link
Member

gsmet commented Apr 5, 2022

/cc @mkouba @manovotn

@mkouba
Copy link
Contributor

mkouba commented Apr 5, 2022

It's not a bug but a problem caused by the fact that kotlin translates List<IProcessor> to something like java.util.List<? extends IProcessor>, i.e. the extracted required type is ? extends IProcessor and wildcard is an illegal injection point type. You can also replace List<IProcessor> with java.util.List<IProcessor> See more information in this comment .

@mkouba mkouba added kind/enhancement New feature or request and removed kind/bug Something isn't working labels Apr 5, 2022
@Dudeplayz
Copy link
Author

Ok, but what is different for the MutableList? Is this a bug in Kotlin or wanted?

@Dudeplayz
Copy link
Author

I have found this older ticket, stating to keep it as it is: https://youtrack.jetbrains.com/issue/KT-5792

@mkouba
Copy link
Contributor

mkouba commented Apr 6, 2022

I don't think it's a bug in Kotlin. The point is that Kotlin's List is not java.util.List (it's read-only and does not translate 1:1 to java.util.List) and therefore you can't expect the @All qualifier to just work. Kotlin's MutableList on the other hand seems to behave like an alias for java.util.List.

@Ladicek and @evanchooly might have a better explanation ;-).

@Ladicek
Copy link
Contributor

Ladicek commented Apr 6, 2022

Right. Kotlin's List is read-only, so it can be and is covariant in its type parameter: interface List<out E>. Since Java/JVM only support use-site variance, usages of that type are represented in bytecode as List<? extends Whatever>. Which is an invalid injection point type.

On the other hand, Kotlin's MutableList is mutable, so it must be (and is) invariant: interface MutableList<E>. In bytecode, usages of that type look like MutableList<Whatever>. And that is a valid injection point type.

Technically, I think we could make a special case for this situation, but I'll leave that decision up to @mkouba.

@mkouba
Copy link
Contributor

mkouba commented Apr 6, 2022

Technically, I think we could make a special case for this situation, but I'll leave that decision up to @mkouba.

Like relax the spec requirements for an injetion point type in this particular case? I'm not a big fan of similar things.

@kdubb
Copy link
Contributor

kdubb commented Apr 6, 2022

I'm not sure how to achieve it but a special case for this situation would be greatly appreciated; every time I use @All I trip over this first haha.

@Dudeplayz
Copy link
Author

Dudeplayz commented Apr 6, 2022

I don't know how complex it is, but would it be possible to add a Kotlin compiler plugin (or something quarkus specific), which fixes such particular places?

Also, a mention in the docs somewhere that this is a problem would be helpful to reduce and avoid this pitfall.

@geoand
Copy link
Contributor

geoand commented Apr 6, 2022

The best we can do without having to go to great lengths is provide an actionable error message

@mkouba
Copy link
Contributor

mkouba commented Apr 6, 2022

The best we can do without having to go to great lengths is provide an actionable error message

Hm, kotlin classes are annotated with a special annotation, right?

@Ladicek
Copy link
Contributor

Ladicek commented Apr 6, 2022

Yes, Kotlin-generated classes have the kotlin.Metadata annotation. ArC already uses it on one place (see SubclassGenerator).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kotlin kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants