From 55a2b68ebe445b8dca3795bd3cdfc5c09d566e74 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Thu, 23 Apr 2026 13:26:30 -0700 Subject: [PATCH] Move DeclaredMembers sorting later This change optimizes DeclaredMembers sorting of methods and fields by first filtering for the fields or methods with relevant annotations, and then only sorting the members of interest. PiperOrigin-RevId: 904608829 --- .../inject/internal/DeclaredMembers.java | 31 ++-- .../internal/ProviderMethodsModule.java | 61 ++++--- .../com/google/inject/spi/InjectionPoint.java | 152 ++++++++++-------- 3 files changed, 136 insertions(+), 108 deletions(-) diff --git a/core/src/com/google/inject/internal/DeclaredMembers.java b/core/src/com/google/inject/internal/DeclaredMembers.java index 376ba62d2f..a0e50eda4c 100644 --- a/core/src/com/google/inject/internal/DeclaredMembers.java +++ b/core/src/com/google/inject/internal/DeclaredMembers.java @@ -15,7 +15,8 @@ */ package com.google.inject.internal; -import com.google.common.collect.Ordering; +import static com.google.common.collect.Comparators.lexicographical; + import java.lang.reflect.Field; import java.lang.reflect.Method; import java.util.Arrays; @@ -36,24 +37,14 @@ public final class DeclaredMembers { private DeclaredMembers() {} - public static Field[] getDeclaredFields(Class type) { - return Arrays.stream(type.getDeclaredFields()) - .sorted( - Comparator.comparing(Field::getName) - .thenComparing(Field::getType, Comparator.comparing(Class::getName))) - .toArray(Field[]::new); - } + public static final Comparator FIELD_COMPARATOR = + Comparator.comparing(Field::getName) + .thenComparing(Field::getType, Comparator.comparing(Class::getName)); - public static Method[] getDeclaredMethods(Class type) { - return Arrays.stream(type.getDeclaredMethods()) - .sorted( - Comparator.comparing(Method::getName) - .thenComparing(Method::getReturnType, Comparator.comparing(Class::getName)) - .thenComparing( - method -> Arrays.asList(method.getParameterTypes()), - // TODO: use Comparators.lexicographical when it's not @Beta. - Ordering.>from(Comparator.comparing(Class::getName)) - .lexicographical())) - .toArray(Method[]::new); - } + public static final Comparator METHOD_COMPARATOR = + Comparator.comparing(Method::getName) + .thenComparing(Method::getReturnType, Comparator.comparing(Class::getName)) + .thenComparing( + method -> Arrays.asList(method.getParameterTypes()), + lexicographical(Comparator.comparing(Class::getName))); } diff --git a/core/src/com/google/inject/internal/ProviderMethodsModule.java b/core/src/com/google/inject/internal/ProviderMethodsModule.java index ffde0cadcc..96450014cf 100644 --- a/core/src/com/google/inject/internal/ProviderMethodsModule.java +++ b/core/src/com/google/inject/internal/ProviderMethodsModule.java @@ -39,6 +39,7 @@ import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; +import java.util.Comparator; import java.util.List; /** @@ -114,30 +115,30 @@ public List> getProviderMethods(Binder binder) { // The highest class in the type hierarchy that contained a provider method definition. Class superMostClass = getDelegateModuleClass(); for (Class c = superMostClass; c != Object.class && c != null; c = c.getSuperclass()) { - for (Method method : DeclaredMembers.getDeclaredMethods(c)) { - Annotation annotation = getAnnotation(binder, method); - if (annotation != null) { - if (isStaticModule() - && !Modifier.isStatic(method.getModifiers()) - && !Modifier.isAbstract(method.getModifiers())) { - binder.addError( - "%s is an instance method, but a class literal was passed. Make this method" - + " static or pass an instance of the module instead.", - method); - continue; - } - if (result == null) { - result = new ArrayList<>(); - methodsAndAnnotations = new ArrayList<>(); - } + for (MethodAndAnnotation methodAndAnnotation : + getDeclaredProviderAnnotatedMethods(c, binder)) { + Annotation annotation = methodAndAnnotation.annotation; + Method method = methodAndAnnotation.method; + if (isStaticModule() + && !Modifier.isStatic(method.getModifiers()) + && !Modifier.isAbstract(method.getModifiers())) { + binder.addError( + "%s is an instance method, but a class literal was passed. Make this method" + + " static or pass an instance of the module instead.", + method); + continue; + } + if (result == null) { + result = new ArrayList<>(); + methodsAndAnnotations = new ArrayList<>(); + } - ProviderMethod providerMethod = createProviderMethod(binder, method, annotation); - if (providerMethod != null) { - result.add(providerMethod); - } - methodsAndAnnotations.add(new MethodAndAnnotation(method, annotation)); - superMostClass = c; + ProviderMethod providerMethod = createProviderMethod(binder, method, annotation); + if (providerMethod != null) { + result.add(providerMethod); } + methodsAndAnnotations.add(methodAndAnnotation); + superMostClass = c; } } if (result == null) { @@ -201,6 +202,22 @@ public List> getProviderMethods(Binder binder) { return result; } + private static final Comparator METHOD_AND_ANNOTATION_COMPARATOR = + Comparator.comparing( + methodAndAnnotation -> methodAndAnnotation.method, DeclaredMembers.METHOD_COMPARATOR); + + private List getDeclaredProviderAnnotatedMethods(Class c, Binder binder) { + List result = new ArrayList<>(); + for (Method method : c.getDeclaredMethods()) { + Annotation annotation = getAnnotation(binder, method); + if (annotation != null) { + result.add(new MethodAndAnnotation(method, annotation)); + } + } + result.sort(METHOD_AND_ANNOTATION_COMPARATOR); + return result; + } + private static class MethodAndAnnotation { final Method method; final Annotation annotation; diff --git a/core/src/com/google/inject/spi/InjectionPoint.java b/core/src/com/google/inject/spi/InjectionPoint.java index 6963328903..ee13c3891d 100644 --- a/core/src/com/google/inject/spi/InjectionPoint.java +++ b/core/src/com/google/inject/spi/InjectionPoint.java @@ -45,6 +45,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -515,6 +516,7 @@ InjectionPoint toInjectionPoint() { static class InjectableMethod extends InjectableMember { final Method method; + /** * true if this method overrode a method that was annotated with com.google.inject.Inject. used * to allow different override behavior for guice inject vs jsr330 Inject @@ -716,74 +718,51 @@ private static Set getInjectionPoints( TypeLiteral current = hierarchy.get(i); - for (Field field : getDeclaredFields(current)) { - if (Modifier.isStatic(field.getModifiers()) == statics) { - Annotation atInject = getAtInject(field); - if (atInject != null) { - InjectableField injectableField = new InjectableField(current, field, atInject); - if (injectableField.specInject && Modifier.isFinal(field.getModifiers())) { - errors.cannotInjectFinalField(field); - } - injectableMembers.add(injectableField); - } + for (InjectableField injectableField : getDeclaredInjectableFields(current, statics)) { + Field field = injectableField.field; + if (injectableField.specInject && Modifier.isFinal(field.getModifiers())) { + errors.cannotInjectFinalField(field); } + injectableMembers.add(injectableField); } - for (Method method : getDeclaredMethods(current)) { - if (isEligibleForInjection(method, statics)) { - Annotation atInject = getAtInject(method); - if (atInject != null) { - InjectableMethod injectableMethod = new InjectableMethod(current, method, atInject); - if (checkForMisplacedBindingAnnotations(method, errors) - || !isValidMethod(injectableMethod, errors)) { - if (overrideIndex != null) { - boolean removed = - overrideIndex.removeIfOverriddenBy(method, false, injectableMethod); - if (removed) { - logger.log( - Level.WARNING, - "Method: {0} is not a valid injectable method (" - + "because it either has misplaced binding annotations " - + "or specifies type parameters) but is overriding a method that is " - + "valid. Because it is not valid, the method will not be injected. " - + "To fix this, make the method a valid injectable method.", - method); - } - } - continue; - } - if (statics) { - injectableMembers.add(injectableMethod); - } else { - if (overrideIndex == null) { - /* - * Creating the override index lazily means that the first type in the hierarchy - * with injectable methods (not necessarily the top most type) will be treated as - * the TOP position and will enjoy the same optimizations (no checks for overridden - * methods, etc.). - */ - overrideIndex = new OverrideIndex(injectableMembers); - } else { - // Forcibly remove the overridden method, otherwise we'll inject - // it twice. - overrideIndex.removeIfOverriddenBy(method, true, injectableMethod); - } - overrideIndex.add(injectableMethod); + for (InjectableMethod injectableMethod : + getDeclaredInjectableMethods(current, overrideIndex, statics)) { + Method method = injectableMethod.method; + if (checkForMisplacedBindingAnnotations(method, errors) + || !isValidMethod(injectableMethod, errors)) { + if (overrideIndex != null) { + boolean removed = overrideIndex.removeIfOverriddenBy(method, false, injectableMethod); + if (removed) { + logger.log( + Level.WARNING, + "Method: {0} is not a valid injectable method (" + + "because it either has misplaced binding annotations " + + "or specifies type parameters) but is overriding a method that is " + + "valid. Because it is not valid, the method will not be injected. " + + "To fix this, make the method a valid injectable method.", + method); } + } + continue; + } + if (statics) { + injectableMembers.add(injectableMethod); + } else { + if (overrideIndex == null) { + /* + * Creating the override index lazily means that the first type in the hierarchy + * with injectable methods (not necessarily the top most type) will be treated as + * the TOP position and will enjoy the same optimizations (no checks for overridden + * methods, etc.). + */ + overrideIndex = new OverrideIndex(injectableMembers); } else { - if (overrideIndex != null) { - boolean removed = overrideIndex.removeIfOverriddenBy(method, false, null); - if (removed) { - logger.log( - Level.WARNING, - "Method: {0} is not annotated with @Inject but " - + "is overriding a method that is annotated with @jakarta.inject.Inject." - + "Because it is not annotated with @Inject, the method will not be " - + "injected. To fix this, annotate the method with @Inject.", - method); - } - } + // Forcibly remove the overridden method, otherwise we'll inject + // it twice. + overrideIndex.removeIfOverriddenBy(method, true, injectableMethod); } + overrideIndex.add(injectableMethod); } } } @@ -805,12 +784,53 @@ private static Set getInjectionPoints( return builder.build(); } - private static Field[] getDeclaredFields(TypeLiteral type) { - return DeclaredMembers.getDeclaredFields(type.getRawType()); + private static final Comparator INJECTABLE_FIELD_COMPARATOR = + Comparator.comparing(field -> field.field, DeclaredMembers.FIELD_COMPARATOR); + + private static List getDeclaredInjectableFields( + TypeLiteral current, boolean statics) { + List fields = new ArrayList<>(); + for (Field field : current.getRawType().getDeclaredFields()) { + if (Modifier.isStatic(field.getModifiers()) == statics) { + Annotation atInject = getAtInject(field); + if (atInject != null) { + fields.add(new InjectableField(current, field, atInject)); + } + } + } + fields.sort(INJECTABLE_FIELD_COMPARATOR); + return fields; } - private static Method[] getDeclaredMethods(TypeLiteral type) { - return DeclaredMembers.getDeclaredMethods(type.getRawType()); + private static final Comparator INJECTABLE_METHOD_COMPARATOR = + Comparator.comparing(method -> method.method, DeclaredMembers.METHOD_COMPARATOR); + + private static List getDeclaredInjectableMethods( + TypeLiteral current, OverrideIndex overrideIndex, boolean statics) { + List methods = new ArrayList<>(); + for (Method method : current.getRawType().getDeclaredMethods()) { + if (isEligibleForInjection(method, statics)) { + Annotation atInject = getAtInject(method); + if (atInject != null) { + methods.add(new InjectableMethod(current, method, atInject)); + } else { + if (overrideIndex != null) { + boolean removed = overrideIndex.removeIfOverriddenBy(method, false, null); + if (removed) { + logger.log( + Level.WARNING, + "Method: {0} is not annotated with @Inject but " + + "is overriding a method that is annotated with @jakarta.inject.Inject." + + "Because it is not annotated with @Inject, the method will not be " + + "injected. To fix this, annotate the method with @Inject.", + method); + } + } + } + } + } + methods.sort(INJECTABLE_METHOD_COMPARATOR); + return methods; } /**