From 623aab09ad9c6377d1e4cacd3d839dbe81afae50 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 1 Jun 2024 09:53:24 +0200 Subject: [PATCH 1/7] use the new `UsesFinder` in `AvoidRecompilingRegex` and `RegexCheck` to improve performance --- .../core/check/complexity/RegexCheck.java | 13 +++++-------- .../core/check/complexity/UnusedImport.java | 5 ++--- .../check/general/AvoidRecompilingRegex.java | 19 ++++--------------- .../core/integrated/CtElementStream.java | 14 ++++++++++++++ 4 files changed, 25 insertions(+), 26 deletions(-) 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 index 4e320f69..cdb0983f 100644 --- 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> 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 index 3acb4384..76143880 100644 --- 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 importedElements, CodeModel model) { + private void checkImport(CtImport ctImport, CtCompilationUnit ctCompilationUnit, Collection 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/autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidRecompilingRegex.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidRecompilingRegex.java index 1f1242ce..a48855ef 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidRecompilingRegex.java +++ b/autograder-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 ctField) { return; } - List 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/integrated/CtElementStream.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CtElementStream.java index 9c36398a..e9be8bc8 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CtElementStream.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CtElementStream.java @@ -168,6 +168,20 @@ public boolean hasAnyMatch(Predicate predicate) { return new CtElementStream<>(baseStream.filter(predicate)).hasAny(); } + /** + * Checks whether all elements in the stream match the given predicate AND the stream is not empty. + * + * @param predicate the predicate to test the elements against + * @return true if all elements match the predicate and the stream is not empty + */ + public boolean hasAnyAndAllMatch(Predicate predicate) { + boolean[] isEmpty = { true }; + return this.allMatch(element -> { + isEmpty[0] = false; + return predicate.test(element); + }) && !isEmpty[0]; + } + /** * Checks whether the stream contains no elements. * @return From 88ac7206df42a3baa5854f43f6aac0ef68f62258 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 8 Jun 2024 09:18:11 +0200 Subject: [PATCH 2/7] =?UTF-8?q?implement=20proof=20of=20concept=20for=20co?= =?UTF-8?q?py-paste=20detection=20=F0=9F=9A=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../firemage/autograder/core/ProblemType.java | 2 +- .../core/check/structure/DuplicateCode.java | 171 ++++++++++++++++++ .../autograder/core/integrated/SpoonUtil.java | 16 +- .../structure/StructuralElement.java | 26 +++ .../structure/StructuralEqualsVisitor.java | 106 +++++++++++ .../structure/StructuralHashCodeVisitor.java | 50 +++++ .../src/main/resources/strings.de.ftl | 2 +- .../src/main/resources/strings.en.ftl | 2 +- .../check/structure/TestDuplicateCode.java | 75 ++++++++ .../TestStructuralEqualsVisitor.java | 128 +++++++++++++ 10 files changed, 570 insertions(+), 8 deletions(-) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateCode.java create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralHashCodeVisitor.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateCode.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/integrated/structure/TestStructuralEqualsVisitor.java 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 index 80380d52..ea5ff16b 100644 --- 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. *
- * There was a time when this check worked, now it is likely broken. + * Very experimental at the moment. */ @HasFalsePositives DUPLICATE_CODE, diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateCode.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateCode.java new file mode 100644 index 00000000..d7db6eff --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateCode.java @@ -0,0 +1,171 @@ +package de.firemage.autograder.core.check.structure; + +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.SpoonUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.structure.StructuralElement; +import de.firemage.autograder.core.integrated.structure.StructuralEqualsVisitor; +import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtComment; +import spoon.reflect.code.CtStatement; +import spoon.reflect.code.CtStatementList; +import spoon.reflect.cu.SourcePosition; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtMethod; +import spoon.reflect.visitor.CtScanner; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Set; + +@ExecutableCheck(reportedProblems = { ProblemType.DUPLICATE_CODE }) +public class DuplicateCode extends IntegratedCheck { + private static final int MINIMUM_DUPLICATE_STATEMENT_SIZE = 10; + + private static String formatSourceRange(CtElement start, CtElement end) { + SourcePosition startPosition = start.getPosition(); + SourcePosition endPosition = end.getPosition(); + + return String.format( + "%s:%d-%d", + SpoonUtil.getBaseName(startPosition.getFile().getName()), + startPosition.getLine(), + endPosition.getEndLine() + ); + } + + private static int countStatements(CtStatement ctStatement) { + int count = ctStatement.getElements(ctElement -> ctElement instanceof CtStatement + && !(ctElement instanceof CtComment) + && !(ctElement instanceof CtStatementList) + && ctElement.getPosition().isValidPosition() + && !ctElement.isImplicit() + ).size(); + + return Math.max(count, 1); + } + + private static Iterable> zip(Iterable keys, Iterable values) { + return () -> new Iterator<>() { + private final Iterator keyIterator = keys.iterator(); + private final Iterator valueIterator = values.iterator(); + + @Override + public boolean hasNext() { + return this.keyIterator.hasNext() && this.valueIterator.hasNext(); + } + + @Override + public Map.Entry next() { + return Map.entry(this.keyIterator.next(), this.valueIterator.next()); + } + }; + } + + @Override + protected void check(StaticAnalysis staticAnalysis) { + Map, List> occurrences = new HashMap<>(); + staticAnalysis.getModel().processWith(new AbstractProcessor() { + @Override + public void process(CtStatement ctStatement) { + if (ctStatement.isImplicit() || !ctStatement.getPosition().isValidPosition()) { + return; + } + + occurrences.computeIfAbsent( + new StructuralElement<>(ctStatement), + key -> new ArrayList<>() + ).add(ctStatement); + } + }); + + /* + Map> collisions = new HashMap<>(); + for (var key : occurrences.keySet()) { + collisions.computeIfAbsent(key.hashCode(), k -> new ArrayList<>()).add(key); + } + + System.out.println("Number of duplicate hashCodes: " + (occurrences.size() - collisions.size()) + " of " + occurrences.size() + " elements");*/ + + Set reported = new HashSet<>(); + staticAnalysis.getModel().getRootPackage().accept(new CtScanner() { + private void checkCtStatement(CtStatement ctStatement) { + if (ctStatement.isImplicit() || !ctStatement.getPosition().isValidPosition()) { + return; + } + + // TODO: use a debug mode to compare that this implementation yields the same results as the occurrences map + /*List duplicates = ctStatement.getFactory() + .getModel() + .filterChildren(ctElement -> ctElement instanceof CtStatement otherStatement + && otherStatement != ctStatement + && StructuralEqualsVisitor.equals(ctStatement, otherStatement) + ).list(CtStatement.class);*/ + List duplicates = occurrences.get(new StructuralElement<>(ctStatement)); + + int initialSize = countStatements(ctStatement); + for (CtStatement duplicate : duplicates) { + if (duplicate == ctStatement || reported.contains(duplicate) || reported.contains(ctStatement)) { + continue; + } + + int duplicateStatementSize = initialSize; + + List leftCode = new ArrayList<>(List.of(ctStatement)); + List rightCode = new ArrayList<>(List.of(duplicate)); + + for (var entry : zip(SpoonUtil.getNextStatements(ctStatement), SpoonUtil.getNextStatements(duplicate))) { + if (!StructuralEqualsVisitor.equals(entry.getKey(), entry.getValue())) { + break; + } + + leftCode.add(entry.getKey()); + rightCode.add(entry.getValue()); + duplicateStatementSize += countStatements(entry.getKey()); + } + + if (duplicateStatementSize >= MINIMUM_DUPLICATE_STATEMENT_SIZE) { + // prevent duplicate reporting of the same code segments + reported.addAll(leftCode); + reported.addAll(rightCode); + + addLocalProblem( + ctStatement, + new LocalizedMessage( + "duplicate-code", + Map.of( + "left", formatSourceRange(leftCode.getFirst(), leftCode.getLast()), + "right", formatSourceRange(rightCode.getFirst(), rightCode.getLast()) + ) + ), + ProblemType.DUPLICATE_CODE + ); + + break; + } + } + } + + @Override + public void visitCtMethod(CtMethod ctMethod) { + if (ctMethod.isImplicit() || !ctMethod.getPosition().isValidPosition() || ctMethod.getBody() == null) { + super.visitCtMethod(ctMethod); + return; + } + + for (CtStatement ctStatement : SpoonUtil.getEffectiveStatements(ctMethod.getBody())) { + this.checkCtStatement(ctStatement); + } + + super.visitCtMethod(ctMethod); + } + }); + } +} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java index 46072116..0a7b828f 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java @@ -1202,16 +1202,22 @@ private static int referenceIndexOf(List list, T element) { * @return the previous statement or an empty optional if there is no previous statement */ public static Optional getPreviousStatement(CtStatement ctStatement) { + List previousStatements = getPreviousStatements(ctStatement); + return previousStatements.isEmpty() ? Optional.empty() : Optional.of(previousStatements.getLast()); + } + + public static List getPreviousStatements(CtStatement ctStatement) { + List result = new ArrayList<>(); if (ctStatement.getParent() instanceof CtStatementList ctStatementList) { List statements = ctStatementList.getStatements(); int index = referenceIndexOf(statements, ctStatement); - if (index > 0) { - return Optional.of(statements.get(index - 1)); + if (index >= 0) { + result.addAll(statements.subList(0, index)); } } - return Optional.empty(); + return result; } public static List getNextStatements(CtStatement ctStatement) { @@ -1220,7 +1226,7 @@ public static List getNextStatements(CtStatement ctStatement) { List statements = ctStatementList.getStatements(); int index = referenceIndexOf(statements, ctStatement); - if (index > 0) { + if (index >= 0) { result.addAll(statements.subList(index + 1, statements.size())); } } @@ -1329,7 +1335,7 @@ public static String formatSourcePosition(SourcePosition sourcePosition) { return String.format("%s:L%d", getBaseName(sourcePosition.getFile().getName()), sourcePosition.getLine()); } - private static String getBaseName(String fileName) { + public static String getBaseName(String fileName) { if (fileName == null) { return null; } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java new file mode 100644 index 00000000..00186d80 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java @@ -0,0 +1,26 @@ +package de.firemage.autograder.core.integrated.structure; + +import spoon.reflect.declaration.CtElement; + +public record StructuralElement(T element) { + public static StructuralElement of(T element) { + return new StructuralElement<>(element); + } + + @Override + public boolean equals(Object otherObject) { + if (this == otherObject) { + return true; + } + if (!(otherObject instanceof StructuralElement(var otherElement))) { + return false; + } + + return StructuralEqualsVisitor.equals(this.element, otherElement); + } + + @Override + public int hashCode() { + return StructuralHashCodeVisitor.computeHashCode(this.element); + } +} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java new file mode 100644 index 00000000..99a6a735 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java @@ -0,0 +1,106 @@ +package de.firemage.autograder.core.integrated.structure; + +import de.firemage.autograder.core.integrated.SpoonUtil; +import spoon.reflect.code.CtExpression; +import spoon.reflect.code.CtLocalVariable; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtField; +import spoon.reflect.declaration.CtParameter; +import spoon.reflect.path.CtRole; +import spoon.support.visitor.equals.EqualsVisitor; + +import java.util.Set; + +public final class StructuralEqualsVisitor extends EqualsVisitor { + private static final boolean IS_IN_DEBUG_MODE = SpoonUtil.isInJunitTest(); + + private static final Set ALLOWED_MISMATCHING_ROLES = Set.of( + // allow mismatching comments + CtRole.COMMENT, CtRole.COMMENT_CONTENT, CtRole.COMMENT_TAG, CtRole.COMMENT_TYPE + ); + + public StructuralEqualsVisitor() { + } + + public static boolean equals(CtElement left, CtElement right) { + return new StructuralEqualsVisitor().checkEquals(left, right); + } + + @Override + public boolean checkEquals(CtElement left, CtElement right) { + if (IS_IN_DEBUG_MODE) { + boolean result = super.checkEquals(left, right); + int leftHashCode = StructuralHashCodeVisitor.computeHashCode(left); + int rightHashCode = StructuralHashCodeVisitor.computeHashCode(right); + + // If two objects are equal according to the equals(Object) method, then calling + // the hashCode method on each of the two objects must produce the same integer result. + if (result && leftHashCode != rightHashCode) { + throw new IllegalStateException("StructuralHashCode is wrong for the equal objects: %s (hashCode=%d), %s (hashCode=%d)".formatted(left, leftHashCode, right, rightHashCode)); + } + + return result; + } + + return super.checkEquals(left, right); + } + + private static boolean isRefactorable(Object element) { + if (!(element instanceof CtElement)) { + return false; + } + + if (element instanceof CtExpression ctExpression) { + // TODO: check for constant expression or simple variable access? + return true; + } + + return false; + } + + private static boolean isInstanceOfAny(Object object, Class... classes) { + for (Class clazz : classes) { + if (clazz.isInstance(object)) { + return true; + } + } + return false; + } + + public static boolean shouldSkip(CtRole role, Object element) { + if (role == null) { + return false; + } + + if (ALLOWED_MISMATCHING_ROLES.contains(role)) { + return true; + } + + // TODO: element can be collections??? + + if (role == CtRole.NAME && isInstanceOfAny(element, CtLocalVariable.class, CtField.class, CtParameter.class)) { + return true; + } + + // TODO: LEFT_OPERAND as well? + if ((role == CtRole.RIGHT_OPERAND || role == CtRole.LEFT_OPERAND) && isRefactorable(element)) { + return true; + } + + return false; + } + + @Override + protected boolean fail(CtRole role, Object element, Object other) { + if (shouldSkip(role, element)) { + return false; + } + + isNotEqual = true; + notEqualRole = role; + notEqualElement = element; + notEqualOther = other; + + return true; + } +} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralHashCodeVisitor.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralHashCodeVisitor.java new file mode 100644 index 00000000..83abde66 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralHashCodeVisitor.java @@ -0,0 +1,50 @@ +package de.firemage.autograder.core.integrated.structure; + +import org.apache.commons.lang3.builder.HashCodeBuilder; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.path.CtRole; +import spoon.reflect.reference.CtTypeReference; +import spoon.reflect.visitor.CtScanner; + +/** + * A visitor that computes a hash code for a spoon element based on its structure. + *
+ * The default hashCode implementation in spoon does not ignore comments or naming of things + * and to be compatible with the {@link StructuralEqualsVisitor} a different hashCode implementation is needed. + * The hash code visitor tries to be as generic as possible, but with a low number of collisions. + */ +public final class StructuralHashCodeVisitor extends CtScanner { + private final HashCodeBuilder builder; + + private StructuralHashCodeVisitor() { + this.builder = new HashCodeBuilder(); + } + + public static int computeHashCode(CtElement element) { + StructuralHashCodeVisitor visitor = new StructuralHashCodeVisitor(); + visitor.scan(element); + return visitor.getHashCode(); + } + + @Override + public void enter(CtElement ctElement) { + if (ctElement instanceof CtTypeReference ctTypeReference) { + this.builder.append(ctTypeReference.getSimpleName()); + } + + this.builder.append(ctElement.getClass().getSimpleName().hashCode()); + } + + @Override + public void scan(CtRole ctRole, CtElement element) { + if (StructuralEqualsVisitor.shouldSkip(ctRole, element)) { + return; + } + this.builder.append(ctRole); + super.scan(ctRole, element); + } + + public int getHashCode() { + return this.builder.toHashCode(); + } +} diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index 668d3f1e..05895822 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -17,7 +17,7 @@ linter-error-prone = error-prone merged-problems = {$message} Weitere Probleme in {$locations}. # CPD -duplicate-code = Duplizierter Code ({$lines}): {$first-path}:{$first-start}-{$first-end} und {$second-path}:{$second-start}-{$second-end} +duplicate-code = Duplizierter Code: {$left} und {$right}. # API diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl index 64c61d6a..6111c269 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -17,7 +17,7 @@ linter-error-prone = error-prone merged-problems = {$message} Other problems in {$locations}. # CPD -duplicate-code = Duplicated code ({$lines}): {$first-path}:{$first-start}-{$first-end} and {$second-path}:{$second-start}-{$second-end} +duplicate-code = Duplicate code: {$left} and {$right}. # API diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateCode.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateCode.java new file mode 100644 index 00000000..2c92f1c6 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateCode.java @@ -0,0 +1,75 @@ +package de.firemage.autograder.core.check.structure; + +import de.firemage.autograder.core.LinterException; +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.Problem; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.AbstractCheckTest; +import de.firemage.autograder.core.compiler.JavaVersion; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestDuplicateCode extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.DUPLICATE_CODE); + + void assertDuplicateCode(Problem problem, String left, String right) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage( + "duplicate-code", + Map.of( + "left", left, + "right", right + ) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testDuplicateDifferingNonConstantExpression() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "StringUtils", + """ + public class StringUtils { + public static String ceasarEncryption(String input, int key) { + char[] inputArray = input.toCharArray(); + for (int i = 0; i < inputArray.length; i++) { + if (Character.isUpperCase(inputArray[i])) { + inputArray[i] = (char) (((inputArray[i] - 65 + key) % 26) + 65); + } else { + inputArray[i] = (char) (((inputArray[i] - 97 + key) % 26) + 97); + } + } + return new String(inputArray); + } + + public static String ceasarDecryption(String input, int key) { + char[] inputArray = input.toCharArray(); + for (int i = 0; i < inputArray.length; i++) { + if (Character.isUpperCase(inputArray[i])) { + inputArray[i] = (char) (((inputArray[i] - 65 + (26 - key)) % 26) + 65); + } else { + inputArray[i] = (char) (((inputArray[i] - 97 + (26 - key)) % 26) + 97); + } + } + return new String(inputArray); + } + } + // the two methods are mostly identical except for two assigned values + // the decryption has a `26 - key` while the encryption has `key` + // key is a param, so the new utility method could have an extra param that applies that shift + """ + ), PROBLEM_TYPES); + + assertDuplicateCode(problems.next(), "StringUtils:3-11", "StringUtils:15-23"); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/integrated/structure/TestStructuralEqualsVisitor.java b/autograder-core/src/test/java/de/firemage/autograder/core/integrated/structure/TestStructuralEqualsVisitor.java new file mode 100644 index 00000000..8a367af9 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/integrated/structure/TestStructuralEqualsVisitor.java @@ -0,0 +1,128 @@ +package de.firemage.autograder.core.integrated.structure; + +import de.firemage.autograder.core.compiler.JavaVersion; +import de.firemage.autograder.core.errorprone.TempLocation; +import de.firemage.autograder.core.file.StringSourceInfo; +import de.firemage.autograder.core.file.UploadedFile; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import spoon.reflect.CtModel; +import spoon.reflect.code.CtStatement; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtMethod; +import spoon.reflect.path.CtRole; + +import java.util.ArrayList; + +import static org.junit.jupiter.api.Assertions.*; + +class TestStructuralEqualsVisitor { + @SuppressWarnings("unchecked") + private static T createStatement(String statement, String arguments) { + if (arguments == null) { + arguments = ""; + } + + UploadedFile file; + try { + file = UploadedFile.build(StringSourceInfo.fromSourceString(JavaVersion.JAVA_17, "Test", "public class Test { void t(%s) { %s; } }".formatted( + arguments, + statement + )), TempLocation.random(), y -> {}, null); + } catch (Exception e) { + throw new IllegalStateException(e); + } + + CtModel ctModel = file.getModel().getModel(); + + CtMethod ctMethod = new ArrayList<>(ctModel.getAllTypes()).get(0).getMethodsByName("t").get(0); + + return (T) ctMethod.getBody().getStatements().get(0); + } + + private static CtRole parseMismatch(String mismatch) { + return mismatch == null || mismatch.isBlank() ? null : CtRole.valueOf(mismatch); + } + + private static void checkStructurallyEqual(CtElement left, CtElement right, CtRole mismatch) { + StructuralEqualsVisitor visitor = new StructuralEqualsVisitor(); + boolean isEqual = visitor.checkEquals(left, right); + + if (mismatch == null) { + assertTrue(isEqual, "\"%s\" != \"%s\". Mismatch (%s): left: %s, right: %s".formatted( + left, + right, + visitor.getNotEqualRole(), + visitor.getNotEqualElement(), + visitor.getNotEqualOther() + )); + } else { + assertFalse(isEqual, "\"%s\" == \"%s\"".formatted(left, right)); + assertEquals(mismatch, visitor.getNotEqualRole()); + } + } + + private static void assertStructurallyEqual(String arguments, String left, String right, CtRole mismatch) { + CtStatement leftStatement = createStatement(left, arguments); + CtStatement rightStatement = createStatement(right, arguments); + + // for equality the following must hold: + // 1. if left == right, then right == left + // 2. if left == other and left == right, then right == other + // 3. left == left and right == right + + // we only test 1 and 3 here + + checkStructurallyEqual(leftStatement, rightStatement, mismatch); + checkStructurallyEqual(rightStatement, leftStatement, mismatch); + + checkStructurallyEqual(leftStatement, leftStatement, null); + checkStructurallyEqual(rightStatement, rightStatement, null); + + } + + @ParameterizedTest + @CsvSource( + delimiter = '|', + useHeadersInDisplayName = true, + value = { + " Arguments | Left | Right | Mismatch ", + " int a | /**/ a = 1 | /**/ a = 1 | ", + " int a | /**/ a = 1 | a = 1 | ", + " int a | /* a */ a = 1 | /* b */ a = 1 | ", + } + ) + void testIgnoresComments(String arguments, String left, String right, String mismatch) { + assertStructurallyEqual(arguments, left, right, parseMismatch(mismatch)); + } + + @ParameterizedTest + @CsvSource( + delimiter = '|', + useHeadersInDisplayName = true, + value = { + " Arguments | Left | Right | Mismatch ", + " int a, int b | a = 1 | b = 1 | NAME ", + " | int a; | int b; | ", + " | record Position(int x, int y) {} | record Position(int horizontal, int vertical) {} | NAME ", + " | record Position(int x, int y) {} | record Coordinate(int horizontal, int vertical) {} | NAME ", + " | class Position { int x; int y; } | class Position { int horizontal; int vertical; } | ", + } + ) + void testDifferentlyNamedVariables(String arguments, String left, String right, String mismatch) { + assertStructurallyEqual(arguments, left, right, parseMismatch(mismatch)); + } + + @ParameterizedTest + @CsvSource( + delimiter = '|', + useHeadersInDisplayName = true, + value = { + " Arguments | Left | Right | Mismatch ", + " char i, int key | char res = (char) (((i - 65 + (26 - key)) % 26) + 65); | char res = (char) (((i - 65 + key) % 26) + 65); | ", + } + ) + void testBinaryOperatorConstExpr(String arguments, String left, String right, String mismatch) { + assertStructurallyEqual(arguments, left, right, parseMismatch(mismatch)); + } +} From 36b09a48b9d4b4ecbd9fe5ff0f764fd6415f5e18 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sun, 9 Jun 2024 17:08:51 +0200 Subject: [PATCH 3/7] improve copy-paste detection and fix some bugs --- .../api/IsEmptyReimplementationCheck.java | 10 +- .../check/oop/InheritanceBadPractices.java | 17 ++- .../autograder/core/integrated/SpoonUtil.java | 57 +------- .../core/integrated/UsesFinder.java | 65 +++++++++ .../core/integrated/evaluator/Evaluator.java | 14 +- .../evaluator/fold/InlineVariableRead.java | 14 +- .../structure/StructuralEqualsVisitor.java | 134 ++++++++++++++++-- 7 files changed, 244 insertions(+), 67 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/IsEmptyReimplementationCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/IsEmptyReimplementationCheck.java index 80837bdf..fafe50d1 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/IsEmptyReimplementationCheck.java +++ b/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) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/InheritanceBadPractices.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/InheritanceBadPractices.java index f43c0635..6c264546 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/InheritanceBadPractices.java +++ b/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>() { @@ -38,8 +42,17 @@ public void process(CtClass ctClass) { List> fields = ctClass.getFields(); Set> methods = ctClass.getMethods(); - List> subtypes = staticAnalysis.getModel() - .getElements(new SubtypeFilter(ctClass.getReference()).includingSelf(false)); + List> 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> 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/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java index 0a7b828f..b05eeafa 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java +++ b/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 CtBinaryOperator swapCtBinaryOperator(CtBinaryOperator c /** * Replaces {@link spoon.reflect.code.CtVariableRead} in the provided expression if they are effectively final * and their value is known. - *

- * 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 CtBinaryOperator swapCtBinaryOperator(CtBinaryOperator c public static CtExpression resolveConstant(CtExpression 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 CtBinaryOperator normalizeBy( BiPredicate, ? super CtExpression> shouldSwap, CtBinaryOperator 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 result = ctBinaryOperator.clone(); @@ -738,15 +726,8 @@ public static boolean isSignatureEqualTo( return true; } - private record SubtypeFilter(CtTypeReference ctTypeReference) implements Filter> { - @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 CtLocalVariable getLocalVariableDeclaration(CtLocalVariableReference 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) candidate; - } - } - - return null; - } - private static int referenceIndexOf(List list, T element) { for (int i = 0; i < list.size(); i++) { if (list.get(i) == element) { diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java index c1739a22..a6da2937 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java +++ b/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> typeUses(CtType type) { return CtElementStream.of(UsesFinder.getFor(type).scanner.typeUses.getOrDefault(type, List.of())).assumeElementType(); } + public static CtElementStream> subtypesOf(CtType type, boolean includeSelf) { + Stream> 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> typeParameterUses = new IdentityHashMap<>(); public final IdentityHashMap> executableUses = new IdentityHashMap<>(); public final IdentityHashMap> typeUses = new IdentityHashMap<>(); + public final Map> 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 void visitCtBlock(CtBlock block) { this.instanceofPatternVariables.pop(); } + @Override + public void visitCtClass(CtClass ctClass) { + // CtType: + // - CtClass + // - CtEnum + // - CtInterface + // - CtRecord + // - CtTypeParameter + // + // CtClass: + // - CtEnum + // - CtRecord + this.recordCtType(ctClass); + super.visitCtClass(ctClass); + } + + @Override + public void visitCtInterface(CtInterface 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 visited = new HashSet<>(); + Deque 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()); + } + } } } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/Evaluator.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/Evaluator.java index 5588b7c6..67a81638 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/Evaluator.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/Evaluator.java @@ -38,7 +38,19 @@ public 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 the type of the element + */ + @SuppressWarnings("unchecked") + public R evaluateUnsafe(R ctElement) { + CtElement result = ctElement; this.root = result; result.accept(this); diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/fold/InlineVariableRead.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/fold/InlineVariableRead.java index 12eea5bd..fc608681 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/evaluator/fold/InlineVariableRead.java +++ b/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,11 +13,18 @@ * 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 @@ -24,7 +32,7 @@ public static Fold create() { public CtExpression foldCtVariableRead(CtVariableRead ctVariableRead) { CtVariable ctVariable = ctVariableRead.getVariable().getDeclaration(); - if (ctVariable == null) { + if (ctVariable == null || this.ignoreLocalVariables && ctVariable instanceof CtLocalVariable) { return ctVariableRead; } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java index 99a6a735..7691af59 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java @@ -1,15 +1,28 @@ package de.firemage.autograder.core.integrated.structure; import de.firemage.autograder.core.integrated.SpoonUtil; +import spoon.reflect.code.CtBinaryOperator; +import spoon.reflect.code.CtConditional; import spoon.reflect.code.CtExpression; +import spoon.reflect.code.CtInvocation; +import spoon.reflect.code.CtLiteral; import spoon.reflect.code.CtLocalVariable; +import spoon.reflect.code.CtTextBlock; +import spoon.reflect.code.CtTypeAccess; +import spoon.reflect.code.CtVariableRead; import spoon.reflect.declaration.CtElement; import spoon.reflect.declaration.CtField; import spoon.reflect.declaration.CtParameter; +import spoon.reflect.declaration.CtVariable; import spoon.reflect.path.CtRole; +import spoon.reflect.visitor.CtScanner; import spoon.support.visitor.equals.EqualsVisitor; +import java.util.LinkedHashSet; +import java.util.SequencedSet; import java.util.Set; +import java.util.function.Predicate; +import java.util.function.UnaryOperator; public final class StructuralEqualsVisitor extends EqualsVisitor { private static final boolean IS_IN_DEBUG_MODE = SpoonUtil.isInJunitTest(); @@ -19,7 +32,12 @@ public final class StructuralEqualsVisitor extends EqualsVisitor { CtRole.COMMENT, CtRole.COMMENT_CONTENT, CtRole.COMMENT_TAG, CtRole.COMMENT_TYPE ); + private final SequencedSet differences; + + public record Difference(CtRole role, Object left, Object right) {} + public StructuralEqualsVisitor() { + this.differences = new LinkedHashSet<>(); } public static boolean equals(CtElement left, CtElement right) { @@ -45,14 +63,104 @@ public boolean checkEquals(CtElement left, CtElement right) { return super.checkEquals(left, right); } + private static boolean isConstantExpressionOr(CtExpression e, Predicate> isAllowedExpression) { + var visitor = new CtScanner() { + private boolean isConstant = false; + private boolean isDone = false; + + // use the exit instead of the enter method, so it checks the deepest nodes first. + // This way, one can assume that when a expression is encountered, + // the and are already guaranteed to be constant. + @Override + protected void exit(CtElement ctElement) { + if (!(ctElement instanceof CtExpression expression) || this.isDone) { + return; + } + + // Of all the Subinterfaces of CtExpression, these are irrelevant: + // - CtAnnotation + // - CtAnnotationFieldAccess + // - CtOperatorAssignment + // - CtArrayWrite + // - CtArrayAccess + // - CtFieldWrite + // - CtFieldAccess + // - CtVariableAccess + // - CtVariableWrite + // - CtCodeSnippetExpression (should never be encountered) + // + // these might be constant expressions, depending on more context: + // - CtArrayRead + // - CtAssignment + // - CtConstructorCall + // - CtExecutableReferenceExpression + // - CtLambda + // - CtNewArray + // - CtNewClass + // - CtSuperAccess + // - CtSwitchExpression + // - CtTargetedExpression + // - CtThisAccess + // - CtTypePattern + // + // and these are the relevant ones: + // - CtLiteral + // - CtUnaryOperator + // - CtBinaryOperator + // - CtFieldRead (if the field itself is static and final) + // - CtInvocation (if the method is static and all arguments are constant expressions) + // - CtTextBlock + // - CtConditional (ternary operator) + // - CtTypeAccess + + if (isInstanceOfAny( + expression, + CtBinaryOperator.class, + UnaryOperator.class, + CtTextBlock.class, + CtConditional.class, + CtTypeAccess.class + )) { + this.isConstant = true; + return; + } + + if (expression instanceof CtInvocation ctInvocation && ctInvocation.getExecutable().isStatic()) { + // the arguments and target of the invocation have already been visited and all of them work in a constant context + // -> the invocation would work in a constant context as well + this.isConstant = true; + return; + } + + if (SpoonUtil.resolveConstant(expression) instanceof CtLiteral || isAllowedExpression.test(expression)) { + this.isConstant = true; + } else { + this.isConstant = false; + this.isDone = true; + } + } + }; + + e.accept(visitor); + + return visitor.isConstant; + } + private static boolean isRefactorable(Object element) { if (!(element instanceof CtElement)) { return false; } if (element instanceof CtExpression ctExpression) { - // TODO: check for constant expression or simple variable access? - return true; + return isConstantExpressionOr(ctExpression, e -> { + // in addition to constant expressions, it is also okay to access parameters (if they have not been modified in the method) + if (e instanceof CtVariableRead ctVariableRead) { + CtVariable ctVariable = SpoonUtil.getVariableDeclaration(ctVariableRead.getVariable()); + return ctVariable instanceof CtParameter ctParameter && SpoonUtil.isEffectivelyFinal(ctParameter); + } + + return false; + }); } return false; @@ -82,8 +190,7 @@ public static boolean shouldSkip(CtRole role, Object element) { return true; } - // TODO: LEFT_OPERAND as well? - if ((role == CtRole.RIGHT_OPERAND || role == CtRole.LEFT_OPERAND) && isRefactorable(element)) { + if ((role == CtRole.LEFT_OPERAND || role == CtRole.RIGHT_OPERAND) && isRefactorable(element)) { return true; } @@ -92,15 +199,26 @@ public static boolean shouldSkip(CtRole role, Object element) { @Override protected boolean fail(CtRole role, Object element, Object other) { + this.differences.add(new Difference(role, element, other)); + if (shouldSkip(role, element)) { return false; } - isNotEqual = true; - notEqualRole = role; - notEqualElement = element; - notEqualOther = other; + this.isNotEqual = true; + this.notEqualRole = role; + this.notEqualElement = element; + this.notEqualOther = other; return true; } + + /** + * Returns the differences between the two elements. + * + * @return the differences + */ + public SequencedSet differences() { + return new LinkedHashSet<>(this.differences); + } } From e50eed43b322762ffa9e035f50a1b823f1b9ee03 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 18 Jun 2024 12:42:23 +0200 Subject: [PATCH 4/7] improve copy-paste detection --- .../core/check/structure/DuplicateCode.java | 166 +++++- .../core/integrated/CtElementStream.java | 24 + .../autograder/core/integrated/SpoonUtil.java | 28 +- .../core/integrated/UsesFinder.java | 52 +- .../structure/StructuralEqualsVisitor.java | 2 +- .../check/structure/TestDuplicateCode.java | 489 ++++++++++++++++++ 6 files changed, 711 insertions(+), 50 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateCode.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateCode.java index d7db6eff..5186bdd2 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateCode.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateCode.java @@ -6,24 +6,38 @@ 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.core.integrated.structure.StructuralElement; import de.firemage.autograder.core.integrated.structure.StructuralEqualsVisitor; import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtAssignment; import spoon.reflect.code.CtComment; import spoon.reflect.code.CtStatement; import spoon.reflect.code.CtStatementList; +import spoon.reflect.code.CtVariableAccess; +import spoon.reflect.code.CtVariableWrite; import spoon.reflect.cu.SourcePosition; import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtField; import spoon.reflect.declaration.CtMethod; +import spoon.reflect.declaration.CtVariable; import spoon.reflect.visitor.CtScanner; +import spoon.reflect.visitor.filter.TypeFilter; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.SequencedSet; import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; @ExecutableCheck(reportedProblems = { ProblemType.DUPLICATE_CODE }) public class DuplicateCode extends IntegratedCheck { @@ -69,6 +83,85 @@ public Map.Entry next() { }; } + private record CodeSegment(List statements) implements Iterable { + public CodeSegment { + statements = new ArrayList<>(statements); + } + + public static CodeSegment of(CtStatement... statement) { + return new CodeSegment(Arrays.asList(statement)); + } + + public void add(CtStatement ctStatement) { + this.statements.add(ctStatement); + } + + public CtStatement getFirst() { + return this.statements.getFirst(); + } + + public CtStatement getLast() { + return this.statements.getLast(); + } + + public List statements() { + return new ArrayList<>(this.statements); + } + + @Override + public Iterator iterator() { + return this.statements().iterator(); + } + + private SequencedSet> declaredVariables() { + SequencedSet> declaredVariables = new LinkedHashSet<>(); + + for (CtStatement ctStatement : this) { + if (ctStatement instanceof CtVariable ctVariable) { + declaredVariables.add(ctVariable); + } + } + + return declaredVariables; + } + + public int countExposedVariables() { + Set> declaredVariables = this.declaredVariables(); + if (declaredVariables.isEmpty()) { + return 0; + } + + int count = 0; + for (CtStatement ctStatement : SpoonUtil.getNextStatements(this.getLast())) { + for (CtVariable declaredVariable : declaredVariables) { + if (UsesFinder.variableUses(declaredVariable).nestedIn(ctStatement).hasAny()) { + count += 1; + } + } + } + return count; + } + + public int countDependencies(Predicate> isDependency, Predicate> isDependencyAccess) { + if (this.statements().isEmpty()) { + return 0; + } + + Set> codeSegmentVariables = this.statements.stream() + .flatMap(ctStatement -> ctStatement.getElements(new TypeFilter>(CtVariable.class)).stream()) + .collect(Collectors.toCollection(() -> Collections.newSetFromMap(new IdentityHashMap<>()))); + + return (int) this.statements.stream() + .flatMap(ctStatement -> ctStatement.getElements(new TypeFilter>(CtVariableAccess.class)).stream()) + .filter(isDependencyAccess) + .map(UsesFinder::getDeclaredVariable) + .unordered() + .distinct() + .filter(ctVariable -> !codeSegmentVariables.contains(ctVariable) && isDependency.test(ctVariable)) + .count(); + } + } + @Override protected void check(StaticAnalysis staticAnalysis) { Map, List> occurrences = new HashMap<>(); @@ -101,13 +194,6 @@ private void checkCtStatement(CtStatement ctStatement) { return; } - // TODO: use a debug mode to compare that this implementation yields the same results as the occurrences map - /*List duplicates = ctStatement.getFactory() - .getModel() - .filterChildren(ctElement -> ctElement instanceof CtStatement otherStatement - && otherStatement != ctStatement - && StructuralEqualsVisitor.equals(ctStatement, otherStatement) - ).list(CtStatement.class);*/ List duplicates = occurrences.get(new StructuralElement<>(ctStatement)); int initialSize = countStatements(ctStatement); @@ -118,8 +204,8 @@ private void checkCtStatement(CtStatement ctStatement) { int duplicateStatementSize = initialSize; - List leftCode = new ArrayList<>(List.of(ctStatement)); - List rightCode = new ArrayList<>(List.of(duplicate)); + CodeSegment leftCode = CodeSegment.of(ctStatement); + CodeSegment rightCode = CodeSegment.of(duplicate); for (var entry : zip(SpoonUtil.getNextStatements(ctStatement), SpoonUtil.getNextStatements(duplicate))) { if (!StructuralEqualsVisitor.equals(entry.getKey(), entry.getValue())) { @@ -131,25 +217,51 @@ private void checkCtStatement(CtStatement ctStatement) { duplicateStatementSize += countStatements(entry.getKey()); } - if (duplicateStatementSize >= MINIMUM_DUPLICATE_STATEMENT_SIZE) { - // prevent duplicate reporting of the same code segments - reported.addAll(leftCode); - reported.addAll(rightCode); - - addLocalProblem( - ctStatement, - new LocalizedMessage( - "duplicate-code", - Map.of( - "left", formatSourceRange(leftCode.getFirst(), leftCode.getLast()), - "right", formatSourceRange(rightCode.getFirst(), rightCode.getLast()) - ) - ), - ProblemType.DUPLICATE_CODE - ); - - break; + if (duplicateStatementSize < MINIMUM_DUPLICATE_STATEMENT_SIZE) { + continue; + } + + // The duplicate code might access variables that are not declared in the code segment. + // The variables would have to be passed as parameters of a helper method. + // + // The problem is that when a variable is reassigned, it can not be passed as a parameter + // -> we would have to ignore the duplicate code segment + int numberOfReassignedVariables = leftCode.countDependencies( + ctVariable -> !(ctVariable instanceof CtField) && !ctVariable.isStatic(), + ctVariableAccess -> ctVariableAccess instanceof CtVariableWrite && ctVariableAccess.getParent() instanceof CtAssignment + ); + + if (numberOfReassignedVariables > 1) { + continue; } + + // Another problem is that the duplicate code segment might declare variables that are used + // after the code segment. + // + // A method can at most return one value (ignoring more complicated solutions like returning a custom object) + int numberOfUsedVariables = Math.max(leftCode.countExposedVariables(), rightCode.countExposedVariables()); + + if (numberOfReassignedVariables + numberOfUsedVariables > 1) { + continue; + } + + // prevent duplicate reporting of the same code segments + reported.addAll(leftCode.statements()); + reported.addAll(rightCode.statements()); + + addLocalProblem( + ctStatement, + new LocalizedMessage( + "duplicate-code", + Map.of( + "left", formatSourceRange(leftCode.getFirst(), leftCode.getLast()), + "right", formatSourceRange(rightCode.getFirst(), rightCode.getLast()) + ) + ), + ProblemType.DUPLICATE_CODE + ); + + break; } } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CtElementStream.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CtElementStream.java index e9be8bc8..dd988e07 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CtElementStream.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/CtElementStream.java @@ -2,10 +2,15 @@ import spoon.reflect.declaration.CtElement; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; import java.util.Comparator; +import java.util.IdentityHashMap; import java.util.Iterator; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.Spliterator; import java.util.function.BiConsumer; import java.util.function.BiFunction; @@ -70,6 +75,10 @@ private CtElementStream(Stream baseStream) { ////////////////////////////// New Methods ////////////////////////////////// ///////////////////////////////////////////////////////////////////////////// + public Stream toStream() { + return this; + } + /** * Like Stream::map, but returns a CtElementStream instead of a Stream. * @param function @@ -138,6 +147,21 @@ public CtElementStream nestedIn(CtElement parent) { return this.filter(e -> SpoonUtil.isNestedOrSame(e, parent)); } + public CtElementStream nestedInAny(CtElement... parents) { + return this.nestedInAny(Arrays.asList(parents)); + } + + public CtElementStream nestedInAny(Collection parents) { + if (parents.isEmpty()) { + return CtElementStream.empty(); + } + + Set potentialParents = Collections.newSetFromMap(new IdentityHashMap<>()); + potentialParents.addAll(parents); + + return this.filter(e -> SpoonUtil.isAnyNestedOrSame(e, potentialParents)); + } + /** * Filters the stream to only include elements that are nested in an element of the given type, or are of the given type. * @param parentType diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java index b05eeafa..9eb24588 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java @@ -9,6 +9,7 @@ import de.firemage.autograder.core.integrated.evaluator.fold.InlineVariableRead; import de.firemage.autograder.core.integrated.evaluator.fold.RemoveRedundantCasts; import org.apache.commons.io.FilenameUtils; +import spoon.processing.FactoryAccessor; import spoon.reflect.CtModel; import spoon.reflect.code.BinaryOperatorKind; import spoon.reflect.code.CtBinaryOperator; @@ -53,6 +54,7 @@ import spoon.reflect.reference.CtTypeParameterReference; import spoon.reflect.reference.CtTypeReference; import spoon.reflect.reference.CtVariableReference; +import spoon.support.reflect.declaration.CtElementImpl; import java.io.File; import java.util.ArrayDeque; @@ -62,6 +64,7 @@ import java.util.Collections; import java.util.Deque; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; @@ -1329,11 +1332,32 @@ public static int getParameterIndex(CtParameter parameter, CtExecutable ex throw new IllegalArgumentException("Parameter not found in executable"); } - public static CtPackage getRootPackage(CtElement element) { + public static CtPackage getRootPackage(FactoryAccessor element) { return element.getFactory().getModel().getRootPackage(); } + + public static boolean isAnyNestedOrSame(CtElement ctElement, Set potentialParents) { + // CtElement::hasParent will recursively call itself until it reaches the root + // => inefficient and might cause a stack overflow + + if (potentialParents.contains(ctElement)) { + return true; + } + + for (CtElement parent : parents(ctElement)) { + if (potentialParents.contains(parent)) { + return true; + } + } + + return false; + } + public static boolean isNestedOrSame(CtElement element, CtElement parent) { - return element == parent || element.hasParent(parent); + Set set = Collections.newSetFromMap(new IdentityHashMap<>()); + set.add(parent); + + return element == parent || SpoonUtil.isAnyNestedOrSame(element, set); } } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java index a6da2937..5af0fca8 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java @@ -1,5 +1,6 @@ package de.firemage.autograder.core.integrated; +import spoon.processing.FactoryAccessor; import spoon.reflect.CtModel; import spoon.reflect.code.CtBlock; import spoon.reflect.code.CtConstructorCall; @@ -28,6 +29,7 @@ import spoon.reflect.reference.CtTypeReference; import spoon.reflect.visitor.CtScanner; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; import java.util.Deque; @@ -39,7 +41,6 @@ import java.util.List; import java.util.Map; import java.util.SequencedSet; -import java.util.Stack; import java.util.stream.Stream; /** @@ -63,8 +64,8 @@ public static void buildFor(CtModel model) { model.getRootPackage().putMetadata(METADATA_KEY, uses); } - public static UsesFinder getFor(CtElement element) { - var uses = (UsesFinder) SpoonUtil.getRootPackage(element).getMetadata(METADATA_KEY); + private static UsesFinder getFor(FactoryAccessor factoryAccessor) { + var uses = (UsesFinder) SpoonUtil.getRootPackage(factoryAccessor).getMetadata(METADATA_KEY); if (uses == null) { throw new IllegalArgumentException("No uses information available for this model"); } @@ -79,17 +80,13 @@ public static UsesFinder getFor(CtElement element) { */ @SuppressWarnings("rawtypes") public static CtElementStream getAllUses(CtNamedElement element) { - if (element instanceof CtVariable variable) { - return UsesFinder.variableUses(variable).asUntypedStream(); - } else if (element instanceof CtTypeParameter typeParameter) { - return UsesFinder.typeParameterUses(typeParameter).asUntypedStream(); - } else if (element instanceof CtExecutable executable) { - return UsesFinder.executableUses(executable).asUntypedStream(); - } else if (element instanceof CtType type) { - return UsesFinder.typeUses(type).asUntypedStream(); - } else { - throw new IllegalArgumentException("Unsupported element: " + element.getClass().getName()); - } + return switch (element) { + case CtVariable variable -> UsesFinder.variableUses(variable).asUntypedStream(); + case CtTypeParameter typeParameter -> UsesFinder.typeParameterUses(typeParameter).asUntypedStream(); + case CtExecutable executable -> UsesFinder.executableUses(executable).asUntypedStream(); + case CtType type -> UsesFinder.typeUses(type).asUntypedStream(); + default -> throw new IllegalArgumentException("Unsupported element: " + element.getClass().getName()); + }; } public static CtElementStream> variableUses(CtVariable variable) { @@ -126,6 +123,19 @@ public static CtElementStream> subtypesOf(CtType type, boolean incl ); } + // It is difficult to determine whether a variable is being accessed by the variable access, + // because references to different variables might be equal or the CtVariable can not be obtained + // from the CtVariableAccess. + // + // This class keeps track of all variable accesses and their corresponding variables, which is why + // this method exists here. + public static boolean isAccessingVariable(CtVariable ctVariable, CtVariableAccess ctVariableAccess) { + return UsesFinder.getDeclaredVariable(ctVariableAccess) == ctVariable; + } + + public static CtVariable getDeclaredVariable(CtVariableAccess ctVariableAccess) { + return UsesFinder.getFor(ctVariableAccess).scanner.variableAccessDeclarations.getOrDefault(ctVariableAccess, null); + } /** * 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. @@ -134,16 +144,17 @@ public static CtElementStream> subtypesOf(CtType type, boolean incl private static class UsesScanner extends CtScanner { // The IdentityHashMaps are very important here, since // E.g. CtVariable's equals method considers locals with the same name to be equal - public final IdentityHashMap> variableUses = new IdentityHashMap<>(); - public final IdentityHashMap> typeParameterUses = new IdentityHashMap<>(); - public final IdentityHashMap> executableUses = new IdentityHashMap<>(); - public final IdentityHashMap> typeUses = new IdentityHashMap<>(); - public final Map> subtypes = new IdentityHashMap<>(); + private final Map> variableUses = new IdentityHashMap<>(); + private final Map variableAccessDeclarations = new IdentityHashMap<>(); + private final Map> typeParameterUses = new IdentityHashMap<>(); + private final Map> executableUses = new IdentityHashMap<>(); + private final Map> typeUses = new IdentityHashMap<>(); + private final Map> 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 // (in reality, the scope is based on 'normal completion', see JLS, but the rules are way too complex for us) - private final Stack> instanceofPatternVariables = new Stack<>(); + private final Deque> instanceofPatternVariables = new ArrayDeque<>(); @Override public void visitCtVariableRead(CtVariableRead variableRead) { @@ -260,6 +271,7 @@ private void recordVariableAccess(CtVariableAccess variableAccess) { if (variable != null) { var accesses = this.variableUses.computeIfAbsent(variable, k -> new ArrayList<>()); accesses.add(variableAccess); + this.variableAccessDeclarations.put(variableAccess, variable); } } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java index 7691af59..56430aa1 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralEqualsVisitor.java @@ -184,7 +184,7 @@ public static boolean shouldSkip(CtRole role, Object element) { return true; } - // TODO: element can be collections??? + // NOTE: element might be a collection of CtElements if (role == CtRole.NAME && isInstanceOfAny(element, CtLocalVariable.class, CtField.class, CtParameter.class)) { return true; diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateCode.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateCode.java index 2c92f1c6..3b23398f 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateCode.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateCode.java @@ -7,6 +7,7 @@ import de.firemage.autograder.core.check.AbstractCheckTest; import de.firemage.autograder.core.compiler.JavaVersion; import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import java.io.IOException; @@ -72,4 +73,492 @@ public static String ceasarDecryption(String input, int key) { problems.assertExhausted(); } + + + @Test + void testTooManyVariablesBeforeDuplicate() throws IOException, LinterException { + // In this test, there is a large duplicate segment of code that writes to variables + // declared in the duplicate segment. + // + // The declared variables are used after the duplicate segment, therefore if one extracts + // the duplicate segment into a method, the variables would have to be returned from the + // method or passed as a parameter (this only works for mutable types like collections or arrays). + // TODO: write test for the collection/array case? + // TODO: we can only pass them as parameter if they are never assigned a new value (excluding arrays) + // + // A method can only return one value. One could work around this by creating a tuple like class (Pair, Triple, ...), + // but this is not a good solution, therefore the duplicate code should be ignored in this case. + + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void callA() { + System.out.println("A"); + } + + public static void callB() { + System.out.println("B"); + } + + public static void a(String input) { + String result = ""; + char[] array = new char[input.length()]; + String other = "other"; + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + other += result; + } + System.out.println(result); + array[i] = c; + } + + // force an end to the duplicate segment + callA(); + + System.out.println(result); + System.out.println(array); + } + + public static void b(String input) { + String result = ""; + char[] array = new char[input.length()]; + String other = "other"; + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + other += result; + } + System.out.println(result); + array[i] = c; + } + + // force an end to the duplicate segment + callB(); + + System.out.println(result); + System.out.println(array); + System.out.println(other); + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testReturnsSingleVariable() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void callA() { + System.out.println("A"); + } + + public static void callB() { + System.out.println("B"); + } + + public static void a(String input) { + String result = ""; + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + } + + // force an end to the duplicate segment + callA(); + + System.out.println(result); + } + + public static void b(String input) { + String result = ""; + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + } + + // force an end to the duplicate segment + callB(); + + System.out.println(result); + } + } + """ + ), PROBLEM_TYPES); + + assertDuplicateCode(problems.next(), "Main:11-21", "Main:30-40"); + + problems.assertExhausted(); + } + + @Test + void testReassignsSingleVariable() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void callA() { + System.out.println("A"); + } + + public static void callB() { + System.out.println("B"); + } + + public static void a(String input) { + String result = ""; + + callA(); + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + } + + // force an end to the duplicate segment + callA(); + + System.out.println(result); + } + + public static void b(String input) { + String result = ""; + + callB(); + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + } + + // force an end to the duplicate segment + callB(); + + System.out.println(result); + } + } + """ + ), PROBLEM_TYPES); + + assertDuplicateCode(problems.next(), "Main:15-23", "Main:36-44"); + + problems.assertExhausted(); + } + + @Test + void testReassignsSingleVariableWithUnrelated() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void callA() { + System.out.println("A"); + } + + public static void callB() { + System.out.println("B"); + } + + public static void a(String input) { + String result = ""; + String other = ""; + + callA(); + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + } + + // force an end to the duplicate segment + callA(); + + System.out.println(result); + System.out.println(other); + } + + public static void b(String input) { + String result = ""; + String other = ""; + + callB(); + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + } + + // force an end to the duplicate segment + callB(); + + System.out.println(result); + System.out.println(other); + } + } + """ + ), PROBLEM_TYPES); + + assertDuplicateCode(problems.next(), "Main:16-24", "Main:39-47"); + + problems.assertExhausted(); + } + + @Test + void testReassignsMultipleVariablesUsedAfterDuplicate() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void callA() { + System.out.println("A"); + } + + public static void callB() { + System.out.println("B"); + } + + public static void a(String input) { + String result = ""; + String other = "other"; + + callA(); + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + other += result; + System.out.println(other); + } + + // force an end to the duplicate segment + callA(); + + System.out.println(result); + System.out.println(other); + } + + public static void b(String input) { + String result = ""; + String left = "other"; + + callB(); + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + left += result; + System.out.println(left); + } + + // force an end to the duplicate segment + callB(); + + System.out.println(result); + System.out.println(left); + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testReassignsMultipleVariablesOnlyUsedInDuplicate() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void callA() { + System.out.println("A"); + } + + public static void callB() { + System.out.println("B"); + } + + public static void a(String input) { + String result = ""; + + callA(); + String other = "other"; + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + other += result; + System.out.println(other); + } + + // force an end to the duplicate segment + callA(); + + System.out.println(result); + } + + public static void b(String input) { + String result = ""; + + callB(); + String other = "other"; + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + other += result; + System.out.println(other); + } + + // force an end to the duplicate segment + callB(); + + System.out.println(result); + } + } + """ + ), PROBLEM_TYPES); + + assertDuplicateCode(problems.next(), "Main:14-26", "Main:38-50"); + + problems.assertExhausted(); + } + + @Test + @Disabled("Will be implemented in the future") + void testReassignsMultipleVariablesOnlyUsedInDuplicateRenamed() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void callA() { + System.out.println("A"); + } + + public static void callB() { + System.out.println("B"); + } + + public static void a(String input) { + String result = ""; + + callA(); + String other = "other"; + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + other += result; + System.out.println(other); + } + + // force an end to the duplicate segment + callA(); + + System.out.println(result); + } + + public static void b(String input) { + String result = ""; + + callB(); + String left = "other"; + + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (Character.isLetter(c)) { + result += c; + } + System.out.println(result); + System.out.println(c); + System.out.println(c); + left += result; + System.out.println(left); + } + + // force an end to the duplicate segment + callB(); + + System.out.println(result); + } + } + """ + ), PROBLEM_TYPES); + + assertDuplicateCode(problems.next(), "Main:14-26", "Main:38-50"); + + problems.assertExhausted(); + } } From 4f7c37e86e851aae9ff4c04b67c48dc865cf0d27 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 18 Jun 2024 12:46:44 +0200 Subject: [PATCH 5/7] remove pmd cpd --- .../de/firemage/autograder/core/Linter.java | 14 ----- .../autograder/core/LinterStatus.java | 1 - .../core/check/general/CopyPasteCheck.java | 22 -------- .../autograder/core/cpd/CPDInCodeProblem.java | 51 ------------------- .../autograder/core/cpd/CPDLinter.java | 44 ---------------- .../src/main/resources/strings.de.ftl | 2 - 6 files changed, 134 deletions(-) delete mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/general/CopyPasteCheck.java delete mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/cpd/CPDInCodeProblem.java delete mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/cpd/CPDLinter.java 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 index b48ae33b..397784d2 100644 --- 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 checkFile( List pmdChecks = new ArrayList<>(); List spotbugsChecks = new ArrayList<>(); - List cpdChecks = new ArrayList<>(); List integratedChecks = new ArrayList<>(); List 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 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 index f35fd428..f2adc24e 100644 --- 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/check/general/CopyPasteCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/CopyPasteCheck.java deleted file mode 100644 index fd547e4c..00000000 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/CopyPasteCheck.java +++ /dev/null @@ -1,22 +0,0 @@ -package de.firemage.autograder.core.check.general; - -import de.firemage.autograder.core.LocalizedMessage; -import de.firemage.autograder.core.check.Check; - -// TODO add executable check annotation and problem type -public class CopyPasteCheck implements Check { - private final int tokenCount; - - public CopyPasteCheck(int tokenCount) { - this.tokenCount = tokenCount; - } - - @Override - public LocalizedMessage getLinter() { - return new LocalizedMessage("linter-cpd"); - } - - public int getTokenCount() { - return tokenCount; - } -} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/cpd/CPDInCodeProblem.java b/autograder-core/src/main/java/de/firemage/autograder/core/cpd/CPDInCodeProblem.java deleted file mode 100644 index d4a266bf..00000000 --- a/autograder-core/src/main/java/de/firemage/autograder/core/cpd/CPDInCodeProblem.java +++ /dev/null @@ -1,51 +0,0 @@ -package de.firemage.autograder.core.cpd; - -import de.firemage.autograder.core.CodePosition; -import de.firemage.autograder.core.ProblemImpl; -import de.firemage.autograder.core.LocalizedMessage; -import de.firemage.autograder.core.ProblemType; -import de.firemage.autograder.core.check.Check; -import de.firemage.autograder.core.file.SourceInfo; -import de.firemage.autograder.core.file.SourcePath; -import net.sourceforge.pmd.cpd.Mark; -import net.sourceforge.pmd.cpd.Match; - -import java.nio.file.Path; -import java.util.Map; - -class CPDInCodeProblem extends ProblemImpl { - private final Match match; - private final SourceInfo sourceInfo; - - public CPDInCodeProblem(Check check, Match match, SourceInfo sourceInfo) { - super( - check, - markToPosition(sourceInfo, match.getFirstMark()), - formatMatch(match, sourceInfo), - ProblemType.DUPLICATE_CODE - ); - this.match = match; - this.sourceInfo = sourceInfo; - } - - private static LocalizedMessage formatMatch(Match match, SourceInfo root) { - SourcePath firstPath = root.getCompilationUnit(Path.of(match.getFirstMark().getFilename())).path(); - SourcePath secondPath = root.getCompilationUnit(Path.of(match.getSecondMark().getFilename())).path(); - - return new LocalizedMessage("duplicate-code", Map.of( - "lines", String.valueOf(match.getLineCount()), - "first-path", firstPath, - "first-start", String.valueOf(match.getFirstMark().getBeginLine()), - "first-end", String.valueOf(match.getFirstMark().getEndLine()), - "second-path", secondPath, - "second-start", String.valueOf(match.getSecondMark().getBeginLine()), - "second-end", String.valueOf(match.getSecondMark().getEndLine()) - )); - } - - private static CodePosition markToPosition(SourceInfo sourceInfo, Mark mark) { - // TODO mark.getFilename() is most likely not correct - return new CodePosition(sourceInfo, SourcePath.of(mark.getFilename()), mark.getBeginLine(), mark.getEndLine(), - mark.getBeginColumn(), mark.getEndColumn()); - } -} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/cpd/CPDLinter.java b/autograder-core/src/main/java/de/firemage/autograder/core/cpd/CPDLinter.java deleted file mode 100644 index 46859245..00000000 --- a/autograder-core/src/main/java/de/firemage/autograder/core/cpd/CPDLinter.java +++ /dev/null @@ -1,44 +0,0 @@ -package de.firemage.autograder.core.cpd; - -import de.firemage.autograder.core.Problem; -import de.firemage.autograder.core.file.CompilationUnit; -import de.firemage.autograder.core.file.UploadedFile; -import de.firemage.autograder.core.check.general.CopyPasteCheck; -import net.sourceforge.pmd.cpd.CPD; -import net.sourceforge.pmd.cpd.CPDConfiguration; -import net.sourceforge.pmd.cpd.JavaLanguage; -import net.sourceforge.pmd.cpd.SourceCode; - -import javax.tools.JavaFileObject; -import java.io.IOException; -import java.util.ArrayList; -import java.util.List; - -public class CPDLinter { - - public List lint(UploadedFile file, List checks) throws IOException { - List problems = new ArrayList<>(); - for (CopyPasteCheck check : checks) { - CPDConfiguration cpdConfig = new CPDConfiguration(); - cpdConfig.setFailOnViolation(false); - cpdConfig.setLanguage(new JavaLanguage()); - cpdConfig.setMinimumTileSize(check.getTokenCount()); - - // NOTE: CPD is marked for removal, but there is no replacement yet - // (CpdAnalysis should be added in the future) - CPD cpd = new CPD(cpdConfig); - - for (CompilationUnit compilationUnit : file.getSource().compilationUnits()) { - JavaFileObject javaFileObject = compilationUnit.toJavaFileObject(); - cpd.add(new SourceCode(new SourceCode.ReaderCodeLoader( - javaFileObject.openReader(true), - compilationUnit.path().toString() - ))); - } - cpd.go(); - cpd.getMatches().forEachRemaining(match -> problems.add(new CPDInCodeProblem(check, match, file.getSource()))); - } - - return problems; - } -} diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index 05895822..da59fd4b 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -2,13 +2,11 @@ status-compiling = Compiling status-spotbugs = Running SpotBugs status-pmd = Running PMD -status-cpd = Running Copy/Paste-Detection status-error-prone = Running error-prone status-model = Building the code model status-integrated = Running integrated analysis # Linters -linter-cpd = Copy/Paste-Detection linter-spotbugs = SpotBugs linter-pmd = PMD linter-integrated = Integrated Analysis From a3ceba95aeae96ba67317da20e0527532a09099f Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 18 Jun 2024 12:57:14 +0200 Subject: [PATCH 6/7] fix tests --- .../de/firemage/autograder/core/compiler/JavaVersion.java | 8 +++++++- autograder-core/src/main/resources/strings.en.ftl | 2 -- .../de/firemage/autograder/core/TestLocalizedStrings.java | 5 ++--- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/compiler/JavaVersion.java b/autograder-core/src/main/java/de/firemage/autograder/core/compiler/JavaVersion.java index 7c053e7d..b487e63f 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/compiler/JavaVersion.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/compiler/JavaVersion.java @@ -1,6 +1,7 @@ package de.firemage.autograder.core.compiler; import java.util.Arrays; +import java.util.Comparator; public enum JavaVersion { JAVA_7("7", 7), @@ -13,7 +14,8 @@ public enum JavaVersion { JAVA_14("14", 14), JAVA_15("15", 15), JAVA_16("16", 16), - JAVA_17("17", 17); + JAVA_17("17", 17), + JAVA_21("21", 21); private final String versionString; private final int versionNumber; @@ -30,6 +32,10 @@ public static JavaVersion fromString(String s) { .orElseThrow(() -> new IllegalArgumentException("Unknown java version")); } + public static JavaVersion latest() { + return Arrays.stream(JavaVersion.values()).max(Comparator.comparingInt(JavaVersion::getVersionNumber)).orElseThrow(); + } + public static boolean isValidJavaVersion(String s) { return Arrays.stream(JavaVersion.class.getEnumConstants()) .anyMatch(v -> v.getVersionString().equals(s)); diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl index 6111c269..36732516 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -2,13 +2,11 @@ status-compiling = Compiling status-spotbugs = Running SpotBugs status-pmd = Running PMD -status-cpd = Running Copy/Paste-Detection status-error-prone = Running error-prone status-model = Building the code model status-integrated = Running integrated analysis # Linters -linter-cpd = Copy/Paste-Detection linter-spotbugs = SpotBugs linter-pmd = PMD linter-integrated = Integrated Analysis diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/TestLocalizedStrings.java b/autograder-core/src/test/java/de/firemage/autograder/core/TestLocalizedStrings.java index 5ac08ca8..c3765b86 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/TestLocalizedStrings.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/TestLocalizedStrings.java @@ -1,5 +1,6 @@ package de.firemage.autograder.core; +import de.firemage.autograder.core.compiler.JavaVersion; import de.firemage.autograder.core.integrated.SpoonUtil; import fluent.bundle.FluentBundle; import fluent.bundle.FluentResource; @@ -63,10 +64,8 @@ class TestLocalizedStrings { "status-compiling", "status-spotbugs", "status-pmd", - "status-cpd", "status-model", "status-integrated", - "linter-cpd", "linter-spotbugs", "linter-pmd", "linter-integrated", @@ -185,7 +184,7 @@ static void readChecksAndResources() { SpoonAPI launcher = new Launcher(); launcher.addInputResource(path.toString()); - launcher.getEnvironment().setComplianceLevel(17); + launcher.getEnvironment().setComplianceLevel(JavaVersion.latest().getVersionNumber()); launcher.buildModel(); CtModel ctModel = launcher.getModel(); From 631a937d5deb6bde06f7140fa513db54362338be Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 18 Jun 2024 12:59:14 +0200 Subject: [PATCH 7/7] fix crash in leaked collection check --- .../autograder/core/check/oop/LeakedCollectionCheck.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java index 194281e8..83789cc7 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/oop/LeakedCollectionCheck.java +++ b/autograder-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; }