Fix broken collection assignability check #36197
Conversation
.../common/processor/src/main/java/org/jboss/resteasy/reactive/common/processor/JandexUtil.java
Outdated
Show resolved
Hide resolved
| private static Set<DotName> findSupertypes(DotName name, IndexView index) { | ||
| Set<DotName> result = new HashSet<>(); | ||
|
|
||
| Deque<DotName> workQueue = new ArrayDeque<>(); |
There was a problem hiding this comment.
I have to admit I've never seen this sort of implementation. Usually it's a recursive function. I assume you're flattening it on purpose to save stack space?
Also, this is not efficient because you will iterate through the work queue's inheritance entirely to look for all supertypes instead of stopping as soon as you find it.
There was a problem hiding this comment.
I pretty much copied this from Arc
There was a problem hiding this comment.
We can of course optimize, but I didn't want to in the remote case we can at some point merge the various util classes
There was a problem hiding this comment.
Yeah, I figured this came from somewhere :( I also wondered why it was not using the existing JandexUtil from quarkus-core-deployment, but this is not usable from independent-projects, same as for Arc. Interestingly, that last util class has isSubclassOf but not for interfaces, which means it must already exist in many forms elsewhere.
@mkouba any reason why Arc's isAssignableFrom uses findSupertypes instead of stopping on success?
There was a problem hiding this comment.
I rewrote this in ArC actually, so I can comment on this. The important thing that ArC does and this utility class does not is memoization. In ArC, we remember the full set of supertypes for each class, which allows us to later answer the "is subtype of" queries instantly -- we do the traversal just once.
There was a problem hiding this comment.
See io.quarkus.arc.processor.AssignabilityCheck.
There was a problem hiding this comment.
Also, the work queue algorithm is very common in compilers, I just mangled the name a long time ago and it kinda stuck. In compilers, it is commonly known as "worklist".
|
Very interesting bug, for sure! |
The check that was being done is broken in Java 21 as that version bring SequencedCollection which List now implements Closes: quarkusio#36170
geoand
dismissed
FroMage’s stale review
Let's leave the algo discussion for when we try to merge things with Arc
| private static Set<DotName> findSupertypes(DotName name, IndexView index) { | ||
| Set<DotName> result = new HashSet<>(); | ||
|
|
||
| Deque<DotName> workQueue = new ArrayDeque<>(); |
There was a problem hiding this comment.
Yeah, I figured this came from somewhere :( I also wondered why it was not using the existing JandexUtil from quarkus-core-deployment, but this is not usable from independent-projects, same as for Arc. Interestingly, that last util class has isSubclassOf but not for interfaces, which means it must already exist in many forms elsewhere.
@mkouba any reason why Arc's isAssignableFrom uses findSupertypes instead of stopping on success?
|
One solution I can think of is to have a smallrye jandex util library that all projects can use cc @Ladicek |
Failing Jobs - Building ffa805a
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 20 #- Failing: extensions/hibernate-orm/deployment
! Skipped: extensions/flyway/deployment extensions/hibernate-envers/deployment extensions/hibernate-reactive/deployment and 87 more📦 extensions/hibernate-orm/deployment✖ |
|
I did think about providing a "type system" library in Jandex, but it's hard. Most of the time, we don't need the full JLS subtyping rules -- like here (and in ArC), that would be just overkill. And there are other rules than just JLS in some contexts (e.g. CDI has its own assignability rules that are in certain aspects quite different from JLS). So I never really got to doing it. The |
The check that was being done is broken
in Java 21 as that version bring SequencedCollection which List now implements
Closes: #36170