From 4941926c57958915b288423f28706a5496fee93c Mon Sep 17 00:00:00 2001
From: Brad Corso <bcorso@google.com>
Date: Thu, 12 Sep 2024 11:09:44 -0700
Subject: [PATCH] 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
---
 .../codegen/binding/BindingFactory.java       |  3 +-
 .../validation/BindsMethodValidator.java      |  8 ++-
 .../codegen/BindsMethodValidationTest.java    | 51 ++++++++++++++++++-
 3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/java/dagger/internal/codegen/binding/BindingFactory.java b/java/dagger/internal/codegen/binding/BindingFactory.java
index 0f65da77ac8..5ee83845650 100644
--- 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
index c3ad90ae4ee..9e877f9c4a8 100644
--- 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
index cb94f756745..8e905e6ebc1 100644
--- 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);
   }