Correctly handle cases where base classes have a package private constructor that isn't visible to the subclass.

RELNOTES=Fixed an issue where base classes with a package private constructor would cause the generated code to fail
PiperOrigin-RevId: 6236625

Author: Chang-Eric

java/dagger/hilt/android/processor/internal/androidentrypoint/ActivityGenerator.java

@@ -95,7 +95,8 @@ public void generate() throws IOException {
       Generators.copyConstructors(
           metadata.baseElement(),
           CodeBlock.builder().addStatement("_initHiltInternal()").build(),
-          builder);
+          builder,
+          metadata.element());
       builder.addMethod(init());
       if (!metadata.overridesAndroidEntryPointClass()) {
         builder

java/dagger/hilt/android/processor/internal/androidentrypoint/BroadcastReceiverGenerator.java

@@ -78,7 +78,7 @@ public void generate() throws IOException {
     JavaPoetExtKt.addOriginatingElement(builder, metadata.element());
     Generators.addGeneratedBaseClassJavadoc(builder, AndroidClassNames.ANDROID_ENTRY_POINT);
     Processors.addGeneratedAnnotation(builder, env, getClass());
-    Generators.copyConstructors(metadata.baseElement(), builder);
+    Generators.copyConstructors(metadata.baseElement(), builder, metadata.element());
 
     metadata.baseElement().getTypeParameters().stream()
         .map(XTypeParameterElement::getTypeVariableName)

java/dagger/hilt/android/processor/internal/androidentrypoint/FragmentGenerator.java

@@ -86,7 +86,7 @@ TypeSpec createTypeSpec() {
     Processors.addGeneratedAnnotation(builder, env, getClass());
     Generators.copyLintAnnotations(metadata.element(), builder);
     Generators.copySuppressAnnotations(metadata.element(), builder);
-    Generators.copyConstructors(metadata.baseElement(), builder);
+    Generators.copyConstructors(metadata.baseElement(), builder, metadata.element());
 
     metadata.baseElement().getTypeParameters().stream()
         .map(XTypeParameterElement::getTypeVariableName)

java/dagger/hilt/android/processor/internal/androidentrypoint/Generators.java

@@ -45,6 +45,7 @@
 import dagger.hilt.android.processor.internal.AndroidClassNames;
 import dagger.hilt.android.processor.internal.androidentrypoint.AndroidEntryPointMetadata.AndroidType;
 import dagger.hilt.processor.internal.ClassNames;
+import dagger.hilt.processor.internal.Processors;
 import java.util.List;
 import java.util.Optional;
 import java.util.stream.Collectors;
@@ -65,15 +66,20 @@ static void addGeneratedBaseClassJavadoc(TypeSpec.Builder builder, ClassName ann
   }
 
   /** Copies all constructors with arguments to the builder. */
-  static void copyConstructors(XTypeElement baseClass, TypeSpec.Builder builder) {
-    copyConstructors(baseClass, CodeBlock.builder().build(), builder);
+  static void copyConstructors(
+      XTypeElement baseClass, TypeSpec.Builder builder, XTypeElement subclassReference) {
+    copyConstructors(baseClass, CodeBlock.builder().build(), builder, subclassReference);
   }
 
   /** Copies all constructors with arguments along with an appended body to the builder. */
-  static void copyConstructors(XTypeElement baseClass, CodeBlock body, TypeSpec.Builder builder) {
+  static void copyConstructors(
+      XTypeElement baseClass,
+      CodeBlock body,
+      TypeSpec.Builder builder,
+      XTypeElement subclassReference) {
     ImmutableList<XConstructorElement> constructors =
         baseClass.getConstructors().stream()
-            .filter(constructor -> !constructor.isPrivate())
+            .filter(constructor -> isConstructorVisibleToSubclass(constructor, subclassReference))
             .collect(toImmutableList());
 
     if (constructors.size() == 1
@@ -86,6 +92,30 @@ && getOnlyElement(constructors).getParameters().isEmpty()
     constructors.forEach(constructor -> builder.addMethod(copyConstructor(constructor, body)));
   }
 
+  /**
+   * Returns true if the constructor is visibile to a subclass in the same package as the reference.
+   * A reference is used because usually for generators the subclass is being generated and so
+   * doesn't actually exist.
+   */
+  static boolean isConstructorVisibleToSubclass(
+      XConstructorElement constructor, XTypeElement subclassReference) {
+    // Check if the constructor has package private visibility and we're outside the package
+    if (Processors.hasJavaPackagePrivateVisibility(constructor)
+        && !constructor
+            .getEnclosingElement()
+            .getPackageName()
+            .contentEquals(subclassReference.getPackageName())) {
+      return false;
+      // Or if it is private, we know generated code can't be in the same file
+    } else if (constructor.isPrivate()) {
+      return false;
+    }
+
+    // Assume this is for a subclass per the name, so both protected and public methods are always
+    // accessible.
+    return true;
+  }
+
   /** Returns Optional with AnnotationSpec for Nullable if found on element, empty otherwise. */
   private static Optional<AnnotationSpec> getNullableAnnotationSpec(XElement element) {
     for (XAnnotation annotation : element.getAllAnnotations()) {

java/dagger/hilt/android/processor/internal/androidentrypoint/ServiceGenerator.java

@@ -16,21 +16,14 @@
 
 package dagger.hilt.android.processor.internal.androidentrypoint;
 
-import static dagger.internal.codegen.extension.DaggerStreams.toImmutableList;
-import static dagger.internal.codegen.xprocessing.XElements.getSimpleName;
-import static java.util.stream.Collectors.joining;
 
 import androidx.room.compiler.processing.JavaPoetExtKt;
-import androidx.room.compiler.processing.XConstructorElement;
-import androidx.room.compiler.processing.XExecutableParameterElement;
 import androidx.room.compiler.processing.XFiler;
 import androidx.room.compiler.processing.XProcessingEnv;
 import androidx.room.compiler.processing.XTypeParameterElement;
-import com.google.common.collect.ImmutableList;
 import com.squareup.javapoet.ClassName;
 import com.squareup.javapoet.JavaFile;
 import com.squareup.javapoet.MethodSpec;
-import com.squareup.javapoet.ParameterSpec;
 import com.squareup.javapoet.TypeSpec;
 import dagger.hilt.android.processor.internal.AndroidClassNames;
 import dagger.hilt.processor.internal.Processors;
@@ -58,7 +51,6 @@ public void generate() throws IOException {
         TypeSpec.classBuilder(generatedClassName.simpleName())
             .superclass(metadata.baseClassName())
             .addModifiers(metadata.generatedClassModifiers())
-            .addMethods(baseClassConstructors())
             .addMethod(onCreateMethod());
 
     JavaPoetExtKt.addOriginatingElement(builder, metadata.element());
@@ -74,36 +66,14 @@ public void generate() throws IOException {
     Generators.addInjectionMethods(metadata, builder);
 
     Generators.addComponentOverride(metadata, builder);
+    Generators.copyConstructors(metadata.baseElement(), builder, metadata.element());
 
     env.getFiler()
         .write(
             JavaFile.builder(generatedClassName.packageName(), builder.build()).build(),
             XFiler.Mode.Isolating);
   }
 
-  private ImmutableList<MethodSpec> baseClassConstructors() {
-    return metadata.baseElement().getConstructors().stream()
-        .map(ServiceGenerator::toMethodSpec)
-        .collect(toImmutableList());
-  }
-
-  private static MethodSpec toMethodSpec(XConstructorElement constructor) {
-    ImmutableList<ParameterSpec> params =
-        constructor.getParameters().stream()
-            .map(ServiceGenerator::toParameterSpec)
-            .collect(toImmutableList());
-
-    return MethodSpec.constructorBuilder()
-        .addParameters(params)
-        .addStatement("super($L)", params.stream().map(p -> p.name).collect(joining(",")))
-        .build();
-  }
-
-  private static ParameterSpec toParameterSpec(XExecutableParameterElement parameter) {
-    return ParameterSpec.builder(parameter.getType().getTypeName(), getSimpleName(parameter))
-        .build();
-  }
-
   // @CallSuper
   // @Override
   // protected void onCreate() {

java/dagger/hilt/android/processor/internal/androidentrypoint/ViewGenerator.java

@@ -78,7 +78,8 @@ public void generate() {
     Generators.addInjectionMethods(metadata, builder);
 
     metadata.baseElement().getConstructors().stream()
-        .filter(this::isConstructorVisibleToGeneratedClass)
+        .filter(constructor -> Generators.isConstructorVisibleToSubclass(
+            constructor, metadata.element()))
         .map(this::constructorMethod)
         .forEach(builder::addMethod);
 
@@ -88,16 +89,6 @@ public void generate() {
             XFiler.Mode.Isolating);
   }
 
-  private boolean isConstructorVisibleToGeneratedClass(XConstructorElement constructor) {
-    if (Processors.hasJavaPackagePrivateVisibility(constructor) && !isInOurPackage(constructor)) {
-      return false;
-    } else if (constructor.isPrivate()) {
-      return false;
-    }
-
-    // We extend the base class, so both protected and public methods are always accessible.
-    return true;
-  }
 
   /**
    * Returns a pass-through constructor matching the base class's provided constructorElement. The
@@ -185,11 +176,4 @@ private static boolean isFirstRestrictedParameter(XType type) {
     return isDeclared(type)
         && Processors.isAssignableFrom(type.getTypeElement(), AndroidClassNames.CONTEXT);
   }
-
-  private boolean isInOurPackage(XConstructorElement constructor) {
-    return constructor
-        .getEnclosingElement()
-        .getPackageName()
-        .contentEquals(metadata.element().getPackageName());
-  }
 }

javatests/dagger/hilt/android/AndroidManifest.xml

@@ -45,6 +45,10 @@
         android:name=".OptionalInjectTestClasses$OptionalSubclassActivity"
         android:exported="false"
         tools:ignore="MissingClass"/>
+    <activity
+        android:name=".PackagePrivateConstructorTest$TestActivity"
+        android:exported="false"
+        tools:ignore="MissingClass"/>
     <activity
         android:name=".QualifierInKotlinFieldsTest$TestActivity"
         android:exported="false"

javatests/dagger/hilt/android/BUILD

@@ -359,6 +359,22 @@ android_local_test(
     ],
 )
 
+android_local_test(
+    name = "PackagePrivateConstructorTest",
+    srcs = ["PackagePrivateConstructorTest.java"],
+    manifest = "AndroidManifest.xml",
+    manifest_values = {
+        "minSdkVersion": "14",
+    },
+    deps = [
+        "//:android_local_test_exports",
+        "//java/dagger/hilt/android:android_entry_point",
+        "//java/dagger/hilt/android:package_info",
+        "//java/dagger/hilt/android/testing:hilt_android_test",
+        "//javatests/dagger/hilt/android/testsubpackage:PackagePrivateConstructorTestClasses",
+    ],
+)
+
 android_local_test(
     name = "QualifierInKotlinFieldsTest",
     srcs = ["QualifierInKotlinFieldsTest.java"],

javatests/dagger/hilt/android/PackagePrivateConstructorTest.java

@@ -0,0 +1,120 @@
+/*
+ * Copyright (C) 2024 The Dagger Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package dagger.hilt.android;
+
+import android.content.Context;
+import android.content.Intent;
+import android.os.Build;
+import android.os.IBinder;
+import androidx.test.core.app.ActivityScenario;
+import androidx.test.core.app.ApplicationProvider;
+import androidx.test.ext.junit.runners.AndroidJUnit4;
+import dagger.hilt.android.testing.HiltAndroidRule;
+import dagger.hilt.android.testing.HiltAndroidTest;
+import dagger.hilt.android.testing.HiltTestApplication;
+import dagger.hilt.android.testsubpackage.PackagePrivateConstructorTestClasses.BaseActivity;
+import dagger.hilt.android.testsubpackage.PackagePrivateConstructorTestClasses.BaseBroadcastReceiver;
+import dagger.hilt.android.testsubpackage.PackagePrivateConstructorTestClasses.BaseFragment;
+import dagger.hilt.android.testsubpackage.PackagePrivateConstructorTestClasses.BaseIntentService;
+import dagger.hilt.android.testsubpackage.PackagePrivateConstructorTestClasses.BaseService;
+import dagger.hilt.android.testsubpackage.PackagePrivateConstructorTestClasses.BaseView;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.Robolectric;
+import org.robolectric.annotation.Config;
+
+/** Regression test for b/331280240. */
+@HiltAndroidTest
+@RunWith(AndroidJUnit4.class)
+// Robolectric requires Java9 to run API 29 and above, so use API 28 instead
+@Config(sdk = Build.VERSION_CODES.P, application = HiltTestApplication.class)
+public final class PackagePrivateConstructorTest {
+  @Rule public final HiltAndroidRule rule = new HiltAndroidRule(this);
+
+  @AndroidEntryPoint(BaseActivity.class)
+  public static final class TestActivity extends Hilt_PackagePrivateConstructorTest_TestActivity {
+  }
+
+  @AndroidEntryPoint(BaseFragment.class)
+  public static final class TestFragment extends Hilt_PackagePrivateConstructorTest_TestFragment {
+  }
+
+  @AndroidEntryPoint(BaseView.class)
+  public static final class TestView extends Hilt_PackagePrivateConstructorTest_TestView {
+      TestView(Context context) {
+        super(context);
+      }
+  }
+
+  @AndroidEntryPoint(BaseService.class)
+  public static final class TestService extends Hilt_PackagePrivateConstructorTest_TestService {
+    @Override
+    public IBinder onBind(Intent intent) {
+      return null;
+    }
+  }
+
+  @AndroidEntryPoint(BaseIntentService.class)
+  public static final class TestIntentService
+      extends Hilt_PackagePrivateConstructorTest_TestIntentService {
+    public TestIntentService() {
+      super("TestIntentServiceName");
+    }
+
+    @Override
+    public void onHandleIntent(Intent intent) {}
+  }
+
+  @AndroidEntryPoint(BaseBroadcastReceiver.class)
+  public static final class TestBroadcastReceiver
+      extends Hilt_PackagePrivateConstructorTest_TestBroadcastReceiver {
+  }
+
+  @Before
+  public void setup() {
+    rule.inject();
+  }
+
+  // Technically all the tests need to do is check for compilation, but might as well make sure the
+  // classes are usable
+  @Test
+  public void testActivityFragmentView() throws Exception {
+    try (ActivityScenario<TestActivity> scenario = ActivityScenario.launch(TestActivity.class)) {
+      scenario.onActivity(
+          activity -> {
+            TestFragment fragment = new TestFragment();
+            activity.getSupportFragmentManager().beginTransaction().add(fragment, "").commitNow();
+            TestView unused = new TestView(fragment.getContext());
+          });
+    }
+  }
+
+  @Test
+  public void testServices() throws Exception {
+    Robolectric.setupService(TestService.class);
+    Robolectric.setupService(TestIntentService.class);
+  }
+
+  @Test
+  public void testBroadcastReceiver() throws Exception {
+    TestBroadcastReceiver testBroadcastReceiver = new TestBroadcastReceiver();
+    Intent intent = new Intent();
+    testBroadcastReceiver.onReceive(ApplicationProvider.getApplicationContext(), intent);
+  }
+}

javatests/dagger/hilt/android/testsubpackage/BUILD

@@ -34,6 +34,18 @@ android_local_test(
     ],
 )
 
+android_library(
+    name = "PackagePrivateConstructorTestClasses",
+    srcs = ["PackagePrivateConstructorTestClasses.java"],
+    deps = [
+        "@maven//:androidx_activity_activity",
+        "@maven//:androidx_fragment_fragment",
+        "@maven//:androidx_lifecycle_lifecycle_common",
+        "@maven//:androidx_lifecycle_lifecycle_viewmodel",
+        "@maven//:androidx_lifecycle_lifecycle_viewmodel_savedstate",
+    ],
+)
+
 exports_files(srcs = [
     "UsesLocalComponentTestBindingsTest.java",
     "UsesSharedComponent1Test.java",