Merge pull request #604 from Luro02/main

Implement some bug fixes

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

@@ -1,6 +1,7 @@
 package de.firemage.autograder.core;
 
 import de.firemage.autograder.core.file.SourceInfo;
+import de.firemage.autograder.core.integrated.DuplicateCodeFinder;
 import de.firemage.autograder.core.integrated.MethodHierarchy;
 import de.firemage.autograder.core.integrated.MethodUtil;
 import de.firemage.autograder.core.integrated.ModelBuildException;
@@ -249,6 +250,7 @@ public void process(CtType<?> type) {
 
             MethodHierarchy.buildFor(model);
             UsesFinder.buildFor(model);
+            DuplicateCodeFinder.buildFor(model);
 
             // Only set the model at the end when everything has been initialized
             this.model = model;

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

@@ -114,9 +114,23 @@ public List<Problem> checkFile(
             }
         }
 
-        AnalysisScheduler scheduler = new AnalysisScheduler(this.threads, classLoader);
 
-        AnalysisResult result;
+        // TODO: refactor AnalysisScheduler/AnalysisThread to be simpler/easier to understand
+        //
+        // The code has been disabled, because of the following issues:
+        // - Spoon Code does not like to be run in parallel
+        // - Having checks run in parallel, resulted in bugs that were hard to reproduce, because they depended on the order
+        //   in which the checks were run
+        // - Crashes resulted in the program looping/hanging, instead of exiting with the thrown exception
+        //
+        // The last issue was the one that made me disable the code, because it made debugging very hard, and
+        // it was not clear how to fix it. It looks like the code is reinventing the wheel, instead of using
+        // existing solutions like ExecutorService.
+        //
+        //AnalysisScheduler scheduler = new AnalysisScheduler(this.threads, classLoader);
+
+        // AnalysisResult result;
+        List<Problem> unreducedProblems = new ArrayList<>();
         try (TempLocation tempLinterLocation = this.tempLocation.createTempDirectory("linter")) {
             for (var entry : linterChecks.entrySet()) {
                 CodeLinter linter = entry.getKey();
@@ -129,27 +143,27 @@ public List<Problem> checkFile(
                     continue;
                 }
 
-                scheduler.submitTask((s, reporter) -> {
+                /*scheduler.submitTask((s, reporter) -> {
                     reporter.reportProblems(linter.lint(
                         file,
                         tempLinterLocation,
                         this.classLoader,
                         associatedChecks,
                         statusConsumer
                     ));
-                });
-            }
-
-            result = scheduler.collectProblems();
-            if (result.failed()) {
-                throw new LinterException(result.thrownException());
+                });*/
+                unreducedProblems.addAll(linter.lint(
+                    file,
+                    tempLinterLocation,
+                    this.classLoader,
+                    associatedChecks,
+                    statusConsumer
+                ));
             }
         }
 
-        var unreducedProblems = result.problems();
         if (!checkConfiguration.problemsToReport().isEmpty()) {
-            unreducedProblems = result
-                .problems()
+            unreducedProblems = unreducedProblems
                 .stream()
                 .filter(problem -> checkConfiguration.problemsToReport().contains(problem.getProblemType()))
                 .toList();

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

@@ -51,6 +51,18 @@ public enum ProblemType implements AbstractProblemType {
      */
     SIMPLIFY_ARRAYS_FILL,
 
+    /**
+     * Reports duplicate catch blocks that could be merged with a multi-catch block.
+     */
+    @HasFalsePositives
+    DUPLICATE_CATCH_BLOCK,
+
+    /**
+     * Reports duplicate if blocks that could be merged by adjusting the condition.
+     */
+    @HasFalsePositives
+    DUPLICATE_IF_BLOCK,
+
     /**
      * Reports unused assignments
      */
@@ -173,12 +185,24 @@ public enum ProblemType implements AbstractProblemType {
     @HasFalsePositives
     MUTABLE_ENUM,
 
+    /**
+     * Suggests using {@link String#substring(int)} instead of {@link String#substring(int, int)} when possible.
+     */
+    @HasFalsePositives
+    SIMPLIFY_STRING_SUBSTRING,
+
     /**
      * Reports code where methods Character.isDigit are reimplemented (e.g. `c >= '0' && c <= '9'`).
      */
     @HasFalsePositives
     CHAR_RANGE,
 
+    /**
+     * Suggests using `Arrays.copyOf` instead of `System.arraycopy`.
+     */
+    @HasFalsePositives
+    USE_ARRAYS_COPY_OF,
+
     /**
      * Similar to {@link ProblemType#INCONSISTENT_COMMENT_LANGUAGE}, but reports comments where the AI thinks that the comment is neither german nor english.
      * <br>
@@ -428,6 +452,12 @@ public enum ProblemType implements AbstractProblemType {
     @HasFalsePositives
     FIELD_SHOULD_BE_CONSTANT,
 
+    /**
+     * Reports code where a local variable could be a constant.
+     */
+    @HasFalsePositives
+    LOCAL_VARIABLE_SHOULD_BE_CONSTANT,
+
     /**
      * Tries to find classes that are exclusively used for constants.
      * <br>
@@ -533,6 +563,14 @@ public enum ProblemType implements AbstractProblemType {
     @HasFalsePositives
     LOOP_SHOULD_BE_FOR,
 
+    /**
+     * Reports loops that can be replaced with a while loop.
+     * <br>
+     * This check is not trivial and likely has false-positives.
+     */
+    @HasFalsePositives
+    LOOP_SHOULD_BE_WHILE,
+
     /**
      * Reports methods that do not have an override annotation, but override a method.
      * <br>
@@ -847,12 +885,6 @@ public enum ProblemType implements AbstractProblemType {
     @HasFalsePositives
     USE_FORMAT_STRING,
 
-    /**
-     * Reports code where a local variable could be a constant.
-     */
-    @HasFalsePositives
-    LOCAL_VARIABLE_SHOULD_BE_CONSTANT,
-
     /**
      * Suggests using EnumMap or EnumSet.
      */

autograder-core/src/main/java/de/firemage/autograder/core/check/api/SimplifyStringSubstring.java

@@ -0,0 +1,62 @@
+package de.firemage.autograder.core.check.api;
+
+
+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.IntegratedCheck;
+import de.firemage.autograder.core.integrated.MethodUtil;
+import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.TypeUtil;
+import spoon.processing.AbstractProcessor;
+import spoon.reflect.code.CtExpression;
+import spoon.reflect.code.CtInvocation;
+
+import java.util.Map;
+
+@ExecutableCheck(reportedProblems = { ProblemType.SIMPLIFY_STRING_SUBSTRING })
+public class SimplifyStringSubstring extends IntegratedCheck {
+    @Override
+    protected void check(StaticAnalysis staticAnalysis) {
+        staticAnalysis.processWith(new AbstractProcessor<CtInvocation<?>>() {
+            @Override
+            public void process(CtInvocation<?> ctInvocation) {
+                if (ctInvocation.isImplicit()
+                    || !ctInvocation.getPosition().isValidPosition()) {
+                    return;
+                }
+
+                if (ctInvocation.getTarget() == null
+                    || ctInvocation.getTarget().getType() == null
+                    || !TypeUtil.isTypeEqualTo(ctInvocation.getTarget().getType(), String.class)
+                    || !MethodUtil.isSignatureEqualTo(ctInvocation.getExecutable(), String.class, "substring", int.class, int.class)) {
+                    return;
+                }
+
+                CtExpression<?> start = ctInvocation.getArguments().get(0);
+                CtExpression<?> end = ctInvocation.getArguments().get(1);
+                // ensure that the end is the length of the string
+                if (!(end instanceof CtInvocation<?> endInvocation)
+                    || !MethodUtil.isSignatureEqualTo(endInvocation.getExecutable(), int.class, "length")
+                    || endInvocation.getTarget() == null
+                    || !(endInvocation.getTarget().equals(ctInvocation.getTarget()))) {
+                    return;
+                }
+
+                addLocalProblem(
+                    ctInvocation,
+                    new LocalizedMessage(
+                        "common-reimplementation",
+                        Map.of(
+                            "suggestion", "%s.substring(%s)".formatted(
+                                ctInvocation.getTarget(),
+                                start
+                            )
+                        )
+                    ),
+                    ProblemType.SIMPLIFY_STRING_SUBSTRING
+                );
+            }
+        });
+    }
+}

autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseArraysCopyOf.java

@@ -0,0 +1,69 @@
+package de.firemage.autograder.core.check.api;
+
+
+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.ExpressionUtil;
+import de.firemage.autograder.core.integrated.IntegratedCheck;
+import de.firemage.autograder.core.integrated.MethodUtil;
+import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.TypeUtil;
+import spoon.processing.AbstractProcessor;
+import spoon.reflect.code.CtExpression;
+import spoon.reflect.code.CtInvocation;
+import spoon.reflect.code.CtTypeAccess;
+
+import java.util.Map;
+
+@ExecutableCheck(reportedProblems = { ProblemType.USE_ARRAYS_COPY_OF })
+public class UseArraysCopyOf extends IntegratedCheck {
+    @Override
+    protected void check(StaticAnalysis staticAnalysis) {
+        if (!staticAnalysis.hasJavaUtilImport()) {
+            return;
+        }
+
+        staticAnalysis.processWith(new AbstractProcessor<CtInvocation<?>>() {
+            @Override
+            public void process(CtInvocation<?> ctInvocation) {
+                if (ctInvocation.isImplicit()
+                    || !ctInvocation.getPosition().isValidPosition()) {
+                    return;
+                }
+
+                if (ctInvocation.getTarget() == null
+                    || ctInvocation.getTarget().getType() == null
+                    || !(ctInvocation.getTarget() instanceof CtTypeAccess<?> ctTypeAccess)
+                    || !TypeUtil.isTypeEqualTo(ctTypeAccess.getAccessedType(), System.class)
+                    || !MethodUtil.isSignatureEqualTo(ctInvocation.getExecutable(), void.class, "arraycopy", Object.class, int.class, Object.class, int.class, int.class)) {
+                    return;
+                }
+                // System.arraycopy(src, srcPos, dest, destPos, length)
+
+                CtExpression<?> source = ctInvocation.getArguments().get(0);
+                CtExpression<?> sourcePosition = ctInvocation.getArguments().get(1);
+                CtExpression<?> destination = ctInvocation.getArguments().get(2);
+                CtExpression<?> destinationPosition = ctInvocation.getArguments().get(3);
+                CtExpression<?> length = ctInvocation.getArguments().get(4);
+
+                if (ExpressionUtil.isIntegerLiteral(sourcePosition, 0) && ExpressionUtil.isIntegerLiteral(destinationPosition, 0)) {
+                    addLocalProblem(
+                        ctInvocation,
+                        new LocalizedMessage(
+                            "common-reimplementation",
+                            Map.of(
+                                "suggestion", "%s = Arrays.copyOf(%s, %s)".formatted(
+                                    destination,
+                                    source,
+                                    length
+                                )
+                            )
+                        ),
+                        ProblemType.USE_ARRAYS_COPY_OF
+                    );
+                }
+            }
+        });
+    }
+}

autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseEntrySet.java

@@ -3,7 +3,6 @@
 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.ExpressionUtil;
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
 import de.firemage.autograder.core.integrated.TypeUtil;
@@ -27,14 +26,23 @@ private static boolean hasInvokedKeySet(CtInvocation<?> ctInvocation) {
             && ctInvocation.getExecutable().getSimpleName().equals("keySet");
     }
 
+    private static String makeSuggestion(CtInvocation<?> ctInvocation) {
+        String suggestion = "%s.entrySet()".formatted(ctInvocation.getTarget());
+        if (suggestion.startsWith(".")) {
+            suggestion = suggestion.substring(1);
+        }
+
+        return suggestion;
+    }
+
     @Override
     protected void check(StaticAnalysis staticAnalysis) {
         staticAnalysis.processWith(new AbstractProcessor<CtForEach>() {
             @Override
             public void process(CtForEach ctForEach) {
                 if (ctForEach.isImplicit()
                     || !ctForEach.getPosition().isValidPosition()
-                    || !(ExpressionUtil.resolveCtExpression(ctForEach.getExpression()) instanceof CtInvocation<?> ctInvocation)
+                    || !(ctForEach.getExpression() instanceof CtInvocation<?> ctInvocation)
                     || !hasInvokedKeySet(ctInvocation)
                     || !ctForEach.getExpression().getPosition().isValidPosition()) {
                     return;
@@ -61,7 +69,12 @@ public void process(CtForEach ctForEach) {
                 if (!invocations.isEmpty()) {
                     addLocalProblem(
                         ctForEach.getExpression(),
-                        new LocalizedMessage("suggest-replacement", Map.of("original", "keySet()", "suggestion", "entrySet()")),
+                        new LocalizedMessage(
+                            "suggest-replacement",
+                            Map.of(
+                                "original", ctInvocation,
+                                "suggestion", makeSuggestion(ctInvocation)
+                            )),
                         ProblemType.USE_ENTRY_SET
                     );
                 }

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

@@ -108,12 +108,20 @@ private static boolean hasAnyJavadocUses(CtReference ctReference, Filter<? exten
                 filter,
                 new TypeFilter<>(CtJavaDoc.class),
                 ctJavaDoc -> {
-                    JavadocParser parser = new JavadocParser(ctJavaDoc.getRawContent(), ctJavaDoc.getParent());
-
-                    return parser.parse()
-                        .stream()
-                        .flatMap(element -> element.accept(new ReferenceFinder()).stream())
-                        .anyMatch(otherReference -> isReferencingTheSameElement(ctReference, otherReference));
+                    try {
+                        JavadocParser parser = new JavadocParser(ctJavaDoc.getRawContent(), ctJavaDoc.getParent());
+
+                        return parser.parse()
+                            .stream()
+                            .flatMap(element -> element.accept(new ReferenceFinder()).stream())
+                            .anyMatch(otherReference -> isReferencingTheSameElement(ctReference, otherReference));
+                    } catch (AssertionError e) {
+                        // the javadoc parser did throw an assertion error on javadoc that was not valid, but the code did compile,
+                        // so this is caught here to prevent the whole thing from crashing;
+                        //
+                        // The javadoc in question was:  "/**\n     * @param length the {@param string} and {@param value}.\n     */"
+                        return false;
+                    }
                 }
             )).first(CtJavaDoc.class) != null;
     }