Skip to content

Commit

Permalink
ArC - fix an issue with bridge methods and generics
Browse files Browse the repository at this point in the history
  • Loading branch information
mkouba committed Mar 24, 2021
1 parent d4828e6 commit a04bc6c
Show file tree
Hide file tree
Showing 6 changed files with 190 additions and 48 deletions.
@@ -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;
}

}
Expand Up @@ -78,6 +78,7 @@ public class BeanDeployment {
private final List<ObserverInfo> observers;

final BeanResolverImpl beanResolver;
private final AssignabilityCheck assignabilityCheck;

private final InterceptorResolver interceptorResolver;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -417,6 +419,10 @@ public BeanResolver getBeanResolver() {
return beanResolver;
}

public AssignabilityCheck getAssignabilityCheck() {
return assignabilityCheck;
}

boolean hasApplicationIndex() {
return applicationIndex != null;
}
Expand Down
Expand Up @@ -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;
Expand All @@ -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<>();
}

Expand Down Expand Up @@ -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;
}
}
Expand All @@ -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);
}
Expand Down
Expand Up @@ -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,
Expand Down Expand Up @@ -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<>();
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
@@ -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;
}

}

}
Expand Up @@ -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() {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
}

}

}

0 comments on commit a04bc6c

Please sign in to comment.