diff --git a/java/dagger/internal/codegen/base/FrameworkTypes.java b/java/dagger/internal/codegen/base/FrameworkTypes.java index e607f766461..04eaf2e9119 100644 --- a/java/dagger/internal/codegen/base/FrameworkTypes.java +++ b/java/dagger/internal/codegen/base/FrameworkTypes.java @@ -55,6 +55,12 @@ public final class FrameworkTypes { TypeNames.PROVIDER, TypeNames.JAKARTA_PROVIDER); + // This is a set of types that are disallowed from use, but also aren't framework types in the + // sense that they aren't supported. Like we shouldn't try to unwrap these if we see them, though + // we shouldn't see them at all if they are correctly caught in validation. + private static final ImmutableSet DISALLOWED_TYPES = + ImmutableSet.of(TypeNames.DAGGER_PROVIDER); + /** Returns true if the type represents a producer-related framework type. */ public static boolean isProducerType(XType type) { return typeIsOneOf(PRODUCTION_TYPES, type); @@ -73,6 +79,10 @@ public static boolean isMapValueFrameworkType(XType type) { return typeIsOneOf(MAP_VALUE_FRAMEWORK_TYPES, type); } + public static boolean isDisallowedType(XType type) { + return typeIsOneOf(DISALLOWED_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 63ecb64c745..93f4f079bd9 100644 --- a/java/dagger/internal/codegen/validation/BindingElementValidator.java +++ b/java/dagger/internal/codegen/validation/BindingElementValidator.java @@ -42,6 +42,7 @@ import dagger.internal.codegen.model.Key; import dagger.internal.codegen.model.Scope; import dagger.internal.codegen.xprocessing.XElements; +import dagger.internal.codegen.xprocessing.XTypes; import java.util.Formatter; import java.util.HashMap; import java.util.Map; @@ -176,6 +177,9 @@ protected void checkAdditionalProperties() {} protected void checkType() { switch (ContributionType.fromBindingElement(element)) { case UNIQUE: + // Basic checks on the types + bindingElementType().ifPresent(this::checkKeyType); + // Validate that a unique binding is not attempting to bind a framework type. This // validation is only appropriate for unique bindings because multibindings may collect // framework types. E.g. Set> is perfectly reasonable. @@ -185,17 +189,22 @@ protected void checkType() { // This validation is only appropriate for unique bindings because multibindings may // collect assisted types. checkAssistedType(); - // fall through + + // Check for any specifically disallowed types + bindingElementType().ifPresent(this::checkDisallowedType); + break; case SET: bindingElementType().ifPresent(this::checkSetValueFrameworkType); break; + case MAP: bindingElementType().ifPresent(this::checkMapValueFrameworkType); break; case SET_VALUES: checkSetValuesType(); + break; } } @@ -360,7 +369,8 @@ private void checkScopes() { */ private void checkFrameworkType() { if (bindingElementType().filter(FrameworkTypes::isFrameworkType).isPresent()) { - report.addError(bindingElements("must not %s framework types", bindingElementTypeVerb())); + report.addError(bindingElements("must not %s framework types: %s", + bindingElementTypeVerb(), XTypes.toStableString(bindingElementType().get()))); } } @@ -368,16 +378,28 @@ private void checkSetValueFrameworkType(XType bindingType) { checkKeyType(bindingType); if (FrameworkTypes.isSetValueFrameworkType(bindingType)) { report.addError(bindingElements( - "with @IntoSet/@ElementsIntoSet must not %s framework types", - bindingElementTypeVerb())); + "with @IntoSet/@ElementsIntoSet must not %s framework types: %s", + bindingElementTypeVerb(), XTypes.toStableString(bindingType))); } + checkDisallowedType(bindingType); } private void checkMapValueFrameworkType(XType bindingType) { checkKeyType(bindingType); if (FrameworkTypes.isMapValueFrameworkType(bindingType)) { report.addError( - bindingElements("with @IntoMap must not %s framework types", bindingElementTypeVerb())); + bindingElements("with @IntoMap must not %s framework types: %s", + bindingElementTypeVerb(), XTypes.toStableString(bindingType))); + } + checkDisallowedType(bindingType); + } + + private void checkDisallowedType(XType bindingType) { + // TODO(erichang): Consider if we want to go inside complex types to ban + // dagger.internal.Provider as well? E.g. List> + if (FrameworkTypes.isDisallowedType(bindingType)) { + report.addError(bindingElements("must not %s disallowed types: %s", + bindingElementTypeVerb(), XTypes.toStableString(bindingElementType().get()))); } } } diff --git a/java/dagger/internal/codegen/validation/DependencyRequestValidator.java b/java/dagger/internal/codegen/validation/DependencyRequestValidator.java index 55100483ead..87bcad03786 100644 --- a/java/dagger/internal/codegen/validation/DependencyRequestValidator.java +++ b/java/dagger/internal/codegen/validation/DependencyRequestValidator.java @@ -18,7 +18,9 @@ import static androidx.room.compiler.processing.XElementKt.isField; import static androidx.room.compiler.processing.XElementKt.isTypeElement; +import static dagger.internal.codegen.base.FrameworkTypes.isDisallowedType; import static dagger.internal.codegen.base.FrameworkTypes.isFrameworkType; +import static dagger.internal.codegen.base.FrameworkTypes.isMapValueFrameworkType; import static dagger.internal.codegen.base.RequestKinds.extractKeyType; import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.isAssistedFactoryType; import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.isAssistedInjectionType; @@ -40,6 +42,7 @@ import androidx.room.compiler.processing.XVariableElement; import com.google.common.collect.ImmutableSet; import dagger.internal.codegen.base.FrameworkTypes; +import dagger.internal.codegen.base.MapType; import dagger.internal.codegen.base.RequestKinds; import dagger.internal.codegen.binding.InjectionAnnotations; import dagger.internal.codegen.javapoet.TypeNames; @@ -154,6 +157,14 @@ private void checkType() { // will just be noise. return; } + if (isDisallowedType(requestType)) { + report.addError( + "Dagger disallows injecting the type: " + XTypes.toStableString(requestType), + requestElement); + // If the requested type is a disallowed type then skip the remaining checks as they + // will just be noise. + return; + } XType keyType = extractKeyType(requestType); if (qualifiers.isEmpty() && isDeclared(keyType)) { XTypeElement typeElement = keyType.getTypeElement(); @@ -191,6 +202,24 @@ && isAssistedFactoryType(typeElement)) { requestElement, keyType.getTypeArguments().get(0))); } } + if (MapType.isMap(keyType)) { + MapType mapType = MapType.from(keyType); + if (!mapType.isRawType()) { + XType valueType = mapType.valueType(); + if (isMapValueFrameworkType(valueType) && isRawParameterizedType(valueType)) { + report.addError( + "Dagger does not support injecting maps of raw framework types: " + + XTypes.toStableString(requestType), + requestElement); + } + if (isDisallowedType(valueType)) { + report.addError( + "Dagger does not support injecting maps of disallowed types: " + + XTypes.toStableString(requestType), + requestElement); + } + } + } } } diff --git a/javatests/dagger/internal/codegen/BindsInstanceValidationTest.java b/javatests/dagger/internal/codegen/BindsInstanceValidationTest.java index 18cc7347332..a3a7ffa8987 100644 --- a/javatests/dagger/internal/codegen/BindsInstanceValidationTest.java +++ b/javatests/dagger/internal/codegen/BindsInstanceValidationTest.java @@ -187,4 +187,28 @@ public void bindsInstanceFrameworkType() { }); } + @Test + public void bindsInstanceDaggerProvider() { + Source bindsDaggerProvider = + CompilerTests.javaSource( + "test.BindsInstanceFrameworkType", + "package test;", + "", + "import dagger.BindsInstance;", + "import dagger.internal.Provider;", + "import dagger.producers.Producer;", + "", + "interface BindsInstanceFrameworkType {", + " @BindsInstance void bindsProvider(Provider objectProvider);", + "}"); + CompilerTests.daggerCompiler(bindsDaggerProvider) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining("@BindsInstance parameters must not be disallowed types") + .onSource(bindsDaggerProvider) + .onLine(8); + }); + } } diff --git a/javatests/dagger/internal/codegen/MembersInjectionTest.java b/javatests/dagger/internal/codegen/MembersInjectionTest.java index b547ec07693..ee02f9fa13b 100644 --- a/javatests/dagger/internal/codegen/MembersInjectionTest.java +++ b/javatests/dagger/internal/codegen/MembersInjectionTest.java @@ -779,18 +779,15 @@ public void fieldInjectionForShadowedMember() { } @Test - public void rawFrameworkTypeField() { + public void throwExceptionInjectedMethod() { Source file = CompilerTests.javaSource( - "test.RawFrameworkTypes", + "test.", "package test;", "", "import javax.inject.Inject;", - "import javax.inject.Provider;", - "", - "class RawProviderField {", - " @Inject", - " Provider fieldWithRawProvider;", + "class SomeClass {", + "@Inject void inject() throws Exception {}", "}"); CompilerTests.daggerCompiler(file) @@ -799,22 +796,26 @@ public void rawFrameworkTypeField() { subject -> { subject.hasErrorCount(1); subject.hasErrorContaining( - "Dagger does not support injecting raw type: javax.inject.Provider") + "Methods with @Inject may not throw checked exceptions. " + + "Please wrap your exceptions in a RuntimeException instead.") .onSource(file) - .onLineContaining("Provider fieldWithRawProvider"); + .onLineContaining("throws Exception"); }); } @Test - public void throwExceptionInjectedMethod() { + public void rawFrameworkTypeField() { Source file = CompilerTests.javaSource( - "test.", + "test.RawProviderField", "package test;", "", "import javax.inject.Inject;", - "class SomeClass {", - "@Inject void inject() throws Exception {}", + "import javax.inject.Provider;", + "", + "class RawProviderField {", + " @Inject", + " Provider fieldWithRawProvider;", "}"); CompilerTests.daggerCompiler(file) @@ -823,10 +824,9 @@ public void throwExceptionInjectedMethod() { subject -> { subject.hasErrorCount(1); subject.hasErrorContaining( - "Methods with @Inject may not throw checked exceptions. " - + "Please wrap your exceptions in a RuntimeException instead.") + "Dagger does not support injecting raw type: javax.inject.Provider") .onSource(file) - .onLineContaining("throws Exception"); + .onLineContaining("Provider fieldWithRawProvider"); }); } @@ -834,7 +834,7 @@ public void throwExceptionInjectedMethod() { public void rawFrameworkMethodTypeParameter() { Source file = CompilerTests.javaSource( - "test.RawFrameworkTypes", + "test.RawProviderParameter", "package test;", "", "import javax.inject.Inject;", @@ -862,7 +862,7 @@ public void rawFrameworkMethodTypeParameter() { public void rawFrameworkConstructorTypeParameter() { Source file = CompilerTests.javaSource( - "test.RawFrameworkTypes", + "test.RawProviderParameter", "package test;", "", "import dagger.Component;", @@ -887,6 +887,274 @@ public void rawFrameworkConstructorTypeParameter() { }); } + @Test + public void rawMapFrameworkConstructorTypeParameter() { + Source file = + CompilerTests.javaSource( + "test.RawMapProviderParameter", + "package test;", + "", + "import dagger.Component;", + "import javax.inject.Inject;", + "import javax.inject.Provider;", + "import java.util.Map;", + "", + "class RawMapProviderParameter {", + " @Inject", + " RawMapProviderParameter(", + " Map rawProviderParameter) {}", + "}"); + + CompilerTests.daggerCompiler(file) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining( + "Dagger does not support injecting maps of raw framework types: " + + "java.util.Map") + .onSource(file) + .onLineContaining("Map rawProviderParameter"); + }); + } + + @Test + public void daggerProviderField() { + Source file = + CompilerTests.javaSource( + "test.DaggerProviderField", + "package test;", + "", + "import dagger.internal.Provider;", + "import javax.inject.Inject;", + "", + "class DaggerProviderField {", + " @Inject", + " Provider fieldWithDaggerProvider;", + "}"); + + CompilerTests.daggerCompiler(file) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining( + "Dagger disallows injecting the type: " + + "dagger.internal.Provider") + .onSource(file) + .onLineContaining("Provider fieldWithDaggerProvider"); + }); + } + + @Test + public void daggerProviderMethodTypeParameter() { + Source file = + CompilerTests.javaSource( + "test.DaggerProviderParameter", + "package test;", + "", + "import dagger.internal.Provider;", + "import javax.inject.Inject;", + "", + "class DaggerProviderParameter {", + " @Inject", + " void methodInjection(", + " Provider daggerProviderParameter) {}", + "}"); + + CompilerTests.daggerCompiler(file) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining( + "Dagger disallows injecting the type: " + + "dagger.internal.Provider") + .onSource(file) + .onLineContaining("Provider daggerProviderParameter"); + }); + } + + @Test + public void daggerProviderConstructorTypeParameter() { + Source file = + CompilerTests.javaSource( + "test.DaggerProviderParameter", + "package test;", + "", + "import dagger.Component;", + "import dagger.internal.Provider;", + "import javax.inject.Inject;", + "", + "class DaggerProviderParameter {", + " @Inject", + " DaggerProviderParameter(", + " Provider daggerProviderParameter) {}", + "}"); + + CompilerTests.daggerCompiler(file) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining( + "Dagger disallows injecting the type: " + + "dagger.internal.Provider") + .onSource(file) + .onLineContaining("Provider daggerProviderParameter"); + }); + } + + @Test + public void rawDaggerProviderConstructorTypeParameter() { + Source file = + CompilerTests.javaSource( + "test.RawDaggerProviderParameter", + "package test;", + "", + "import dagger.Component;", + "import dagger.internal.Provider;", + "import javax.inject.Inject;", + "", + "class RawDaggerProviderParameter {", + " @Inject", + " RawDaggerProviderParameter(", + " Provider rawDaggerProviderParameter) {}", + "}"); + + CompilerTests.daggerCompiler(file) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining( + "Dagger disallows injecting the type: dagger.internal.Provider") + .onSource(file) + .onLineContaining("Provider rawDaggerProviderParameter"); + }); + } + + @Test + public void daggerMapProviderField() { + Source file = + CompilerTests.javaSource( + "test.DaggerMapProviderField", + "package test;", + "", + "import dagger.internal.Provider;", + "import javax.inject.Inject;", + "import java.util.Map;", + "", + "class DaggerMapProviderField {", + " @Inject", + " Map> fieldWithDaggerMapProvider;", + "}"); + + CompilerTests.daggerCompiler(file) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining( + "Dagger does not support injecting maps of disallowed types: " + + "java.util.Map>") + .onSource(file) + .onLineContaining("Map> fieldWithDaggerMapProvider"); + }); + } + + @Test + public void daggerMapProviderMethodTypeParameter() { + Source file = + CompilerTests.javaSource( + "test.DaggerMapProviderParameter", + "package test;", + "", + "import dagger.internal.Provider;", + "import javax.inject.Inject;", + "import java.util.Map;", + "", + "class DaggerMapProviderParameter {", + " @Inject", + " void methodInjection(", + " Map> daggerMapProviderParameter) {}", + "}"); + + CompilerTests.daggerCompiler(file) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining( + "Dagger does not support injecting maps of disallowed types: " + + "java.util.Map>") + .onSource(file) + .onLineContaining("Map> daggerMapProviderParameter"); + }); + } + + @Test + public void daggerMapProviderConstructorTypeParameter() { + Source file = + CompilerTests.javaSource( + "test.DaggerMapProviderParameter", + "package test;", + "", + "import dagger.Component;", + "import dagger.internal.Provider;", + "import javax.inject.Inject;", + "import java.util.Map;", + "", + "class DaggerMapProviderParameter {", + " @Inject", + " DaggerMapProviderParameter(", + " Map> daggerMapProviderParameter) {}", + "}"); + + CompilerTests.daggerCompiler(file) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining( + "Dagger does not support injecting maps of disallowed types: " + + "java.util.Map>") + .onSource(file) + .onLineContaining("Map> daggerMapProviderParameter"); + }); + } + + @Test + public void rawDaggerMapProviderConstructorTypeParameter() { + Source file = + CompilerTests.javaSource( + "test.RawDaggerMapProviderParameter", + "package test;", + "", + "import dagger.Component;", + "import dagger.internal.Provider;", + "import javax.inject.Inject;", + "import java.util.Map;", + "", + "class RawDaggerMapProviderParameter {", + " @Inject", + " RawDaggerMapProviderParameter(", + " Map rawDaggerMapProviderParameter) {}", + "}"); + + CompilerTests.daggerCompiler(file) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining( + "Dagger does not support injecting maps of disallowed types: " + + "java.util.Map") + .onSource(file) + .onLineContaining("Map rawDaggerMapProviderParameter"); + }); + } + @Test public void injectsPrimitive() throws Exception { Source injectedType = diff --git a/javatests/dagger/internal/codegen/ModuleFactoryGeneratorTest.java b/javatests/dagger/internal/codegen/ModuleFactoryGeneratorTest.java index d315b77e0d3..ccff741a8b6 100644 --- a/javatests/dagger/internal/codegen/ModuleFactoryGeneratorTest.java +++ b/javatests/dagger/internal/codegen/ModuleFactoryGeneratorTest.java @@ -77,6 +77,19 @@ public void providesMethodReturnsJakartaProvider() { .hasError("@Provides methods must not return framework types"); } + @Test + public void providesMethodReturnsDaggerInternalProvider() { + assertThatModuleMethod("@Provides dagger.internal.Provider provideProvider() {}") + .hasError("@Provides methods must not return disallowed types"); + } + + @Test + public void providesIntoSetMethodReturnsDaggerInternalProvider() { + assertThatModuleMethod( + "@Provides @IntoSet dagger.internal.Provider provideProvider() {}") + .hasError("@Provides methods must not return disallowed types"); + } + @Test public void providesMethodReturnsLazy() { assertThatModuleMethod("@Provides Lazy provideLazy() {}") @@ -118,12 +131,31 @@ public void providesMethodReturnsProduced() { .hasError("@Provides methods annotated with @ElementsIntoSet cannot return a raw Set"); } + @Test public void providesElementsIntoSetMethodReturnsSetDaggerProvider() { + assertThatModuleMethod( + "@Provides @ElementsIntoSet Set> provideProvider() {}") + .hasError("@Provides methods must not return disallowed types"); + } + @Test public void providesMethodSetValuesNotASet() { assertThatModuleMethod( "@Provides @ElementsIntoSet List provideStrings() { return null; }") .hasError("@Provides methods annotated with @ElementsIntoSet must return a Set"); } + @Test + public void bindsMethodReturnsProvider() { + assertThatModuleMethod("@Binds abstract Provider bindsProvider(Provider impl);") + .hasError("@Binds methods must not return framework types"); + } + + @Test + public void bindsMethodReturnsDaggerProvider() { + assertThatModuleMethod("@Binds abstract dagger.internal.Provider " + + "bindsProvider(dagger.internal.Provider impl);") + .hasError("@Binds methods must not return disallowed types"); + } + @Test public void modulesWithTypeParamsMustBeAbstract() { Source moduleFile = CompilerTests.javaSource(