Require @nullable explicitly on @BINDS methods.

The way we currently calculate the nullability of @BINDS methods is misleading. In particular, the nullability is based on the nullability of the implementation, and we ignore the nullability on the actual method. This CL makes 2 changes: 1. The nullability of delegate bindings is based on the nullability of the method/return type 2. The nullability of the delegate method/return type must match the nullability of the method parameter. RELNOTES=Require @nullable explictly on @BINDS methods. PiperOrigin-RevId: 673922090
google · Sep 12, 2024 · 4941926 · 4941926
1 parent d02798b
commit 4941926
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 4 deletions.
diff --git a/java/dagger/internal/codegen/binding/BindingFactory.java b/java/dagger/internal/codegen/binding/BindingFactory.java
@@ -396,8 +396,7 @@ private DelegateBinding delegateBinding(
         .bindingElement(delegateDeclaration.bindingElement().get())
         .contributingModule(delegateDeclaration.contributingModule().get())
         .delegateRequest(delegateDeclaration.delegateRequest())
-        .nullability(
-            actualBinding.map(ContributionBinding::nullability).orElse(Nullability.NOT_NULLABLE))
+        .nullability(Nullability.of(delegateDeclaration.bindingElement().get()))
         .bindingType(bindingType)
         .key(
             bindingType == BindingType.PRODUCTION

diff --git a/java/dagger/internal/codegen/validation/BindsMethodValidator.java b/java/dagger/internal/codegen/validation/BindsMethodValidator.java
@@ -32,6 +32,7 @@
 import dagger.internal.codegen.base.SetType;
 import dagger.internal.codegen.binding.BindsTypeChecker;
 import dagger.internal.codegen.binding.InjectionAnnotations;
+import dagger.internal.codegen.binding.Nullability;
 import dagger.internal.codegen.javapoet.TypeNames;
 import javax.inject.Inject;
 
@@ -45,7 +46,6 @@ final class BindsMethodValidator extends BindingMethodValidator {
       BindsTypeChecker bindsTypeChecker,
       DaggerSuperficialValidation superficialValidation,
       XProcessingEnv processingEnv,
-
       DependencyRequestValidator dependencyRequestValidator,
       InjectionAnnotations injectionAnnotations) {
     super(
@@ -110,6 +110,12 @@ protected void checkParameter(XVariableElement parameter) {
         // Set.addAll(Collection<? extends E>)
         report.addError("@Binds methods' parameter type must be assignable to the return type");
       }
+
+      Nullability parameterNullability = Nullability.of(parameter);
+      Nullability methodNullability = Nullability.of(method);
+      if (parameterNullability.isNullable() != methodNullability.isNullable()) {
+        report.addError("@Binds methods' nullability must match the nullability of its parameter");
+      }
     }
 
     private XType boxIfNecessary(XType maybePrimitive) {

diff --git a/javatests/dagger/internal/codegen/BindsMethodValidationTest.java b/javatests/dagger/internal/codegen/BindsMethodValidationTest.java
@@ -276,7 +276,6 @@ public void bindsMissingTypeInParameterHierarchy() {
             });
   }
 
-
   @Test
   public void bindsMissingTypeInReturnTypeHierarchy() {
     Source module =
@@ -342,6 +341,56 @@ public void bindsMissingTypeInReturnTypeHierarchy() {
             });
   }
 
+  @Test
+  public void bindsNullableToNonNullable_fails() {
+    Source module =
+        CompilerTests.javaSource(
+            "test.TestComponent",
+            "package test;",
+            "",
+            "import dagger.Binds;",
+            "import dagger.Module;",
+            "import javax.annotation.Nullable;",
+            "",
+            "@Module",
+            "interface TestModule {",
+            "  @Binds Object bind(@Nullable String str);",
+            "}");
+
+    CompilerTests.daggerCompiler(module)
+        .compile(
+            subject -> {
+              subject.hasErrorCount(1);
+              subject.hasErrorContaining(
+                  "@Binds methods' nullability must match the nullability of its parameter");
+            });
+  }
+
+  @Test
+  public void bindsNonNullableToNullable_fails() {
+    Source module =
+        CompilerTests.javaSource(
+            "test.TestComponent",
+            "package test;",
+            "",
+            "import dagger.Binds;",
+            "import dagger.Module;",
+            "import javax.annotation.Nullable;",
+            "",
+            "@Module",
+            "interface TestModule {",
+            "  @Binds @Nullable Object bind(String str);",
+            "}");
+
+    CompilerTests.daggerCompiler(module)
+        .compile(
+            subject -> {
+              subject.hasErrorCount(1);
+              subject.hasErrorContaining(
+                  "@Binds methods' nullability must match the nullability of its parameter");
+            });
+  }
+
   private DaggerModuleMethodSubject assertThatMethod(String method) {
     return assertThatModuleMethod(method).withDeclaration(moduleDeclaration);
   }