diff options
author | Eric Chang <erichang@google.com> | 2024-04-10 18:10:59 -0700 |
---|---|---|
committer | Dagger Team <dagger-dev+copybara@google.com> | 2024-04-10 18:14:42 -0700 |
commit | db25237df0f59943e47b89486383a7d7a5605b3c (patch) | |
tree | f94873302d1f9e985df788257fa42009c216ea82 | |
parent | 18ce1b5d0b3f3015d347ea44d7f1273c502fd5f2 (diff) | |
download | dagger2-db25237df0f59943e47b89486383a7d7a5605b3c.tar.gz |
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: 623662553
11 files changed, 275 insertions, 56 deletions
diff --git a/java/dagger/hilt/android/processor/internal/androidentrypoint/ActivityGenerator.java b/java/dagger/hilt/android/processor/internal/androidentrypoint/ActivityGenerator.java index e1bf74f50..dfee51ad0 100644 --- a/java/dagger/hilt/android/processor/internal/androidentrypoint/ActivityGenerator.java +++ b/java/dagger/hilt/android/processor/internal/androidentrypoint/ActivityGenerator.java @@ -95,7 +95,8 @@ public final class ActivityGenerator { Generators.copyConstructors( metadata.baseElement(), CodeBlock.builder().addStatement("_initHiltInternal()").build(), - builder); + builder, + metadata.element()); builder.addMethod(init()); if (!metadata.overridesAndroidEntryPointClass()) { builder diff --git a/java/dagger/hilt/android/processor/internal/androidentrypoint/BroadcastReceiverGenerator.java b/java/dagger/hilt/android/processor/internal/androidentrypoint/BroadcastReceiverGenerator.java index 78a7976d1..d2005acef 100644 --- a/java/dagger/hilt/android/processor/internal/androidentrypoint/BroadcastReceiverGenerator.java +++ b/java/dagger/hilt/android/processor/internal/androidentrypoint/BroadcastReceiverGenerator.java @@ -78,7 +78,7 @@ public final class BroadcastReceiverGenerator { 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) diff --git a/java/dagger/hilt/android/processor/internal/androidentrypoint/FragmentGenerator.java b/java/dagger/hilt/android/processor/internal/androidentrypoint/FragmentGenerator.java index 45017e42f..873e65457 100644 --- a/java/dagger/hilt/android/processor/internal/androidentrypoint/FragmentGenerator.java +++ b/java/dagger/hilt/android/processor/internal/androidentrypoint/FragmentGenerator.java @@ -86,7 +86,7 @@ public final class FragmentGenerator { 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) diff --git a/java/dagger/hilt/android/processor/internal/androidentrypoint/Generators.java b/java/dagger/hilt/android/processor/internal/androidentrypoint/Generators.java index dc2339b69..a79a3e907 100644 --- a/java/dagger/hilt/android/processor/internal/androidentrypoint/Generators.java +++ b/java/dagger/hilt/android/processor/internal/androidentrypoint/Generators.java @@ -45,6 +45,7 @@ import com.squareup.javapoet.TypeSpec; 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 @@ final class Generators { } /** 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 @@ final class Generators { 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()) { diff --git a/java/dagger/hilt/android/processor/internal/androidentrypoint/ServiceGenerator.java b/java/dagger/hilt/android/processor/internal/androidentrypoint/ServiceGenerator.java index 8e1405e4a..7c63152e1 100644 --- a/java/dagger/hilt/android/processor/internal/androidentrypoint/ServiceGenerator.java +++ b/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 final class ServiceGenerator { TypeSpec.classBuilder(generatedClassName.simpleName()) .superclass(metadata.baseClassName()) .addModifiers(metadata.generatedClassModifiers()) - .addMethods(baseClassConstructors()) .addMethod(onCreateMethod()); JavaPoetExtKt.addOriginatingElement(builder, metadata.element()); @@ -74,6 +66,7 @@ public final class ServiceGenerator { Generators.addInjectionMethods(metadata, builder); Generators.addComponentOverride(metadata, builder); + Generators.copyConstructors(metadata.baseElement(), builder, metadata.element()); env.getFiler() .write( @@ -81,29 +74,6 @@ public final class ServiceGenerator { 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() { diff --git a/java/dagger/hilt/android/processor/internal/androidentrypoint/ViewGenerator.java b/java/dagger/hilt/android/processor/internal/androidentrypoint/ViewGenerator.java index 799ba1f59..237b06802 100644 --- a/java/dagger/hilt/android/processor/internal/androidentrypoint/ViewGenerator.java +++ b/java/dagger/hilt/android/processor/internal/androidentrypoint/ViewGenerator.java @@ -78,7 +78,8 @@ public final class ViewGenerator { 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 final class ViewGenerator { 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 @@ public final class ViewGenerator { return isDeclared(type) && Processors.isAssignableFrom(type.getTypeElement(), AndroidClassNames.CONTEXT); } - - private boolean isInOurPackage(XConstructorElement constructor) { - return constructor - .getEnclosingElement() - .getPackageName() - .contentEquals(metadata.element().getPackageName()); - } } diff --git a/javatests/dagger/hilt/android/AndroidManifest.xml b/javatests/dagger/hilt/android/AndroidManifest.xml index ed24e45cb..24efa4d0b 100644 --- a/javatests/dagger/hilt/android/AndroidManifest.xml +++ b/javatests/dagger/hilt/android/AndroidManifest.xml @@ -46,6 +46,10 @@ 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" tools:ignore="MissingClass"/> diff --git a/javatests/dagger/hilt/android/BUILD b/javatests/dagger/hilt/android/BUILD index b3da0d2a6..f3ae20f2b 100644 --- a/javatests/dagger/hilt/android/BUILD +++ b/javatests/dagger/hilt/android/BUILD @@ -360,6 +360,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"], manifest = "AndroidManifest.xml", diff --git a/javatests/dagger/hilt/android/PackagePrivateConstructorTest.java b/javatests/dagger/hilt/android/PackagePrivateConstructorTest.java new file mode 100644 index 000000000..d20ff9828 --- /dev/null +++ b/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); + } +} diff --git a/javatests/dagger/hilt/android/testsubpackage/BUILD b/javatests/dagger/hilt/android/testsubpackage/BUILD index 114c861a6..287a0adbf 100644 --- a/javatests/dagger/hilt/android/testsubpackage/BUILD +++ b/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", diff --git a/javatests/dagger/hilt/android/testsubpackage/PackagePrivateConstructorTestClasses.java b/javatests/dagger/hilt/android/testsubpackage/PackagePrivateConstructorTestClasses.java new file mode 100644 index 000000000..a8ff38156 --- /dev/null +++ b/javatests/dagger/hilt/android/testsubpackage/PackagePrivateConstructorTestClasses.java @@ -0,0 +1,82 @@ +/* + * 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.testsubpackage; + +import android.app.IntentService; +import android.app.Service; +import android.content.BroadcastReceiver; +import android.content.Context; +import androidx.fragment.app.Fragment; +import androidx.fragment.app.FragmentActivity; +import android.util.AttributeSet; +import android.widget.LinearLayout; + +public final class PackagePrivateConstructorTestClasses { + + public abstract static class BaseActivity extends FragmentActivity { + public BaseActivity() {} + + BaseActivity(int unused) {} + } + + public abstract static class BaseFragment extends Fragment { + public BaseFragment() {} + + BaseFragment(int unused) {} + } + + public abstract static class BaseView extends LinearLayout { + public BaseView(Context context) { + super(context); + } + + public BaseView(Context context, AttributeSet attrs) { + super(context, attrs); + } + + public BaseView(Context context, AttributeSet attrs, int defStyleAttr) { + super(context, attrs, defStyleAttr); + } + + BaseView(Context context, int unused) { + super(context); + } + } + + public abstract static class BaseService extends Service { + public BaseService() {} + + BaseService(int unused) {} + } + + public abstract static class BaseIntentService extends IntentService { + public BaseIntentService(String name) { + super(name); + } + + BaseIntentService(String name, int unused) { + super(name); + } + } + + public abstract static class BaseBroadcastReceiver extends BroadcastReceiver { + public BaseBroadcastReceiver() {} + + BaseBroadcastReceiver(int unused) {} + } + +} |