Merge pull request #185 from Luro02/main

Fix bugs and migrate PMD to rc3

autograder-core/pom.xml

@@ -14,7 +14,7 @@
 
     <properties>
         <spoon.version>10.3.0</spoon.version>
-        <pmd.version>7.0.0-rc2</pmd.version>
+        <pmd.version>7.0.0-rc3</pmd.version>
         <spotbugs.version>6.45.0</spotbugs.version>
 
         <docker.version>3.3.1</docker.version>

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

@@ -166,6 +166,11 @@ public List<Problem> checkFile(
         Iterable<? extends Check> checks,
         Consumer<LinterStatus> statusConsumer
     ) throws LinterException, IOException {
+        // the file is null if the student did not upload source code
+        if (file == null) {
+            return new ArrayList<>();
+        }
+
         List<PMDCheck> pmdChecks = new ArrayList<>();
         List<SpotbugsCheck> spotbugsChecks = new ArrayList<>();
         List<CopyPasteCheck> cpdChecks = new ArrayList<>();

autograder-core/src/main/java/de/firemage/autograder/core/check/debug/PrintStackTraceCheck.java

@@ -5,6 +5,7 @@
 import de.firemage.autograder.core.check.ExecutableCheck;
 import de.firemage.autograder.core.dynamic.DynamicAnalysis;
 import de.firemage.autograder.core.integrated.IntegratedCheck;
+import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
 import spoon.processing.AbstractProcessor;
 import spoon.reflect.code.CtInvocation;
@@ -15,9 +16,7 @@ public class PrintStackTraceCheck extends IntegratedCheck {
     private static boolean hasInvokedPrintStackTrace(CtInvocation<?> ctInvocation) {
         return ctInvocation.getTarget() instanceof CtVariableRead<?> ctVariableRead
             // ensure the method is called on the correct type
-            && ctVariableRead.getType().isSubtypeOf(
-                ctInvocation.getFactory().Type().createReference(java.lang.Throwable.class)
-            )
+            && SpoonUtil.isSubtypeOf(ctVariableRead.getType(), java.lang.Throwable.class)
             && ctInvocation.getExecutable().getSimpleName().equals("printStackTrace");
     }
 

autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseDifferentVisibility.java

@@ -1,6 +1,5 @@
 package de.firemage.autograder.core.check.general;
 
-import com.google.common.collect.Iterators;
 import com.google.common.collect.Sets;
 import de.firemage.autograder.core.LocalizedMessage;
 import de.firemage.autograder.core.ProblemType;
@@ -19,7 +18,6 @@
 import spoon.reflect.declaration.CtType;
 import spoon.reflect.declaration.CtTypeMember;
 import spoon.reflect.reference.CtReference;
-import spoon.reflect.visitor.CtScanner;
 import spoon.reflect.visitor.filter.DirectReferenceFilter;
 
 import java.util.Iterator;

autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseGuardClauses.java

@@ -34,7 +34,8 @@ private static <T> CtUnaryOperator<T> makeCtUnaryOperator(CtExpression<T> ctExpr
         CtUnaryOperator<T> ctUnaryOperator = ctExpression.getFactory().createUnaryOperator();
 
         ctUnaryOperator.setKind(kind);
-        ctUnaryOperator.setOperand(ctExpression);
+        // the clone is necessary, so the original expression from the CtModel does not get a new parent
+        ctUnaryOperator.setOperand(ctExpression.clone());
 
         return ctUnaryOperator;
     }

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

@@ -101,14 +101,19 @@ public void lint(List<IntegratedCheck> checks, Consumer<LinterStatus> statusCons
 
         statusConsumer.accept(LinterStatus.RUNNING_INTEGRATED_CHECKS);
 
-        for (IntegratedCheck check : checks) {
-            scheduler.submitTask((s, reporter) -> {
+        // Execute checks in sequence to avoid race conditions in spoon.
+        // Some queries on the spoon model have writes that are not synchronized.
+        //
+        // This has caused a crash, where one queried if a type (from java.lang) is a subtype of another type,
+        // which would invoke the shadow model. This is built lazily, and seems to cause a race-condition.
+        scheduler.submitTask((s, reporter) -> {
+            for (IntegratedCheck check : checks) {
                 long beforeTime = System.nanoTime();
                 reporter.reportProblems(check.run(this.staticAnalysis, this.dynamicAnalysis, this.file.getSource().getPath()));
                 long afterTime = System.nanoTime();
                 logger.info("Completed check " + check.getClass().getSimpleName() + " in " + ((afterTime - beforeTime) / 1_000_000 + "ms"));
-            });
-        }
+            }
+        });
     }
 
     public StaticAnalysis getStaticAnalysis() {

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

@@ -1,5 +1,6 @@
 package de.firemage.autograder.core.integrated;
 
+import com.google.common.collect.Streams;
 import de.firemage.autograder.core.integrated.effects.AssignmentStatement;
 import de.firemage.autograder.core.integrated.effects.Effect;
 import de.firemage.autograder.core.integrated.effects.TerminalEffect;
@@ -30,6 +31,8 @@
 import spoon.reflect.declaration.CtTypeMember;
 import spoon.reflect.declaration.CtTypedElement;
 import spoon.reflect.declaration.ModifierKind;
+import spoon.reflect.factory.TypeFactory;
+import spoon.reflect.reference.CtExecutableReference;
 import spoon.reflect.reference.CtFieldReference;
 import spoon.reflect.reference.CtTypeReference;
 import spoon.reflect.reference.CtVariableReference;
@@ -227,7 +230,7 @@ public static CtExpression<?> negate(CtExpression<?> ctExpression) {
         CtUnaryOperator ctUnaryOperator = ctExpression.getFactory().createUnaryOperator();
 
         ctUnaryOperator.setKind(UnaryOperatorKind.NEG);
-        ctUnaryOperator.setOperand(ctExpression);
+        ctUnaryOperator.setOperand(ctExpression.clone());
 
         return ctUnaryOperator;
     }
@@ -331,21 +334,60 @@ public static boolean isVoidMethod(CtMethod<?> method) {
         return method.getType().getQualifiedName().equals("void");
     }
 
+    public static boolean isSignatureEqualTo(
+        CtExecutableReference<?> ctExecutableReference,
+        Class<?> returnType,
+        String methodName,
+        Class<?>... parameterTypes
+    ) {
+        TypeFactory factory = ctExecutableReference.getFactory().Type();
+        return SpoonUtil.isSignatureEqualTo(
+            ctExecutableReference,
+            factory.createReference(returnType),
+            methodName,
+            Arrays.stream(parameterTypes).map(factory::createReference).toArray(CtTypeReference[]::new)
+        );
+    }
+
+    public static boolean isSignatureEqualTo(
+        CtExecutableReference<?> ctExecutableReference,
+        CtTypeReference<?> returnType,
+        String methodName,
+        CtTypeReference<?>... parameterTypes
+    ) {
+        // check that they both return the same type
+        return SpoonUtil.isTypeEqualTo(ctExecutableReference.getType(), returnType)
+            // their names should match:
+            && ctExecutableReference.getSimpleName().equals(methodName)
+            // the number of parameters should match
+            && ctExecutableReference.getParameters().size() == parameterTypes.length
+            && Streams.zip(
+                // combine both the parameters of the executable and the expected types
+                ctExecutableReference.getParameters().stream(),
+                Arrays.stream(parameterTypes),
+                // evaluate if the type of the parameter is equal to the expected type:
+                SpoonUtil::isTypeEqualTo
+                // On this stream of booleans, check if all are true
+            ).allMatch(value -> value);
+    }
+
     public static boolean isEqualsMethod(CtMethod<?> method) {
-        return method.getSimpleName().equals("equals")
-               && method.isPublic()
-               && method.getType().getQualifiedName().equals("boolean")
-               && method.getParameters().size() == 1
-               && method.getParameters().get(0).getType().getQualifiedName().equals("java.lang.Object");
+        return SpoonUtil.isSignatureEqualTo(
+            method.getReference(),
+            boolean.class,
+            "equals",
+            java.lang.Object.class
+        );
     }
 
     public static boolean isCompareToMethod(CtMethod<?> method) {
-        return method.getSimpleName().equals("compareTo")
-               && method.isPublic()
-               && method.getType().getQualifiedName().equals("boolean")
-               && method.getParameters().size() == 1
-               && method.getParameters().get(0).getType().getQualifiedName()
-                        .equals(method.getDeclaringType().getQualifiedName());
+        return method.isPublic()
+                && SpoonUtil.isSignatureEqualTo(
+                    method.getReference(),
+                    method.getFactory().Type().createReference(int.class),
+                    "compareTo",
+                    method.getDeclaringType().getReference()
+                );
     }
 
     public static Optional<CtJavaDoc> getJavadoc(CtElement element) {
@@ -421,22 +463,32 @@ public static Optional<CtExpression<?>> getEffectivelyFinalExpression(
     }
 
     public static boolean isTypeEqualTo(CtTypeReference<?> ctType, Class<?>... expected) {
-        return Arrays.stream(expected)
-            .map(ctClass -> ctType.getFactory().Type().createReference(ctClass))
-            .anyMatch(ctType::equals);
+        TypeFactory factory = ctType.getFactory().Type();
+        return SpoonUtil.isTypeEqualTo(
+            ctType,
+            Arrays.stream(expected)
+                .map(factory::createReference)
+                .toArray(CtTypeReference[]::new)
+        );
+    }
+
+    public static boolean isTypeEqualTo(CtTypeReference<?> ctType, CtTypeReference<?>... expected) {
+        return Arrays.asList(expected).contains(ctType);
     }
 
     public static boolean isSubtypeOf(CtTypeReference<?> ctTypeReference, Class<?> expected) {
         return ctTypeReference.isSubtypeOf(ctTypeReference.getFactory().Type().createReference(expected));
     }
 
     public static boolean isMainMethod(CtMethod<?> method) {
-        return method.getSimpleName().equals("main")
-            && method.getType().getQualifiedName().equals("void")
-            && method.isStatic()
+        return method.isStatic()
             && method.isPublic()
-            && method.getParameters().size() == 1
-            && method.getParameters().get(0).getType().getQualifiedName().equals("java.lang.String[]");
+            && SpoonUtil.isSignatureEqualTo(
+                method.getReference(),
+                void.class,
+                "main",
+                java.lang.String[].class
+            );
     }
 
     /**

autograder-core/src/main/java/de/firemage/autograder/core/pmd/PMDInCodeProblem.java

@@ -12,7 +12,7 @@ public class PMDInCodeProblem extends ProblemImpl {
     public PMDInCodeProblem(PMDCheck check, RuleViolation violation, Path root) {
         super(check,
             new CodePosition(
-                relativize(root, Path.of(violation.getFilename())),
+                relativize(root, Path.of(violation.getFileId().getAbsolutePath())),
                 violation.getBeginLine(),
                 violation.getBeginLine(),
                 violation.getBeginColumn(),

autograder-core/src/main/java/de/firemage/autograder/core/pmd/PMDLinter.java

@@ -10,6 +10,7 @@
 import net.sourceforge.pmd.lang.Language;
 import net.sourceforge.pmd.lang.LanguageRegistry;
 import net.sourceforge.pmd.lang.document.FileCollector;
+import net.sourceforge.pmd.lang.document.FileId;
 
 import javax.tools.JavaFileObject;
 import java.io.IOException;
@@ -53,8 +54,8 @@ public List<Problem> lint(UploadedFile file, List<PMDCheck> checks, ClassLoader
             FileCollector collector = pmd.files();
             for (JavaFileObject compilationUnit : file.getSource().compilationUnits()) {
                 collector.addSourceFile(
-                    compilationUnit.getCharContent(false).toString(),
-                    compilationUnit.getName()
+                    FileId.fromPathLikeString(compilationUnit.getName()),
+                    compilationUnit.getCharContent(false).toString()
                 );
             }
 

autograder-core/src/main/java/de/firemage/autograder/core/pmd/ProblemRenderer.java

@@ -23,7 +23,7 @@ public ProblemRenderer(Map<String, PMDCheck> checks, Path root) {
         super("Custom renderer", "Creates InCodeProblems");
         this.checks = checks;
         this.root = root;
-        super.setWriter(new NullWriter());
+        super.setWriter(NullWriter.INSTANCE);
     }
 
     @Override