From 92d881ed9e031021ea01c43b168048468ebd324e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89amonn=20McManus?= Date: Fri, 1 Dec 2023 17:38:16 -0800 Subject: [PATCH] Don't call `Types.asMemberOf` unnecessarily. If method `m` is defined in class `C`, there's no point in calling `asMemberOf(C, m)`. `asMemberOf` is only useful when `m` is defined in a subtype of `C`. This is a very minor optimization, but it also avoids an issue where `asMemberOf` can return a method that has lost type annotations on its return type. We still have that issue for inherited methods, but at least in the common case of locally-defined methods we can avoid it. RELNOTES=Annotations on type parameters, like `abstract @Nullable T foo()`, are now better propagated to fields and constructor parameters. PiperOrigin-RevId: 587175179 --- .../google/auto/value/AutoValueJava8Test.java | 54 +++++++++++++++++++ .../processor/AutoValueishProcessor.java | 2 +- .../auto/value/processor/EclipseHack.java | 30 ++++++----- 3 files changed, 73 insertions(+), 13 deletions(-) diff --git a/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java b/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java index dfd825ca30..6cd7cd00ca 100644 --- a/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java +++ b/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java @@ -38,6 +38,7 @@ import java.lang.annotation.Target; import java.lang.reflect.AnnotatedType; import java.lang.reflect.Constructor; +import java.lang.reflect.Field; import java.lang.reflect.Method; import java.lang.reflect.TypeVariable; import java.util.ArrayList; @@ -1047,4 +1048,57 @@ public void nullableVariableBound() { assertThat(x.nullOne()).isNull(); assertThat(x.nullTwo()).isNull(); } + + @AutoValue + public abstract static class NotNullableVariableBound { + public abstract T t(); + + public abstract @Nullable T nullableT(); + + public abstract String string(); + + public static Builder builder() { + return new AutoValue_AutoValueJava8Test_NotNullableVariableBound.Builder<>(); + } + + @AutoValue.Builder + public abstract static class Builder { + public abstract Builder setT(T t); + + public abstract Builder setNullableT(@Nullable T nullableT); + + public abstract Builder setString(String string); + + public abstract NotNullableVariableBound build(); + } + } + + @Test + public void typeParameterBuilderFieldsAreNullable() throws ReflectiveOperationException { + assertThrows(NullPointerException.class, () -> NotNullableVariableBound.builder().setT(null)); + + // Even though neither t() nor string() has a @Nullable return type, the corresponding builder + // fields should be @Nullable. This test depends on the knowledge that for a property `t`, we + // will have a field also called `t`. + Field builderT = NotNullableVariableBound.builder().getClass().getDeclaredField("t"); + assertThat(builderT.getAnnotatedType().getAnnotations()).asList().contains(nullable()); + Field builderNullableT = + NotNullableVariableBound.builder().getClass().getDeclaredField("nullableT"); + assertThat(builderNullableT.getAnnotatedType().getAnnotations()).asList().contains(nullable()); + + // Meanwhile the AutoValue class itself should have @Nullable on the private field, the getter + // method, and the constructor parameter for nullableT. + Class autoValueClass = AutoValue_AutoValueJava8Test_NotNullableVariableBound.class; + Field nullableTField = autoValueClass.getDeclaredField("nullableT"); + assertThat(nullableTField.getAnnotatedType().getAnnotations()).asList().contains(nullable()); + Method nullableTMethod = autoValueClass.getMethod("nullableT"); + assertThat(nullableTMethod.getAnnotatedReturnType().getAnnotations()) + .asList() + .contains(nullable()); + Constructor autoValueConstructor = + autoValueClass.getDeclaredConstructor(Object.class, Object.class, String.class); + assertThat(autoValueConstructor.getAnnotatedParameterTypes()[1].getAnnotations()) + .asList() + .contains(nullable()); + } } diff --git a/value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java b/value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java index bfe64c3420..e695ecd500 100644 --- a/value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java +++ b/value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java @@ -224,7 +224,7 @@ private static String builderInitializer( * *
    *
  • the property is not primitive; - *
  • the property is not already nullable; + *
  • the property type does not already have a {@code @Nullable} annotation; *
  • there is no explicit initializer (for example {@code Optional} properties start off as * {@code Optional.empty()}); *
  • we have found a {@code @Nullable} type annotation that can be applied. diff --git a/value/src/main/java/com/google/auto/value/processor/EclipseHack.java b/value/src/main/java/com/google/auto/value/processor/EclipseHack.java index 727e32875c..65af63e2a5 100644 --- a/value/src/main/java/com/google/auto/value/processor/EclipseHack.java +++ b/value/src/main/java/com/google/auto/value/processor/EclipseHack.java @@ -146,21 +146,27 @@ ImmutableMap methodReturnTypes( ImmutableMap.Builder map = ImmutableMap.builder(); Map noArgMethods = null; for (ExecutableElement method : methods) { - TypeMirror returnType = null; - try { - TypeMirror methodMirror = typeUtils.asMemberOf(in, method); - returnType = MoreTypes.asExecutable(methodMirror).getReturnType(); - } catch (IllegalArgumentException e) { - if (method.getParameters().isEmpty()) { - if (noArgMethods == null) { - noArgMethods = noArgMethodsIn(in); + TypeMirror returnType = method.getReturnType(); + if (!in.asElement().equals(method.getEnclosingElement())) { + // If this method is *not* inherited, but directly defined in `in`, then asMemberOf + // shouldn't have any effect. So the if-check here is an optimization. But it also avoids an + // issue where the compiler may return an ExecutableType that has lost any annotations that + // were present in the original. + // We can still hit that issue in the case where the method *is* inherited. Fixing it in + // general would probably involve keeping track of type annotations ourselves, separately + // from TypeMirror instances. + try { + TypeMirror methodMirror = typeUtils.asMemberOf(in, method); + returnType = MoreTypes.asExecutable(methodMirror).getReturnType(); + } catch (IllegalArgumentException e) { + if (method.getParameters().isEmpty()) { + if (noArgMethods == null) { + noArgMethods = noArgMethodsIn(in); + } + returnType = noArgMethods.get(method.getSimpleName()).getReturnType(); } - returnType = noArgMethods.get(method.getSimpleName()).getReturnType(); } } - if (returnType == null) { - returnType = method.getReturnType(); - } map.put(method, returnType); } return map.build();