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

ArC: beans injected into All List injection points should be unremovable #33903

Merged
merged 1 commit into from
Jun 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package io.quarkus.arc.deployment;

import static io.quarkus.arc.processor.KotlinUtils.isKotlinClass;
import static io.quarkus.deployment.annotations.ExecutionTime.RUNTIME_INIT;
import static io.quarkus.deployment.annotations.ExecutionTime.STATIC_INIT;

Expand All @@ -9,7 +8,6 @@
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand All @@ -23,11 +21,9 @@
import jakarta.enterprise.context.ApplicationScoped;
import jakarta.enterprise.inject.AmbiguousResolutionException;
import jakarta.enterprise.inject.UnsatisfiedResolutionException;
import jakarta.enterprise.inject.spi.DefinitionException;

import org.jboss.jandex.AnnotationInstance;
import org.jboss.jandex.AnnotationTarget;
import org.jboss.jandex.AnnotationValue;
import org.jboss.jandex.ClassInfo;
import org.jboss.jandex.DotName;
import org.jboss.jandex.FieldInfo;
Expand All @@ -46,7 +42,6 @@
import io.quarkus.arc.deployment.UnremovableBeanBuildItem.BeanTypeExclusion;
import io.quarkus.arc.deployment.ValidationPhaseBuildItem.ValidationErrorBuildItem;
import io.quarkus.arc.processor.AlternativePriorities;
import io.quarkus.arc.processor.Annotations;
import io.quarkus.arc.processor.AnnotationsTransformer;
import io.quarkus.arc.processor.BeanConfigurator;
import io.quarkus.arc.processor.BeanDefiningAnnotation;
Expand All @@ -56,13 +51,10 @@
import io.quarkus.arc.processor.BeanProcessor;
import io.quarkus.arc.processor.BeanRegistrar;
import io.quarkus.arc.processor.BeanResolver;
import io.quarkus.arc.processor.Beans;
import io.quarkus.arc.processor.BytecodeTransformer;
import io.quarkus.arc.processor.ContextConfigurator;
import io.quarkus.arc.processor.ContextRegistrar;
import io.quarkus.arc.processor.DotNames;
import io.quarkus.arc.processor.InjectionPointInfo;
import io.quarkus.arc.processor.InjectionPointInfo.TypeAndQualifiers;
import io.quarkus.arc.processor.ObserverConfigurator;
import io.quarkus.arc.processor.ObserverRegistrar;
import io.quarkus.arc.processor.ReflectionRegistration;
Expand Down Expand Up @@ -466,39 +458,6 @@ public ObserverRegistrationPhaseBuildItem registerSyntheticObservers(BeanRegistr
// Initialize the type -> bean map
beanRegistrationPhase.getBeanProcessor().getBeanDeployment().initBeanByTypeMap();

// Register a synthetic bean for each List<?> with qualifier @All
List<InjectionPointInfo> listAll = beanRegistrationPhase.getInjectionPoints().stream()
.filter(this::isListAllInjectionPoint).collect(Collectors.toList());
for (InjectionPointInfo injectionPoint : listAll) {
// Note that at this point we can be sure that the required type is List<>
Type typeParam = injectionPoint.getType().asParameterizedType().arguments().get(0);
if (typeParam.kind() == Type.Kind.WILDCARD_TYPE) {
ClassInfo declaringClass;
if (injectionPoint.isField()) {
declaringClass = injectionPoint.getTarget().asField().declaringClass();
} else {
declaringClass = injectionPoint.getTarget().asMethod().declaringClass();
}
if (isKotlinClass(declaringClass)) {
validationErrors.produce(new ValidationErrorBuildItem(
new DefinitionException(
"kotlin.collections.List cannot be used together with the @All qualifier, please use MutableList or java.util.List instead: "
+ injectionPoint.getTargetInfo())));
} else {
validationErrors.produce(new ValidationErrorBuildItem(
new DefinitionException(
"Wildcard is not a legal type argument for " + injectionPoint.getTargetInfo())));
}
} else if (typeParam.kind() == Type.Kind.TYPE_VARIABLE) {
validationErrors.produce(new ValidationErrorBuildItem(new DefinitionException(
"Type variable is not a legal type argument for " + injectionPoint.getTargetInfo())));
}
}
if (!listAll.isEmpty()) {
registerUnremovableListBeans(beanRegistrationPhase, listAll, reflectiveMethods, reflectiveFields,
unremovableBeans);
}

BeanProcessor beanProcessor = beanRegistrationPhase.getBeanProcessor();
ObserverRegistrar.RegistrationContext registrationContext = beanProcessor.registerSyntheticObservers();

Expand Down Expand Up @@ -793,60 +752,6 @@ void registerContextPropagation(ArcConfig config, BuildProducer<ThreadContextPro
}
}

private void registerUnremovableListBeans(BeanRegistrationPhaseBuildItem beanRegistrationPhase,
List<InjectionPointInfo> injectionPoints, BuildProducer<ReflectiveMethodBuildItem> reflectiveMethods,
BuildProducer<ReflectiveFieldBuildItem> reflectiveFields,
BuildProducer<UnremovableBeanBuildItem> unremovableBeans) {
BeanDeployment beanDeployment = beanRegistrationPhase.getBeanProcessor().getBeanDeployment();
List<TypeAndQualifiers> unremovables = new ArrayList<>();

for (InjectionPointInfo injectionPoint : injectionPoints) {
// All qualifiers but @All
Set<AnnotationInstance> qualifiers = new HashSet<>(injectionPoint.getRequiredQualifiers());
for (Iterator<AnnotationInstance> it = qualifiers.iterator(); it.hasNext();) {
AnnotationInstance qualifier = it.next();
if (DotNames.ALL.equals(qualifier.name())) {
it.remove();
}
}
if (qualifiers.isEmpty()) {
// If no other qualifier is used then add @Any
qualifiers.add(AnnotationInstance.create(DotNames.ANY, null, new AnnotationValue[] {}));
}

Type elementType = injectionPoint.getType().asParameterizedType().arguments().get(0);

// make note of all types inside @All List<X> to make sure they are unremovable
unremovables.add(new TypeAndQualifiers(
elementType.name().equals(DotNames.INSTANCE_HANDLE)
? elementType.asParameterizedType().arguments().get(0)
: elementType,
qualifiers));
}
if (!unremovables.isEmpty()) {
// New beans were registered - we need to re-init the type -> bean map
// Also make all beans that match the List<> injection points unremovable
beanDeployment.initBeanByTypeMap();
// And make all the matching beans unremovable
unremovableBeans.produce(new UnremovableBeanBuildItem(new Predicate<BeanInfo>() {
@Override
public boolean test(BeanInfo bean) {
for (TypeAndQualifiers tq : unremovables) {
if (Beans.matches(bean, tq)) {
return true;
}
}
return false;
}
}));
}
}

private boolean isListAllInjectionPoint(InjectionPointInfo injectionPoint) {
return DotNames.LIST.equals(injectionPoint.getRequiredType().name())
&& Annotations.contains(injectionPoint.getRequiredQualifiers(), DotNames.ALL);
}

private abstract static class AbstractCompositeApplicationClassesPredicate<T> implements Predicate<T> {

private final IndexView applicationClassesIndex;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,8 @@ private Set<DecoratorInfo> removeUnusedDecorators(Set<DecoratorInfo> removedDeco
}

private Set<BeanInfo> removeUnusedBeans(Set<BeanInfo> declaresObserver, List<Predicate<BeanInfo>> allUnusedExclusions) {
Set<BeanInfo> removableBeans = UnusedBeans.findRemovableBeans(this.beans, this.injectionPoints, declaresObserver,
Set<BeanInfo> removableBeans = UnusedBeans.findRemovableBeans(beanResolver, this.beans, this.injectionPoints,
declaresObserver,
allUnusedExclusions);
if (!removableBeans.isEmpty()) {
this.beans.removeAll(removableBeans);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.quarkus.arc.processor;

import static io.quarkus.arc.processor.IndexClassLookupUtils.getClassByName;
import static io.quarkus.arc.processor.KotlinUtils.isKotlinClass;

import java.lang.reflect.Member;
import java.util.HashSet;
Expand Down Expand Up @@ -64,7 +65,7 @@ public enum BuiltinBean {
BuiltinBean::validateEventMetadata, DotNames.EVENT_METADATA),
LIST(BuiltinBean::generateListBytecode,
(ip, names) -> cdiAndRawTypeMatches(ip, DotNames.LIST) && ip.getRequiredQualifier(DotNames.ALL) != null,
DotNames.LIST),
BuiltinBean::validateList, DotNames.LIST),
;

private final DotName[] rawTypeDotNames;
Expand Down Expand Up @@ -443,6 +444,38 @@ private static void validateInstance(InjectionTargetInfo injectionTarget, Inject
}
}

private static void validateList(InjectionTargetInfo injectionTarget, InjectionPointInfo injectionPoint,
Consumer<Throwable> errors) {
if (injectionPoint.getType().kind() != Kind.PARAMETERIZED_TYPE) {
errors.accept(
new DefinitionException("An injection point of raw type is defined: " + injectionPoint.getTargetInfo()));
} else {
// Note that at this point we can be sure that the required type is List<>
Type typeParam = injectionPoint.getType().asParameterizedType().arguments().get(0);
if (typeParam.kind() == Type.Kind.WILDCARD_TYPE) {
ClassInfo declaringClass;
if (injectionPoint.isField()) {
declaringClass = injectionPoint.getTarget().asField().declaringClass();
} else {
declaringClass = injectionPoint.getTarget().asMethod().declaringClass();
}
if (isKotlinClass(declaringClass)) {
errors.accept(
new DefinitionException(
"kotlin.collections.List cannot be used together with the @All qualifier, please use MutableList or java.util.List instead: "
+ injectionPoint.getTargetInfo()));
} else {
errors.accept(
new DefinitionException(
"Wildcard is not a legal type argument for: " + injectionPoint.getTargetInfo()));
}
} else if (typeParam.kind() == Type.Kind.TYPE_VARIABLE) {
errors.accept(new DefinitionException(
"Type variable is not a legal type argument for: " + injectionPoint.getTargetInfo()));
}
}
}

private static void validateInjectionPoint(InjectionTargetInfo injectionTarget, InjectionPointInfo injectionPoint,
Consumer<Throwable> errors) {
if (injectionTarget.kind() != TargetKind.BEAN || !BuiltinScope.DEPENDENT.is(injectionTarget.asBean().getScope())) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,41 +1,72 @@
package io.quarkus.arc.processor;

import static java.util.function.Predicate.not;

import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import org.jboss.jandex.AnnotationInstance;
import org.jboss.jandex.Type;
import org.jboss.logging.Logger;

import io.quarkus.arc.processor.InjectionPointInfo.TypeAndQualifiers;

final class UnusedBeans {

private static final Logger LOG = Logger.getLogger(UnusedBeans.class);

private UnusedBeans() {
}

static Set<BeanInfo> findRemovableBeans(Collection<BeanInfo> beans, Collection<InjectionPointInfo> injectionPoints,
Set<BeanInfo> declaresObserver, List<Predicate<BeanInfo>> allUnusedExclusions) {
static Set<BeanInfo> findRemovableBeans(BeanResolver beanResolver, Collection<BeanInfo> beans,
Iterable<InjectionPointInfo> injectionPoints, Set<BeanInfo> declaresObserver,
List<Predicate<BeanInfo>> allUnusedExclusions) {
Set<BeanInfo> removableBeans = new HashSet<>();

Set<BeanInfo> unusedProducers = new HashSet<>();
Set<BeanInfo> unusedButDeclaresProducer = new HashSet<>();
List<BeanInfo> producers = beans.stream().filter(BeanInfo::isProducer)
.collect(Collectors.toList());
List<InjectionPointInfo> instanceInjectionPoints = injectionPoints.stream()
.filter(InjectionPointInfo::isProgrammaticLookup)
.collect(Collectors.toList());
// Collect all injected beans; skip delegate injection points and injection points that resolve to a built-in bean
Set<BeanInfo> injected = injectionPoints.stream()
.filter(not(InjectionPointInfo::isDelegate).and(InjectionPointInfo::hasResolvedBean))
.map(InjectionPointInfo::getResolvedBean)
.collect(Collectors.toSet());
// Collect all:
// - injected beans; skip delegate injection points and injection points that resolve to a built-in bean
// - Instance<> injection points
// - @All List<> injection points
Set<BeanInfo> injected = new HashSet<>();
List<InjectionPointInfo> instanceInjectionPoints = new ArrayList<>();
List<TypeAndQualifiers> listAllInjectionPoints = new ArrayList<>();

for (InjectionPointInfo injectionPoint : injectionPoints) {
if (injectionPoint.isProgrammaticLookup()) {
instanceInjectionPoints.add(injectionPoint);
} else if (!injectionPoint.isDelegate()) {
BeanInfo resolved = injectionPoint.getResolvedBean();
if (resolved != null) {
injected.add(resolved);
} else {
BuiltinBean builtin = BuiltinBean.resolve(injectionPoint);
if (builtin == BuiltinBean.LIST) {
Type requiredType = injectionPoint.getType().asParameterizedType().arguments().get(0);
if (requiredType.name().equals(DotNames.INSTANCE_HANDLE)) {
requiredType = requiredType.asParameterizedType().arguments().get(0);
}
Set<AnnotationInstance> qualifiers = new HashSet<>(injectionPoint.getRequiredQualifiers());
for (Iterator<AnnotationInstance> it = qualifiers.iterator(); it.hasNext();) {
AnnotationInstance qualifier = it.next();
if (qualifier.name().equals(DotNames.ALL)) {
it.remove();
}
}
listAllInjectionPoints.add(new TypeAndQualifiers(requiredType, qualifiers));
}
}
}
}
Set<BeanInfo> declaresProducer = producers.stream().map(BeanInfo::getDeclaringBean).collect(Collectors.toSet());

// Beans - first pass to find unused beans that do not declare a producer
Expand Down Expand Up @@ -70,12 +101,20 @@ static Set<BeanInfo> findRemovableBeans(Collection<BeanInfo> beans, Collection<I
// Instance<Foo>
for (InjectionPointInfo injectionPoint : instanceInjectionPoints) {
if (Beans.hasQualifiers(bean, injectionPoint.getRequiredQualifiers())
&& bean.getDeployment().getBeanResolver().matchesType(bean,
&& beanResolver.matchesType(bean,
injectionPoint.getType().asParameterizedType().arguments().get(0))) {
LOG.debugf("Unremovable - programmatic lookup: %s", bean);
continue test;
}
}
// @All List<Foo>
for (TypeAndQualifiers tq : listAllInjectionPoints) {
if (Beans.hasQualifiers(bean, tq.qualifiers)
&& beanResolver.matchesType(bean, tq.type)) {
LOG.debugf("Unremovable - @All List: %s", bean);
continue test;
}
}
// Declares a producer - see also second pass
if (declaresProducer.contains(bean)) {
unusedButDeclaresProducer.add(bean);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ public class ListAllTest {
public ArcTestContainer container = new ArcTestContainer(Service.class, ServiceAlpha.class, ServiceBravo.class,
MyQualifier.class, Foo.class);

@SuppressWarnings("serial")
@Test
public void testSelectAll() {
verifyHandleInjection(Arc.container().listAll(Service.class), Object.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import java.math.BigDecimal;
import java.math.BigInteger;
import java.util.List;

import jakarta.annotation.PostConstruct;
import jakarta.annotation.Priority;
Expand All @@ -22,6 +23,7 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.arc.All;
import io.quarkus.arc.Arc;
import io.quarkus.arc.ArcContainer;
import io.quarkus.arc.impl.ArcContainerImpl;
Expand All @@ -32,10 +34,9 @@ public class RemoveUnusedBeansTest extends RemoveUnusedComponentsTest {
@RegisterExtension
public ArcTestContainer container = ArcTestContainer.builder()
.beanClasses(HasObserver.class, Foo.class, FooAlternative.class, HasName.class, UnusedProducers.class,
InjectedViaInstance.class, InjectedViaInstanceWithWildcard.class,
InjectedViaProvider.class, Excluded.class,
UsedProducers.class,
UnusedProducerButInjected.class, UsedViaInstanceWithUnusedProducer.class, UsesBeanViaInstance.class)
InjectedViaInstance.class, InjectedViaInstanceWithWildcard.class, InjectedViaProvider.class, Excluded.class,
UsedProducers.class, UnusedProducerButInjected.class, UsedViaInstanceWithUnusedProducer.class,
UsesBeanViaInstance.class, UsedViaAllList.class)
.removeUnusedBeans(true)
.addRemovalExclusion(b -> b.getBeanClass().toString().equals(Excluded.class.getName()))
.build();
Expand Down Expand Up @@ -67,6 +68,7 @@ public void testRemoval() {
assertFalse(ArcContainerImpl.instance().getRemovedBeans().isEmpty());
assertNotPresent(UnusedBean.class);
assertNotPresent(OnlyInjectedInUnusedBean.class);
assertPresent(UsedViaAllList.class);
}

@Dependent
Expand Down Expand Up @@ -197,6 +199,10 @@ static class UsesBeanViaInstance {

@Inject
Instance<UsedViaInstanceWithUnusedProducer> instance;

@Inject
@All
List<UsedViaAllList> list;
}

@Singleton
Expand All @@ -212,4 +218,9 @@ static class OnlyInjectedInUnusedBean {

}

@Singleton
static class UsedViaAllList {

}

}