Skip to content

Commit

Permalink
Better error messages for invalid @Multibinds types.
Browse files Browse the repository at this point in the history 
Fixes #3677

This CL also cleans up an old TODO.

RELNOTES=Fixes #3677: Better error messages for invalid @Multibinds types.
PiperOrigin-RevId: 500752608
  • Loading branch information
@bcorso
bcorso authored and Dagger Team committed Jan 9, 2023
1 parent e9116f0 commit 3528314
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 36 deletions.
6 changes: 1 addition & 5 deletions java/dagger/internal/codegen/validation/BindingElementValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import static androidx.room.compiler.processing.XTypeKt.isArray;
import static androidx.room.compiler.processing.XTypeKt.isVoid;
import static com.google.common.base.Verify.verifyNotNull;
import static com.google.common.collect.Iterables.getOnlyElement;
import static dagger.internal.codegen.base.Util.reentrantComputeIfAbsent;
import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.isAssistedFactoryType;
import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.isAssistedInjectionType;
Expand Down Expand Up @@ -246,10 +245,7 @@ protected final void checkSetValuesType(XType type) {
if (setType.isRawType()) {
report.addError(elementsIntoSetRawSetMessage());
} else {
// TODO(bcorso): Use setType.elementType() once setType is fully converted to XProcessing.
// However, currently SetType returns TypeMirror instead of XType and we have no
// conversion from TypeMirror to XType, so we just get the type ourselves.
checkKeyType(getOnlyElement(type.getTypeArguments()));
checkKeyType(setType.elementType());
}
}
}
Expand Down
48 changes: 31 additions & 17 deletions java/dagger/internal/codegen/validation/MultibindsMethodValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
import static dagger.internal.codegen.validation.BindingElementValidator.AllowsScoping.NO_SCOPING;
import static dagger.internal.codegen.validation.BindingMethodValidator.Abstractness.MUST_BE_ABSTRACT;
import static dagger.internal.codegen.validation.BindingMethodValidator.ExceptionSuperclass.NO_EXCEPTIONS;
import static dagger.internal.codegen.xprocessing.XElements.getSimpleName;
import static dagger.internal.codegen.xprocessing.XTypes.isWildcard;

import androidx.room.compiler.processing.XMethodElement;
import androidx.room.compiler.processing.XProcessingEnv;
import androidx.room.compiler.processing.XType;
import com.google.common.collect.ImmutableSet;
import dagger.internal.codegen.base.MapType;
import dagger.internal.codegen.base.SetType;
Expand Down Expand Up @@ -77,29 +77,43 @@ protected void checkParameters() {
/** Adds an error unless the method returns a {@code Map<K, V>} or {@code Set<T>}. */
@Override
protected void checkType() {
if (!isPlainMap(method.getReturnType()) && !isPlainSet(method.getReturnType())) {
report.addError(bindingMethods("must return Map<K, V> or Set<T>"));
if (MapType.isMap(method.getReturnType())) {
checkMapType(MapType.from(method.getReturnType()));
} else if (SetType.isSet(method.getReturnType())) {
checkSetType(SetType.from(method.getReturnType()));
} else {
report.addError(bindingMethods("return type must be either a Set or Map type."));
}
}

private boolean isPlainMap(XType returnType) {
if (!MapType.isMap(returnType)) {
return false;
private void checkMapType(MapType mapType) {
if (mapType.isRawType()) {
report.addError(bindingMethods("return type cannot be a raw Map type"));
} else if (isWildcard(mapType.keyType())) {
report.addError(
bindingMethods("return type cannot use a wildcard as the Map key type."));
} else if (isWildcard(mapType.valueType())) {
report.addError(
bindingMethods("return type cannot use a wildcard as the Map value type."));
} else if (isFrameworkType(mapType.valueType())) {
String frameworkTypeName = getSimpleName(mapType.valueType().getTypeElement());
report.addError(
bindingMethods(
"return type cannot use '%s' in the Map value type.", frameworkTypeName));
}
MapType mapType = MapType.from(returnType);
return !mapType.isRawType()
&& !isWildcard(mapType.valueType())
&& !isFrameworkType(mapType.valueType());
}

private boolean isPlainSet(XType returnType) {
if (!SetType.isSet(returnType)) {
return false;
private void checkSetType(SetType setType) {
if (setType.isRawType()) {
report.addError(bindingMethods("return type cannot be a raw Set type"));
} else if (isWildcard(setType.elementType())) {
report.addError(bindingMethods("return type cannot use a wildcard as the Set value type."));
} else if (isFrameworkType(setType.elementType())) {
String frameworkTypeName = getSimpleName(setType.elementType().getTypeElement());
report.addError(
bindingMethods(
"return type cannot use '%s' in the Set value type.", frameworkTypeName));
}
SetType setType = SetType.from(returnType);
return !setType.isRawType()
&& !isWildcard(setType.elementType())
&& !isFrameworkType(setType.elementType());
}
}
}
56 changes: 42 additions & 14 deletions javatests/dagger/internal/codegen/MultibindsValidationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,84 +54,112 @@ public void notWithinModule() {
public void voidMethod() {
assertThatModuleMethod("@Multibinds abstract void voidMethod();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("@Multibinds methods return type must be either a Set or Map type.");
}

@Test
public void primitiveMethod() {
assertThatModuleMethod("@Multibinds abstract int primitive();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("@Multibinds methods return type must be either a Set or Map type.");
}

@Test
public void rawMap() {
assertThatModuleMethod("@Multibinds abstract Map rawMap();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("return type cannot be a raw Map type");
}

@Test
public void wildcardMap() {
assertThatModuleMethod("@Multibinds abstract Map<?, ?> wildcardMap();")
public void wildcardMapValue() {
assertThatModuleMethod("@Multibinds abstract Map<String, ?> wildcardMap();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("return type cannot use a wildcard as the Map value type.");
}

@Test
public void wildcardMapValueWithBounds() {
assertThatModuleMethod("@Multibinds abstract Map<String, ? extends Number> wildcardMap();")
.withDeclaration(moduleDeclaration)
.hasError("return type cannot use a wildcard as the Map value type.");
}

@Test
public void wildcardMapKey() {
assertThatModuleMethod("@Multibinds abstract Map<?, String> wildcardMap();")
.withDeclaration(moduleDeclaration)
.hasError("return type cannot use a wildcard as the Map key type.");
}

@Test
public void wildcardMapKeyWithBounds() {
assertThatModuleMethod("@Multibinds abstract Map<? extends Number, String> wildcardMap();")
.withDeclaration(moduleDeclaration)
.hasError("return type cannot use a wildcard as the Map key type.");
}

@Test
public void providerMap() {
assertThatModuleMethod("@Multibinds abstract Map<String, Provider<Object>> providerMap();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("return type cannot use 'Provider' in the Map value type.");
}

@Test
public void producerMap() {
assertThatModuleMethod("@Multibinds abstract Map<String, Producer<Object>> producerMap();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("return type cannot use 'Producer' in the Map value type.");
}

@Test
public void producedMap() {
assertThatModuleMethod("@Multibinds abstract Map<String, Produced<Object>> producedMap();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("return type cannot use 'Produced' in the Map value type.");
}

@Test
public void rawSet() {
assertThatModuleMethod("@Multibinds abstract Set rawSet();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("return type cannot be a raw Set type");
}

@Test
public void wildcardSet() {
assertThatModuleMethod("@Multibinds abstract Set<?> wildcardSet();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("return type cannot use a wildcard as the Set value type.");
}

@Test
public void wildcardSetWithBounds() {
assertThatModuleMethod("@Multibinds abstract Set<? extends Number> wildcardSet();")
.withDeclaration(moduleDeclaration)
.hasError("return type cannot use a wildcard as the Set value type.");
}

@Test
public void providerSet() {
assertThatModuleMethod("@Multibinds abstract Set<Provider<Object>> providerSet();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("return type cannot use 'Provider' in the Set value type.");
}

@Test
public void producerSet() {
assertThatModuleMethod("@Multibinds abstract Set<Producer<Object>> producerSet();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("return type cannot use 'Producer' in the Set value type.");
}

@Test
public void producedSet() {
assertThatModuleMethod("@Multibinds abstract Set<Produced<Object>> producedSet();")
.withDeclaration(moduleDeclaration)
.hasError("@Multibinds methods must return Map<K, V> or Set<T>");
.hasError("return type cannot use 'Produced' in the Set value type.");
}

@Test
Expand Down

0 comments on commit 3528314

Please sign in to comment.