improve copy-paste detection and fix some bugs

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

@@ -116,11 +116,11 @@ private void checkIsEmptyReimplementation(CtExpression<?> target, CtBinaryOperat
 
 
         CtBinaryOperator<?> result = SpoonUtil.normalizeBy(
-            (left, right) -> isLiteral.test(left) && !isLiteral.test(right),
+            (left, right) -> isLiteral.test(SpoonUtil.resolveConstant(left)) && !isLiteral.test(SpoonUtil.resolveConstant(right)),
             ctBinaryOperator
         );
 
-        if (!(result.getRightHandOperand() instanceof CtLiteral<?> ctLiteral) || !(ctLiteral.getValue() instanceof Number number)) {
+        if (!(result.getRightHandOperand() instanceof CtLiteral<?> ctLiteral) || !(ctLiteral.getValue() instanceof Number number) || SpoonUtil.isNullLiteral(ctLiteral)) {
             return;
         }
 
@@ -154,8 +154,14 @@ private void checkIsEmptyReimplementation(CtExpression<?> target, CtBinaryOperat
 
     private void checkEqualsCall(CtExpression<?> target, CtInvocation<?> ctInvocation, ProblemType problemType) {
         CtExpression<?> argument = ctInvocation.getArguments().get(0);
+
+        if (SpoonUtil.isNullLiteral(argument)) {
+            return;
+        }
+
         if (SpoonUtil.isStringLiteral(SpoonUtil.resolveConstant(argument), "")) {
             this.reportProblem(ctInvocation, ctInvocation.toString(), buildIsEmptySuggestion(target).toString(), problemType);
+            return;
         }
 
         // detect "".equals(s)

autograder-core/src/main/java/de/firemage/autograder/core/check/oop/InheritanceBadPractices.java

@@ -6,7 +6,9 @@
 
 import de.firemage.autograder.core.integrated.IdentifierNameUtils;
 import de.firemage.autograder.core.integrated.IntegratedCheck;
+import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.UsesFinder;
 import spoon.processing.AbstractProcessor;
 import spoon.reflect.declaration.CtClass;
 import spoon.reflect.declaration.CtConstructor;
@@ -30,6 +32,8 @@
     ProblemType.COMPOSITION_OVER_INHERITANCE
 })
 public class InheritanceBadPractices extends IntegratedCheck {
+    private static final boolean IS_IN_DEBUG_MODE = SpoonUtil.isInJunitTest();
+
     @Override
     protected void check(StaticAnalysis staticAnalysis) {
         staticAnalysis.processWith(new AbstractProcessor<CtClass<?>>() {
@@ -38,8 +42,17 @@ public void process(CtClass<?> ctClass) {
                 List<CtField<?>> fields = ctClass.getFields();
                 Set<CtMethod<?>> methods = ctClass.getMethods();
 
-                List<CtType<?>> subtypes = staticAnalysis.getModel()
-                    .getElements(new SubtypeFilter(ctClass.getReference()).includingSelf(false));
+                List<CtType<?>> subtypes = UsesFinder.subtypesOf(ctClass, false).toList();
+
+                // I do not trust my UsesFinder implementation, so in debug mode it will be checked against the slower
+                // implementation
+                if (IS_IN_DEBUG_MODE) {
+                    List<CtType<?>> otherSubtypes = staticAnalysis.getModel()
+                        .getElements(new SubtypeFilter(ctClass.getReference()).includingSelf(false));
+                    if (!subtypes.equals(otherSubtypes)) {
+                        throw new IllegalStateException("Subtypes for %s are not equal".formatted(ctClass.getQualifiedName()));
+                    }
+                }
 
                 // skip if the class is not inherited from:
                 if (subtypes.isEmpty()) {

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

@@ -6,7 +6,6 @@
 import de.firemage.autograder.core.integrated.effects.TerminalStatement;
 import de.firemage.autograder.core.integrated.evaluator.Evaluator;
 import de.firemage.autograder.core.integrated.evaluator.fold.FoldUtils;
-import de.firemage.autograder.core.integrated.evaluator.fold.InferOperatorTypes;
 import de.firemage.autograder.core.integrated.evaluator.fold.InlineVariableRead;
 import de.firemage.autograder.core.integrated.evaluator.fold.RemoveRedundantCasts;
 import org.apache.commons.io.FilenameUtils;
@@ -24,11 +23,9 @@
 import spoon.reflect.code.CtJavaDoc;
 import spoon.reflect.code.CtLambda;
 import spoon.reflect.code.CtLiteral;
-import spoon.reflect.code.CtLocalVariable;
 import spoon.reflect.code.CtStatement;
 import spoon.reflect.code.CtStatementList;
 import spoon.reflect.code.CtTypeAccess;
-import spoon.reflect.code.CtTypePattern;
 import spoon.reflect.code.CtUnaryOperator;
 import spoon.reflect.code.CtVariableWrite;
 import spoon.reflect.code.LiteralBase;
@@ -52,15 +49,10 @@
 import spoon.reflect.factory.TypeFactory;
 import spoon.reflect.reference.CtExecutableReference;
 import spoon.reflect.reference.CtFieldReference;
-import spoon.reflect.reference.CtLocalVariableReference;
 import spoon.reflect.reference.CtReference;
 import spoon.reflect.reference.CtTypeParameterReference;
 import spoon.reflect.reference.CtTypeReference;
 import spoon.reflect.reference.CtVariableReference;
-import spoon.reflect.visitor.Filter;
-import spoon.reflect.visitor.filter.CompositeFilter;
-import spoon.reflect.visitor.filter.FilteringOperator;
-import spoon.reflect.visitor.filter.TypeFilter;
 
 import java.io.File;
 import java.util.ArrayDeque;
@@ -484,8 +476,6 @@ public static <T> CtBinaryOperator<T> swapCtBinaryOperator(CtBinaryOperator<T> c
     /**
      * Replaces {@link spoon.reflect.code.CtVariableRead} in the provided expression if they are effectively final
      * and their value is known.
-     * <p>
-     * Additionally, it will fix broken operators that do not have a type.
      *
      * @param ctExpression the expression to resolve. If it is {@code null}, then {@code null} is returned
      * @return the resolved expression. It will be cloned and detached from the {@link CtModel}
@@ -494,12 +484,9 @@ public static <T> CtBinaryOperator<T> swapCtBinaryOperator(CtBinaryOperator<T> c
     public static <T> CtExpression<T> resolveConstant(CtExpression<T> ctExpression) {
         if (ctExpression == null) return null;
 
-        PartialEvaluator evaluator = new Evaluator(
-            InferOperatorTypes.create(),
-            InlineVariableRead.create()
-        );
+        Evaluator evaluator = new Evaluator(InlineVariableRead.create(true));
 
-        return evaluator.evaluate(ctExpression);
+        return evaluator.evaluateUnsafe(ctExpression);
     }
 
     /**
@@ -517,8 +504,9 @@ public static <T> CtBinaryOperator<T> normalizeBy(
         BiPredicate<? super CtExpression<?>, ? super CtExpression<?>> shouldSwap,
         CtBinaryOperator<T> ctBinaryOperator
     ) {
-        CtExpression<?> left = SpoonUtil.resolveConstant(ctBinaryOperator.getLeftHandOperand());
-        CtExpression<?> right = SpoonUtil.resolveConstant(ctBinaryOperator.getRightHandOperand());
+        CtExpression<?> left = ctBinaryOperator.getLeftHandOperand();
+        CtExpression<?> right = ctBinaryOperator.getRightHandOperand();
+
         BinaryOperatorKind operator = ctBinaryOperator.getKind();
 
         CtBinaryOperator<T> result = ctBinaryOperator.clone();
@@ -738,15 +726,8 @@ public static boolean isSignatureEqualTo(
         return true;
     }
 
-    private record SubtypeFilter(CtTypeReference<?> ctTypeReference) implements Filter<CtType<?>> {
-        @Override
-        public boolean matches(CtType<?> element) {
-            return element != this.ctTypeReference.getTypeDeclaration() && element.isSubtypeOf(this.ctTypeReference);
-        }
-    }
-
     public static boolean hasSubtype(CtType<?> ctType) {
-        return ctType.getFactory().getModel().filterChildren(new SubtypeFilter(ctType.getReference())).first() != null;
+        return UsesFinder.subtypesOf(ctType, false).hasAny();
     }
 
     /**
@@ -1156,35 +1137,9 @@ public static CtVariable<?> getVariableDeclaration(CtVariableReference<?> ctVari
             target = ctFieldReference.getFieldDeclaration();
         }
 
-        if (target == null && ctVariableReference instanceof CtLocalVariableReference<?> ctLocalVariableReference) {
-            target = getLocalVariableDeclaration(ctLocalVariableReference);
-        }
-
         return target;
     }
 
-    @SuppressWarnings("unchecked")
-    private static <T> CtLocalVariable<T> getLocalVariableDeclaration(CtLocalVariableReference<T> ctLocalVariableReference) {
-        if (ctLocalVariableReference.getDeclaration() != null) {
-            return ctLocalVariableReference.getDeclaration();
-        }
-
-        // handle the special case, where we have an instanceof Pattern:
-        for (CtElement parent : parents(ctLocalVariableReference)) {
-            CtLocalVariable<?> candidate = parent.filterChildren(new TypeFilter<>(CtTypePattern.class)).filterChildren(new CompositeFilter<>(
-                FilteringOperator.INTERSECTION,
-                new TypeFilter<>(CtLocalVariable.class),
-                element -> element.getReference().equals(ctLocalVariableReference)
-            )).first();
-
-            if (candidate != null) {
-                return (CtLocalVariable<T>) candidate;
-            }
-        }
-
-        return null;
-    }
-
     private static <T> int referenceIndexOf(List<T> list, T element) {
         for (int i = 0; i < list.size(); i++) {
             if (list.get(i) == element) {

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

@@ -12,8 +12,10 @@
 import spoon.reflect.code.CtVariableAccess;
 import spoon.reflect.code.CtVariableRead;
 import spoon.reflect.code.CtVariableWrite;
+import spoon.reflect.declaration.CtClass;
 import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtExecutable;
+import spoon.reflect.declaration.CtInterface;
 import spoon.reflect.declaration.CtNamedElement;
 import spoon.reflect.declaration.CtType;
 import spoon.reflect.declaration.CtTypeParameter;
@@ -27,10 +29,18 @@
 import spoon.reflect.visitor.CtScanner;
 
 import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Deque;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.IdentityHashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
 import java.util.List;
+import java.util.Map;
+import java.util.SequencedSet;
 import java.util.Stack;
+import java.util.stream.Stream;
 
 /**
  * Provides usage-relationships between code elements in the code model.
@@ -108,6 +118,14 @@ public static CtElementStream<CtTypeReference<?>> typeUses(CtType<?> type) {
         return CtElementStream.of(UsesFinder.getFor(type).scanner.typeUses.getOrDefault(type, List.of())).assumeElementType();
     }
 
+    public static CtElementStream<CtType<?>> subtypesOf(CtType<?> type, boolean includeSelf) {
+        Stream<CtType<?>> selfStream = includeSelf ? Stream.of(type) : Stream.empty();
+        return CtElementStream.concat(
+            selfStream,
+            CtElementStream.of(UsesFinder.getFor(type).scanner.subtypes.getOrDefault(type, new LinkedHashSet<>())).assumeElementType()
+        );
+    }
+
     /**
      * 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.
@@ -120,6 +138,7 @@ private static class UsesScanner extends CtScanner {
         public final IdentityHashMap<CtTypeParameter, List<CtTypeParameterReference>> typeParameterUses = new IdentityHashMap<>();
         public final IdentityHashMap<CtExecutable, List<CtElement>> executableUses = new IdentityHashMap<>();
         public final IdentityHashMap<CtType, List<CtTypeReference>> typeUses = new IdentityHashMap<>();
+        public final Map<CtType, SequencedSet<CtType>> subtypes = 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
@@ -197,6 +216,28 @@ public <R> void visitCtBlock(CtBlock<R> block) {
             this.instanceofPatternVariables.pop();
         }
 
+        @Override
+        public <T> void visitCtClass(CtClass<T> ctClass) {
+            // CtType:
+            // - CtClass
+            // - CtEnum
+            // - CtInterface
+            // - CtRecord
+            // - CtTypeParameter
+            //
+            // CtClass:
+            // - CtEnum
+            // - CtRecord
+            this.recordCtType(ctClass);
+            super.visitCtClass(ctClass);
+        }
+
+        @Override
+        public <T> void visitCtInterface(CtInterface<T> ctInterface) {
+            this.recordCtType(ctInterface);
+            super.visitCtInterface(ctInterface);
+        }
+
         private void recordVariableAccess(CtVariableAccess<?> variableAccess) {
             var variable = variableAccess.getVariable().getDeclaration();
 
@@ -249,5 +290,29 @@ private void recordTypeReference(CtTypeReference reference) {
                 uses.add(reference);
             }
         }
+
+        @SuppressWarnings("unchecked")
+        private void recordCtType(CtType<?> ctType) {
+            CtTypeReference<?> superType = ctType.getSuperclass();
+            while (superType != null && superType.getTypeDeclaration() != null) {
+                this.subtypes.computeIfAbsent(superType.getTypeDeclaration(), k -> new LinkedHashSet<>()).add(ctType);
+                superType = superType.getSuperclass();
+            }
+
+            Collection<CtTypeReference> visited = new HashSet<>();
+            Deque<CtTypeReference> superInterfaces = new LinkedList<>(ctType.getSuperInterfaces());
+            while (!superInterfaces.isEmpty()) {
+                CtTypeReference superInterface = superInterfaces.poll();
+                // skip already visited interfaces
+                if (!visited.add(superInterface)) {
+                    continue;
+                }
+
+                if (superInterface.getTypeDeclaration() != null) {
+                    this.subtypes.computeIfAbsent(superInterface.getTypeDeclaration(), k -> new LinkedHashSet<>()).add(ctType);
+                }
+                superInterfaces.addAll(superInterface.getSuperInterfaces());
+            }
+        }
     }
 }

autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/Evaluator.java

@@ -38,7 +38,19 @@ public <R extends CtElement> R evaluate(R ctElement) {
         // the clone detaches the element from the model
         //
         // any modifications must not affect the model
-        CtElement result = ctElement.clone();
+        return this.evaluateUnsafe((R) ctElement.clone());
+    }
+
+    /**
+     * Works like {@link #evaluate(CtElement)} but does not clone the input element.
+     *
+     * @param ctElement the element to evaluate
+     * @return the evaluated element
+     * @param <R> the type of the element
+     */
+    @SuppressWarnings("unchecked")
+    public <R extends CtElement> R evaluateUnsafe(R ctElement) {
+        CtElement result = ctElement;
         this.root = result;
 
         result.accept(this);

autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/fold/InlineVariableRead.java

@@ -3,6 +3,7 @@
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import spoon.reflect.code.CtExpression;
 import spoon.reflect.code.CtLiteral;
+import spoon.reflect.code.CtLocalVariable;
 import spoon.reflect.code.CtVariableRead;
 import spoon.reflect.declaration.CtVariable;
 
@@ -12,19 +13,26 @@
  * Inline reads of constant variables with its value.
  */
 public final class InlineVariableRead implements Fold {
-    private InlineVariableRead() {
+    private final boolean ignoreLocalVariables;
+
+    private InlineVariableRead(boolean ignoreLocalVariables) {
+        this.ignoreLocalVariables = ignoreLocalVariables;
     }
 
     public static Fold create() {
-        return new InlineVariableRead();
+        return new InlineVariableRead(false);
+    }
+
+    public static Fold create(boolean ignoreLocalVariables) {
+        return new InlineVariableRead(ignoreLocalVariables);
     }
 
     @Override
     @SuppressWarnings("unchecked")
     public <T> CtExpression<T> foldCtVariableRead(CtVariableRead<T> ctVariableRead) {
         CtVariable<T> ctVariable = ctVariableRead.getVariable().getDeclaration();
 
-        if (ctVariable == null) {
+        if (ctVariable == null || this.ignoreLocalVariables && ctVariable instanceof CtLocalVariable<T>) {
             return ctVariableRead;
         }