From d9e5d7d8cf6e408eacec51bcf38660bba7190e4c Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 17 Sep 2024 17:22:05 +0200 Subject: [PATCH 1/9] refactor code into `MethodUtil` for extracting methods from statements --- .../core/check/structure/DuplicateCode.java | 92 +------- .../core/integrated/MethodUtil.java | 197 ++++++++++++++++-- 2 files changed, 194 insertions(+), 95 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 e0a322f7..2e919193 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 @@ -5,39 +5,28 @@ import de.firemage.autograder.core.check.ExecutableCheck; import de.firemage.autograder.core.integrated.CoreUtil; import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.MethodUtil; import de.firemage.autograder.core.integrated.StatementUtil; 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.Set; -import java.util.function.Predicate; -import java.util.stream.Collectors; @ExecutableCheck(reportedProblems = { ProblemType.DUPLICATE_CODE }) public class DuplicateCode extends IntegratedCheck { @@ -112,54 +101,6 @@ public List statements() { public Iterator iterator() { return this.statements().iterator(); } - - private Set> declaredVariables() { - Set> 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 : StatementUtil.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 @@ -179,12 +120,18 @@ public void process(CtStatement ctStatement) { } }); - /* Map> collisions = new HashMap<>(); for (var key : occurrences.keySet()) { collisions.computeIfAbsent(key.hashCode(), k -> new ArrayList<>()).add(key); } + /* + var mostCommonCollisions = collisions.values() + .stream() + .filter(list -> list.size() > 1) + .sorted((a, b) -> Integer.compare(b.size(), a.size())) + .limit(10) + .toList(); System.out.println("Number of duplicate hashCodes: " + (occurrences.size() - collisions.size()) + " of " + occurrences.size() + " elements");*/ Set reported = new HashSet<>(); @@ -221,27 +168,10 @@ private void checkCtStatement(CtStatement ctStatement) { 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()); + MethodUtil.UnnamedMethod leftMethod = MethodUtil.createMethodFrom(null, leftCode.statements()); + MethodUtil.UnnamedMethod rightMethod = MethodUtil.createMethodFrom(null, rightCode.statements()); - if (numberOfReassignedVariables + numberOfUsedVariables > 1) { + if (!leftMethod.canBeMethod() || !rightMethod.canBeMethod()) { continue; } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java index 51fa44ee..545f407d 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/MethodUtil.java @@ -1,18 +1,31 @@ package de.firemage.autograder.core.integrated; +import spoon.reflect.code.CtAssignment; import spoon.reflect.code.CtConstructorCall; import spoon.reflect.code.CtInvocation; import spoon.reflect.code.CtLambda; import spoon.reflect.code.CtStatement; -import spoon.reflect.code.CtTypeAccess; +import spoon.reflect.code.CtVariableAccess; +import spoon.reflect.code.CtVariableWrite; import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtField; import spoon.reflect.declaration.CtMethod; +import spoon.reflect.declaration.CtType; +import spoon.reflect.declaration.CtVariable; import spoon.reflect.factory.TypeFactory; import spoon.reflect.reference.CtExecutableReference; import spoon.reflect.reference.CtTypeReference; +import spoon.reflect.visitor.filter.TypeFilter; import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; public final class MethodUtil { private MethodUtil() { @@ -108,27 +121,183 @@ public static boolean isInMainMethod(CtElement ctElement) { public static boolean isGetter(CtMethod method) { return method.getSimpleName().startsWith("get") - && method.getParameters().isEmpty() - && !method.getType().getSimpleName().equals("void") - && (method.isAbstract() || StatementUtil.getEffectiveStatements(method.getBody()).size() == 1); + && method.getParameters().isEmpty() + && !method.getType().getSimpleName().equals("void") + && (method.isAbstract() || StatementUtil.getEffectiveStatements(method.getBody()).size() == 1); } public static boolean isSetter(CtMethod method) { return method.getSimpleName().startsWith("set") - && method.getParameters().size() == 1 - && method.getType().getSimpleName().equals("void") - && (method.isAbstract() || StatementUtil.getEffectiveStatements(method.getBody()).size() == 1); - } - - public static boolean isStaticCallTo(CtInvocation invocation, String typeName, String methodName) { - return invocation.getExecutable().isStatic() - && invocation.getTarget() instanceof CtTypeAccess access - && access.getAccessedType().getQualifiedName().equals(typeName) - && invocation.getExecutable().getSimpleName().equals(methodName); + && method.getParameters().size() == 1 + && method.getType().getSimpleName().equals("void") + && (method.isAbstract() || StatementUtil.getEffectiveStatements(method.getBody()).size() == 1); } public static boolean isInSetter(CtElement ctElement) { CtMethod parent = ctElement.getParent(CtMethod.class); return parent != null && isSetter(parent); } + + /** + * Creates a method from the given statements that can be added to the given target type. + * + * @param targetType the type the method should be added to or null if the method should be static + * @param statements the statements that should be in the method + * @return a new method that can be added to the target type + */ + public static UnnamedMethod createMethodFrom(CtType targetType, List statements) { + if (targetType == null) { + targetType = statements.get(0).getParent(CtType.class); + } + CtType finalTargetType = targetType; + Map, List>> args = dependencies( + statements, + // filter out all variable accesses that are of the target type (those variables do not have to be passed as arguments) + ctVariable -> finalTargetType == null + || !(ctVariable instanceof CtField && ctVariable.getParent(CtType.class) == finalTargetType) + || !ctVariable.isStatic(), + ctVariableAccess -> true + ); + + // return variables are those that + // - are assigned to + // - are declared in this code segment and used after the last statement -> they have to be returned + Map, List>> readVariables = new IdentityHashMap<>(); + Map, List>> assignedVariables = new IdentityHashMap<>(); + + // now go through all arguments and check if they are assigned to or just read + args.forEach((ctVariable, ctVariableAccesses) -> { + boolean isAssigned = ctVariableAccesses.stream().anyMatch( + ctVariableAccess -> ctVariableAccess instanceof CtVariableWrite + && ctVariableAccess.getParent() instanceof CtAssignment + ); + + if (isAssigned) { + assignedVariables.put(ctVariable, ctVariableAccesses); + } else { + readVariables.put(ctVariable, ctVariableAccesses); + } + }); + + return new UnnamedMethod( + targetType, + List.copyOf(statements), + readVariables, + assignedVariables, + exposedVariables(statements) + ); + } + + /** + * Represents a method that does not have a name yet or a concrete return type. + *
+ * Instead, it separates the accessed variables into those that are assigned to and those that are not. + * + * @param parentType the type the method is a part of + * @param statements the statements of the method + * @param readVariables the variables that have to be passed as arguments to the method (and lists how they were used) + * @param assignedVariables variables that are assigned to in the method + * @param exposedVariables variables that are declared in the method and used after the last statement (those would have to be returned) + */ + public record UnnamedMethod( + CtType parentType, + List statements, + Map, List>> readVariables, + Map, List>> assignedVariables, + Set> exposedVariables + ) { + /** + * Counts the number of variables (not declared in the code segment) that are assigned in the code segment. + * @return the number of assigned variables + */ + private int countAssignedVariables() { + return this.assignedVariables.size(); + } + + public boolean canBeMethod() { + // 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 = this.countAssignedVariables(); + if (numberOfReassignedVariables > 1) { + return false; + } + + // 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 = this.exposedVariables().size(); + + return numberOfReassignedVariables + numberOfUsedVariables <= 1; + } + } + + private static Set> declaredVariables(Collection statements) { + return statements.stream() + .filter(ctStatement -> ctStatement instanceof CtVariable) + .map(ctStatement -> (CtVariable) ctStatement) + .collect(Collectors.toCollection(MethodUtil::identitySet)); + } + + /** + * Finds all variables that are declared in the list of statements and are used after the last statement. + * @param statements the statements to check + * @return a set of all variables that are used after the last statement + */ + private static Set> exposedVariables(List statements) { + Set> declaredVariables = declaredVariables(statements); + Set> result = identitySet(); + + if (declaredVariables.isEmpty()) { + return result; + } + + // check which declared variables are used after the last statement + for (CtStatement ctStatement : StatementUtil.getNextStatements(statements.get(statements.size() - 1))) { + for (CtVariable declaredVariable : declaredVariables) { + if (UsesFinder.variableUses(declaredVariable).nestedIn(ctStatement).hasAny()) { + result.add(declaredVariable); + } + } + } + return result; + } + + private static Set identitySet() { + return Collections.newSetFromMap(new IdentityHashMap<>()); + } + + /** + * Finds all variables that are used in the given statements and are not declared in them. + * + * @param statements the statements to check + * @param isDependency a predicate that is used to further filter variables. For example, one might want to ignore all class variables. + * @param isDependencyAccess the predicate can be used to filter out certain variable accesses like assignments or reads. + * @return a map of all variables that are used in the statements and match the given predicates, where the value is the list of accesses + */ + private static Map, List>> dependencies( + Collection statements, + Predicate> isDependency, + Predicate> isDependencyAccess + ) { + if (statements.isEmpty()) { + return new IdentityHashMap<>(); + } + + // all variables declared in the code segment (including nested ones) + Set> codeSegmentVariables = statements.stream() + .flatMap(ctStatement -> ctStatement.getElements(new TypeFilter>(CtVariable.class)).stream()) + .collect(Collectors.toCollection(MethodUtil::identitySet)); + + return statements.stream() + .flatMap(ctStatement -> ctStatement.getElements(new TypeFilter>(CtVariableAccess.class)).stream()) + .filter(isDependencyAccess) + .filter(ctVariableAccess -> UsesFinder.getDeclaredVariable(ctVariableAccess) != null) + .map(ctVariableAccess -> Map.entry(UsesFinder.getDeclaredVariable(ctVariableAccess), ctVariableAccess)) + .filter(entry -> !codeSegmentVariables.contains(entry.getKey()) && isDependency.test(entry.getKey())) + .collect(Collectors.groupingBy(Map.Entry::getKey, IdentityHashMap::new, Collectors.mapping(Map.Entry::getValue, Collectors.toList()))); + } } From 7b33e826fa98e0ca5b007c092ef94c231a52200f Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 17 Sep 2024 17:28:09 +0200 Subject: [PATCH 2/9] disable NumberFormatExceptionIgnored when exception handling is never used #558 --- .../NumberFormatExceptionIgnored.java | 6 ++++ .../TestNumberFormatExceptionIgnored.java | 34 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/NumberFormatExceptionIgnored.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/NumberFormatExceptionIgnored.java index 93a14ae5..78b077a8 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/NumberFormatExceptionIgnored.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/NumberFormatExceptionIgnored.java @@ -27,6 +27,12 @@ private static boolean isNFECaught(CtInvocation ctInvocation) { } @Override protected void check(StaticAnalysis staticAnalysis) { + boolean hasCaughtAnyException = staticAnalysis.getModel().filterChildren(CtCatch.class::isInstance).first() != null; + // if exception handling is not present, we don't need to check for ignored exceptions + if (!hasCaughtAnyException) { + return; + } + staticAnalysis.processWith(new AbstractProcessor>() { @Override public void process(CtInvocation ctInvocation) { diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestNumberFormatExceptionIgnored.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestNumberFormatExceptionIgnored.java index 43547e27..9b8013ff 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestNumberFormatExceptionIgnored.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestNumberFormatExceptionIgnored.java @@ -36,6 +36,14 @@ public class Main { public static void main(String[] args) { int number = Integer.parseInt("123"); } + + private void catcher() { + try { + Integer.parseInt("123"); + } catch (NumberFormatException e) { + System.out.println("Error"); + } + } } """ ), PROBLEM_TYPES); @@ -59,10 +67,36 @@ public static void main(String[] args) { System.out.println("Error"); } } + + private void catcher() { + try { + Integer.parseInt("123"); + } catch (NumberFormatException e) { + System.out.println("Error"); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testNoExceptionHandling() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void main(String[] args) { + int number = Integer.parseInt("123"); + } } """ ), PROBLEM_TYPES); problems.assertExhausted(); } + } From 6153116289fd5a824ce31742df298dae1182c6ae Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 17 Sep 2024 18:05:23 +0200 Subject: [PATCH 3/9] allow variable declarations in loop should be for between counter and loop #539 --- .../core/check/general/LoopShouldBeFor.java | 9 +++-- .../check/general/TestLoopShouldBeFor.java | 37 +++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeFor.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeFor.java index b5db18e3..8855d7cd 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeFor.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeFor.java @@ -1,6 +1,5 @@ package de.firemage.autograder.core.check.general; -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; @@ -63,7 +62,7 @@ public String toString() { } } - private static LoopSuggestion getCounter(CtLoop ctLoop, CodeModel model) { + private static LoopSuggestion getCounter(CtLoop ctLoop) { List statements = StatementUtil.getEffectiveStatements(ctLoop.getBody()); if (statements.isEmpty()) { @@ -72,6 +71,10 @@ private static LoopSuggestion getCounter(CtLoop ctLoop, CodeModel model) { CtStatement previous = StatementUtil.getPreviousStatement(ctLoop).orElse(null); + while (previous instanceof CtLocalVariable ctLocalVariable && !TypeUtil.isPrimitiveNumeric(ctLocalVariable.getType())) { + previous = StatementUtil.getPreviousStatement(previous).orElse(null); + } + if (!(previous instanceof CtLocalVariable ctLocalVariable) || !TypeUtil.isPrimitiveNumeric(ctLocalVariable.getType())) { return null; } @@ -203,7 +206,7 @@ public void process(CtLoop ctLoop) { return; } - LoopSuggestion forLoop = getCounter(ctLoop, staticAnalysis.getCodeModel()); + LoopSuggestion forLoop = getCounter(ctLoop); if (forLoop != null) { addLocalProblem( diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestLoopShouldBeFor.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestLoopShouldBeFor.java index 7473a2da..95a8753e 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestLoopShouldBeFor.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestLoopShouldBeFor.java @@ -257,6 +257,43 @@ void test(String[] array) { problems.assertExhausted(); } + @Test + void testCounterDeclaredWithOtherVariablesBeforeLoop() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + String test(String sentence) { + int c = 0; + boolean spaceBefore = true; + String word = ""; + while (c < sentence.length()) { + if (Character.isWhitespace(sentence.charAt(c))) { + spaceBefore = true; + } else if (spaceBefore) { + word += sentence.charAt(c); + spaceBefore = false; + } + c++; + } + return word; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsFor( + problems.next(), + "int c = 0", + "c < sentence.length()", + "c++", + "{%n if (Character.isWhitespace(sentence.charAt(c))) {%n spaceBefore = true;%n } else if (spaceBefore) {%n word += sentence.charAt(c);%n spaceBefore = false;%n }%n}".formatted() + ); + + problems.assertExhausted(); + } + @Test void testMissingUpdate() throws IOException, LinterException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( From e8b70fd2609d4ca7fc9f150d110239a1631be57c Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Wed, 18 Sep 2024 14:29:35 +0200 Subject: [PATCH 4/9] implement #528 --- .../check/api/ForLoopCanBeInvocation.java | 50 ++++++++++++------- .../autograder/core/check/api/UseSubList.java | 27 ++++++---- .../core/check/api/TestUseSubList.java | 41 ++++++++++----- 3 files changed, 79 insertions(+), 39 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/ForLoopCanBeInvocation.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/ForLoopCanBeInvocation.java index ab6d49b5..cdb4e48b 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/ForLoopCanBeInvocation.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/ForLoopCanBeInvocation.java @@ -9,6 +9,7 @@ import de.firemage.autograder.core.integrated.MethodUtil; import de.firemage.autograder.core.integrated.TypeUtil; import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtExpression; import spoon.reflect.code.CtForEach; import spoon.reflect.code.CtInvocation; import spoon.reflect.code.CtStatement; @@ -20,6 +21,13 @@ @ExecutableCheck(reportedProblems = { ProblemType.FOR_LOOP_CAN_BE_INVOCATION }) public class ForLoopCanBeInvocation extends IntegratedCheck { + static boolean isCollectionAddInvocation(CtInvocation ctInvocation) { + return ctInvocation.getTarget() != null + && TypeUtil.isSubtypeOf(ctInvocation.getTarget().getType(), java.util.Collection.class) + && MethodUtil.isSignatureEqualTo(ctInvocation.getExecutable(), boolean.class, "add", Object.class) + && ctInvocation.getExecutable().getParameters().size() == 1; + } + @Override protected void check(StaticAnalysis staticAnalysis) { staticAnalysis.processWith(new AbstractProcessor() { @@ -34,28 +42,36 @@ public void process(CtForEach ctFor) { return; } - if (statements.get(0) instanceof CtInvocation ctInvocation - && TypeUtil.isSubtypeOf(ctInvocation.getTarget().getType(), java.util.Collection.class) - && MethodUtil.isSignatureEqualTo(ctInvocation.getExecutable(), boolean.class, "add", Object.class) - && ctInvocation.getExecutable().getParameters().size() == 1 - // ensure that the add argument simply accesses the for variable: + // the body must be a single invocation of the add method on a collection + if (!(statements.get(0) instanceof CtInvocation ctInvocation) + || !isCollectionAddInvocation(ctInvocation)) { + return; + } + + CtExpression addArgument = ctInvocation.getArguments().get(0); + // allow explicit casting, for example you might do: + // for (int i : array) { + // collection.add((short) i); + // } + // which could not be replaced with a simple addAll invocation + if (!addArgument.getTypeCasts().isEmpty()) { + return; + } + + // handle edge case where the variable is implicitly cast in the invocation (Character in List, but char in iterable) + List> actualTypeArguments = ctInvocation.getTarget().getType().getActualTypeArguments(); + if (!actualTypeArguments.isEmpty() && !ctFor.getVariable().getType().equals(actualTypeArguments.get(0))) { + return; + } + + if (// ensure that the add argument simply accesses the for variable: // for (T t : array) { // collection.add(t); // } - && ctInvocation.getArguments().get(0) instanceof CtVariableRead ctVariableRead + addArgument instanceof CtVariableRead ctVariableRead && ctVariableRead.getVariable().equals(ctFor.getVariable().getReference())) { - // allow explicit casting - if (!ctInvocation.getArguments().get(0).getTypeCasts().isEmpty()) { - return; - } - - // handle edge case where the variable is implicitly cast in the invocation (Character in List, but char in iterable) - List> actualTypeArguments = ctInvocation.getTarget().getType().getActualTypeArguments(); - if (!actualTypeArguments.isEmpty() && !ctFor.getVariable().getType().equals(actualTypeArguments.get(0))) { - return; - } - + // special case for arrays String addAllArg = ctFor.getExpression().toString(); if (ctFor.getExpression().getType().isArray()) { addAllArg = "Arrays.asList(%s)".formatted(addAllArg); diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseSubList.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseSubList.java index cedb82c0..80ee5dfa 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseSubList.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseSubList.java @@ -8,18 +8,21 @@ import de.firemage.autograder.core.integrated.ExpressionUtil; import de.firemage.autograder.core.integrated.ForLoopRange; import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.StatementUtil; import de.firemage.autograder.core.integrated.StaticAnalysis; import spoon.processing.AbstractProcessor; import spoon.reflect.code.CtFor; +import spoon.reflect.code.CtInvocation; import spoon.reflect.code.CtLiteral; +import spoon.reflect.code.CtStatement; import spoon.reflect.declaration.CtVariable; import spoon.reflect.reference.CtTypeReference; +import java.util.List; import java.util.Map; @ExecutableCheck(reportedProblems = {ProblemType.COMMON_REIMPLEMENTATION_SUBLIST}) public class UseSubList extends IntegratedCheck { - private void checkSubList(CtFor ctFor) { ForLoopRange forLoopRange = ForLoopRange.fromCtFor(ctFor).orElse(null); @@ -28,6 +31,7 @@ private void checkSubList(CtFor ctFor) { } // ensure that the variable is only used to access the list elements via get + // like list.get(i) CtVariable ctListVariable = ForToForEachLoop.getForEachLoopVariable( ctFor, forLoopRange, @@ -38,12 +42,6 @@ private void checkSubList(CtFor ctFor) { return; } - CtTypeReference listElementType = ctFor.getFactory().createCtTypeReference(java.lang.Object.class); - // size != 1, if the list is a raw type: List list = new ArrayList(); - if (ctListVariable.getType().getActualTypeArguments().size() == 1) { - listElementType = ctListVariable.getType().getActualTypeArguments().get(0); - } - // check if the loop iterates over the whole list (then it is covered by the foreach loop check) if (ExpressionUtil.resolveConstant(forLoopRange.start()) instanceof CtLiteral ctLiteral && ctLiteral.getValue() == 0 @@ -51,14 +49,25 @@ private void checkSubList(CtFor ctFor) { return; } + // look for a single statement in the loop body, which should be a single invocation to add + List statementList = StatementUtil.getEffectiveStatements(ctFor.getBody()); + if (statementList.size() != 1) { + return; + } + + // should look like this: result.add(list.get(i)) + if (!(statementList.get(0) instanceof CtInvocation ctInvocation) || !ForLoopCanBeInvocation.isCollectionAddInvocation(ctInvocation)) { + return; + } + this.addLocalProblem( ctFor, new LocalizedMessage( "common-reimplementation", Map.of( - "suggestion", "for (%s value : %s.subList(%s, %s)) { ... }".formatted( - listElementType.unbox(), + "suggestion", "%s.addAll(%s.subList(%s, %s))".formatted( + ctInvocation.getTarget(), ctListVariable.getSimpleName(), forLoopRange.start(), forLoopRange.end() diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseSubList.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseSubList.java index 05f9f995..85658953 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseSubList.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseSubList.java @@ -39,32 +39,39 @@ void testSubList() throws LinterException, IOException { """ import java.util.ArrayList; import java.util.List; + import java.util.Collection; public class Test { - public static void printList(List list, int start, int end) { + public static Collection printList(List list, int start, int end) { + Collection result = new ArrayList<>(); for (int i = start; i < end; i++) { - System.out.println(list.get(i)); + result.add(list.get(i)); } + return result; } - public static void printRawList(List list, int start, int end) { + public static Collection printRawList(List list, int start, int end) { + Collection result = new ArrayList(); for (int i = start; i < end; i++) { - System.out.println(list.get(i)); + result.add(list.get(i)); } + return result; } - public static void printList2(List list, int start, int end) { + public static Collection printList2(List list, int start, int end) { + Collection result = new ArrayList<>(); for (int i = start; i < end; i++) { - System.out.println(list.get(i)); + result.add(list.get(i)); } + return result; } } """ ), PROBLEM_TYPES); - assertEqualsReimplementation(problems.next(), "for (T value : list.subList(start, end)) { ... }"); - assertEqualsReimplementation(problems.next(), "for (Object value : list.subList(start, end)) { ... }"); - assertEqualsReimplementation(problems.next(), "for (int value : list.subList(start, end)) { ... }"); + assertEqualsReimplementation(problems.next(), "result.addAll(list.subList(start, end))"); + assertEqualsReimplementation(problems.next(), "result.addAll(list.subList(start, end))"); + assertEqualsReimplementation(problems.next(), "result.addAll(list.subList(start, end))"); problems.assertExhausted(); } @@ -75,12 +82,16 @@ void testEntireList() throws LinterException, IOException { "Test", """ import java.util.List; + import java.util.ArrayList; + import java.util.Collection; public class Test { - public static void iter(List storage) { + public static Collection iter(List storage) { + Collection result = new ArrayList<>(); for (int i = 0; i < storage.size(); i++) { - System.out.println(storage.get(i)); + result.add(storage.get(i)); } + return result; } } """ @@ -96,12 +107,16 @@ void testMap() throws LinterException, IOException { "Test", """ import java.util.Map; + import java.util.ArrayList; + import java.util.Collection; public class Test { - public static void iterMap(Map storage) { + public static Collection iterMap(Map storage) { + Collection result = new ArrayList<>(); for (int i = 0; i < storage.size(); i++) { - System.out.println(storage.get(i)); + result.add(storage.get(i)); } + return result; } } """ From 838926fd5ead5088263bfafa9423054c54fd7b61 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Wed, 18 Sep 2024 15:05:15 +0200 Subject: [PATCH 5/9] implement #502 --- .../check/api/CheckIterableDuplicates.java | 120 +++++++++++++++--- .../api/TestCheckIterableDuplicates.java | 106 ++++++++++++++++ 2 files changed, 205 insertions(+), 21 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CheckIterableDuplicates.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CheckIterableDuplicates.java index 66044d7c..06ac5052 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CheckIterableDuplicates.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CheckIterableDuplicates.java @@ -21,7 +21,9 @@ import spoon.reflect.code.CtVariableRead; import spoon.reflect.code.UnaryOperatorKind; import spoon.reflect.reference.CtTypeReference; +import spoon.reflect.reference.CtVariableReference; +import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -45,6 +47,24 @@ private static String buildSuggestion(CtExpression ctExpression, boolean isNe return "new HashSet<>(%s).size() == %s".formatted(leftSide, rightSide); } + private static boolean isAddInvocationOnSet(CtInvocation ctInvocation, CtVariableReference argument) { + return TypeUtil.isTypeEqualTo(ctInvocation.getExecutable().getType(), boolean.class) + && ctInvocation.getExecutable().getSimpleName().equals("add") + && ctInvocation.getArguments().size() == 1 + && ctInvocation.getArguments().get(0) instanceof CtVariableRead ctVariableRead + && ctVariableRead.getVariable().equals(argument) + && TypeUtil.isSubtypeOf(ctInvocation.getTarget().getType(), java.util.Set.class); + } + + private static boolean isContainsInvocationOnSet(CtInvocation ctInvocation, CtVariableReference argument) { + return TypeUtil.isTypeEqualTo(ctInvocation.getExecutable().getType(), boolean.class) + && ctInvocation.getExecutable().getSimpleName().equals("contains") + && ctInvocation.getArguments().size() == 1 + && ctInvocation.getArguments().get(0) instanceof CtVariableRead ctVariableRead + && ctVariableRead.getVariable().equals(argument) + && TypeUtil.isSubtypeOf(ctInvocation.getTarget().getType(), java.util.Set.class); + } + @Override protected void check(StaticAnalysis staticAnalysis) { staticAnalysis.processWith(new AbstractProcessor() { @@ -55,34 +75,76 @@ public void process(CtForEach ctForEach) { } List statements = StatementUtil.getEffectiveStatements(ctForEach.getBody()); - if (statements.size() != 1 || !(statements.get(0) instanceof CtIf ctIf)) { + if (statements.isEmpty() || !(statements.get(0) instanceof CtIf ctIf) || ctIf.getThenStatement() == null) { return; } - // the if should only have a then statement - if (ctIf.getElseStatement() != null || ctIf.getThenStatement() == null) { - return; - } + // one can implement this in multiple ways, for example: + // for (var elem : list) { + // if (!set.add(elem)) { + // return false; + // } + // } + // or one could have a contains and then an add statement: + // for (var elem : list) { + // if (set.contains(elem)) { + // return false; + // } + // set.add(elem); + // } List ifStatements = StatementUtil.getEffectiveStatements(ctIf.getThenStatement()); if (ifStatements.isEmpty()) { return; } - CtLiteral effectValue = null; - if (ifStatements.size() == 1 - && ifStatements.get(0) instanceof CtReturn ctReturn - && ctReturn.getReturnedExpression() instanceof CtLiteral ctLiteral) { - effectValue = ctLiteral; + // TODO: the contains might be negated + if ((ctIf.getElseStatement() != null || statements.size() == 2) + && ctIf.getCondition() instanceof CtInvocation ctInvocation + && isContainsInvocationOnSet(ctInvocation, ctForEach.getVariable().getReference())) { + // it invokes contains, so the else must have the add invocation: + + List elseStatements = new ArrayList<>(); + if (statements.size() == 2) { + elseStatements.add(statements.get(1)); + } else { + elseStatements = StatementUtil.getEffectiveStatements(ctIf.getElseStatement()); + } + + CtLiteral effectValue = getEffectValue(ifStatements); + if (effectValue != null + && effectValue.getValue() instanceof Boolean value + && elseStatements.size() == 1 + && elseStatements.get(0) instanceof CtInvocation ctElseInvocation + && isAddInvocationOnSet(ctElseInvocation, ctForEach.getVariable().getReference())) { + String suggestion = buildSuggestion(ctForEach.getExpression(), Boolean.TRUE.equals(value)); + + addLocalProblem( + ctForEach, + new LocalizedMessage( + "common-reimplementation", + Map.of( + "suggestion", suggestion + ) + ), + ProblemType.COMMON_REIMPLEMENTATION_ITERABLE_DUPLICATES + ); + return; + } } - if (ifStatements.size() == 2 - && ifStatements.get(0) instanceof CtAssignment ctAssignment - && ctAssignment.getAssignment() instanceof CtLiteral ctLiteral - && ifStatements.get(1) instanceof CtBreak) { - effectValue = ctLiteral; + if (statements.size() != 1) { + return; } + + // the if should only have a then statement + if (ctIf.getElseStatement() != null) { + return; + } + + CtLiteral effectValue = getEffectValue(ifStatements); + if (effectValue == null || !(effectValue.getValue() instanceof Boolean value)) { return; } @@ -93,12 +155,7 @@ public void process(CtForEach ctForEach) { if (!(ctIf.getCondition() instanceof CtUnaryOperator ctUnaryOperator && ctUnaryOperator.getKind() == UnaryOperatorKind.NOT && ctUnaryOperator.getOperand() instanceof CtInvocation ctInvocation - && TypeUtil.isTypeEqualTo(ctInvocation.getExecutable().getType(), boolean.class) - && ctInvocation.getExecutable().getSimpleName().equals("add") - && ctInvocation.getArguments().size() == 1 - && ctInvocation.getArguments().get(0) instanceof CtVariableRead ctVariableRead - && ctVariableRead.getVariable().equals(ctForEach.getVariable().getReference()) - && TypeUtil.isSubtypeOf(ctInvocation.getTarget().getType(), java.util.Set.class))) + && isAddInvocationOnSet(ctInvocation, ctForEach.getVariable().getReference()))) { return; } @@ -118,4 +175,25 @@ public void process(CtForEach ctForEach) { } }); } + + // this extracts the return value from the statements, depending on where it is used it could be a + // return + // or + // = ; break; (this would be used in a loop) + private static CtLiteral getEffectValue(List ifStatements) { + CtLiteral effectValue = null; + if (ifStatements.size() == 1 + && ifStatements.get(0) instanceof CtReturn ctReturn + && ctReturn.getReturnedExpression() instanceof CtLiteral ctLiteral) { + effectValue = ctLiteral; + } + + if (ifStatements.size() == 2 + && ifStatements.get(0) instanceof CtAssignment ctAssignment + && ctAssignment.getAssignment() instanceof CtLiteral ctLiteral + && ifStatements.get(1) instanceof CtBreak) { + effectValue = ctLiteral; + } + return effectValue; + } } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCheckIterableDuplicates.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCheckIterableDuplicates.java index a547e478..c61efaa2 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCheckIterableDuplicates.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCheckIterableDuplicates.java @@ -30,6 +30,112 @@ private void assertReimplementation(Problem problem, String suggestion) { ); } + @Test + void testContainsImplicitElse() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.Set; + import java.util.HashSet; + import java.util.List; + + public class Test { + public static void main(String[] args) { + } + + private static boolean hasDuplicates(List list) { + Set uniqueElements = new HashSet<>(); + + for (String element : list) { + if (uniqueElements.contains(element)) { + return true; // Found a duplicate + } + + uniqueElements.add(element); + } + + return false; // No duplicates found + } + } + """ + ), PROBLEM_TYPES); + + assertReimplementation(problems.next(), "new HashSet<>(list).size() != list.size()"); + + problems.assertExhausted(); + } + + @Test + void testContainsExplicitElse() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.Set; + import java.util.HashSet; + import java.util.List; + + public class Test { + public static void main(String[] args) { + } + + private static boolean hasDuplicates(List list) { + Set uniqueElements = new HashSet<>(); + + for (String element : list) { + if (uniqueElements.contains(element)) { + return true; // Found a duplicate + } else { + uniqueElements.add(element); + } + } + + return false; // No duplicates found + } + } + """ + ), PROBLEM_TYPES); + + assertReimplementation(problems.next(), "new HashSet<>(list).size() != list.size()"); + + problems.assertExhausted(); + } + + @Test + void testContainsAfterIf() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.Set; + import java.util.HashSet; + import java.util.List; + + public class Test { + public static void main(String[] args) { + } + + private static boolean hasDuplicates(List list) { + Set uniqueElements = new HashSet<>(); + + for (String element : list) { + uniqueElements.add(element); + + if (uniqueElements.contains(element)) { + return true; // Found a duplicate + } + } + + return false; // No duplicates found + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + @Test void testReturnList() throws LinterException, IOException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( From 6157e5101d0c26a0747b2ea0b28e5cf89ccaef42 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Wed, 18 Sep 2024 15:05:46 +0200 Subject: [PATCH 6/9] remove committed todo --- .../autograder/core/check/api/CheckIterableDuplicates.java | 1 - 1 file changed, 1 deletion(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CheckIterableDuplicates.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CheckIterableDuplicates.java index 06ac5052..79cf45a5 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CheckIterableDuplicates.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CheckIterableDuplicates.java @@ -98,7 +98,6 @@ public void process(CtForEach ctForEach) { return; } - // TODO: the contains might be negated if ((ctIf.getElseStatement() != null || statements.size() == 2) && ctIf.getCondition() instanceof CtInvocation ctInvocation && isContainsInvocationOnSet(ctInvocation, ctForEach.getVariable().getReference())) { From e3d1e7bb530bf8df1c88c8a92ff3685c0e386900 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 21 Sep 2024 08:47:19 +0200 Subject: [PATCH 7/9] reduce avoid shadowing annotations #525 --- .../core/check/general/AvoidShadowing.java | 42 +++++++++++++------ .../core/integrated/UsesFinder.java | 11 ++--- .../check/general/TestAvoidShadowing.java | 21 ++++++++++ 3 files changed, 56 insertions(+), 18 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidShadowing.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidShadowing.java index 0fe7e598..b353c1fd 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidShadowing.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidShadowing.java @@ -6,8 +6,10 @@ import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.StaticAnalysis; import de.firemage.autograder.core.integrated.MethodUtil; +import de.firemage.autograder.core.integrated.TypeUtil; import de.firemage.autograder.core.integrated.UsesFinder; import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtVariableRead; import spoon.reflect.declaration.CtConstructor; import spoon.reflect.declaration.CtElement; import spoon.reflect.declaration.CtMethod; @@ -15,7 +17,6 @@ import spoon.reflect.declaration.CtTypeInformation; import spoon.reflect.declaration.CtVariable; import spoon.reflect.reference.CtFieldReference; -import spoon.reflect.reference.CtTypeReference; import java.util.ArrayList; import java.util.Collection; @@ -25,23 +26,25 @@ @ExecutableCheck(reportedProblems = {ProblemType.AVOID_SHADOWING}) public class AvoidShadowing extends IntegratedCheck { + // a lower bound for the number of reads on a hidden field, before it is reported + private static final int MINIMUM_FIELD_READS = 2; private static final List ALLOWED_FIELDS = List.of("serialVersionUID"); private static Collection> getAllVisibleFields(CtTypeInformation ctTypeInformation) { - Collection> result = new ArrayList<>(ctTypeInformation.getDeclaredFields()); - CtTypeReference parent = ctTypeInformation.getSuperclass(); - while (parent != null) { + for (CtType parent : TypeUtil.allSuperTypes(ctTypeInformation)) { + if (parent.isInterface()) { + continue; + } + result.addAll( parent.getDeclaredFields() - .stream() - // only non-private fields are visible to a subclass - .filter(ctFieldReference -> !ctFieldReference.getFieldDeclaration().isPrivate()) - .toList() + .stream() + // only non-private fields are visible to a subclass + .filter(ctFieldReference -> !ctFieldReference.getFieldDeclaration().isPrivate()) + .toList() ); - - parent = parent.getSuperclass(); } return result; @@ -90,12 +93,25 @@ public void process(CtVariable ctVariable) { CtElement variableParent = ctVariable.getParent(); - // there might be multiple fields hidden by the variable (e.g. subclass hides superclass field) - boolean isFieldRead = hiddenFields.stream().anyMatch(ctFieldReference -> UsesFinder.variableUses(ctFieldReference.getFieldDeclaration()).nestedIn(variableParent).hasAny()); + // there might be multiple fields hidden by the variable, like for example: + // class A { + // int a; + // } + // + // class B extends A { + // int a; + // void foo(int a) {} // param hides field of A and B + // } + int numberOfFieldReads = Math.toIntExact(hiddenFields.stream() + .map(ctFieldReference -> UsesFinder.variableUses(ctFieldReference.getFieldDeclaration()) + .nestedIn(variableParent) + .filter(ctVariableAccess -> ctVariableAccess instanceof CtVariableRead) + .count()).max(Long::compareTo).orElse(0L)); + // to reduce the number of annotations, we only report a problem if the variable AND the hidden field are read in // the same context - if (UsesFinder.variableUses(ctVariable).nestedIn(variableParent).hasAny() && isFieldRead) { + if (UsesFinder.variableUses(ctVariable).nestedIn(variableParent).hasAny() && numberOfFieldReads >= MINIMUM_FIELD_READS) { addLocalProblem( ctVariable, new LocalizedMessage("avoid-shadowing", Map.of("name", ctVariable.getSimpleName())), 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 14f22c8e..3be22dbb 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 @@ -31,6 +31,7 @@ import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Collections; import java.util.Deque; import java.util.HashMap; import java.util.IdentityHashMap; @@ -90,17 +91,17 @@ public static CtElementStream getAllUses(CtNamedElement element) { } public static CtElementStream> variableUses(CtVariable variable) { - return CtElementStream.of(UsesFinder.getFor(variable).scanner.variableUses.getOrDefault(variable, List.of())).assumeElementType(); + return CtElementStream.of(UsesFinder.getFor(variable).scanner.variableUses.getOrDefault(variable, Set.of())).assumeElementType(); } @SuppressWarnings("unchecked") public static CtElementStream> variableWrites(CtVariable variable) { - return (CtElementStream>) (Object) CtElementStream.of(UsesFinder.getFor(variable).scanner.variableUses.getOrDefault(variable, List.of())).assumeElementType().ofType(CtVariableWrite.class); + return (CtElementStream>) (Object) CtElementStream.of(UsesFinder.getFor(variable).scanner.variableUses.getOrDefault(variable, Set.of())).assumeElementType().ofType(CtVariableWrite.class); } @SuppressWarnings("unchecked") public static CtElementStream> variableReads(CtVariable variable) { - return (CtElementStream>) (Object) CtElementStream.of(UsesFinder.getFor(variable).scanner.variableUses.getOrDefault(variable, List.of())).assumeElementType().ofType(CtVariableRead.class); + return (CtElementStream>) (Object) CtElementStream.of(UsesFinder.getFor(variable).scanner.variableUses.getOrDefault(variable, Set.of())).assumeElementType().ofType(CtVariableRead.class); } public static CtElementStream typeParameterUses(CtTypeParameter typeParameter) { @@ -192,7 +193,7 @@ public static CtVariable getDeclaredVariable(CtVariableAccess ctVariableAc 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 - private final Map> variableUses = 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<>(); @@ -317,7 +318,7 @@ private void recordVariableAccess(CtVariableAccess variableAccess) { } if (variable != null) { - var accesses = this.variableUses.computeIfAbsent(variable, k -> new ArrayList<>()); + var accesses = this.variableUses.computeIfAbsent(variable, k -> Collections.newSetFromMap(new IdentityHashMap<>())); accesses.add(variableAccess); this.variableAccessDeclarations.put(variableAccess, variable); } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestAvoidShadowing.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestAvoidShadowing.java index 2f4ecf2b..be8225c0 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestAvoidShadowing.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestAvoidShadowing.java @@ -108,6 +108,8 @@ public Main() { public void doSomething() { // both fields are used: System.out.println(this.number); + System.out.println(this.number); + System.out.println(super.number); System.out.println(super.number); } @@ -145,6 +147,7 @@ public Main() { public void doSomething() { int number = 6; + System.out.println(this.number); System.out.println(this.number); System.out.println(number); } @@ -198,8 +201,11 @@ public void doSomething() { int number = 6; System.out.println(this.number); + System.out.println(this.number); + System.out.println(super.number); System.out.println(super.number); System.out.println(number); + System.out.println(number); } public static void main(String[] args) { @@ -246,8 +252,11 @@ public void doSomething() { int number = 6; System.out.println(Main.number); + System.out.println(Main.number); + System.out.println(super.number); System.out.println(super.number); System.out.println(number); + System.out.println(number); } public static void main(String[] args) { @@ -294,8 +303,12 @@ public B(int b, int c) { this.c = c; System.out.println(b); + System.out.println(b); + System.out.println(this.b); System.out.println(this.b); System.out.println(c); + System.out.println(c); + System.out.println(this.c); System.out.println(this.c); } @@ -310,10 +323,14 @@ private void foo() { int y = 5; /*# not ok #*/ final int z = 5; /*# ok #*/ + System.out.println("" + a + this.a); System.out.println("" + a + this.a); System.out.println("" + x + this.x); + System.out.println("" + x + this.x); + System.out.println("" + y + A.y); System.out.println("" + y + A.y); System.out.println("" + z); + System.out.println("" + z); } } """ @@ -329,8 +346,12 @@ class C extends A { void doSomething() { System.out.println("" + super.a + this.a); + System.out.println("" + super.a + this.a); + System.out.println("" + super.x + this.x); System.out.println("" + super.x + this.x); System.out.println("" + A.y + C.y); + System.out.println("" + A.y + C.y); + System.out.println("" + z); System.out.println("" + z); } } From cfaf760f1a5044d4ddfbce91a957897f83e96768 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 21 Sep 2024 09:36:03 +0200 Subject: [PATCH 8/9] implement #540 --- .../firemage/autograder/core/ProblemType.java | 6 + .../core/check/api/IsLetterOrDigit.java | 109 ++++++++++++++++++ .../core/check/api/TestIsLetterOrDigit.java | 70 +++++++++++ sample_config.yaml | 1 + 4 files changed, 186 insertions(+) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/api/IsLetterOrDigit.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestIsLetterOrDigit.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 279c6232..82fb060b 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 @@ -1030,6 +1030,12 @@ public enum ProblemType implements AbstractProblemType { @HasFalsePositives AVOID_STRING_CONCAT, + /** + * Reports code where {@link Character#isLetterOrDigit(char)} could be used. + */ + @HasFalsePositives + IS_LETTER_OR_DIGIT, + /** * Reports unnecessary comments. */ diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/IsLetterOrDigit.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/IsLetterOrDigit.java new file mode 100644 index 00000000..2a813066 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/IsLetterOrDigit.java @@ -0,0 +1,109 @@ +package de.firemage.autograder.core.check.api; + + +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.ExecutableCheck; +import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.MethodUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.TypeUtil; +import spoon.processing.AbstractProcessor; +import spoon.reflect.code.BinaryOperatorKind; +import spoon.reflect.code.CtBinaryOperator; +import spoon.reflect.code.CtInvocation; +import spoon.reflect.code.CtTypeAccess; +import spoon.reflect.code.CtUnaryOperator; +import spoon.reflect.code.UnaryOperatorKind; +import spoon.reflect.declaration.CtElement; + +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +@ExecutableCheck(reportedProblems = { ProblemType.IS_LETTER_OR_DIGIT }) +public class IsLetterOrDigit extends IntegratedCheck { + private static final Set SUPPORTED_OPERATORS = Set.of( + BinaryOperatorKind.OR, + BinaryOperatorKind.AND + ); + + private static boolean isLetterInvocation(CtInvocation ctInvocation) { + return ctInvocation.getTarget() != null + && ctInvocation.getTarget() instanceof CtTypeAccess ctTypeAccess + && TypeUtil.isTypeEqualTo(ctTypeAccess.getAccessedType(), Character.class) + && MethodUtil.isSignatureEqualTo(ctInvocation.getExecutable(), boolean.class, "isLetter", char.class); + } + + private static boolean isDigitInvocation(CtInvocation ctInvocation) { + return ctInvocation.getTarget() != null + && ctInvocation.getTarget() instanceof CtTypeAccess ctTypeAccess + && TypeUtil.isTypeEqualTo(ctTypeAccess.getAccessedType(), Character.class) + && MethodUtil.isSignatureEqualTo(ctInvocation.getExecutable(), boolean.class, "isDigit", char.class); + } + + @Override + protected void check(StaticAnalysis staticAnalysis) { + staticAnalysis.processWith(new AbstractProcessor>() { + @Override + public void process(CtBinaryOperator ctBinaryOperator) { + if (ctBinaryOperator.isImplicit() + || !ctBinaryOperator.getPosition().isValidPosition() + || !SUPPORTED_OPERATORS.contains(ctBinaryOperator.getKind())) { + return; + } + + CtInvocation leftInvocation; + CtInvocation rightInvocation; + boolean isNegated = false; + + // check for !left && !right + if (ctBinaryOperator.getKind() == BinaryOperatorKind.AND + && ctBinaryOperator.getLeftHandOperand() instanceof CtUnaryOperator leftUnaryOperator + && ctBinaryOperator.getRightHandOperand() instanceof CtUnaryOperator rightUnaryOperator + && leftUnaryOperator.getKind() == UnaryOperatorKind.NOT + && rightUnaryOperator.getKind() == UnaryOperatorKind.NOT + && leftUnaryOperator.getOperand() instanceof CtInvocation left + && rightUnaryOperator.getOperand() instanceof CtInvocation right) { + leftInvocation = left; + rightInvocation = right; + isNegated = true; + } else if (ctBinaryOperator.getKind() == BinaryOperatorKind.AND) { + return; + } else if (ctBinaryOperator.getLeftHandOperand() instanceof CtInvocation left + && ctBinaryOperator.getRightHandOperand() instanceof CtInvocation right) { + leftInvocation = left; + rightInvocation = right; + } else { + return; + } + + if ((!(isLetterInvocation(leftInvocation) && isDigitInvocation(rightInvocation)) + && !(isDigitInvocation(leftInvocation) && isLetterInvocation(rightInvocation))) + || !leftInvocation.getArguments().equals(rightInvocation.getArguments())) { + return; + } + + String suggestion = "Character.isLetterOrDigit(%s)".formatted(leftInvocation.getArguments() + .stream() + .map(CtElement::toString) + .collect(Collectors.joining(", "))); + + if (isNegated) { + suggestion = "!" + suggestion; + } + + addLocalProblem( + ctBinaryOperator, + new LocalizedMessage( + "common-reimplementation", + Map.of( + "suggestion", suggestion + ) + ), + ProblemType.IS_LETTER_OR_DIGIT + ); + } + }); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestIsLetterOrDigit.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestIsLetterOrDigit.java new file mode 100644 index 00000000..8153639a --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestIsLetterOrDigit.java @@ -0,0 +1,70 @@ +package de.firemage.autograder.core.check.api; + +import de.firemage.autograder.api.JavaVersion; +import de.firemage.autograder.api.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.file.StringSourceInfo; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestIsLetterOrDigit extends AbstractCheckTest { + private static final String LOCALIZED_MESSAGE_KEY = "common-reimplementation"; + private static final List PROBLEM_TYPES = List.of(ProblemType.IS_LETTER_OR_DIGIT); + + private void assertEqualsLetterOrDigit(Problem problem, String suggestion) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + LOCALIZED_MESSAGE_KEY, + Map.of("suggestion", suggestion) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @ParameterizedTest + @CsvSource( + delimiter = ';', + useHeadersInDisplayName = true, + value = { + " Expression ; Expected ", + " Character.isLetter(a) || Character.isDigit(a) ; Character.isLetterOrDigit(a) ", + " !(Character.isLetter(a) || Character.isDigit(a)) ; Character.isLetterOrDigit(a) ", + " !(Character.isLetter(a) || Character.isDigit(b)) ; ", + " !Character.isLetter(a) || !Character.isDigit(b) ; ", + " Character.isLetter(a) || Character.isDigit(b) ; ", + " Character.isLetter(a) || Character.isLetter(a) ; ", + " Character.isDigit(a) || Character.isDigit(a) ; ", + " Character.isLetter(a) && Character.isDigit(a) ; ", + " !Character.isLetter(a) && !Character.isDigit(a) ; !Character.isLetterOrDigit(a) ", + } + ) + void testExpressions(String expression, String expected) throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static boolean test(char a, char b, char c) { + return %s; + } + } + """.formatted(expression) + ), PROBLEM_TYPES); + + if (expected != null) { + assertEqualsLetterOrDigit(problems.next(), expected); + } + + problems.assertExhausted(); + } +} diff --git a/sample_config.yaml b/sample_config.yaml index 6bf46190..175cf96f 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -157,3 +157,4 @@ problemsToReport: - LOOP_SHOULD_BE_FOR - METHOD_SHOULD_BE_STATIC - METHOD_SHOULD_BE_STATIC_NOT_PUBLIC + - IS_LETTER_OR_DIGIT From cdc6b6e61239dd51e98eb215d6c3c747809f6e7a Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 24 Sep 2024 08:43:10 +0200 Subject: [PATCH 9/9] expose maximum problems for check in `autograder-api` (necessary for artemis4j) --- .../java/de/firemage/autograder/api/AbstractProblem.java | 5 +++++ .../src/main/java/de/firemage/autograder/core/Problem.java | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/autograder-api/src/main/java/de/firemage/autograder/api/AbstractProblem.java b/autograder-api/src/main/java/de/firemage/autograder/api/AbstractProblem.java index adf4e022..d99d204f 100644 --- a/autograder-api/src/main/java/de/firemage/autograder/api/AbstractProblem.java +++ b/autograder-api/src/main/java/de/firemage/autograder/api/AbstractProblem.java @@ -1,5 +1,7 @@ package de.firemage.autograder.api; +import java.util.Optional; + public interface AbstractProblem { String getCheckName(); Translatable getLinterName(); @@ -7,4 +9,7 @@ public interface AbstractProblem { String getDisplayLocation(); AbstractCodePosition getPosition(); String getType(); + default Optional getMaximumProblemsForCheck() { + return Optional.empty(); + } } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/Problem.java b/autograder-core/src/main/java/de/firemage/autograder/core/Problem.java index 3ffbe092..7a44dfed 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/Problem.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/Problem.java @@ -4,6 +4,8 @@ import de.firemage.autograder.api.Translatable; import de.firemage.autograder.core.check.Check; +import java.util.Optional; + /** * Contains the default implementation of most {@link AbstractProblem} methods. */ @@ -63,6 +65,11 @@ public String getType() { return this.problemType.toString(); } + @Override + public Optional getMaximumProblemsForCheck() { + return this.getCheck().maximumProblems(); + } + public ProblemType getProblemType() { return problemType; }