From 15a30ca15b1ff36ca188a4f4721b8a563f1ef6cb Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 21 Oct 2024 14:04:06 -0700 Subject: [PATCH] Restrict multibindings from providing framework types that conflict with multibinding types Dagger provides. RELNOTES=Restrict multibindings from providing framework types that conflict with multibinding types Dagger provides. This is technically a breaking change but existing issues should either be for unused code or code that should have already broken anyway. PiperOrigin-RevId: 688265205 --- .../internal/codegen/base/FrameworkTypes.java | 17 ++ .../validation/BindingElementValidator.java | 27 ++- .../MapMultibindingValidationTest.java | 78 +++++++ .../SetMultibindingValidationTest.java | 212 ++++++++++++++++++ 4 files changed, 330 insertions(+), 4 deletions(-) create mode 100644 javatests/dagger/internal/codegen/SetMultibindingValidationTest.java diff --git a/java/dagger/internal/codegen/base/FrameworkTypes.java b/java/dagger/internal/codegen/base/FrameworkTypes.java index e39aeabeadc..2a56e1d8bba 100644 --- a/java/dagger/internal/codegen/base/FrameworkTypes.java +++ b/java/dagger/internal/codegen/base/FrameworkTypes.java @@ -16,6 +16,7 @@ package dagger.internal.codegen.base; +import static dagger.internal.codegen.extension.DaggerStreams.toImmutableSet; import static dagger.internal.codegen.xprocessing.XTypes.isTypeOf; import androidx.room.compiler.processing.XType; @@ -41,6 +42,14 @@ public final class FrameworkTypes { private static final ImmutableSet ALL_FRAMEWORK_TYPES = ImmutableSet.builder().addAll(PROVISION_TYPES).addAll(PRODUCTION_TYPES).build(); + private static final ImmutableSet SET_VALUE_FRAMEWORK_TYPES = + ImmutableSet.of(TypeNames.PRODUCED); + + public static final ImmutableSet MAP_VALUE_FRAMEWORK_TYPES = + MapType.VALID_FRAMEWORK_REQUEST_KINDS.stream() + .map(RequestKinds::frameworkClassName) + .collect(toImmutableSet()); + /** Returns true if the type represents a producer-related framework type. */ public static boolean isProducerType(XType type) { return typeIsOneOf(PRODUCTION_TYPES, type); @@ -51,6 +60,14 @@ public static boolean isFrameworkType(XType type) { return typeIsOneOf(ALL_FRAMEWORK_TYPES, type); } + public static boolean isSetValueFrameworkType(XType type) { + return typeIsOneOf(SET_VALUE_FRAMEWORK_TYPES, type); + } + + public static boolean isMapValueFrameworkType(XType type) { + return typeIsOneOf(MAP_VALUE_FRAMEWORK_TYPES, type); + } + private static boolean typeIsOneOf(Set classNames, XType type) { return classNames.stream().anyMatch(className -> isTypeOf(type, className)); } diff --git a/java/dagger/internal/codegen/validation/BindingElementValidator.java b/java/dagger/internal/codegen/validation/BindingElementValidator.java index c7c9f76c493..63ecb64c745 100644 --- a/java/dagger/internal/codegen/validation/BindingElementValidator.java +++ b/java/dagger/internal/codegen/validation/BindingElementValidator.java @@ -144,7 +144,7 @@ private ValidationReport validate() { checkType(); checkQualifiers(); checkMapKeys(); - checkMultibindings(); + checkMultibindingAnnotations(); checkScopes(); checkAdditionalProperties(); return report.build(); @@ -188,8 +188,10 @@ protected void checkType() { // fall through case SET: + bindingElementType().ifPresent(this::checkSetValueFrameworkType); + break; case MAP: - bindingElementType().ifPresent(this::checkKeyType); + bindingElementType().ifPresent(this::checkMapValueFrameworkType); break; case SET_VALUES: @@ -245,7 +247,7 @@ protected final void checkSetValuesType(XType type) { if (setType.isRawType()) { report.addError(elementsIntoSetRawSetMessage()); } else { - checkKeyType(setType.elementType()); + checkSetValueFrameworkType(setType.elementType()); } } } @@ -301,7 +303,7 @@ private void checkMapKeys() { * dagger.producers.Produces} annotation has a {@code type} parameter. * */ - private void checkMultibindings() { + private void checkMultibindingAnnotations() { ImmutableSet multibindingAnnotations = XElements.getAllAnnotations(element, MULTIBINDING_ANNOTATIONS); @@ -361,6 +363,23 @@ private void checkFrameworkType() { report.addError(bindingElements("must not %s framework types", bindingElementTypeVerb())); } } + + private void checkSetValueFrameworkType(XType bindingType) { + checkKeyType(bindingType); + if (FrameworkTypes.isSetValueFrameworkType(bindingType)) { + report.addError(bindingElements( + "with @IntoSet/@ElementsIntoSet must not %s framework types", + bindingElementTypeVerb())); + } + } + + private void checkMapValueFrameworkType(XType bindingType) { + checkKeyType(bindingType); + if (FrameworkTypes.isMapValueFrameworkType(bindingType)) { + report.addError( + bindingElements("with @IntoMap must not %s framework types", bindingElementTypeVerb())); + } + } } /** Whether to check multibinding annotations. */ diff --git a/javatests/dagger/internal/codegen/MapMultibindingValidationTest.java b/javatests/dagger/internal/codegen/MapMultibindingValidationTest.java index 8a8ebbede5a..6b9c52644bf 100644 --- a/javatests/dagger/internal/codegen/MapMultibindingValidationTest.java +++ b/javatests/dagger/internal/codegen/MapMultibindingValidationTest.java @@ -374,6 +374,84 @@ public void inconsistentMapKeyAnnotations() { }); } + @Test + public void mapBindingOfProvider_provides() { + Source providesModule = + CompilerTests.javaSource( + "test.MapModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Provides;", + "import dagger.multibindings.IntoMap;", + "import dagger.multibindings.StringKey;", + "import javax.inject.Provider;", + "", + "@Module", + "abstract class MapModule {", + "", + " @Provides", + " @IntoMap", + " @StringKey(\"foo\")", + " static Provider provideProvider() {", + " return null;", + " }", + "}"); + + // Entry points aren't needed because the check we care about here is a module validation + Source providesComponent = component(""); + + CompilerTests.daggerCompiler(providesModule, providesComponent) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(2); + subject.hasErrorContaining( + "@Provides methods with @IntoMap must not return framework types"); + subject.hasErrorContaining("test.MapModule has errors") + .onSource(providesComponent) + .onLineContaining("@Component(modules = {MapModule.class})"); + }); + } + + @Test + public void mapBindingOfProvider_binds() { + Source bindsModule = + CompilerTests.javaSource( + "test.MapModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Binds;", + "import dagger.multibindings.IntoMap;", + "import dagger.multibindings.StringKey;", + "import javax.inject.Provider;", + "", + "@Module", + "abstract class MapModule {", + "", + " @Binds", + " @IntoMap", + " @StringKey(\"foo\")", + " abstract Provider provideProvider(Provider provider);", + "}"); + + // Entry points aren't needed because the check we care about here is a module validation + Source bindsComponent = component(""); + + CompilerTests.daggerCompiler(bindsModule, bindsComponent) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(2); + subject.hasErrorContaining( + "@Binds methods with @IntoMap must not return framework types"); + subject.hasErrorContaining("test.MapModule has errors") + .onSource(bindsComponent) + .onLineContaining("@Component(modules = {MapModule.class})"); + }); + } + private static Source component(String... entryPoints) { return CompilerTests.javaSource( "test.TestComponent", diff --git a/javatests/dagger/internal/codegen/SetMultibindingValidationTest.java b/javatests/dagger/internal/codegen/SetMultibindingValidationTest.java new file mode 100644 index 00000000000..5933a43754a --- /dev/null +++ b/javatests/dagger/internal/codegen/SetMultibindingValidationTest.java @@ -0,0 +1,212 @@ +/* + * 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.internal.codegen; + +import androidx.room.compiler.processing.util.Source; +import com.google.common.collect.ImmutableList; +import dagger.testing.compile.CompilerTests; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; + +@RunWith(Parameterized.class) +public class SetMultibindingValidationTest { + @Parameters(name = "{0}") + public static ImmutableList parameters() { + return CompilerMode.TEST_PARAMETERS; + } + + private final CompilerMode compilerMode; + + public SetMultibindingValidationTest(CompilerMode compilerMode) { + this.compilerMode = compilerMode; + } + + @Test + public void setBindingOfProduced_provides() { + Source providesModule = + CompilerTests.javaSource( + "test.SetModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Provides;", + "import dagger.multibindings.IntoSet;", + "import dagger.producers.Produced;", + "", + "@Module", + "abstract class SetModule {", + "", + " @Provides", + " @IntoSet", + " static Produced provideProducer() {", + " return null;", + " }", + "}"); + + // Entry points aren't needed because the check we care about here is a module validation + Source providesComponent = component(""); + + CompilerTests.daggerCompiler(providesModule, providesComponent) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(2); + subject.hasErrorContaining( + "@Provides methods with @IntoSet/@ElementsIntoSet must not return framework " + + "types"); + subject.hasErrorContaining("test.SetModule has errors") + .onSource(providesComponent) + .onLineContaining("@Component(modules = {SetModule.class})"); + }); + } + + @Test + public void setBindingOfProduced_binds() { + + Source bindsModule = + CompilerTests.javaSource( + "test.SetModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Binds;", + "import dagger.multibindings.IntoSet;", + "import dagger.producers.Produced;", + "", + "@Module", + "abstract class SetModule {", + "", + " @Binds", + " @IntoSet", + " abstract Produced provideProvider(Produced impl);", + "}"); + + // Entry points aren't needed because the check we care about here is a module validation + Source bindsComponent = component(""); + + CompilerTests.daggerCompiler(bindsModule, bindsComponent) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(2); + subject.hasErrorContaining( + "@Binds methods with @IntoSet/@ElementsIntoSet must not return framework types"); + subject.hasErrorContaining("test.SetModule has errors") + .onSource(bindsComponent) + .onLineContaining("@Component(modules = {SetModule.class})"); + }); + } + + @Test + public void elementsIntoSetBindingOfProduced_provides() { + Source providesModule = + CompilerTests.javaSource( + "test.SetModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Provides;", + "import dagger.multibindings.ElementsIntoSet;", + "import dagger.producers.Produced;", + "import java.util.Set;", + "", + "@Module", + "abstract class SetModule {", + "", + " @Provides", + " @ElementsIntoSet", + " static Set> provideProducer() {", + " return null;", + " }", + "}"); + + // Entry points aren't needed because the check we care about here is a module validation + Source providesComponent = component(""); + + CompilerTests.daggerCompiler(providesModule, providesComponent) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(2); + subject.hasErrorContaining( + "@Provides methods with @IntoSet/@ElementsIntoSet must not return framework " + + "types"); + subject.hasErrorContaining("test.SetModule has errors") + .onSource(providesComponent) + .onLineContaining("@Component(modules = {SetModule.class})"); + }); + } + + @Test + public void elementsIntoSetBindingOfProduced_binds() { + + Source bindsModule = + CompilerTests.javaSource( + "test.SetModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Binds;", + "import dagger.multibindings.ElementsIntoSet;", + "import dagger.producers.Produced;", + "import java.util.Set;", + "", + "@Module", + "abstract class SetModule {", + "", + " @Binds", + " @ElementsIntoSet", + " abstract Set> provideProvider(Set> impl);", + "}"); + + // Entry points aren't needed because the check we care about here is a module validation + Source bindsComponent = component(""); + + CompilerTests.daggerCompiler(bindsModule, bindsComponent) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(2); + subject.hasErrorContaining( + "@Binds methods with @IntoSet/@ElementsIntoSet must not return framework types"); + subject.hasErrorContaining("test.SetModule has errors") + .onSource(bindsComponent) + .onLineContaining("@Component(modules = {SetModule.class})"); + }); + } + + private static Source component(String... entryPoints) { + return CompilerTests.javaSource( + "test.TestComponent", + ImmutableList.builder() + .add( + "package test;", + "", + "import dagger.Component;", + "import dagger.producers.Producer;", + "import java.util.Set;", + "import javax.inject.Provider;", + "", + "@Component(modules = {SetModule.class})", + "interface TestComponent {") + .add(entryPoints) + .add("}") + .build()); + } +}