Have ThrowsCheckedException also consider supertypes

This prevents the `MonoFromSupplier` Refaster rule from suggesting non-compilable code.
PicnicSupermarket · Dec 10, 2023 · 68a5d88 · 68a5d88
1 parent 351b886
commit 68a5d88
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 29 deletions.
diff --git a/...upport/src/main/java/tech/picnic/errorprone/refaster/matchers/ThrowsCheckedException.java b/...upport/src/main/java/tech/picnic/errorprone/refaster/matchers/ThrowsCheckedException.java
@@ -1,13 +1,11 @@
 package tech.picnic.errorprone.refaster.matchers;
 
-import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.LambdaExpressionTree;
 import com.sun.source.tree.MemberReferenceTree;
-import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.code.Types.FunctionDescriptorLookupError;
 import java.util.Collection;
@@ -25,42 +23,45 @@ public ThrowsCheckedException() {}
 
   @Override
   public boolean matches(ExpressionTree tree, VisitorState state) {
-    return containsCheckedException(getThrownTypes(tree, state), state);
-  }
-
-  private static Collection<Type> getThrownTypes(ExpressionTree tree, VisitorState state) {
     if (tree instanceof LambdaExpressionTree) {
-      return ASTHelpers.getThrownExceptions(((LambdaExpressionTree) tree).getBody(), state);
+      return throwsCheckedException((LambdaExpressionTree) tree, state);
     }
 
     if (tree instanceof MemberReferenceTree) {
-      Symbol symbol = ASTHelpers.getSymbol(tree);
-      if (symbol == null) {
-        return ImmutableSet.of();
-      }
-
-      return symbol.type.getThrownTypes();
+      return throwsCheckedException((MemberReferenceTree) tree, state);
     }
 
     Type type = ASTHelpers.getType(tree);
-    if (type == null) {
-      return ImmutableSet.of();
-    }
+    return type != null && throwsCheckedException(type, state);
+  }
 
+  private static boolean throwsCheckedException(LambdaExpressionTree tree, VisitorState state) {
+    return containsCheckedException(ASTHelpers.getThrownExceptions(tree.getBody(), state), state);
+  }
+
+  private static boolean throwsCheckedException(MemberReferenceTree tree, VisitorState state) {
+    return containsCheckedException(ASTHelpers.getSymbol(tree).type.getThrownTypes(), state);
+  }
+
+  private static boolean throwsCheckedException(Type type, VisitorState state) {
     try {
-      return state.getTypes().findDescriptorType(type).getThrownTypes();
+      return containsCheckedException(
+          state.getTypes().findDescriptorType(type).getThrownTypes(), state);
     } catch (
         @SuppressWarnings("java:S1166" /* Not exceptional. */)
         FunctionDescriptorLookupError e) {
-      return ImmutableSet.of();
+      /* This isn't a functional interface: check its supertypes. */
+      return state.getTypes().directSupertypes(type).stream()
+          .anyMatch(t -> throwsCheckedException(t, state));
     }
   }
 
   private static boolean containsCheckedException(Collection<Type> types, VisitorState state) {
-    return !types.stream()
-        .allMatch(
-            t ->
-                ASTHelpers.isSubtype(t, state.getSymtab().runtimeExceptionType, state)
-                    || ASTHelpers.isSubtype(t, state.getSymtab().errorType, state));
+    return types.stream().anyMatch(type -> isCheckedException(type, state));
+  }
+
+  private static boolean isCheckedException(Type type, VisitorState state) {
+    return !ASTHelpers.isSubtype(type, state.getSymtab().runtimeExceptionType, state)
+        && !ASTHelpers.isSubtype(type, state.getSymtab().errorType, state);
   }
 }
diff --git a/...rt/src/test/java/tech/picnic/errorprone/refaster/matchers/ThrowsCheckedExceptionTest.java b/...rt/src/test/java/tech/picnic/errorprone/refaster/matchers/ThrowsCheckedExceptionTest.java
@@ -26,22 +26,42 @@ void matches() {
             "  }",
             "",
             "  void negative3() {",
-            "    callableSink(() -> toString());",
+            "    supplierSink((Supplier<?> & Iterable<?>) null);",
             "  }",
             "",
             "  void negative4() {",
-            "    supplierSink(() -> toString());",
+            "    callableSink(() -> toString());",
             "  }",
             "",
             "  void negative5() {",
-            "    callableSink(this::toString);",
+            "    supplierSink(() -> toString());",
             "  }",
             "",
             "  void negative6() {",
-            "    supplierSink(this::toString);",
+            "    callableSink(this::toString);",
             "  }",
             "",
             "  void negative7() {",
+            "    supplierSink(this::toString);",
+            "  }",
+            "",
+            "  void negative8() {",
+            "    callableSink(A::throwsRuntimeException);",
+            "  }",
+            "",
+            "  void negative9() {",
+            "    supplierSink(A::throwsRuntimeException);",
+            "  }",
+            "",
+            "  void negative10() {",
+            "    callableSink(A::throwsError);",
+            "  }",
+            "",
+            "  void negative11() {",
+            "    supplierSink(A::throwsError);",
+            "  }",
+            "",
+            "  void negative12() {",
             "    supplierSink(",
             "        new Supplier<>() {",
             "          @Override",
@@ -53,15 +73,25 @@ void matches() {
             "",
             "  void positive1() {",
             "    // BUG: Diagnostic contains:",
-            "    callableSink(() -> getClass().getDeclaredConstructor());",
+            "    callableSink((Callable<?>) null);",
             "  }",
             "",
             "  void positive2() {",
             "    // BUG: Diagnostic contains:",
-            "    callableSink(getClass()::getDeclaredConstructor);",
+            "    callableSink((Callable<?> & Iterable<?>) null);",
             "  }",
             "",
             "  void positive3() {",
+            "    // BUG: Diagnostic contains:",
+            "    callableSink(() -> getClass().getDeclaredConstructor());",
+            "  }",
+            "",
+            "  void positive4() {",
+            "    // BUG: Diagnostic contains:",
+            "    callableSink(getClass()::getDeclaredConstructor);",
+            "  }",
+            "",
+            "  void positive5() {",
             "    callableSink(",
             "        // BUG: Diagnostic contains:",
             "        new Callable<>() {",
@@ -72,6 +102,14 @@ void matches() {
             "        });",
             "  }",
             "",
+            "  private static Object throwsRuntimeException() throws IllegalStateException {",
+            "    return null;",
+            "  }",
+            "",
+            "  private static Object throwsError() throws AssertionError {",
+            "    return null;",
+            "  }",
+            "",
             "  private static void callableSink(Callable<?> callable) {}",
             "",
             "  private static void supplierSink(Supplier<?> supplier) {}",