Merge pull request #15983 from mkouba/arc-bridge-methods-generics-fix

ArC - fix an issue with bridge methods and generics

independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/AssignabilityCheck.java

@@ -0,0 +1,51 @@
+package io.quarkus.arc.processor;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import org.jboss.jandex.ClassInfo;
+import org.jboss.jandex.CompositeIndex;
+import org.jboss.jandex.DotName;
+import org.jboss.jandex.IndexView;
+import org.jboss.jandex.Type;
+
+final class AssignabilityCheck {
+
+    private final ConcurrentMap<DotName, Set<DotName>> cache;
+    private final IndexView index;
+
+    public AssignabilityCheck(IndexView beanArchiveIndex, IndexView applicationIndex) {
+        this.cache = new ConcurrentHashMap<>();
+        this.index = applicationIndex != null ? CompositeIndex.create(beanArchiveIndex, applicationIndex) : beanArchiveIndex;
+    }
+
+    boolean isAssignableFrom(Type type1, Type type2) {
+        // java.lang.Object is assignable from any type
+        if (type1.name().equals(DotNames.OBJECT)) {
+            return true;
+        }
+        // type1 is the same as type2
+        if (type1.name().equals(type2.name())) {
+            return true;
+        }
+        // type1 is a superclass
+        return getAssignables(type1.name()).contains(type2.name());
+    }
+
+    Set<DotName> getAssignables(DotName name) {
+        return cache.computeIfAbsent(name, this::findAssignables);
+    }
+
+    private Set<DotName> findAssignables(DotName name) {
+        Set<DotName> assignables = new HashSet<>();
+        for (ClassInfo subclass : index.getAllKnownSubclasses(name)) {
+            assignables.add(subclass.name());
+        }
+        for (ClassInfo implementor : index.getAllKnownImplementors(name)) {
+            assignables.add(implementor.name());
+        }
+        return assignables;
+    }
+
+}

independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanDeployment.java

@@ -78,6 +78,7 @@ public class BeanDeployment {
     private final List<ObserverInfo> observers;
 
     final BeanResolverImpl beanResolver;
+    private final AssignabilityCheck assignabilityCheck;
 
     private final InterceptorResolver interceptorResolver;
 
@@ -180,6 +181,7 @@ public class BeanDeployment {
         this.beans = new CopyOnWriteArrayList<>();
         this.observers = new CopyOnWriteArrayList<>();
 
+        this.assignabilityCheck = new AssignabilityCheck(beanArchiveIndex, applicationIndex);
         this.beanResolver = new BeanResolverImpl(this);
         this.interceptorResolver = new InterceptorResolver(this);
         this.transformUnproxyableClasses = builder.transformUnproxyableClasses;
@@ -417,6 +419,10 @@ public BeanResolver getBeanResolver() {
         return beanResolver;
     }
 
+    public AssignabilityCheck getAssignabilityCheck() {
+        return assignabilityCheck;
+    }
+
     boolean hasApplicationIndex() {
         return applicationIndex != null;
     }

independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanResolverImpl.java

@@ -16,13 +16,9 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
-import java.util.function.Function;
 import javax.enterprise.inject.AmbiguousResolutionException;
 import org.jboss.jandex.AnnotationInstance;
-import org.jboss.jandex.ClassInfo;
 import org.jboss.jandex.ClassType;
-import org.jboss.jandex.DotName;
 import org.jboss.jandex.Type;
 import org.jboss.jandex.Type.Kind;
 import org.jboss.jandex.TypeVariable;
@@ -32,33 +28,10 @@ class BeanResolverImpl implements BeanResolver {
 
     private final BeanDeployment beanDeployment;
 
-    private final ConcurrentMap<DotName, Set<DotName>> assignableFromMap;
-
-    private final Function<DotName, Set<DotName>> assignableFromMapFunction;
-
     private final Map<TypeAndQualifiers, List<BeanInfo>> resolved;
 
     BeanResolverImpl(BeanDeployment beanDeployment) {
         this.beanDeployment = beanDeployment;
-        this.assignableFromMap = new ConcurrentHashMap<>();
-        this.assignableFromMapFunction = name -> {
-            Set<DotName> assignables = new HashSet<>();
-            for (ClassInfo subclass : beanDeployment.getBeanArchiveIndex().getAllKnownSubclasses(name)) {
-                assignables.add(subclass.name());
-            }
-            for (ClassInfo implementor : beanDeployment.getBeanArchiveIndex().getAllKnownImplementors(name)) {
-                assignables.add(implementor.name());
-            }
-            if (beanDeployment.hasApplicationIndex()) {
-                for (ClassInfo subclass : beanDeployment.getApplicationIndex().getAllKnownSubclasses(name)) {
-                    assignables.add(subclass.name());
-                }
-                for (ClassInfo implementor : beanDeployment.getApplicationIndex().getAllKnownImplementors(name)) {
-                    assignables.add(implementor.name());
-                }
-            }
-            return assignables;
-        };
         this.resolved = new ConcurrentHashMap<>();
     }
 
@@ -248,7 +221,7 @@ boolean parametersMatch(WildcardType requiredParameter, TypeVariable beanParamet
 
     boolean parametersMatch(Type requiredParameter, TypeVariable beanParameter) {
         for (Type bound : getUppermostTypeVariableBounds(beanParameter)) {
-            if (!isAssignableFrom(bound, requiredParameter)) {
+            if (!beanDeployment.getAssignabilityCheck().isAssignableFrom(bound, requiredParameter)) {
                 return false;
             }
         }
@@ -271,27 +244,14 @@ boolean boundsMatch(List<Type> bounds, List<Type> stricterBounds) {
         stricterBounds = getUppermostBounds(stricterBounds);
         for (Type bound : bounds) {
             for (Type stricterBound : stricterBounds) {
-                if (!isAssignableFrom(bound, stricterBound)) {
+                if (!beanDeployment.getAssignabilityCheck().isAssignableFrom(bound, stricterBound)) {
                     return false;
                 }
             }
         }
         return true;
     }
 
-    boolean isAssignableFrom(Type type1, Type type2) {
-        // java.lang.Object is assignable from any type
-        if (type1.name().equals(DotNames.OBJECT)) {
-            return true;
-        }
-        // type1 is the same as type2
-        if (type1.name().equals(type2.name())) {
-            return true;
-        }
-        // type1 is a superclass
-        return assignableFromMap.computeIfAbsent(type1.name(), assignableFromMapFunction).contains(type2.name());
-    }
-
     boolean lowerBoundsOfWildcardMatch(Type parameter, WildcardType requiredParameter) {
         return lowerBoundsOfWildcardMatch(singletonList(parameter), requiredParameter);
     }

independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Methods.java

@@ -143,7 +143,8 @@ static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment beanDeploym
             List<AnnotationInstance> classLevelBindings, Consumer<BytecodeTransformer> bytecodeTransformerConsumer,
             boolean transformUnproxyableClasses) {
         return addInterceptedMethodCandidates(beanDeployment, classInfo, candidates, classLevelBindings,
-                bytecodeTransformerConsumer, transformUnproxyableClasses, new SubclassSkipPredicate(), false);
+                bytecodeTransformerConsumer, transformUnproxyableClasses,
+                new SubclassSkipPredicate(beanDeployment.getAssignabilityCheck()::isAssignableFrom), false);
     }
 
     static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment beanDeployment, ClassInfo classInfo,
@@ -396,10 +397,15 @@ public MethodVisitor visitMethod(int access, String name, String descriptor, Str
      */
     static class SubclassSkipPredicate implements Predicate<MethodInfo> {
 
+        private final BiFunction<Type, Type, Boolean> assignableFromFun;
         private ClassInfo clazz;
         private List<MethodInfo> regularMethods;
         private Set<MethodInfo> bridgeMethods = new HashSet<>();
 
+        public SubclassSkipPredicate(BiFunction<Type, Type, Boolean> assignableFromFun) {
+            this.assignableFromFun = assignableFromFun;
+        }
+
         void startProcessing(ClassInfo clazz) {
             this.clazz = clazz;
             this.regularMethods = new ArrayList<>();
@@ -459,8 +465,7 @@ private boolean hasImplementation(MethodInfo bridge) {
                     for (int i = 0; i < bridgeParams.size(); i++) {
                         Type bridgeParam = bridgeParams.get(i);
                         Type param = params.get(i);
-                        if (param.name().equals(bridgeParam.name())
-                                || bridgeParam.name().equals(DotNames.OBJECT)) {
+                        if (assignableFromFun.apply(bridgeParam, param)) {
                             continue;
                         } else {
                             paramsNotMatching = true;
@@ -477,8 +482,8 @@ private boolean hasImplementation(MethodInfo bridge) {
                             // both cases are a match
                             return true;
                         } else {
-                            // as a last resort, we simply check equality of return Type
-                            return bridge.returnType().name().equals(declaredMethod.returnType().name());
+                            // as a last resort, we simply check assignability of the return type
+                            return assignableFromFun.apply(bridge.returnType(), declaredMethod.returnType());
                         }
                     }
                     return true;

independent-projects/arc/processor/src/test/java/io/quarkus/arc/processor/SubclassSkipPredicateTest.java

@@ -0,0 +1,80 @@
+package io.quarkus.arc.processor;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import io.quarkus.arc.processor.Methods.SubclassSkipPredicate;
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+import javax.enterprise.context.ApplicationScoped;
+import org.jboss.jandex.ClassInfo;
+import org.jboss.jandex.DotName;
+import org.jboss.jandex.IndexView;
+import org.jboss.jandex.MethodInfo;
+import org.junit.jupiter.api.Test;
+
+public class SubclassSkipPredicateTest {
+
+    @Test
+    public void testPredicate() throws IOException {
+        IndexView index = Basics.index(Base.class, Submarine.class, Long.class, Number.class);
+        AssignabilityCheck assignabilityCheck = new AssignabilityCheck(index, null);
+        SubclassSkipPredicate predicate = new SubclassSkipPredicate(assignabilityCheck::isAssignableFrom);
+
+        ClassInfo submarineClass = index.getClassByName(DotName.createSimple(Submarine.class.getName()));
+        predicate.startProcessing(submarineClass);
+
+        List<MethodInfo> echos = submarineClass.methods().stream().filter(m -> m.name().equals("echo"))
+                .collect(Collectors.toList());
+        assertEquals(2, echos.size());
+        assertPredicate(echos, predicate);
+
+        List<MethodInfo> getNames = submarineClass.methods().stream().filter(m -> m.name().equals("getName"))
+                .collect(Collectors.toList());
+        assertEquals(2, getNames.size());
+        assertPredicate(getNames, predicate);
+
+        predicate.methodsProcessed();
+    }
+
+    private void assertPredicate(List<MethodInfo> methods, SubclassSkipPredicate predicate) {
+        for (MethodInfo method : methods) {
+            if (Methods.isBridge(method)) {
+                // Bridge method with impl
+                assertTrue(predicate.test(method));
+            } else {
+                assertFalse(predicate.test(method));
+            }
+        }
+    }
+
+    static class Base<T extends Number, UNUSED> {
+
+        String echo(T payload) {
+            return payload.toString().toUpperCase();
+        }
+
+        T getName() {
+            return null;
+        }
+
+    }
+
+    @ApplicationScoped
+    static class Submarine extends Base<Long, Boolean> {
+
+        @Override
+        String echo(Long payload) {
+            return payload.toString();
+        }
+
+        @Override
+        Long getName() {
+            return 10l;
+        }
+
+    }
+
+}

independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/bridge/BridgeMethodInterceptionTest.java

@@ -17,7 +17,7 @@ public class BridgeMethodInterceptionTest {
     @RegisterExtension
     public ArcTestContainer container = new ArcTestContainer(Base.class, Submarine.class, Ubot.class, Ponorka.class,
             Counter.class, Simple.class, SimpleInterceptor.class, ExampleApi.class, ExampleResource.class,
-            AbstractResource.class);
+            AbstractResource.class, NextBase.class, NextSubmarine.class);
 
     @Test
     public void testInterception() {
@@ -54,6 +54,17 @@ public void testInterception() {
         assertEquals("TRUE", basePonorka.echo(true));
         assertNull(basePonorka.getName());
         assertEquals(4, counter.get());
+
+        counter.reset();
+        NextSubmarine nextSubmarine = container.instance(NextSubmarine.class).get();
+        assertEquals("foo", nextSubmarine.echo("foo"));
+        assertEquals(NextSubmarine.class.getSimpleName(), nextSubmarine.getName());
+        assertEquals(2, counter.get());
+        // Now let's invoke the bridge method...
+        NextBase<String> nextBase = nextSubmarine;
+        assertEquals("foo", nextBase.echo("foo"));
+        assertEquals(NextSubmarine.class.getSimpleName(), nextBase.getName());
+        assertEquals(4, counter.get());
     }
 
     @Test
@@ -123,4 +134,33 @@ static class Ponorka extends Base<Boolean> {
 
     }
 
+    static class NextBase<T extends Comparable<T>> {
+
+        String echo(T payload) {
+            return payload.toString().toUpperCase();
+        }
+
+        T getName() {
+            return null;
+        }
+
+    }
+
+    @ApplicationScoped
+    static class NextSubmarine extends NextBase<String> {
+
+        @Simple
+        @Override
+        String echo(String payload) {
+            return payload.toString();
+        }
+
+        @Simple
+        @Override
+        String getName() {
+            return NextSubmarine.class.getSimpleName();
+        }
+
+    }
+
 }