Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow providing or injecting dagger.internal.Provider. This will help with IDEs accidentally suggesting importing dagger.internal.Provider. #4567

Merged
merged 1 commit into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading