Skip to content

Commit

Permalink
Disallow providing or injecting dagger.internal.Provider. This will…
Browse files Browse the repository at this point in the history
… help with IDEs accidentally suggesting importing `dagger.internal.Provider`.

Also disallow injections of raw Provider in Maps, for both javax and dagger Providers. This matches the policy of disallowing raw Provider injections in general.

RELNOTES=Disallow providing or injecting `dagger.internal.Provider`. Also disallow injections of raw Provider in Maps, for both javax and dagger Providers.
PiperOrigin-RevId: 713755026
  • Loading branch information
Chang-Eric authored and Dagger Team committed Jan 9, 2025
1 parent 7ca9977 commit 962bb33
Show file tree
Hide file tree
Showing 6 changed files with 408 additions and 23 deletions.
10 changes: 10 additions & 0 deletions java/dagger/internal/codegen/base/FrameworkTypes.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<ClassName> 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);
Expand All @@ -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<ClassName> classNames, XType type) {
return classNames.stream().anyMatch(className -> isTypeOf(type, className));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Provider<Foo>> is perfectly reasonable.
Expand All @@ -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;
}
}

Expand Down Expand Up @@ -360,24 +369,37 @@ 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())));
}
}

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<dagger.internal.Provider<Foo>>
if (FrameworkTypes.isDisallowedType(bindingType)) {
report.addError(bindingElements("must not %s disallowed types: %s",
bindingElementTypeVerb(), XTypes.toStableString(bindingElementType().get())));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
}
}
}
}

Expand Down
24 changes: 24 additions & 0 deletions javatests/dagger/internal/codegen/BindsInstanceValidationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object> 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);
});
}
}
Loading

0 comments on commit 962bb33

Please sign in to comment.