From e66e4a148a886d82c6f8bcc7f7ba83ba3d478321 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 21 May 2024 09:02:25 +0200 Subject: [PATCH] enable exception message check for any exception #434 --- .../exceptions/ExceptionMessageCheck.java | 44 ++++-- .../core/integrated/ExceptionUtil.java | 45 ------ .../exceptions/TestExceptionMessageCheck.java | 129 ++++++++++++++++++ 3 files changed, 159 insertions(+), 59 deletions(-) delete mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExceptionUtil.java diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/ExceptionMessageCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/ExceptionMessageCheck.java index 934f59c3..a1dbeccb 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/ExceptionMessageCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/ExceptionMessageCheck.java @@ -4,7 +4,6 @@ import de.firemage.autograder.core.ProblemType; import de.firemage.autograder.core.check.ExecutableCheck; -import de.firemage.autograder.core.integrated.ExceptionUtil; import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.SpoonUtil; import de.firemage.autograder.core.integrated.StaticAnalysis; @@ -12,33 +11,50 @@ import spoon.reflect.code.CtCase; import spoon.reflect.code.CtConstructorCall; import spoon.reflect.code.CtExpression; +import spoon.reflect.code.CtInvocation; import spoon.reflect.code.CtThrow; import spoon.reflect.declaration.CtConstructor; import spoon.reflect.declaration.CtElement; -import java.util.List; - @ExecutableCheck(reportedProblems = ProblemType.EXCEPTION_WITHOUT_MESSAGE) public class ExceptionMessageCheck extends IntegratedCheck { private static boolean isExceptionWithoutMessage(CtExpression expression) { - return expression instanceof CtConstructorCall ctorCall - && ExceptionUtil.isRuntimeException(ctorCall.getType()) - && !hasMessage(ctorCall.getArguments()); - } + if (!(expression instanceof CtConstructorCall ctConstructorCall) || + !(SpoonUtil.isSubtypeOf(expression.getType(), java.lang.Exception.class))) { + return false; + } - private static boolean hasMessage(List> arguments) { - if (arguments.isEmpty()) { + // check if the invoked constructor passes a message to the exception + if (ctConstructorCall.getExecutable().getExecutableDeclaration() instanceof CtConstructor ctConstructor + && ctConstructor.getBody().filterChildren(ctElement -> ctElement instanceof CtInvocation ctInvocation + // we just check if there is any invocation with a message, because this is easier and might be enough + // for most cases. + // + // this way will not result in false positives, only in false negatives + && ctInvocation.getExecutable().isConstructor() + && hasMessage(ctInvocation.getArguments()) + ).first() != null) { return false; } - CtExpression ctExpression = arguments.get(0); - String literal = SpoonUtil.tryGetStringLiteral(ctExpression).orElse(null); + return !hasMessage(ctConstructorCall.getArguments()); + } - if (literal != null) { - return !literal.isBlank(); + private static boolean hasMessage(Iterable> arguments) { + for (CtExpression ctExpression : arguments) { + // consider a passed throwable as having message + if (SpoonUtil.isSubtypeOf(ctExpression.getType(), java.lang.Throwable.class)) { + return true; + } + + String literal = SpoonUtil.tryGetStringLiteral(ctExpression).orElse(null); + + if (literal != null) { + return !literal.isBlank(); + } } - return true; + return false; } private static boolean isInAllowedContext(CtElement ctElement) { diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExceptionUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExceptionUtil.java deleted file mode 100644 index 6ded5d1a..00000000 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExceptionUtil.java +++ /dev/null @@ -1,45 +0,0 @@ -package de.firemage.autograder.core.integrated; - -import spoon.reflect.reference.CtTypeReference; - -import java.util.Set; - -public final class ExceptionUtil { - private static final Set RUNTIME_EXCEPTIONS = Set.of( - "java.lang.RuntimeException", - "java.lang.IllegalArgumentException", - "java.lang.NullPointerException", - "java.util.NoSuchElementException", - "java.lang.NumberFormatException", - "java.lang.ArithmeticException", - "java.lang.ArrayIndexOutOfBoundsException", - "java.lang.SecurityException", - "java.lang.NegativeArraySizeException", - "java.lang.ClassCastException", - "java.lang.ArrayStoreException", - "java.lang.EnumConstantNotPresentException", - "java.lang.IllegalStateException", - "java.lang.UnsupportedOperationException", - "java.lang.IndexOutOfBoundsException", - "java.lang.StringIndexOutOfBoundsException" - ); - - private static final Set ERRORS = Set.of( - "java.lang.Error", - "java.lang.AssertionError", - "java.lang.OutOfMemoryError", - "java.lang.StackOverflowError" - ); - - private ExceptionUtil() { - - } - - public static boolean isRuntimeException(CtTypeReference type) { - return RUNTIME_EXCEPTIONS.contains(type.getQualifiedName()); - } - - public static boolean isError(CtTypeReference type) { - return ERRORS.contains(type.getQualifiedName()); - } -} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestExceptionMessageCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestExceptionMessageCheck.java index a6446754..e6dc6797 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestExceptionMessageCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestExceptionMessageCheck.java @@ -11,6 +11,7 @@ import java.io.IOException; import java.util.List; +import java.util.Map; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -164,4 +165,132 @@ public String[] readFile() { problems.assertExhausted(); } + + @Test + void testExceptionFactoryMethod() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "MyException", + """ + public class MyException extends Exception { + public static Exception create() { + return new MyException(); + } + } + """ + ), + Map.entry( + "Main", + """ + public class Main { + public static void main(String[] args) throws Exception { + throw MyException.create(); /*# ok #*/ + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testCustomExceptionNoMessage() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "MyException", + """ + public class MyException extends Exception { + public MyException() { + super(); + } + } + """ + ), + Map.entry( + "Main", + """ + public class Main { + public static void main(String[] args) throws Exception { + throw new MyException(); /*# not ok #*/ + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + assertMissingMessage(problems.next()); + + problems.assertExhausted(); + } + + @Test + void testCustomExceptionInvalidMessage() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "MyException", + """ + public class MyException extends Exception { + public MyException() { + super(" "); + } + } + """ + ), + Map.entry( + "Main", + """ + public class Main { + public static void main(String[] args) throws Exception { + throw new MyException(); /*# not ok #*/ + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + assertMissingMessage(problems.next()); + + problems.assertExhausted(); + } + + @Test + void testCustomExceptionValidMessage() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "MyException", + """ + public class MyException extends Exception { + public MyException() { + super("failed to start the application"); + } + } + """ + ), + Map.entry( + "Main", + """ + public class Main { + public static void main(String[] args) throws Exception { + throw new MyException(); /*# ok #*/ + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } }