SONARJAVA-6305 Fix the expected exceptions filter to work without semantic information

java-checks-test-sources/default/src/main/java/checks/InstantConversionsCheckSample.java → java-checks-test-sources/default/src/main/java/checks/DateTimeConversionsCheckSample.java

@@ -16,7 +16,7 @@
 import java.time.ZonedDateTime;
 import java.time.temporal.TemporalAccessor;
 
-public class InstantConversionsCheckSample {
+public class DateTimeConversionsCheckSample {
 
   private Instant instant;
   private TemporalAccessor temporalAccessor;

java-checks/src/main/java/org/sonar/java/checks/InstantConversionsCheck.java → java-checks/src/main/java/org/sonar/java/checks/DateTimeConversionsCheck.java

@@ -29,7 +29,7 @@
 import org.sonar.plugins.java.api.tree.TypeCastTree;
 
 @Rule(key = "S8220")
-public class InstantConversionsCheck extends AbstractMethodDetection implements JavaVersionAwareVisitor {
+public class DateTimeConversionsCheck extends AbstractMethodDetection implements JavaVersionAwareVisitor {
 
   private static final String INSTANT = "java.time.Instant";
   private static final String TEMPORAL_ACCESSOR = "java.time.temporal.TemporalAccessor";

java-checks/src/main/java/org/sonar/java/filters/ExpectedExceptionFilter.java

@@ -19,11 +19,9 @@
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
-import org.sonar.java.checks.InstantConversionsCheck;
-import org.sonar.java.checks.helpers.MethodTreeUtils;
+import org.sonar.java.checks.DateTimeConversionsCheck;
 import org.sonar.java.model.ExpressionUtils;
 import org.sonar.plugins.java.api.JavaCheck;
-import org.sonar.plugins.java.api.semantic.MethodMatchers;
 import org.sonar.plugins.java.api.semantic.Type;
 import org.sonar.plugins.java.api.tree.AnnotationTree;
 import org.sonar.plugins.java.api.tree.Arguments;
@@ -39,198 +37,211 @@
 import org.sonar.plugins.java.api.tree.TypeTree;
 import org.sonar.plugins.java.api.tree.UnionTypeTree;
 
-import static org.sonar.java.checks.helpers.UnitTestUtils.isTryCatchFail;
+import static org.sonar.java.checks.helpers.MethodTreeUtils.consecutiveMethodInvocation;
 
+/**
+ * A filter to deactivate rules that catch expressions raising specific exceptions in contexts where these exceptions are expected.
+ * <p>
+ * This filter is willingly broad and works directly with type and method names rather than their types, to be resilient even in the face of missing semantic information.
+ * </p>
+ */
 public class ExpectedExceptionFilter extends BaseTreeVisitorIssueFilter {
 
-  private static final String ASSERTJ_ASSERTIONS = "org.assertj.core.api.Assertions";
-
-  private static final MethodMatchers ASSERT_THROWS_MATCHER = MethodMatchers.create()
-    .ofTypes("org.junit.Assert", "org.junit.jupiter.api.Assertions", "org.testng.Assert", "org.testng.AssertJUnit")
-    .names("assertThrows", "assertThrowsExactly", "expectThrows")
-    .withAnyParameters()
-    .build();
-
-  private static final MethodMatchers ASSERTJ_CATCH_THROWABLE_OF_TYPE = MethodMatchers.create()
-    .ofTypes(ASSERTJ_ASSERTIONS)
-    .names("catchThrowableOfType")
-    .addParametersMatcher("org.assertj.core.api.ThrowableAssert$ThrowingCallable", "java.lang.Class")
-    .build();
-
-  private static final MethodMatchers ASSERTJ_ASSERT_CODE = MethodMatchers.create()
-    .ofTypes(ASSERTJ_ASSERTIONS)
-    .names("assertThatCode", "assertThatThrownBy")
-    .withAnyParameters()
-    .build();
-
-  private static final MethodMatchers ASSERTJ_EXCEPTION_OF_TYPE = MethodMatchers.create()
-    .ofTypes(ASSERTJ_ASSERTIONS, "org.assertj.core.api.BDDAssertions")
-    .names("assertThatExceptionOfType", "thenExceptionOfType")
-    .addParametersMatcher("java.lang.Class")
-    .build();
-
-  private static final MethodMatchers ASSERTJ_TYPED_EXCEPTION = MethodMatchers.create()
-    .ofTypes(ASSERTJ_ASSERTIONS, "org.assertj.core.api.BDDAssertions")
-    .names("assertThatException", "assertThatRuntimeException", "thenException", "thenRuntimeException")
-    .withAnyParameters()
-    .build();
-
-  private static final MethodMatchers ASSERTJ_IS_THROWN_BY = MethodMatchers.create()
-    .ofTypes("org.assertj.core.api.ThrowableTypeAssert")
-    .names("isThrownBy")
-    .addParametersMatcher("org.assertj.core.api.ThrowableAssert$ThrowingCallable")
-    .build();
-
-  private static final MethodMatchers ASSERTJ_INSTANCE_OF_PREDICATES = MethodMatchers.create()
-    .ofSubTypes("org.assertj.core.api.Assert")
-    .names("isInstanceOf", "isInstanceOfAny")
-    .withAnyParameters()
-    .build();
-
-  private static final MethodMatchers ASSERTJ_EXACT_INSTANCE_OF_PREDICATES = MethodMatchers.create()
-    .ofSubTypes("org.assertj.core.api.Assert")
-    .names("isExactlyInstanceOf", "isOfAnyClassIn")
-    .withAnyParameters()
-    .build();
-
-  private static final String DATE_TIME_EXCEPTION = "java.time.DateTimeException";
-
-  private static final Set<String> DATE_TIME_EXCEPTION_SUPERTYPES = Set.of(
-    "java.lang.RuntimeException",
-    "java.lang.Exception",
-    "java.lang.Throwable"
+  private static final Set<String> DATE_TIME_EXCEPTION_TYPES = Set.of(
+    "DateTimeException",
+    "DateTimeParseException",
+    "RuntimeException",
+    "Exception",
+    "Throwable"
+  );
+
+  private static final Set<String> ASSERT_THROWS_METHODS = Set.of(
+    "assertThrows",
+    "assertThrowsExactly",
+    "expectThrows"
+  );
+
+  private static final Set<String> ASSERT_CODE_METHODS = Set.of(
+    "assertThatCode",
+    "assertThatThrownBy"
+  );
+
+  private static final Set<String> INSTANCEOF_METHODS = Set.of(
+    "isInstanceOf",
+    "isInstanceOfAny",
+    "isExactlyInstanceOf",
+    "isOfAnyClassIn"
+  );
+
+  private static final Set<String> ASSERT_EXCEPTION_METHODS = Set.of(
+    "assertThatException",
+    "assertThatRuntimeException",
+    "thenException",
+    "thenRuntimeException"
+  );
+
+  private static final Set<String> ASSERT_OF_TYPE_METHODS = Set.of(
+    "assertThatExceptionOfType",
+    "thenExceptionOfType"
   );
 
   @Override
   public Set<Class<? extends JavaCheck>> filteredRules() {
-    return Set.of(InstantConversionsCheck.class);
+    return Set.of(DateTimeConversionsCheck.class);
   }
 
   @Override
   public void visitMethod(MethodTree tree) {
-    if (hasExpectedDateTimeExceptionAnnotation(tree.modifiers().annotations())) {
-      excludeLines(tree, InstantConversionsCheck.class);
+    if (containsExpectedExceptions(tree.modifiers().annotations(), DATE_TIME_EXCEPTION_TYPES)) {
+      excludeLines(tree, DateTimeConversionsCheck.class);
     }
     super.visitMethod(tree);
   }
 
   @Override
   public void visitTryStatement(TryStatementTree tree) {
-    if (isTryCatchFailExpectingDateTimeException(tree)) {
-      excludeLines(tree.block(), InstantConversionsCheck.class);
+    if (catchesExpectedException(tree, DATE_TIME_EXCEPTION_TYPES)) {
+      excludeLines(tree.block(), DateTimeConversionsCheck.class);
     }
     super.visitTryStatement(tree);
   }
 
   @Override
   public void visitMethodInvocation(MethodInvocationTree tree) {
-    excludeExpectedDateTimeExceptionIssues(tree);
+    excludeExpectedExceptions(tree, DATE_TIME_EXCEPTION_TYPES, DateTimeConversionsCheck.class);
     super.visitMethodInvocation(tree);
   }
 
-  private void excludeExpectedDateTimeExceptionIssues(MethodInvocationTree mit) {
-    if (ASSERT_THROWS_MATCHER.matches(mit)) {
-      excludeAssertThrowsExecutableForDateTimeException(mit);
-    } else if (ASSERTJ_CATCH_THROWABLE_OF_TYPE.matches(mit) && isDateTimeExceptionClass(mit.arguments().get(1), false)) {
-      excludeLines(mit.arguments().get(0), InstantConversionsCheck.class);
-    } else if (ASSERTJ_ASSERT_CODE.matches(mit)) {
-      MethodTreeUtils.subsequentMethodInvocation(mit, ASSERTJ_INSTANCE_OF_PREDICATES).ifPresent(subsequentMit -> {
-        if (hasDateTimeExceptionType(subsequentMit.arguments(), false)) {
-          excludeLines(mit.arguments().get(0), InstantConversionsCheck.class);
-        }
-      });
-      MethodTreeUtils.subsequentMethodInvocation(mit, ASSERTJ_EXACT_INSTANCE_OF_PREDICATES).ifPresent(subsequentMit -> {
-        if (hasDateTimeExceptionType(subsequentMit.arguments(), true)) {
-          excludeLines(mit.arguments().get(0), InstantConversionsCheck.class);
-        }
-      });
-    } else if (ASSERTJ_TYPED_EXCEPTION.matches(mit) || (ASSERTJ_EXCEPTION_OF_TYPE.matches(mit) && isDateTimeExceptionClass(mit.arguments().get(0), false))) {
-      MethodTreeUtils.subsequentMethodInvocation(mit, ASSERTJ_IS_THROWN_BY).ifPresent(subsequentMit ->
-        excludeLines(subsequentMit.arguments().get(0), InstantConversionsCheck.class)
-      );
+  /**
+   * Filter a given rule for parts of a method invocation expression when it expects a given set of exception types.
+   */
+  private void excludeExpectedExceptions(MethodInvocationTree tree, Set<String> expectedExceptions, Class<? extends JavaCheck> filteredRule) {
+    String methodName = tree.methodSymbol().name();
+    Arguments arguments = tree.arguments();
+    if (ASSERT_THROWS_METHODS.contains(methodName) && arguments.size() >= 2) {
+      excludeFromAssertThrows(arguments, expectedExceptions, filteredRule);
+    } else if ("catchThrowableOfType".equals(methodName) && arguments.size() > 1 && containsExpectedExceptions(arguments.get(1), expectedExceptions)) {
+      excludeLines(arguments.get(0), filteredRule);
+    } else if (ASSERT_CODE_METHODS.contains(methodName)) {
+      excludeFromAssertCode(tree, arguments, expectedExceptions, filteredRule);
+    } else if (ASSERT_EXCEPTION_METHODS.contains(methodName) ||
+      (ASSERT_OF_TYPE_METHODS.contains(methodName) && !arguments.isEmpty() && containsExpectedExceptions(arguments.get(0), expectedExceptions))) {
+      excludeIsThrownBy(tree, filteredRule);
     }
   }
 
-  private void excludeAssertThrowsExecutableForDateTimeException(MethodInvocationTree mit) {
-    Arguments arguments = mit.arguments();
-    if (arguments.size() < 2) {
-      return;
-    }
+  private void excludeFromAssertThrows(Arguments arguments, Set<String> expectedExceptions, Class<? extends JavaCheck> filteredRule) {
     int expectedTypeIndex = firstArgumentIsMessage(arguments) ? 1 : 0;
     int executableIndex = expectedTypeIndex + 1;
-    if (arguments.size() > executableIndex && isDateTimeExceptionClass(arguments.get(expectedTypeIndex), "assertThrowsExactly".equals(mit.methodSymbol().name()))) {
-      excludeLines(arguments.get(executableIndex), InstantConversionsCheck.class);
+    if (arguments.size() > executableIndex && containsExpectedExceptions(arguments.get(expectedTypeIndex), expectedExceptions)) {
+      excludeLines(arguments.get(executableIndex), filteredRule);
     }
   }
 
-  private static boolean firstArgumentIsMessage(Arguments arguments) {
-    return arguments.size() >= 3 && arguments.get(0).symbolType().is("java.lang.String");
+  private void excludeFromAssertCode(MethodInvocationTree tree, Arguments arguments, Set<String> expectedExceptions, Class<? extends JavaCheck> filteredRule) {
+    subsequentMethodInvocation(tree, INSTANCEOF_METHODS).ifPresent(mit -> {
+      if (!arguments.isEmpty() && mit.arguments().stream().anyMatch(expression -> containsExpectedExceptions(expression, expectedExceptions))) {
+        excludeLines(arguments.get(0), filteredRule);
+      }
+    });
   }
 
-  private static boolean isTryCatchFailExpectingDateTimeException(TryStatementTree tryStatement) {
-    return isTryCatchFail(tryStatement.block()) && tryStatement.catches().stream().anyMatch(ExpectedExceptionFilter::isDateTimeExceptionCatch);
+  private void excludeIsThrownBy(MethodInvocationTree tree, Class<? extends JavaCheck> filteredRule) {
+    subsequentMethodInvocation(tree, Set.of("isThrownBy")).ifPresent(mit -> {
+      Arguments mitArguments = mit.arguments();
+      if (!mitArguments.isEmpty()) {
+        excludeLines(mitArguments.get(0), filteredRule);
+      }
+    });
   }
 
-  private static boolean isDateTimeException(Type type, boolean exact) {
-    return type.isSubtypeOf(DATE_TIME_EXCEPTION) || (!exact && DATE_TIME_EXCEPTION_SUPERTYPES.stream().anyMatch(type::is));
+  /**
+   * Check if a list of annotations contains a {@code Test} annotation expecting specific exception types.
+   */
+  private static boolean containsExpectedExceptions(List<AnnotationTree> annotations, Set<String> expectedExceptions) {
+    return annotations.stream()
+      .anyMatch(annotation ->
+        "Test".equals(annotation.symbolType().name()) && containsExpectedExceptions(annotation.arguments(), expectedExceptions)
+      );
   }
 
-  private static boolean isDateTimeExceptionClass(ExpressionTree expression, boolean exact) {
-    if (expression.is(Tree.Kind.NEW_ARRAY)) {
-      return ((NewArrayTree) expression).initializers().stream()
-        .anyMatch(initializer -> isDateTimeExceptionClass(initializer, exact));
+  /**
+   * Check if a {@code Test} annotation has an {@code expected} or {@code expectedException} argument matching a set of expected exception types.
+   */
+  private static boolean containsExpectedExceptions(Arguments arguments, Set<String> expectedExceptions) {
+    return arguments.stream()
+      .filter(ExpectedExceptionFilter::isExpectedExceptionArgument)
+      .anyMatch(argument -> containsExpectedExceptions(annotationValue(argument), expectedExceptions));
+  }
+
+  /**
+   * Check that an annotation argument is an {@code expected} or {@code expectedException} attribute.
+   */
+  private static boolean isExpectedExceptionArgument(ExpressionTree expression) {
+    String annotationAttributeName = ExpressionUtils.annotationAttributeName(expression);
+    return "expected".equals(annotationAttributeName) || "expectedExceptions".equals(annotationAttributeName);
+  }
+
+  /**
+   * Check if an expression contains a given exception type.
+   */
+  private static boolean containsExpectedExceptions(ExpressionTree expression, Set<String> expectedExceptions) {
+    if (expression instanceof NewArrayTree newArray) {
+      return newArray.initializers().stream()
+        .anyMatch(initializer -> containsExpectedExceptions(initializer, expectedExceptions));
     }
-    return classLiteralType(expression)
-      .map(type -> isDateTimeException(type, exact))
-      .orElse(false);
+    return classLiteralType(expression).map(type -> expectedExceptions.contains(type.name())).orElse(false);
+  }
+
+  /**
+   * Get the value of an annotation attribute.
+   */
+  private static ExpressionTree annotationValue(ExpressionTree expression) {
+    return expression.is(Tree.Kind.ASSIGNMENT) ? ((AssignmentExpressionTree) expression).expression() : expression;
   }
 
+  /**
+   * Extract the type name of a class literal expression.
+   */
   private static Optional<Type> classLiteralType(ExpressionTree expression) {
-    if (expression.is(Tree.Kind.MEMBER_SELECT)) {
-      MemberSelectExpressionTree memberSelect = (MemberSelectExpressionTree) expression;
-      if ("class".equals(memberSelect.identifier().name())) {
-        return Optional.of(memberSelect.expression().symbolType());
-      }
+    if (expression instanceof MemberSelectExpressionTree memberSelect && "class".equals(memberSelect.identifier().name())) {
+      return Optional.of(memberSelect.expression().symbolType());
     }
     return Optional.empty();
   }
 
-  private static boolean isDateTimeExceptionCatch(CatchTree catchTree) {
+  /**
+   * Check if a try statement catches a set of expected exception types.
+   */
+  private static boolean catchesExpectedException(TryStatementTree tryStatement, Set<String> expectedExceptions) {
+    return tryStatement.catches().stream()
+      .anyMatch(catchTree -> catchesExpectedExceptions(catchTree, expectedExceptions));
+  }
+
+  private static boolean catchesExpectedExceptions(CatchTree catchTree, Set<String> expectedExceptions) {
     return exceptionTypes(catchTree.parameter().type()).stream()
-      .anyMatch(type -> isDateTimeException(type.symbolType(), false));
+      .anyMatch(type -> expectedExceptions.contains(type.symbolType().name()));
   }
 
   private static List<TypeTree> exceptionTypes(TypeTree typeTree) {
-    if (typeTree.is(Tree.Kind.UNION_TYPE)) {
-      return ((UnionTypeTree) typeTree).typeAlternatives();
+    if (typeTree instanceof UnionTypeTree unionType) {
+      return unionType.typeAlternatives();
     }
     return List.of(typeTree);
   }
 
-  private static boolean hasExpectedDateTimeExceptionAnnotation(List<AnnotationTree> annotations) {
-    return annotations.stream().anyMatch(annotation -> {
-      Type annotationType = annotation.annotationType().symbolType();
-      if (annotationType.is("org.junit.Test")) {
-        return hasExpectedDateTimeExceptionArgument(annotation, "expected");
-      } else if (annotationType.is("org.testng.annotations.Test")) {
-        return hasExpectedDateTimeExceptionArgument(annotation, "expectedExceptions");
-      }
-      return false;
-    });
-  }
-
-  private static boolean hasExpectedDateTimeExceptionArgument(AnnotationTree annotation, String attributeName) {
-    return annotation.arguments().stream()
-      .filter(argument -> attributeName.equals(ExpressionUtils.annotationAttributeName(argument)))
-      .anyMatch(argument -> isDateTimeExceptionClass(annotationValue(argument), false));
-  }
-
-  private static ExpressionTree annotationValue(ExpressionTree expression) {
-    return expression.is(Tree.Kind.ASSIGNMENT) ? ((AssignmentExpressionTree) expression).expression() : expression;
+  private static boolean firstArgumentIsMessage(Arguments arguments) {
+    return arguments.size() >= 3 && arguments.get(0).symbolType().is("java.lang.String");
   }
 
-  private static boolean hasDateTimeExceptionType(List<ExpressionTree> expressions, boolean exact) {
-    return expressions.stream().anyMatch(expression -> isDateTimeExceptionClass(expression, exact));
+  /**
+   * Get the next chained method invocation whose identifier matches a set of expected method names.
+   */
+  private static Optional<MethodInvocationTree> subsequentMethodInvocation(MethodInvocationTree tree, Set<String> expectedMethodNames) {
+    return consecutiveMethodInvocation(tree)
+      .map(consecutiveMethod ->
+        expectedMethodNames.contains(consecutiveMethod.methodSymbol().name()) ?
+          consecutiveMethod : subsequentMethodInvocation(consecutiveMethod, expectedMethodNames).orElse(null));
   }
 
 }