Merge pull request #549 from Luro02/main

Implement copy-paste detection
Feuermagier · Jun 18, 2024 · 1f85b2d · 1f85b2d
2 parents e832fe7 + 631a937
commit 1f85b2d
Show file tree

Hide file tree

Showing 27 changed files with 1,523 additions and 255 deletions.
diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/Linter.java b/autograder-core/src/main/java/de/firemage/autograder/core/Linter.java
@@ -2,9 +2,6 @@
 
 import de.firemage.autograder.core.check.Check;
 import de.firemage.autograder.core.check.ExecutableCheck;
-import de.firemage.autograder.core.check.general.CopyPasteCheck;
-import de.firemage.autograder.core.check.general.MagicString;
-import de.firemage.autograder.core.cpd.CPDLinter;
 import de.firemage.autograder.core.errorprone.ErrorProneCheck;
 import de.firemage.autograder.core.errorprone.ErrorProneLinter;
 import de.firemage.autograder.core.errorprone.TempLocation;
@@ -36,7 +33,6 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.function.Consumer;
-import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
 public final class Linter {
@@ -155,15 +151,12 @@ public List<Problem> checkFile(
 
         List<PMDCheck> pmdChecks = new ArrayList<>();
         List<SpotbugsCheck> spotbugsChecks = new ArrayList<>();
-        List<CopyPasteCheck> cpdChecks = new ArrayList<>();
         List<IntegratedCheck> integratedChecks = new ArrayList<>();
         List<ErrorProneCheck> errorProneChecks = new ArrayList<>();
 
         for (Check check : checks) {
             if (check instanceof PMDCheck pmdCheck) {
                 pmdChecks.add(pmdCheck);
-            } else if (check instanceof CopyPasteCheck cpdCheck) {
-                cpdChecks.add(cpdCheck);
             } else if (check instanceof SpotbugsCheck spotbugsCheck) {
                 spotbugsChecks.add(spotbugsCheck);
             } else if (check instanceof IntegratedCheck integratedCheck) {
@@ -185,13 +178,6 @@ public List<Problem> checkFile(
             });
         }
 
-        if (!cpdChecks.isEmpty()) {
-            scheduler.submitTask((s, reporter) -> {
-                statusConsumer.accept(LinterStatus.RUNNING_CPD);
-                reporter.reportProblems(new CPDLinter().lint(file, cpdChecks));
-            });
-        }
-
         if (!spotbugsChecks.isEmpty()) {
             scheduler.submitTask((s, reporter) -> {
                 statusConsumer.accept(LinterStatus.RUNNING_SPOTBUGS);

diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/LinterStatus.java b/autograder-core/src/main/java/de/firemage/autograder/core/LinterStatus.java
@@ -4,7 +4,6 @@ public enum LinterStatus {
     COMPILING(new LocalizedMessage("status-compiling")),
     RUNNING_SPOTBUGS(new LocalizedMessage("status-spotbugs")),
     RUNNING_PMD(new LocalizedMessage("status-pmd")),
-    RUNNING_CPD(new LocalizedMessage("status-cpd")),
     RUNNING_ERROR_PRONE(new LocalizedMessage("status-error-prone")),
     BUILDING_CODE_MODEL(new LocalizedMessage("status-model")),
     RUNNING_INTEGRATED_CHECKS(new LocalizedMessage("status-integrated"));

diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java b/autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java
@@ -446,7 +446,7 @@ public enum ProblemType {
     /**
      * Reports duplicate code.
      * <br>
-     * There was a time when this check worked, now it is likely broken.
+     * Very experimental at the moment.
      */
     @HasFalsePositives
     DUPLICATE_CODE,

diff --git a/...ore/src/main/java/de/firemage/autograder/core/check/api/IsEmptyReimplementationCheck.java b/...ore/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)

diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RegexCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RegexCheck.java
@@ -6,6 +6,7 @@
 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 de.firemage.autograder.treeg.InvalidRegExSyntaxException;
 import de.firemage.autograder.treeg.RegExParser;
 import de.firemage.autograder.treeg.RegularExpression;
@@ -26,11 +27,9 @@
 import spoon.reflect.code.CtInvocation;
 import spoon.reflect.code.CtLiteral;
 import spoon.reflect.code.CtTypeAccess;
-import spoon.reflect.code.CtVariableAccess;
 import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtVariable;
 import spoon.reflect.reference.CtExecutableReference;
-import spoon.reflect.visitor.filter.VariableAccessFilter;
 
 import java.util.List;
 import java.util.Map;
@@ -77,12 +76,10 @@ private static boolean isInAllowedContext(CtLiteral<?> ctLiteral) {
         CtElement parent = ctLiteral.getParent();
         if (parent instanceof CtVariable<?> ctVariable
             && SpoonUtil.isEffectivelyFinal(ctVariable)) {
-            List<CtVariableAccess<?>> invocations = parent.getFactory().getModel().getElements(new VariableAccessFilter<>(ctVariable.getReference()));
-
-            return !invocations.isEmpty() &&
-                invocations
-                    .stream()
-                    .allMatch(ctVariableAccess -> ctVariableAccess.getParent() instanceof CtInvocation<?> ctInvocation && isRegexInvocation(ctInvocation));
+            // Check if the variable is only used in a regex invocation (e.g. Pattern.compile)
+            return UsesFinder.variableUses(ctVariable)
+                .hasAnyAndAllMatch(ctVariableAccess -> ctVariableAccess.getParent() instanceof CtInvocation<?> ctInvocation
+                    && isRegexInvocation(ctInvocation));
         }
 
         return parent instanceof CtInvocation<?> ctInvocation && isRegexInvocation(ctInvocation);

diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/UnusedImport.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/UnusedImport.java
@@ -1,6 +1,5 @@
 package de.firemage.autograder.core.check.complexity;
 
-import de.firemage.autograder.core.CodeModel;
 import de.firemage.autograder.core.LocalizedMessage;
 import de.firemage.autograder.core.ProblemType;
 import de.firemage.autograder.core.check.ExecutableCheck;
@@ -126,7 +125,7 @@ private boolean isInSamePackage(CtElement ctElement, CtCompilationUnit ctCompila
         return Objects.equals(declaredPackage, ctCompilationUnit.getDeclaredPackage());
     }
 
-    private void checkImport(CtImport ctImport, CtCompilationUnit ctCompilationUnit, Collection<? super CtElement> importedElements, CodeModel model) {
+    private void checkImport(CtImport ctImport, CtCompilationUnit ctCompilationUnit, Collection<? super CtElement> importedElements) {
         // check if the import is from the java.lang package, which is redundant
 
         // inner class imports might not be redundant, therefore, they are skipped here
@@ -220,7 +219,7 @@ protected void check(StaticAnalysis staticAnalysis) {
                     continue;
                 }
 
-                this.checkImport(ctImport, ctCompilationUnit, importedElements, staticAnalysis.getCodeModel());
+                this.checkImport(ctImport, ctCompilationUnit, importedElements);
             }
         });
     }

diff --git a/...r-core/src/main/java/de/firemage/autograder/core/check/general/AvoidRecompilingRegex.java b/...r-core/src/main/java/de/firemage/autograder/core/check/general/AvoidRecompilingRegex.java
@@ -6,14 +6,13 @@
 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.code.CtExpression;
 import spoon.reflect.code.CtInvocation;
 import spoon.reflect.code.CtLiteral;
 import spoon.reflect.code.CtTypeAccess;
 import spoon.reflect.declaration.CtField;
-import spoon.reflect.reference.CtReference;
-import spoon.reflect.visitor.filter.DirectReferenceFilter;
 
 import java.util.List;
 import java.util.Map;
@@ -45,19 +44,9 @@ public void process(CtField<String> ctField) {
                     return;
                 }
 
-                List<CtReference> references = staticAnalysis.getModel()
-                    .getElements(new DirectReferenceFilter<>(ctField.getReference()));
-
-                boolean isPattern = false;
-
-                for (CtReference ctReference : references) {
-                    CtInvocation<?> ctInvocation = ctReference.getParent(CtInvocation.class);
-                    if (ctInvocation == null || !isPatternInvocation(ctInvocation)) {
-                        return;
-                    }
-
-                    isPattern = true;
-                }
+                boolean isPattern = UsesFinder.variableUses(ctField)
+                    .hasAnyAndAllMatch(ctVariableAccess -> ctVariableAccess.getParent() instanceof CtInvocation<?> ctInvocation
+                        && isPatternInvocation(ctInvocation));
 
                 if (isPattern) {
                     addLocalProblem(

diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/CopyPasteCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/CopyPasteCheck.java
diff --git a/...der-core/src/main/java/de/firemage/autograder/core/check/oop/InheritanceBadPractices.java b/...der-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()) {

diff --git a/...rader-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java b/...rader-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java
@@ -294,7 +294,7 @@ private void checkCtExecutableReturn(CtExecutable<?> ctExecutable) {
             CtField<?> field = ctFieldRead.getVariable().getFieldDeclaration();
 
             // if the field is not private, it can be mutated anyway.
-            if (!field.isPrivate()) {
+            if (field == null || !field.isPrivate()) {
                 continue;
             }