Skip to content

Commit

Permalink
Correctly handle cases where base classes have a package private cons…
Browse files Browse the repository at this point in the history
…tructor 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
  • Loading branch information
Chang-Eric authored and Dagger Team committed Apr 11, 2024
1 parent 18ce1b5 commit db25237
Show file tree
Hide file tree
Showing 11 changed files with 275 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand All @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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
Expand Down Expand Up @@ -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());
}
}
4 changes: 4 additions & 0 deletions javatests/dagger/hilt/android/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
16 changes: 16 additions & 0 deletions javatests/dagger/hilt/android/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
120 changes: 120 additions & 0 deletions javatests/dagger/hilt/android/PackagePrivateConstructorTest.java
Original file line number Diff line number Diff line change
@@ -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);
}
}
12 changes: 12 additions & 0 deletions javatests/dagger/hilt/android/testsubpackage/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Loading

0 comments on commit db25237

Please sign in to comment.