From 7335ba2402aa0b725978c3686f144e6f3de11ef1 Mon Sep 17 00:00:00 2001
From: ghm <ghm@google.com>
Date: Wed, 10 May 2023 01:54:41 -0700
Subject: [PATCH] Don't consider overloads to have a functional interface clash
 if the parameters are subtypes of another overload.

JLS defines that more specific overloads win, so, for example, `ImmutableFunction` wins over `Function`.

PiperOrigin-RevId: 530843370
---
 .../bugpatterns/FunctionalInterfaceClash.java | 34 ++++++++++++++++---
 .../FunctionalInterfaceClashTest.java         | 14 ++++++++
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClash.java b/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClash.java
index 7fa1e944b90..fb2b227c4b5 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClash.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClash.java
@@ -17,10 +17,12 @@
 package com.google.errorprone.bugpatterns;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.Streams.zip;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.isSubtype;
 import static java.util.stream.Collectors.joining;
 
 import com.google.common.collect.HashMultimap;
@@ -39,6 +41,7 @@
 import com.sun.tools.javac.code.Symbol.ClassSymbol;
 import com.sun.tools.javac.code.Symbol.CompletionFailure;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Symbol.TypeSymbol;
 import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.comp.Check;
@@ -58,7 +61,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
     ClassSymbol origin = getSymbol(tree);
     Types types = state.getTypes();
     // collect declared and inherited methods whose signature contains a functional interface
-    SetMultimap<String, MethodSymbol> methods = HashMultimap.create();
+    SetMultimap<String, MethodSymbol> methodsByName = HashMultimap.create();
     for (Symbol sym :
         types.membersClosure(getType(tree), /* skipInterface= */ false).getSymbols()) {
       if (!(sym instanceof MethodSymbol)) {
@@ -72,7 +75,27 @@ public Description matchClass(ClassTree tree, VisitorState state) {
       if (msym.isConstructor() && !msym.owner.equals(origin)) {
         continue;
       }
-      methods.put(functionalInterfaceSignature(state, msym), msym);
+      methodsByName.put(msym.getSimpleName().toString(), msym);
+    }
+
+    // Only consider methods which don't have strictly more specific overloads; these won't actually
+    // clash.
+    SetMultimap<String, MethodSymbol> methodsBySignature = HashMultimap.create();
+    for (MethodSymbol msym : methodsByName.values()) {
+      if (methodsByName.get(msym.getSimpleName().toString()).stream()
+          .anyMatch(
+              o ->
+                  !msym.overrides(o, (TypeSymbol) msym.owner, types, /* checkResult= */ true)
+                      && !o.equals(msym)
+                      && o.getParameters().length() == msym.getParameters().length()
+                      && zip(
+                              msym.getParameters().stream(),
+                              o.getParameters().stream(),
+                              (a, b) -> isSubtype(a.type, b.type, state))
+                          .allMatch(x -> x))) {
+        continue;
+      }
+      methodsBySignature.put(functionalInterfaceSignature(state, msym), msym);
     }
     // check if any declared members clash with another declared or inherited member
     // (don't report clashes between inherited members)
@@ -86,9 +109,10 @@ public Description matchClass(ClassTree tree, VisitorState state) {
         continue;
       }
       List<MethodSymbol> clash =
-          new ArrayList<>(methods.removeAll(functionalInterfaceSignature(state, msym)));
+          new ArrayList<>(methodsBySignature.removeAll(functionalInterfaceSignature(state, msym)));
 
-      // Ignore inherited methods that are overridden in the original class. Note that we have to
+      // Ignore inherited methodsBySignature that are overridden in the original class. Note that we
+      // have to
       // handle transitive inheritance explicitly to handle cases where the visibility of an
       // overridden method is expanded somewhere in the type hierarchy.
       Deque<MethodSymbol> worklist = new ArrayDeque<>();
@@ -105,7 +129,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
       }
 
       if (!clash.isEmpty()) {
-        // ignore if there are overridden clashing methods in class
+        // ignore if there are overridden clashing methodsBySignature in class
         if (ASTHelpers.findSuperMethod(msym, types).isPresent()
             && clash.stream()
                 .anyMatch(
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClashTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClashTest.java
index 7ff0394c6ba..12bddfcfef5 100644
--- a/core/src/test/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClashTest.java
+++ b/core/src/test/java/com/google/errorprone/bugpatterns/FunctionalInterfaceClashTest.java
@@ -369,4 +369,18 @@ public void positive_overridesInClassAndNewClashingPairInSameClass() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void oneIsMoreSpecific_notAmbiguous() {
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            "import java.util.function.Consumer;",
+            "public class Test {",
+            "  void foo(Consumer<String> c) {}",
+            "  void foo(SubConsumer<String> c) {}",
+            "  interface SubConsumer<T> extends Consumer<T> {}",
+            "}")
+        .doTest();
+  }
 }