implement detecting too large try-catch blocks #530

autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java

@@ -501,6 +501,14 @@ public enum ProblemType implements AbstractProblemType {
     @HasFalsePositives
     TRY_CATCH_COMPLEXITY,
 
+    /**
+     * Reports code where the try block contains statements that do not throw an exception and could therefore be moved outside the try block.
+     * <br>
+     * It might have false-positives, because it is difficult to detect what code throws which exceptions.
+     */
+    @HasFalsePositives
+    TRY_BLOCK_SIZE,
+
     /**
      * Reports static blocks in classes.
      */

autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/TryCatchComplexity.java

@@ -73,6 +73,8 @@ private List<CtStatement> filterMethodInvocations(List<CtStatement> statements)
     private List<CtStatement> extractMethodInvocations(List<CtStatement> statements) {
         return statements.stream().filter(MethodUtil::isInvocation).toList();
     }
+
+    // TODO: this could be combined with the duplicatecode check code
     private void visitStatement(CtStatement statement, List<CtStatement> allStatements) {
         // The CtStatement may be null if the body of an if statement is empty. for example "if (true);"
         if (statement == null || allStatements.size() > MAX_ALLOWED_STATEMENTS) {

autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/TryBlockSize.java

@@ -0,0 +1,215 @@
+package de.firemage.autograder.core.check.exceptions;
+
+import de.firemage.autograder.core.LocalizedMessage;
+import de.firemage.autograder.core.ProblemType;
+import de.firemage.autograder.core.check.ExecutableCheck;
+import de.firemage.autograder.core.integrated.CoreUtil;
+import de.firemage.autograder.core.integrated.IntegratedCheck;
+import de.firemage.autograder.core.integrated.MethodUtil;
+import de.firemage.autograder.core.integrated.StatementUtil;
+import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.TypeUtil;
+import de.firemage.autograder.core.integrated.UsesFinder;
+import spoon.processing.AbstractProcessor;
+import spoon.reflect.code.CtCatch;
+import spoon.reflect.code.CtCatchVariable;
+import spoon.reflect.code.CtConstructorCall;
+import spoon.reflect.code.CtExecutableReferenceExpression;
+import spoon.reflect.code.CtExpression;
+import spoon.reflect.code.CtInvocation;
+import spoon.reflect.code.CtNewClass;
+import spoon.reflect.code.CtStatement;
+import spoon.reflect.code.CtThrow;
+import spoon.reflect.code.CtTry;
+import spoon.reflect.cu.SourcePosition;
+import spoon.reflect.declaration.CtElement;
+import spoon.reflect.declaration.CtType;
+import spoon.reflect.reference.CtExecutableReference;
+import spoon.reflect.reference.CtTypeReference;
+import spoon.reflect.visitor.CtScanner;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.function.Predicate;
+
+@ExecutableCheck(reportedProblems = { ProblemType.TRY_BLOCK_SIZE })
+public class TryBlockSize extends IntegratedCheck {
+    private static boolean noneThrow(CtStatement ctStatement, Predicate<? super CtTypeReference<?>> isMatch) {
+        List<CtTypeReference<?>> thrownExceptions = new ArrayList<>();
+        ctStatement.accept(new CtScanner() {
+            @Override
+            public void visitCtThrow(CtThrow ctThrow) {
+                thrownExceptions.add(ctThrow.getThrownExpression().getType());
+                super.visitCtThrow(ctThrow);
+            }
+
+            private <T> void recordExecutableReference(CtExecutableReference<?> ctExecutableReference) {
+                var executable = MethodUtil.getExecutableDeclaration(ctExecutableReference);
+                if (executable != null) {
+                    thrownExceptions.addAll(executable.getThrownTypes());
+                }
+            }
+
+            @Override
+            public <T> void visitCtInvocation(CtInvocation<T> invocation) {
+                this.recordExecutableReference(invocation.getExecutable());
+                super.visitCtInvocation(invocation);
+            }
+
+            @Override
+            public <T> void visitCtConstructorCall(CtConstructorCall<T> ctConstructorCall) {
+                this.recordExecutableReference(ctConstructorCall.getExecutable());
+                super.visitCtConstructorCall(ctConstructorCall);
+            }
+
+
+            @Override
+            public <T> void visitCtNewClass(CtNewClass<T> ctNewClass) {
+                this.recordExecutableReference(ctNewClass.getExecutable());
+                super.visitCtNewClass(ctNewClass);
+            }
+
+            @Override
+            public <T, E extends CtExpression<?>> void visitCtExecutableReferenceExpression(CtExecutableReferenceExpression<T, E> expression) {
+                this.recordExecutableReference(expression.getExecutable());
+                super.visitCtExecutableReferenceExpression(expression);
+            }
+        });
+
+        return thrownExceptions.stream().noneMatch(isMatch);
+    }
+
+    private static String formatSourceRange(List<? extends CtElement> ctElements) {
+        if (ctElements.isEmpty()) {
+            return null;
+        }
+
+        SourcePosition position = ctElements.get(0).getPosition();
+        String result = "L%d".formatted(position.getLine());
+
+        if (position.getLine() == position.getEndLine() && ctElements.size() == 1) {
+            return result;
+        }
+
+        int endLine = position.getEndLine();
+        if (ctElements.size() > 1) {
+            endLine = ctElements.get(ctElements.size() - 1).getPosition().getEndLine();
+        }
+
+        return result + "-%d".formatted(endLine);
+    }
+
+    @Override
+    public void check(StaticAnalysis staticAnalysis) {
+        staticAnalysis.processWith(new AbstractProcessor<CtTry>() {
+            @Override
+            public void process(CtTry ctTry) {
+                if (ctTry.isImplicit() || !ctTry.getPosition().isValidPosition()) {
+                    return;
+                }
+
+                List<CtStatement> statements = StatementUtil.getEffectiveStatements(ctTry.getBody());
+                if (statements.isEmpty()) {
+                    return;
+                }
+
+                // these are all exceptions that are caught by the try-catch block
+                Set<CtType<?>> caughtExceptions = ctTry.getCatchers()
+                    .stream()
+                    .map(CtCatch::getParameter)
+                    .map(CtCatchVariable::getMultiTypes)
+                    .flatMap(List::stream)
+                    // filter out RuntimeExceptions, because they are hard to track via code analysis
+                    .filter(type -> !TypeUtil.isSubtypeOf(type, java.lang.RuntimeException.class))
+                    .map(CtTypeReference::getTypeDeclaration)
+                    .filter(Objects::nonNull)
+                    .collect(CoreUtil.toIdentitySet());
+
+                // in case only RuntimeExceptions are caught, ignore the block
+                if (caughtExceptions.isEmpty()) {
+                    return;
+                }
+
+                // The noneThrow method will extract thrown types from the given statement and call this predicate with them.
+                //
+                // The predicate then checks if any of the thrown types are caught by the try-catch block.
+                Predicate<? super CtTypeReference<?>> isMatch = ctTypeReference -> {
+                    var type = ctTypeReference.getTypeDeclaration();
+
+                    // this can happen, but I don't remember when this happens
+                    if (type == null) {
+                        return false;
+                    }
+
+                    // here it checks via the subtype relation, because subtypes are instances of their parent type.
+                    return caughtExceptions.stream().anyMatch(caughtException -> UsesFinder.isSubtypeOf(type, caughtException));
+                };
+
+                // TODO: what about code like this?
+                //
+                // try {
+                //    var variable = methodThatThrows();
+                //
+                //    // code that does not throw, but uses the variable
+                //    System.out.println(variable);
+                // } catch (InvalidArgumentException e) {
+                //    // handle exception
+                // }
+                //
+                // Should that code be linted?
+                // TODO: if it should, document a possible solution for this in the wiki
+
+                // go through each statement and check which do not throw exceptions that are later caught (these are irrelevant)
+                List<CtStatement> irrelevantLeadingStatements = new ArrayList<>();
+                CtStatement lastCheckedStatement = null;
+                for (CtStatement statement : statements) {
+                    lastCheckedStatement = statement;
+                    if (!noneThrow(statement, isMatch)) {
+                        break;
+                    }
+
+                    irrelevantLeadingStatements.add(statement);
+                }
+
+                List<CtStatement> irrelevantTrailingStatements = new ArrayList<>();
+                for (int i = statements.size() - 1; i >= 0; i--) {
+                    CtStatement statement = statements.get(i);
+                    if (statement == lastCheckedStatement || !noneThrow(statement, isMatch)) {
+                        break;
+                    }
+
+                    irrelevantTrailingStatements.add(statement);
+                }
+
+                Collections.reverse(irrelevantTrailingStatements);
+
+                if (!irrelevantLeadingStatements.isEmpty() || !irrelevantTrailingStatements.isEmpty()) {
+                    String start = formatSourceRange(irrelevantLeadingStatements);
+                    String end = formatSourceRange(irrelevantTrailingStatements);
+
+                    String result = start;
+                    if (start == null) {
+                        result = end;
+                    } else if (end != null) {
+                        result = "%s, %s".formatted(start, end);
+                    }
+
+                    addLocalProblem(
+                        ctTry,
+                        new LocalizedMessage(
+                            "try-block-size",
+                            Map.of(
+                                "lines", Objects.requireNonNull(result)
+                            )
+                        ),
+                        ProblemType.TRY_BLOCK_SIZE
+                    );
+                }
+            }
+        });
+    }
+}

autograder-core/src/main/java/de/firemage/autograder/core/integrated/CoreUtil.java

@@ -11,9 +11,14 @@
 
 import java.io.File;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.IdentityHashMap;
 import java.util.Optional;
+import java.util.Set;
 import java.util.StringJoiner;
 import java.util.function.Consumer;
+import java.util.stream.Collector;
+import java.util.stream.Collectors;
 
 /**
  * Utility class for functionality that does not fit in any other utility class.
@@ -73,6 +78,10 @@ public static void visitCtCompilationUnit(CtModel ctModel, Consumer<? super CtCo
             .forEach(lambda);
     }
 
+    public static <T> Collector<T, ?, Set<T>> toIdentitySet() {
+        return Collectors.toCollection(() -> Collections.newSetFromMap(new IdentityHashMap<>()));
+    }
+
     /**
      * Converts the provided source position into a human-readable string.
      *

autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java

@@ -133,6 +133,16 @@ public static boolean isInMainMethod(CtElement ctElement) {
         return MethodUtil.isMainMethod(ctMethod);
     }
 
+    /**
+     * Returns the declaration of the given executable reference.
+     *
+     * @param ctExecutableReference the reference to get the declaration of
+     * @return the declaration or null if the declaration could not be found
+     */
+    public static CtExecutable<?> getExecutableDeclaration(CtExecutableReference<?> ctExecutableReference) {
+        return UsesFinder.getExecutableDeclaration(ctExecutableReference);
+    }
+
     public static boolean isGetter(CtMethod<?> method) {
         return method.getSimpleName().startsWith("get")
             && method.getParameters().isEmpty()

autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java

@@ -185,6 +185,11 @@ public static boolean isAccessingVariable(CtVariable<?> ctVariable, CtVariableAc
     public static CtVariable<?> getDeclaredVariable(CtVariableAccess<?> ctVariableAccess) {
         return UsesFinder.getFor(ctVariableAccess).scanner.variableAccessDeclarations.getOrDefault(ctVariableAccess, null);
     }
+
+    static CtExecutable<?> getExecutableDeclaration(CtExecutableReference<?> ctExecutableReference) {
+        return UsesFinder.getFor(ctExecutableReference).scanner.executableDeclarations.computeIfAbsent(ctExecutableReference, CtExecutableReference::getExecutableDeclaration);
+    }
+
     /**
      * The scanner searches for uses of supported code elements in a single pass over the entire model.
      * Since inserting into the hash map requires (amortized) constant time, usage search runs in O(n) time.
@@ -199,6 +204,7 @@ private static class UsesScanner extends CtScanner {
         private final Map<CtExecutable, List<CtElement>> executableUses = new IdentityHashMap<>();
         private final Map<CtType, List<CtTypeReference>> typeUses = new IdentityHashMap<>();
         private final Map<CtType, Set<CtType>> subtypes = new IdentityHashMap<>();
+        private final Map<CtExecutableReference, CtExecutable> executableDeclarations = new IdentityHashMap<>();
 
         // Caches the current instanceof pattern variables, since Spoon doesn't track them yet
         // We are conservative: A pattern introduces a variable until the end of the current block
@@ -335,6 +341,7 @@ private void recordTypeParameterReference(CtTypeParameterReference reference) {
         private void recordExecutableReference(CtExecutableReference reference, CtElement referencingElement) {
             var executable = reference.getExecutableDeclaration();
             if (executable != null) {
+                this.executableDeclarations.put(reference, executable);
                 var uses = this.executableUses.computeIfAbsent(executable, k -> new ArrayList<>());
                 uses.add(referencingElement);
             }

autograder-core/src/main/resources/strings.de.ftl

@@ -134,6 +134,7 @@ exception-message = Eine Exception sollte immer eine Nachricht dabei haben, die
 
 number-format-exception-ignored = 'NumberFormatException' sollte gefangen werden und entweder behandelt oder durch eine eigene Exception ersetzt werden.
 
+try-block-size = Keine der im catch-Block gefangenen Exceptions, wird in den Zeilen {$lines} geworfen. Diese Zeilen sollten vor oder nach dem try-catch-Block stehen.
 # General
 
 compare-objects-exp = Der Typ '{$type}' sollte 'equals' implementieren und nicht über 'toString' verglichen werden.

autograder-core/src/main/resources/strings.en.ftl

@@ -139,6 +139,8 @@ exception-message = An exception should always have a message that explains what
 
 number-format-exception-ignored = 'NumberFormatException' should be caught and/or replaced by self-defined exception.
 
+try-block-size = None of the exceptions caught in the catch block are thrown in the lines {$lines}. These lines should be placed before or after the try-catch block.
+
 # General
 
 compare-objects-exp = Implement an equals method for type '{$type}' and use it for comparisons