From 5a5b8eee1e2a631bd70ccaee954a345fad14a6f4 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 28 Sep 2024 09:37:25 +0200 Subject: [PATCH 01/17] fix endless loop in leaked collection check --- .../core/check/oop/LeakedCollectionCheck.java | 169 ++++++++++-------- .../check/oop/TestLeakedCollectionCheck.java | 26 +++ 2 files changed, 124 insertions(+), 71 deletions(-) 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 47cc450c..c23265df 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 @@ -44,9 +44,11 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; // What should be supported: @@ -116,92 +118,117 @@ && isMutableExpression(defaultExpression)) { && isMutableExpression(ctAssignment.getAssignment())); } - /** - * Checks if the state of the given expression can be mutated. - * - * @param ctExpression the expression to check - * @return true if the expression is mutable, false otherwise - */ - private static boolean isMutableExpression(CtExpression ctExpression) { - if (ctExpression instanceof CtNewArray) { - return true; + private record MutableExpressionChecker(Set> currentlyChecking) { + private static boolean check(CtExpression ctExpression) { + var instance = new MutableExpressionChecker(Collections.newSetFromMap(new IdentityHashMap<>())); + return instance.isMutableExpression(ctExpression); } - // we only care about arrays and collections for now - if (!TypeUtil.isSubtypeOf(ctExpression.getType(), java.util.Collection.class)) { - // not a collection - return false; - } + private boolean isMutableExpression(CtExpression ctExpression) { + if (ctExpression instanceof CtNewArray) { + return true; + } - // something like new ArrayList<>() is always mutable - if (ctExpression instanceof CtConstructorCall) { - return true; - } + // we only care about arrays and collections for now + if (!TypeUtil.isSubtypeOf(ctExpression.getType(), java.util.Collection.class)) { + // not a collection + return false; + } + + // something like new ArrayList<>() is always mutable + if (ctExpression instanceof CtConstructorCall) { + return true; + } + + CtExecutable ctExecutable = ctExpression.getParent(CtExecutable.class); - CtExecutable ctExecutable = ctExpression.getParent(CtExecutable.class); - - // Sometimes we have this - // - // ``` - // List values = new ArrayList<>(); - // values.add("foo"); - // this.values = values; - // ``` - // - // or this - // - // ``` - // public Constructor(List values) { - // this.values = values; - // } - // ``` - // - // In both cases the assigned expression is mutable, and we have to detect this. - // To do this, we check if the assigned variable is mutable. - if (ctExpression instanceof CtVariableRead ctVariableRead && ctExecutable != null) { - CtVariable ctVariable = VariableUtil.getVariableDeclaration(ctVariableRead.getVariable()); - - // this is a special case for enums, where the constructor is private and mutability can only be introduced - // through the enum constructor calls - if (ctExecutable instanceof CtConstructor ctConstructor - && ctConstructor.getDeclaringType() instanceof CtEnum ctEnum - && hasAssignedParameterReference(ctExpression, ctConstructor)) { - // figure out the index of the parameter reference: - CtParameter ctParameterToFind = findParameterReference(ctExpression, ctConstructor).orElseThrow(); - int index = -1; - for (CtParameter ctParameter : ctConstructor.getParameters()) { - index += 1; - if (ctParameter == ctParameterToFind) { - break; + // Sometimes we have this + // + // ``` + // List values = new ArrayList<>(); + // values.add("foo"); + // this.values = values; + // ``` + // + // or this + // + // ``` + // public Constructor(List values) { + // this.values = values; + // } + // ``` + // + // In both cases the assigned expression is mutable, and we have to detect this. + // To do this, we check if the assigned variable is mutable. + if (ctExpression instanceof CtVariableRead ctVariableRead && ctExecutable != null) { + CtVariable ctVariable = VariableUtil.getVariableDeclaration(ctVariableRead.getVariable()); + + // this is a special case for enums, where the constructor is private and mutability can only be introduced + // through the enum constructor calls + if (ctExecutable instanceof CtConstructor ctConstructor + && ctConstructor.getDeclaringType() instanceof CtEnum ctEnum + && hasAssignedParameterReference(ctExpression, ctConstructor)) { + // figure out the index of the parameter reference: + CtParameter ctParameterToFind = findParameterReference(ctExpression, ctConstructor).orElseThrow(); + int index = -1; + for (CtParameter ctParameter : ctConstructor.getParameters()) { + index += 1; + if (ctParameter == ctParameterToFind) { + break; + } } - } - if (index >= ctConstructor.getParameters().size() || index == -1) { - throw new IllegalStateException("Could not find parameter reference of %s in %s".formatted(ctExpression, ctConstructor)); - } + if (index >= ctConstructor.getParameters().size() || index == -1) { + throw new IllegalStateException("Could not find parameter reference of %s in %s".formatted(ctExpression, ctConstructor)); + } - for (CtEnumValue ctEnumValue : ctEnum.getEnumValues()) { - if (ctEnumValue.getDefaultExpression() instanceof CtConstructorCall ctConstructorCall - && ctConstructorCall.getExecutable().getExecutableDeclaration() == ctConstructor - && isMutableExpression(ctConstructorCall.getArguments().get(index))) { - return true; + for (CtEnumValue ctEnumValue : ctEnum.getEnumValues()) { + if (ctEnumValue.getDefaultExpression() instanceof CtConstructorCall ctConstructorCall + && ctConstructorCall.getExecutable().getExecutableDeclaration() == ctConstructor + && isMutableExpression(ctConstructorCall.getArguments().get(index))) { + return true; + } } + + return false; } - return false; - } + if (hasAssignedParameterReference(ctVariableRead, ctExecutable) || (ctVariable.getDefaultExpression() != null && isMutableExpression(ctVariable.getDefaultExpression()))) { + return true; + } - if (hasAssignedParameterReference(ctVariableRead, ctExecutable) || (ctVariable.getDefaultExpression() != null && isMutableExpression(ctVariable.getDefaultExpression()))) { - return true; + // the variable is mutable if it is assigned a mutable value in the declaration or through any write + return UsesFinder.variableWrites(ctVariable) + .hasAnyMatch(ctVariableWrite -> { + if (!(ctVariableWrite.getParent() instanceof CtAssignment ctAssignment)) { + return false; + } + + // this prevents infinite recursion when the variable is assigned to itself or there are assignment cycles + if (!this.currentlyChecking.add(ctAssignment.getAssignment())) { + return false; + } + + boolean result = this.isMutableExpression(ctAssignment.getAssignment()); + + this.currentlyChecking.remove(ctAssignment.getAssignment()); + + return result; + }); } - // the variable is mutable if it is assigned a mutable value in the declaration or through any write - return UsesFinder.variableWrites(ctVariable) - .hasAnyMatch(ctVariableWrite -> ctVariableWrite.getParent() instanceof CtAssignment ctAssignment - && isMutableExpression(ctAssignment.getAssignment())); + return false; } + } - return false; + /** + * Checks if the state of the given expression can be mutated. + * + * @param ctExpression the expression to check + * @return true if the expression is mutable, false otherwise + */ + private static boolean isMutableExpression(CtExpression ctExpression) { + return MutableExpressionChecker.check(ctExpression); } private static boolean isParameterOf(CtVariable ctVariable, CtExecutable ctExecutable) { diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestLeakedCollectionCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestLeakedCollectionCheck.java index 135c2554..e026b4c0 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestLeakedCollectionCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/oop/TestLeakedCollectionCheck.java @@ -985,4 +985,30 @@ public record Zoo(String name, Collection animals) { problems.assertExhausted(); } + + + @Test + void testSelfAssignment() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Zoo", + """ + import java.util.Collection; + + public class Zoo { + private Collection animals = null; + + public Zoo() { + this.animals = animals; + } + + public Collection getAnimals() { + return this.animals; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } } From b42d979247179f8ac5c8cac8076ddd015d5a8236 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sun, 29 Sep 2024 08:37:13 +0200 Subject: [PATCH 02/17] disable the multi-threading code, which resulted in endless loops when something crashed --- .../de/firemage/autograder/core/Linter.java | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/Linter.java b/autograder-core/src/main/java/de/firemage/autograder/core/Linter.java index 13f91554..8d9727db 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 @@ -114,9 +114,23 @@ public List checkFile( } } - AnalysisScheduler scheduler = new AnalysisScheduler(this.threads, classLoader); - AnalysisResult result; + // TODO: refactor AnalysisScheduler/AnalysisThread to be simpler/easier to understand + // + // The code has been disabled, because of the following issues: + // - Spoon Code does not like to be run in parallel + // - Having checks run in parallel, resulted in bugs that were hard to reproduce, because they depended on the order + // in which the checks were run + // - Crashes resulted in the program looping/hanging, instead of exiting with the thrown exception + // + // The last issue was the one that made me disable the code, because it made debugging very hard, and + // it was not clear how to fix it. It looks like the code is reinventing the wheel, instead of using + // existing solutions like ExecutorService. + // + //AnalysisScheduler scheduler = new AnalysisScheduler(this.threads, classLoader); + + // AnalysisResult result; + List unreducedProblems = new ArrayList<>(); try (TempLocation tempLinterLocation = this.tempLocation.createTempDirectory("linter")) { for (var entry : linterChecks.entrySet()) { CodeLinter linter = entry.getKey(); @@ -129,7 +143,7 @@ public List checkFile( continue; } - scheduler.submitTask((s, reporter) -> { + /*scheduler.submitTask((s, reporter) -> { reporter.reportProblems(linter.lint( file, tempLinterLocation, @@ -137,19 +151,19 @@ public List checkFile( associatedChecks, statusConsumer )); - }); - } - - result = scheduler.collectProblems(); - if (result.failed()) { - throw new LinterException(result.thrownException()); + });*/ + unreducedProblems.addAll(linter.lint( + file, + tempLinterLocation, + this.classLoader, + associatedChecks, + statusConsumer + )); } } - var unreducedProblems = result.problems(); if (!checkConfiguration.problemsToReport().isEmpty()) { - unreducedProblems = result - .problems() + unreducedProblems = unreducedProblems .stream() .filter(problem -> checkConfiguration.problemsToReport().contains(problem.getProblemType())) .toList(); From ac3564a7ea081326108bd1d2c66032bc43f455c5 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sun, 29 Sep 2024 08:37:51 +0200 Subject: [PATCH 03/17] catch `AssertionError` in `UnusedImport` which is thrown when the javadoc is malformed --- .../core/check/complexity/UnusedImport.java | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) 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 3fad6441..d5e51155 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 @@ -108,12 +108,20 @@ private static boolean hasAnyJavadocUses(CtReference ctReference, Filter(CtJavaDoc.class), ctJavaDoc -> { - JavadocParser parser = new JavadocParser(ctJavaDoc.getRawContent(), ctJavaDoc.getParent()); - - return parser.parse() - .stream() - .flatMap(element -> element.accept(new ReferenceFinder()).stream()) - .anyMatch(otherReference -> isReferencingTheSameElement(ctReference, otherReference)); + try { + JavadocParser parser = new JavadocParser(ctJavaDoc.getRawContent(), ctJavaDoc.getParent()); + + return parser.parse() + .stream() + .flatMap(element -> element.accept(new ReferenceFinder()).stream()) + .anyMatch(otherReference -> isReferencingTheSameElement(ctReference, otherReference)); + } catch (AssertionError e) { + // the javadoc parser did throw an assertion error on javadoc that was not valid, but the code did compile, + // so this is caught here to prevent the whole thing from crashing; + // + // The javadoc in question was: "/**\n * @param length the {@param string} and {@param value}.\n */" + return false; + } } )).first(CtJavaDoc.class) != null; } From fdcbbb96381fac0313549672de61548b3b856ac3 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sun, 29 Sep 2024 09:40:28 +0200 Subject: [PATCH 04/17] migrate `UseEntrySet` tests to new test style and improve message --- .../core/check/api/UseEntrySet.java | 16 +- .../core/integrated/MethodUtil.java | 13 +- .../core/check/api/TestUseEntrySet.java | 146 ++++++++++++++++++ .../check_tests/UseEntrySet/code/Test.java | 104 ------------- .../core/check_tests/UseEntrySet/config.txt | 13 -- 5 files changed, 172 insertions(+), 120 deletions(-) create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseEntrySet.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseEntrySet/code/Test.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseEntrySet/config.txt diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseEntrySet.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseEntrySet.java index 5ca20226..948abbd1 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseEntrySet.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseEntrySet.java @@ -27,6 +27,15 @@ private static boolean hasInvokedKeySet(CtInvocation ctInvocation) { && ctInvocation.getExecutable().getSimpleName().equals("keySet"); } + private static String makeSuggestion(CtInvocation ctInvocation) { + String suggestion = "%s.entrySet()".formatted(ctInvocation.getTarget()); + if (suggestion.startsWith(".")) { + suggestion = suggestion.substring(1); + } + + return suggestion; + } + @Override protected void check(StaticAnalysis staticAnalysis) { staticAnalysis.processWith(new AbstractProcessor() { @@ -61,7 +70,12 @@ public void process(CtForEach ctForEach) { if (!invocations.isEmpty()) { addLocalProblem( ctForEach.getExpression(), - new LocalizedMessage("suggest-replacement", Map.of("original", "keySet()", "suggestion", "entrySet()")), + new LocalizedMessage( + "suggest-replacement", + Map.of( + "original", ctInvocation, + "suggestion", makeSuggestion(ctInvocation) + )), ProblemType.USE_ENTRY_SET ); } 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 545f407d..a897eb18 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 @@ -42,6 +42,15 @@ public static boolean isMainMethod(CtMethod method) { ); } + /** + * Checks if the given executable reference has the given signature. + * + * @param ctExecutableReference the executable reference to check + * @param returnType the expected return type or null if the return type should not be checked + * @param methodName the name of the method + * @param parameterTypes the expected parameter types + * @return true if the signature matches, false otherwise + */ public static boolean isSignatureEqualTo( CtExecutableReference ctExecutableReference, Class returnType, @@ -51,7 +60,7 @@ public static boolean isSignatureEqualTo( TypeFactory factory = ctExecutableReference.getFactory().Type(); return MethodUtil.isSignatureEqualTo( ctExecutableReference, - factory.createReference(returnType), + returnType == null ? null : factory.createReference(returnType), methodName, Arrays.stream(parameterTypes).map(factory::createReference).toArray(CtTypeReference[]::new) ); @@ -64,7 +73,7 @@ public static boolean isSignatureEqualTo( CtTypeReference... parameterTypes ) { // check that they both return the same type - if (!TypeUtil.isTypeEqualTo(ctExecutableReference.getType(), returnType)) { + if (returnType != null && !TypeUtil.isTypeEqualTo(ctExecutableReference.getType(), returnType)) { return false; } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseEntrySet.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseEntrySet.java new file mode 100644 index 00000000..ed66845a --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseEntrySet.java @@ -0,0 +1,146 @@ +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.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import java.io.IOException; +import java.util.Arrays; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestUseEntrySet extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.USE_ENTRY_SET); + + private void assertShouldUseEntrySet(Problem problem, String original, String suggestion) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + "suggest-replacement", + Map.of( + "original", + original, + "suggestion", + suggestion + )) + ), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testUsesKeySetToGetValues() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.*; + + enum Color { RED, GREEN, BLUE; } + + public class Test { + public static void main(String[] args) { + Map storedColors = new HashMap<>(); + Map result = new HashMap<>(); + + for (Color color : storedColors.keySet()) { /*# not ok #*/ + if (storedColors.get(color) > 0) { + result.put(color, storedColors.get(color)); + } + } + } + } + """ + ), PROBLEM_TYPES); + + assertShouldUseEntrySet(problems.next(), "storedColors.keySet()", "storedColors.entrySet()"); + problems.assertExhausted(); + } + + // Test taken from https://github.com/SonarSource/sonar-java/blob/4c1473548b7e2e9fe1da212955f532c6fbc64d18/java-checks-test-sources/src/main/java/checks/KeySetInsteadOfEntrySet.java#L3 + @ParameterizedTest + @CsvSource( + delimiter = '|', + useHeadersInDisplayName = true, + value = { + " Iterable | Suggestion | Accesses ", + " list | | ", + " list() | | ", + " new String[] {} | | ", + " map.keySet() | | map.get(key); map.get(getKey()); map2.get(mapKey); ", + " this.map3[0].keySet() | this.map3[0].entrySet() | this.map3[0].get(mapKey) ", + " inner.inner.map.keySet() | inner.inner.map.entrySet() | inner.inner.map.get(mapKey) ", + " keySet() | | map3[0].get(mapKey) ", + " super.inner.map.keySet() | super.inner.map.entrySet() | super.inner.map.get(mapKey) ", + " this.inner.map.keySet() | this.inner.map.entrySet() | this.inner.map.get(mapKey) ", + " map.entrySet() | | map.get(mapKey) ", + " keySet() | entrySet() | get(mapKey) ", + " this.keySet() | this.entrySet() | this.get(mapKey) ", + " super.keySet() | super.entrySet() | super.get(mapKey) ", + " map.keySet() | map.entrySet() | map.get(mapKey) ", + " super.map.keySet() | super.map.entrySet() | super.map.get(mapKey) ", + " super.map.keySet() | | this.map.get(mapKey) ", + " this.map.keySet() | this.map.entrySet() | this.map.get(mapKey) ", + } + ) + void testParametric(String iterable, String suggestion, String accesses) throws LinterException, IOException { + List body = List.of(); + if (accesses != null) { + body = Arrays.stream(accesses.split(";", -1)).map("System.out.println(%s);%n"::formatted).toList(); + } + + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.*; + + class KeySetInsteadOfEntrySetCheck extends java.util.HashMap { + KeySetInsteadOfEntrySetCheck inner; + java.util.HashMap map; + } + + class Test extends KeySetInsteadOfEntrySetCheck { + public class InnerClass { + InnerClass inner; + java.util.HashMap map; + } + + private String key; + private java.util.List list; + java.util.HashMap map, map2; + java.util.HashMap[] map3; + + InnerClass inner; + + private String getKey() { + return key; + } + private java.util.List list() { + return list; + } + + public void method() { + for (var mapKey : %s) { + %s + } + } + } + """.formatted(iterable, String.join("", body)) + ), PROBLEM_TYPES); + + if (suggestion != null) { + assertShouldUseEntrySet(problems.next(), iterable, suggestion); + } + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseEntrySet/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseEntrySet/code/Test.java deleted file mode 100644 index cf3b9644..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseEntrySet/code/Test.java +++ /dev/null @@ -1,104 +0,0 @@ -package de.firemage.autograder.core.check_tests.UseEntrySet.code; - -import java.util.*; - -enum Color { RED, GREEN, BLUE; } - -public class Test { - public static void main(String[] args) { - Map storedColors = new HashMap<>(); - Map result = new HashMap<>(); - - for (Color color : storedColors.keySet()) { /*# not ok #*/ - if (storedColors.get(color) > 0) { - result.put(color, storedColors.get(color)); - } - } - } -} - -// Test taken from https://github.com/SonarSource/sonar-java/blob/4c1473548b7e2e9fe1da212955f532c6fbc64d18/java-checks-test-sources/src/main/java/checks/KeySetInsteadOfEntrySet.java#L3 -class KeySetInsteadOfEntrySetCheck extends java.util.HashMap { - KeySetInsteadOfEntrySetCheck inner; - java.util.HashMap map; -} - -class KeySetInsteadOfEntrySetCheckExtendedClass extends KeySetInsteadOfEntrySetCheck { - - public class InnerClass { - InnerClass inner; - java.util.HashMap map; - } - - private String key; - - private String getKey() { - return key; - } - - private java.util.List list; - - private java.util.List list() { - return list; - } - - java.util.HashMap map, map2; - java.util.HashMap[] map3; - - InnerClass inner; - - public void method() { - for (String value : list) { /*# ok #*/ - } - for (String value : list()) { /*# ok #*/ - } - for (String value : new String[] {}) { /*# ok #*/ - } - for (String key2 : map.keySet()) { /*# ok #*/ - Object value1 = map.get(key); - Object value2 = map.get(getKey()); - Object value3 = map2.get(key2); - } - for (String key5 : this.map3[0].keySet()) { /*# not ok #*/ - Object value = this.map3[0].get(key5); - } - for (String key5 : inner.inner.map.keySet()) { /*# not ok #*/ - Object value = inner.inner.map.get(key5); - } - for (String key5 : keySet()) { /*# ok #*/ - Object value = map3[0].get(key5); - } - for (String key5 : super.inner.map.keySet()) { /*# not ok #*/ - Object value = super.inner.map.get(key5); - } - for (String key5 : this.inner.map.keySet()) { /*# not ok #*/ - Object value = this.inner.map.get(key5); - } - for (java.util.Map.Entry key6 : map.entrySet()) { /*# ok #*/ - Object value = map.get(key6); - } - - for (String key3 : keySet()) { /*# not ok #*/ - Object value = get(key3); - } - for (String key4 : this.keySet()) { /*# not ok #*/ - Object value = this.get(key4); - } - for (String key5 : super.keySet()) { /*# not ok #*/ - Object value = super.get(key5); - } - for (String key5 : map.keySet()) { /*# not ok #*/ - Object value = map.get(key5); - } - for (String key5 : super.map.keySet()) { /*# not ok #*/ - Object value = super.map.get(key5); - } - for (String key5 : super.map.keySet()) { /*# ok #*/ - Object value = this.map.get(key5); - } - for (String key5 : this.map.keySet()) { /*# not ok #*/ - Object value = this.map.get(key5); - } - } - -} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseEntrySet/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseEntrySet/config.txt deleted file mode 100644 index 9a8bbf2b..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseEntrySet/config.txt +++ /dev/null @@ -1,13 +0,0 @@ -api.UseEntrySet -Use Map#entrySet when possible -Test.java:12 -Test.java:62 -Test.java:65 -Test.java:71 -Test.java:74 -Test.java:81 -Test.java:84 -Test.java:87 -Test.java:90 -Test.java:93 -Test.java:99 From 7d165a4a26a1525c8865c0af478cf2521efc3dee Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sun, 29 Sep 2024 11:09:46 +0200 Subject: [PATCH 05/17] fix bug in `UseEntrySet` --- .../core/check/api/UseEntrySet.java | 3 +-- .../autograder/core/integrated/TypeUtil.java | 6 ++--- .../core/check/api/TestUseEntrySet.java | 25 +++++++++++++++++++ 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseEntrySet.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseEntrySet.java index 948abbd1..ef5b49dd 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseEntrySet.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseEntrySet.java @@ -3,7 +3,6 @@ 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.ExpressionUtil; import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.StaticAnalysis; import de.firemage.autograder.core.integrated.TypeUtil; @@ -43,7 +42,7 @@ protected void check(StaticAnalysis staticAnalysis) { public void process(CtForEach ctForEach) { if (ctForEach.isImplicit() || !ctForEach.getPosition().isValidPosition() - || !(ExpressionUtil.resolveCtExpression(ctForEach.getExpression()) instanceof CtInvocation ctInvocation) + || !(ctForEach.getExpression() instanceof CtInvocation ctInvocation) || !hasInvokedKeySet(ctInvocation) || !ctForEach.getExpression().getPosition().isValidPosition()) { return; diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/TypeUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/TypeUtil.java index 7164aa30..0be69f63 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/TypeUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/TypeUtil.java @@ -135,15 +135,13 @@ public static boolean isTypeEqualTo(CtTypeReference ctType, CtTypeReference ctTypeReference, Class expected) { - // NOTE: calling isSubtypeOf on CtTypeParameterReference will result in a crash CtType expectedType = ctTypeReference.getFactory().Type().get(expected); - if (ctTypeReference.getTypeDeclaration() == null) { + if (ctTypeReference.getTypeDeclaration() == null || ctTypeReference instanceof CtTypeParameterReference) { return ctTypeReference.isSubtypeOf(expectedType.getReference()); } - return !(ctTypeReference instanceof CtTypeParameterReference) - && UsesFinder.isSubtypeOf(ctTypeReference.getTypeDeclaration(), expectedType); + return UsesFinder.isSubtypeOf(ctTypeReference.getTypeDeclaration(), expectedType); } /** diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseEntrySet.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseEntrySet.java index ed66845a..fb3ac4b5 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseEntrySet.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseEntrySet.java @@ -143,4 +143,29 @@ public void method() { } problems.assertExhausted(); } + + @Test + void testResolvesGeneric() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.Map; + import java.util.HashMap; + + public class Test { + private static > void execute(T map) { + for (var mapKey : map.keySet()) { + System.out.println(map.get(mapKey)); + System.out.println(map.get(mapKey)); + } + } + } + """ + ), PROBLEM_TYPES); + + assertShouldUseEntrySet(problems.next(), "map.keySet()", "map.entrySet()"); + + problems.assertExhausted(); + } } From 272124b8f0f43b4976e8ab4820d1f51e9a74defd Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sun, 29 Sep 2024 11:17:31 +0200 Subject: [PATCH 06/17] migrate `BooleanIdentifierCheck` to new test format --- .../check/naming/BooleanIdentifierCheck.java | 1 + .../naming/TestBooleanIdentifierCheck.java | 94 +++++++++++++++++++ .../BooleanIdentifierCheck/code/Test.java | 17 ---- .../BooleanIdentifierCheck/config.txt | 3 - 4 files changed, 95 insertions(+), 20 deletions(-) create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/naming/TestBooleanIdentifierCheck.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/BooleanIdentifierCheck/code/Test.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/BooleanIdentifierCheck/config.txt diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/BooleanIdentifierCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/BooleanIdentifierCheck.java index 92b4c97c..0c7c7828 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/BooleanIdentifierCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/BooleanIdentifierCheck.java @@ -21,6 +21,7 @@ public void process(CtMethod ctMethod) { return; } + // String methodName = ctMethod.getSimpleName(); if (ctMethod.getType().equals(ctMethod.getFactory().Type().booleanPrimitiveType()) && methodName.startsWith("get")) { diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/naming/TestBooleanIdentifierCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/naming/TestBooleanIdentifierCheck.java new file mode 100644 index 00000000..56f9c989 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/naming/TestBooleanIdentifierCheck.java @@ -0,0 +1,94 @@ +package de.firemage.autograder.core.check.naming; + +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.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestBooleanIdentifierCheck extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.BOOLEAN_GETTER_NOT_CALLED_IS); + + private void assertName(Problem problem, String methodName, String newName) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + "bool-getter-name", + Map.of( + "oldName", methodName, + "newName", newName + ) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testGetter() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public boolean getValue() { /*# not ok #*/ + return true; + } + } + """ + ), PROBLEM_TYPES); + + assertName(problems.next(), "getValue", "isValue"); + problems.assertExhausted(); + } + + @Test + void testCorrectlyNamedGetter() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public boolean hasValue() { + return true; + } + + public boolean isValue() { + return true; + } + + public boolean value() { + return true; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testBooleanArray() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public boolean[] getValue() { + return null; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/BooleanIdentifierCheck/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/BooleanIdentifierCheck/code/Test.java deleted file mode 100644 index d08c3c5e..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/BooleanIdentifierCheck/code/Test.java +++ /dev/null @@ -1,17 +0,0 @@ -package de.firemage.autograder.core.check_tests.BooleanIdentifierCheck.code; - -public class Test { - public static void main(String[] args) {} - - public boolean getValue() { /*# not ok #*/ - return true; - } - - public boolean value() { /*# ok #*/ - return true; - } - - public boolean isValue() { /*# ok #*/ - return true; - } -} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/BooleanIdentifierCheck/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/BooleanIdentifierCheck/config.txt deleted file mode 100644 index 119c8cbb..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/BooleanIdentifierCheck/config.txt +++ /dev/null @@ -1,3 +0,0 @@ -naming.BooleanIdentifierCheck -Boolean identifier should be prefixed by a verb like is -Test.java:6 From c2895bf906626239aa009b411c503924513da81f Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sun, 29 Sep 2024 11:26:33 +0200 Subject: [PATCH 07/17] skip field should be final check if main method is missing #429 --- .../check/general/FieldShouldBeFinal.java | 4 ++ .../check/general/TestFieldShouldBeFinal.java | 52 ++++++++++++++++++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/FieldShouldBeFinal.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/FieldShouldBeFinal.java index f194c73f..73f4032e 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/FieldShouldBeFinal.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/FieldShouldBeFinal.java @@ -116,6 +116,10 @@ private static boolean canBeFinal(CtField ctField) { @Override protected void check(StaticAnalysis staticAnalysis) { + if (!staticAnalysis.getCodeModel().hasMainMethod()) { + return; + } + staticAnalysis.processWith(new AbstractProcessor>() { @Override public void process(CtField ctField) { diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestFieldShouldBeFinal.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestFieldShouldBeFinal.java index ad3846a6..b5da5364 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestFieldShouldBeFinal.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestFieldShouldBeFinal.java @@ -45,6 +45,8 @@ class Test { void foo() { this.value = "Value"; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -69,6 +71,8 @@ class Test { public String toString() { return this.value; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -90,6 +94,8 @@ class Test { Test() { this.value = 2; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -109,6 +115,8 @@ class Test { Test() { this.value = 2; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -132,6 +140,8 @@ protected A() { protected A(String value) { this.value = value; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -152,6 +162,8 @@ class User { User() { this.id = nextId++; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -172,6 +184,8 @@ class User { User() { this.id = nextId; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -195,6 +209,8 @@ class User { nextId = 1; this.id = nextId; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -216,6 +232,8 @@ class User { nextId = next; this.id = next; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -234,6 +252,8 @@ public User(String name, int id) { this.name = name; this.id = id; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -258,6 +278,8 @@ public String getName() { public int getId() { return id; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -289,6 +311,8 @@ public String getName() { public int getId() { return id; } + + public static void main(String[] args) {} } """ ), @@ -329,6 +353,8 @@ public User(String name, int id) { this.id = id; this.name = name; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -361,6 +387,8 @@ public User(int id) { public User() { // no name and id init -> name = null, id = 0 } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -386,6 +414,8 @@ public User(String name) { this.id = 1; } } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -413,6 +443,8 @@ public User(String name) { this.id = 0; } } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -443,6 +475,8 @@ public User(String name) { this.id = 5; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -469,6 +503,8 @@ public User(String name) { this.id = 0; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -494,6 +530,8 @@ public User(String name) { this.id = 1; } } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -520,6 +558,8 @@ public User(String name) { public void update() { next += 1; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -542,6 +582,8 @@ public User(String name) { this.name = name; this.id = 0; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -557,6 +599,8 @@ void testNoConstructorButValue() throws LinterException, IOException { """ public class User { private int id = 1; + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -573,7 +617,9 @@ void testRecordImplicitConstructor() throws LinterException, IOException { JavaVersion.JAVA_17, "User", """ - public record User(int id, String name) {} + public record User(int id, String name) { + public static void main(String[] args) {} + } """ ), PROBLEM_TYPES); @@ -588,6 +634,8 @@ void testRecordStaticField() throws LinterException, IOException { """ public record User(int id, String name) { private static String ADMIN_NAME = "admin"; + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); @@ -613,6 +661,8 @@ public User() { private void setUser(String name) { this.name = name; } + + public static void main(String[] args) {} } """ ), PROBLEM_TYPES); From 5cc4e4c4eb3bb31a334e47afc1f4259a792b0e15 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 30 Sep 2024 08:58:37 +0200 Subject: [PATCH 08/17] minor code improvements to `LoopShouldBeFor` --- .../core/check/general/LoopShouldBeFor.java | 69 ++++++++++++------- 1 file changed, 43 insertions(+), 26 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 8855d7cd..96cb327f 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 @@ -24,6 +24,7 @@ import spoon.reflect.code.CtUnaryOperator; import spoon.reflect.code.CtVariableWrite; import spoon.reflect.code.CtWhile; +import spoon.reflect.declaration.CtVariable; import spoon.reflect.factory.Factory; import spoon.reflect.reference.CtFieldReference; @@ -62,24 +63,26 @@ public String toString() { } } - private static LoopSuggestion getCounter(CtLoop ctLoop) { - List statements = StatementUtil.getEffectiveStatements(ctLoop.getBody()); - - if (statements.isEmpty()) { - return null; - } - + @SuppressWarnings({"unchecked", "rawtypes"}) + private static CtFieldRead createUncheckedFieldRead( + CtExpression targetExpression, + String fieldName + ) { + Factory factory = targetExpression.getFactory(); + CtFieldReference fieldReference = factory.createFieldReference(); + fieldReference.setDeclaringType(targetExpression.getType().clone()); + fieldReference.setSimpleName(fieldName); - CtStatement previous = StatementUtil.getPreviousStatement(ctLoop).orElse(null); - while (previous instanceof CtLocalVariable ctLocalVariable && !TypeUtil.isPrimitiveNumeric(ctLocalVariable.getType())) { - previous = StatementUtil.getPreviousStatement(previous).orElse(null); - } + CtFieldRead fieldRead = factory.createFieldRead(); + fieldRead.setTarget(targetExpression.clone()); + fieldRead.setVariable(fieldReference); - if (!(previous instanceof CtLocalVariable ctLocalVariable) || !TypeUtil.isPrimitiveNumeric(ctLocalVariable.getType())) { - return null; - } + return fieldRead; + } - CtStatement lastStatement = null; + private static CtStatement findLastStatement(List statements, CtVariable ctLocalVariable) { + // this finds the last statement that uses the control variable + // and ensures that it is an assignment to the control variable for (int i = statements.size() - 1; i >= 0; i--) { CtStatement statement = statements.get(i); @@ -90,8 +93,7 @@ private static LoopSuggestion getCounter(CtLoop ctLoop) { || (statement instanceof CtUnaryOperator ctUnaryOperator && ctUnaryOperator.getOperand() instanceof CtVariableWrite ctWrite && ctWrite.getVariable().equals(ctLocalVariable.getReference()))) { - lastStatement = statement; - break; + return statement; } // the control variable is used after the update statement or there is no update statement @@ -100,15 +102,36 @@ private static LoopSuggestion getCounter(CtLoop ctLoop) { } } + return null; + } + + private static LoopSuggestion getCounter(CtLoop ctLoop) { + List statements = StatementUtil.getEffectiveStatements(ctLoop.getBody()); + + if (statements.isEmpty()) { + return null; + } + + + 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; + } + + CtStatement lastStatement = findLastStatement(statements, ctLocalVariable); + // could not find an update statement => ignoring loop suggestion if (lastStatement == null) { return null; } // ignore loops where the control variable is updated more than once in the body - CtStatement finalLastStatement = lastStatement; boolean isUpdatedMultipleTimes = statements.stream() - .filter(statement -> statement != finalLastStatement) + .filter(statement -> statement != lastStatement) .anyMatch( statement -> UsesFinder.variableUses(ctLocalVariable).ofType(CtVariableWrite.class).nestedIn(statement).hasAny()); @@ -158,13 +181,7 @@ private static LoopSuggestion getCounter(CtLoop ctLoop) { CtExpression upperBound; if (ctForEach.getExpression().getType().isArray()) { - CtFieldReference arrayLengthReference = factory.createFieldReference(); - arrayLengthReference.setDeclaringType(ctForEach.getExpression().getType().clone()); - arrayLengthReference.setSimpleName("length"); - CtFieldRead fieldRead = factory.createFieldRead(); - fieldRead.setTarget(ctForEach.getExpression().clone()); - fieldRead.setVariable(arrayLengthReference); - upperBound = fieldRead; + upperBound = createUncheckedFieldRead(ctForEach.getExpression(), "length"); } else { var methods = ctForEach.getExpression().getType().getTypeDeclaration().getMethodsByName("size"); if (methods.isEmpty()) { From 5425753e050e3a31b75cf93116a7ef9b6985e3a7 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 30 Sep 2024 09:31:10 +0200 Subject: [PATCH 09/17] implement `LoopShouldBeWhile` #370 --- .../firemage/autograder/core/ProblemType.java | 8 + .../core/check/general/LoopShouldBeWhile.java | 69 +++++++ .../src/main/resources/strings.de.ftl | 1 + .../src/main/resources/strings.en.ftl | 1 + .../check/general/TestLoopShouldBeWhile.java | 180 ++++++++++++++++++ .../TestUnusedCodeElementCheck.java | 61 ++++++ sample_config.yaml | 1 + 7 files changed, 321 insertions(+) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeWhile.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestLoopShouldBeWhile.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 82fb060b..25795a2c 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 @@ -533,6 +533,14 @@ public enum ProblemType implements AbstractProblemType { @HasFalsePositives LOOP_SHOULD_BE_FOR, + /** + * Reports loops that can be replaced with a while loop. + *
+ * This check is not trivial and likely has false-positives. + */ + @HasFalsePositives + LOOP_SHOULD_BE_WHILE, + /** * Reports methods that do not have an override annotation, but override a method. *
diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeWhile.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeWhile.java new file mode 100644 index 00000000..4b2556ab --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeWhile.java @@ -0,0 +1,69 @@ +package de.firemage.autograder.core.check.general; + +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.StatementUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtExpression; +import spoon.reflect.code.CtFor; +import spoon.reflect.code.CtStatement; +import spoon.reflect.code.CtWhile; + +import java.util.List; +import java.util.Map; + +@ExecutableCheck(reportedProblems = { ProblemType.LOOP_SHOULD_BE_WHILE }) +public class LoopShouldBeWhile extends IntegratedCheck { + private static CtWhile createCtWhile( + CtExpression condition, + CtStatement body + ) { + CtWhile result = body.getFactory().Core().createWhile(); + + if (condition != null) { + result.setLoopingExpression(condition.clone()); + } + result.setBody(body.clone()); + return result; + } + + @Override + protected void check(StaticAnalysis staticAnalysis) { + staticAnalysis.processWith(new AbstractProcessor() { + @Override + public void process(CtFor ctFor) { + if (ctFor.isImplicit() || !ctFor.getPosition().isValidPosition() || ctFor.getBody() == null) { + return; + } + + List forInit = ctFor.getForInit(); + CtExpression condition = ctFor.getExpression(); + List forUpdate = ctFor.getForUpdate(); + + if (condition == null || !forInit.isEmpty() || !forUpdate.isEmpty()) { + return; + } + + List statements = StatementUtil.getEffectiveStatements(ctFor.getBody()); + + if (statements.isEmpty()) { + return; + } + + addLocalProblem( + ctFor, + new LocalizedMessage( + "loop-should-be-while", + Map.of( + "suggestion", "%n%s".formatted(createCtWhile(condition, ctFor.getBody()).toString().stripTrailing()) + ) + ), + ProblemType.LOOP_SHOULD_BE_WHILE + ); + } + }); + } +} diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index 7f15a9df..6bef027e 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -191,6 +191,7 @@ magic-string = Der String '{$value}' sollte in eine Konstante ausgelagert werden loop-should-be-do-while = Diese Schleife sollte eine do-while Schleife sein, weil der Code vor der Schleife, der gleiche wie in der Schleife ist: {$suggestion} loop-should-be-for = Diese Schleife sollte eine Zählschleife (for) sein: {$suggestion} +loop-should-be-while = Diese Schleife sollte eine while Schleife sein: {$suggestion} # Naming bool-getter-name = Für boolean getter bietet es sich an ein Verb als Präfix zu verwenden. Zum Beispiel '{$newName}' statt '{$oldName}'. diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl index aaf71bae..8ea5bdca 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -191,6 +191,7 @@ magic-string = The string '{$value}' should be in a constant. See the wiki artic loop-should-be-do-while = This loop should be a do-while loop, because the code before the loop is the same as the code in the loop: {$suggestion} loop-should-be-for = This loop should be a for-loop: {$suggestion} +loop-should-be-while = This loop should be a while-loop: {$suggestion} # Naming bool-getter-name = For boolean getters it is recommended to use a verb as a prefix. For example '{$newName}' instead of '{$oldName}'. diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestLoopShouldBeWhile.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestLoopShouldBeWhile.java new file mode 100644 index 00000000..1ce5bc93 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestLoopShouldBeWhile.java @@ -0,0 +1,180 @@ +package de.firemage.autograder.core.check.general; + +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.api.Disabled; +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 TestLoopShouldBeWhile extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.LOOP_SHOULD_BE_WHILE); + + private static T debugPrint(T value) { + System.out.println("\"%s\"".formatted(value.toString().replace("\n", "\\n").replace("\r", "\\r"))); + return value; + } + + void assertEqualsWhile(Problem problem, String condition, String body) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage( + "loop-should-be-while", + Map.of("suggestion", """ + %nwhile (%s) %s""".formatted(condition, body)) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testForWithUsedCounter() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + void test(String[] array) { + for (int i = 0; i < array.length; i++) { + System.out.println("i: " + i + " array[i]: " + array[i]); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + + @Test + void testForWithoutInit() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + void test(String[] array) { + int i = 0; + for (; i < array.length; i++) { + System.out.println("i: " + i + " array[i]: " + array[i]); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testForWithoutInitAndUpdate() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + void test(String[] array) { + int i = 0; + for (; i < array.length;) { + System.out.println("i: " + i + " array[i]: " + array[i]); + i++; + } + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsWhile( + problems.next(), + "i < array.length", + "{%n System.out.println(\"i: \" + i + \" array[i]: \" + array[i]);%n i++;%n}".formatted() + ); + + problems.assertExhausted(); + } + + @Test + void testEndlessFor() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + void test(String[] array) { + for (;;) { + System.out.println("hello"); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + @Disabled("Might be implemented in the future") + void testUnusedCounter() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + void test(int count) { + String result = ""; + for (int i = 0; result.length() < count; i++) { + result += "a"; + } + System.out.println(result); + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsWhile( + problems.next(), + "result.length() < count", + "{%n result += \"a\";%n}".formatted() + ); + + problems.assertExhausted(); + } + + @Test + @Disabled("Might be implemented in the future") + void testUnusedCounterExternalInit() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + void test(int count) { + String result = ""; + int i = 0; + for (; result.length() < count; i++) { + result += "a"; + } + System.out.println(result); + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsWhile( + problems.next(), + "result.length() < count", + "{%n result += \"a\";%n}".formatted() + ); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java index 13568e48..2014cd50 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java @@ -1027,4 +1027,65 @@ public Foo() { problems.assertExhausted(); } + + @Test + void testUnusedForCounter() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + void test(int count) { + String result = ""; + for (int i = 0; result.length() < count;) { + result += "a"; + } + System.out.println(result); + } + + public static void main(String[] args) { + new Test().test(10); + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsUnused(problems.next(), "i"); + + problems.assertExhausted(); + } + + @Test + void testUsedForCounter() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + void test(int count) { + String result = ""; + for (int i = 0; result.length() < count; i++) { + result += "a" + i; + } + System.out.println(result); + test2(count); + } + + void test2(int count) { + String result = ""; + for (int i = 0; i < result.length() + count; i++) { + result += "a"; + } + System.out.println(result); + } + + public static void main(String[] args) { + new Test().test(10); + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } } diff --git a/sample_config.yaml b/sample_config.yaml index 175cf96f..9e4c470c 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -158,3 +158,4 @@ problemsToReport: - METHOD_SHOULD_BE_STATIC - METHOD_SHOULD_BE_STATIC_NOT_PUBLIC - IS_LETTER_OR_DIGIT + - LOOP_SHOULD_BE_WHILE From 07ba95aa7bf95aea2f73b9f586829ea1631af716 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 30 Sep 2024 10:20:58 +0200 Subject: [PATCH 10/17] rename `ConstantNamingAndQualifierCheck` to `VariableShouldBeConstant` #399 --- .../de/firemage/autograder/core/ProblemType.java | 12 ++++++------ .../autograder/core/integrated/TypeUtil.java | 3 +-- ...ifierCheck.java => VariableShouldBeConstant.java} | 7 +++---- ...rCheck.java => TestVariableShouldBeConstant.java} | 2 +- 4 files changed, 11 insertions(+), 13 deletions(-) rename autograder-extra/src/main/java/de/firemage/autograder/extra/check/general/{ConstantNamingAndQualifierCheck.java => VariableShouldBeConstant.java} (92%) rename autograder-extra/src/test/java/de/firemage/autograder/extra/check/general/{TestConstantNamingAndQualifierCheck.java => TestVariableShouldBeConstant.java} (97%) 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 25795a2c..9526b780 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 @@ -428,6 +428,12 @@ public enum ProblemType implements AbstractProblemType { @HasFalsePositives FIELD_SHOULD_BE_CONSTANT, + /** + * Reports code where a local variable could be a constant. + */ + @HasFalsePositives + LOCAL_VARIABLE_SHOULD_BE_CONSTANT, + /** * Tries to find classes that are exclusively used for constants. *
@@ -855,12 +861,6 @@ public enum ProblemType implements AbstractProblemType { @HasFalsePositives USE_FORMAT_STRING, - /** - * Reports code where a local variable could be a constant. - */ - @HasFalsePositives - LOCAL_VARIABLE_SHOULD_BE_CONSTANT, - /** * Suggests using EnumMap or EnumSet. */ diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/TypeUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/TypeUtil.java index 0be69f63..56557f0d 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/TypeUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/TypeUtil.java @@ -192,8 +192,7 @@ public static boolean isImmutable(CtTypeReference ctTypeReference) { } // primitive types and strings are immutable as well: - if (ctType.getReference().unbox().isPrimitive() - || isTypeEqualTo(ctType.getReference(), String.class)) { + if (ctType.getReference().unbox().isPrimitive() || isString(ctType.getReference()) || ctType.isEnum()) { continue; } diff --git a/autograder-extra/src/main/java/de/firemage/autograder/extra/check/general/ConstantNamingAndQualifierCheck.java b/autograder-extra/src/main/java/de/firemage/autograder/extra/check/general/VariableShouldBeConstant.java similarity index 92% rename from autograder-extra/src/main/java/de/firemage/autograder/extra/check/general/ConstantNamingAndQualifierCheck.java rename to autograder-extra/src/main/java/de/firemage/autograder/extra/check/general/VariableShouldBeConstant.java index d796336b..3e2c4972 100644 --- a/autograder-extra/src/main/java/de/firemage/autograder/extra/check/general/ConstantNamingAndQualifierCheck.java +++ b/autograder-extra/src/main/java/de/firemage/autograder/extra/check/general/VariableShouldBeConstant.java @@ -26,7 +26,7 @@ ProblemType.FIELD_SHOULD_BE_CONSTANT, ProblemType.LOCAL_VARIABLE_SHOULD_BE_CONSTANT }) -public class ConstantNamingAndQualifierCheck extends IntegratedCheck { +public class VariableShouldBeConstant extends IntegratedCheck { private static final Set IGNORE_FIELDS = Set.of("serialVersionUID"); private static String getVisibilityString(CtModifiable ctModifiable) { @@ -67,9 +67,8 @@ public void process(CtVariable ctVariable) { return; } - // only check primitive types and strings, because other types may be mutable like list - // and should therefore not be static, even if they are final - if (!ctVariable.getType().unbox().isPrimitive() && !TypeUtil.isString(ctVariable.getType())) { + // only immutable types can be constant, mutable types would be global state which should be avoided + if (!TypeUtil.isImmutable(ctVariable.getType())) { return; } diff --git a/autograder-extra/src/test/java/de/firemage/autograder/extra/check/general/TestConstantNamingAndQualifierCheck.java b/autograder-extra/src/test/java/de/firemage/autograder/extra/check/general/TestVariableShouldBeConstant.java similarity index 97% rename from autograder-extra/src/test/java/de/firemage/autograder/extra/check/general/TestConstantNamingAndQualifierCheck.java rename to autograder-extra/src/test/java/de/firemage/autograder/extra/check/general/TestVariableShouldBeConstant.java index c16393bc..d3b77490 100644 --- a/autograder-extra/src/test/java/de/firemage/autograder/extra/check/general/TestConstantNamingAndQualifierCheck.java +++ b/autograder-extra/src/test/java/de/firemage/autograder/extra/check/general/TestVariableShouldBeConstant.java @@ -15,7 +15,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; -class TestConstantNamingAndQualifierCheck extends AbstractCheckTest { +class TestVariableShouldBeConstant extends AbstractCheckTest { private static final List PROBLEM_TYPES = List.of( ProblemType.FIELD_SHOULD_BE_CONSTANT, ProblemType.LOCAL_VARIABLE_SHOULD_BE_CONSTANT From 1f406400859e1cd095b656721b79010af6797364 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 30 Sep 2024 10:52:23 +0200 Subject: [PATCH 11/17] suggest using `Arrays.copyOf` #513 --- .../firemage/autograder/core/ProblemType.java | 6 ++ .../core/check/api/UseArraysCopyOf.java | 69 +++++++++++++++++++ .../core/check/api/TestUseArraysCopyOf.java | 61 ++++++++++++++++ sample_config.yaml | 1 + 4 files changed, 137 insertions(+) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseArraysCopyOf.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseArraysCopyOf.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 9526b780..915d51d9 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 @@ -179,6 +179,12 @@ public enum ProblemType implements AbstractProblemType { @HasFalsePositives CHAR_RANGE, + /** + * Suggests using `Arrays.copyOf` instead of `System.arraycopy`. + */ + @HasFalsePositives + USE_ARRAYS_COPY_OF, + /** * Similar to {@link ProblemType#INCONSISTENT_COMMENT_LANGUAGE}, but reports comments where the AI thinks that the comment is neither german nor english. *
diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseArraysCopyOf.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseArraysCopyOf.java new file mode 100644 index 00000000..42a16439 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseArraysCopyOf.java @@ -0,0 +1,69 @@ +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.ExpressionUtil; +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.CtExpression; +import spoon.reflect.code.CtInvocation; +import spoon.reflect.code.CtTypeAccess; + +import java.util.Map; + +@ExecutableCheck(reportedProblems = { ProblemType.USE_ARRAYS_COPY_OF }) +public class UseArraysCopyOf extends IntegratedCheck { + @Override + protected void check(StaticAnalysis staticAnalysis) { + if (!staticAnalysis.hasJavaUtilImport()) { + return; + } + + staticAnalysis.processWith(new AbstractProcessor>() { + @Override + public void process(CtInvocation ctInvocation) { + if (ctInvocation.isImplicit() + || !ctInvocation.getPosition().isValidPosition()) { + return; + } + + if (ctInvocation.getTarget() == null + || ctInvocation.getTarget().getType() == null + || !(ctInvocation.getTarget() instanceof CtTypeAccess ctTypeAccess) + || !TypeUtil.isTypeEqualTo(ctTypeAccess.getAccessedType(), System.class) + || !MethodUtil.isSignatureEqualTo(ctInvocation.getExecutable(), void.class, "arraycopy", Object.class, int.class, Object.class, int.class, int.class)) { + return; + } + // System.arraycopy(src, srcPos, dest, destPos, length) + + CtExpression source = ctInvocation.getArguments().get(0); + CtExpression sourcePosition = ctInvocation.getArguments().get(1); + CtExpression destination = ctInvocation.getArguments().get(2); + CtExpression destinationPosition = ctInvocation.getArguments().get(3); + CtExpression length = ctInvocation.getArguments().get(4); + + if (ExpressionUtil.isIntegerLiteral(sourcePosition, 0) && ExpressionUtil.isIntegerLiteral(destinationPosition, 0)) { + addLocalProblem( + ctInvocation, + new LocalizedMessage( + "common-reimplementation", + Map.of( + "suggestion", "%s = Arrays.copyOf(%s, %s)".formatted( + destination, + source, + length + ) + ) + ), + ProblemType.USE_ARRAYS_COPY_OF + ); + } + } + }); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseArraysCopyOf.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseArraysCopyOf.java new file mode 100644 index 00000000..e3cb8d1a --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestUseArraysCopyOf.java @@ -0,0 +1,61 @@ +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.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestUseArraysCopyOf extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.USE_ARRAYS_COPY_OF); + + private void assertUseCopyOf(Problem problem, String destination, String source, String length) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + "common-reimplementation", + Map.of("suggestion", "%s = Arrays.copyOf(%s, %s)".formatted(destination, source, length)) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testArraysCopyOf() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.Arrays; + + public class Test { + public static void main(String[] args) { + String[] destination = new String[10]; + System.arraycopy(args, 0, destination, 0, args.length); + System.out.println(Arrays.toString(destination)); + + System.arraycopy(args, 1, destination, 0, 3); + System.out.println(Arrays.toString(destination)); + + System.arraycopy(args, 0, destination, 0, 4); + System.out.println(Arrays.toString(destination)); + } + } + """ + ), PROBLEM_TYPES); + + assertUseCopyOf(problems.next(), "destination", "args", "args.length"); + assertUseCopyOf(problems.next(), "destination", "args", "4"); + + problems.assertExhausted(); + } +} diff --git a/sample_config.yaml b/sample_config.yaml index 9e4c470c..edab66f9 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -159,3 +159,4 @@ problemsToReport: - METHOD_SHOULD_BE_STATIC_NOT_PUBLIC - IS_LETTER_OR_DIGIT - LOOP_SHOULD_BE_WHILE + - USE_ARRAYS_COPY_OF From 1f6f038f079819133513469397ca0e853ad319bf Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 30 Sep 2024 11:06:23 +0200 Subject: [PATCH 12/17] suggest `String#substring(int)` #603 --- .../firemage/autograder/core/ProblemType.java | 6 ++ .../check/api/SimplifyStringSubstring.java | 62 +++++++++++++++++++ .../api/TestSimplifyStringSubstring.java | 58 +++++++++++++++++ sample_config.yaml | 1 + 4 files changed, 127 insertions(+) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/api/SimplifyStringSubstring.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestSimplifyStringSubstring.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 915d51d9..2b02cdf2 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 @@ -173,6 +173,12 @@ public enum ProblemType implements AbstractProblemType { @HasFalsePositives MUTABLE_ENUM, + /** + * Suggests using {@link String#substring(int)} instead of {@link String#substring(int, int)} when possible. + */ + @HasFalsePositives + SIMPLIFY_STRING_SUBSTRING, + /** * Reports code where methods Character.isDigit are reimplemented (e.g. `c >= '0' && c <= '9'`). */ diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SimplifyStringSubstring.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SimplifyStringSubstring.java new file mode 100644 index 00000000..aaafba8d --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SimplifyStringSubstring.java @@ -0,0 +1,62 @@ +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.CtExpression; +import spoon.reflect.code.CtInvocation; + +import java.util.Map; + +@ExecutableCheck(reportedProblems = { ProblemType.SIMPLIFY_STRING_SUBSTRING }) +public class SimplifyStringSubstring extends IntegratedCheck { + @Override + protected void check(StaticAnalysis staticAnalysis) { + staticAnalysis.processWith(new AbstractProcessor>() { + @Override + public void process(CtInvocation ctInvocation) { + if (ctInvocation.isImplicit() + || !ctInvocation.getPosition().isValidPosition()) { + return; + } + + if (ctInvocation.getTarget() == null + || ctInvocation.getTarget().getType() == null + || !TypeUtil.isTypeEqualTo(ctInvocation.getTarget().getType(), String.class) + || !MethodUtil.isSignatureEqualTo(ctInvocation.getExecutable(), String.class, "substring", int.class, int.class)) { + return; + } + + CtExpression start = ctInvocation.getArguments().get(0); + CtExpression end = ctInvocation.getArguments().get(1); + // ensure that the end is the length of the string + if (!(end instanceof CtInvocation endInvocation) + || !MethodUtil.isSignatureEqualTo(endInvocation.getExecutable(), int.class, "length") + || endInvocation.getTarget() == null + || !(endInvocation.getTarget().equals(ctInvocation.getTarget()))) { + return; + } + + addLocalProblem( + ctInvocation, + new LocalizedMessage( + "common-reimplementation", + Map.of( + "suggestion", "%s.substring(%s)".formatted( + ctInvocation.getTarget(), + start + ) + ) + ), + ProblemType.SIMPLIFY_STRING_SUBSTRING + ); + } + }); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestSimplifyStringSubstring.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestSimplifyStringSubstring.java new file mode 100644 index 00000000..1fbed66e --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestSimplifyStringSubstring.java @@ -0,0 +1,58 @@ +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.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestSimplifyStringSubstring extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.SIMPLIFY_STRING_SUBSTRING); + + private void assertSubstring(Problem problem, String target, String start) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + "common-reimplementation", + Map.of("suggestion", "%s.substring(%s)".formatted(target, start)) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testSubstring() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + String example = "example"; + + System.out.println(example.substring(1, example.length())); + System.out.println(example.substring(3, example.length())); + System.out.println(example.substring(args.length, example.length())); + System.out.println(example.substring(0, 7)); + System.out.println(example.substring(0, example.length() - 1)); + } + } + """ + ), PROBLEM_TYPES); + + assertSubstring(problems.next(), "example", "1"); + assertSubstring(problems.next(), "example", "3"); + assertSubstring(problems.next(), "example", "args.length"); + + problems.assertExhausted(); + } +} diff --git a/sample_config.yaml b/sample_config.yaml index edab66f9..f123497b 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -160,3 +160,4 @@ problemsToReport: - IS_LETTER_OR_DIGIT - LOOP_SHOULD_BE_WHILE - USE_ARRAYS_COPY_OF + - SIMPLIFY_STRING_SUBSTRING From 970d94a7bbc4f3e494ad06340c0aafe29ce12491 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 1 Oct 2024 09:34:36 +0200 Subject: [PATCH 13/17] detect duplicate catch blocks #407 --- .../firemage/autograder/core/CodeModel.java | 2 + .../firemage/autograder/core/ProblemType.java | 6 + .../check/exceptions/DuplicateCatchBlock.java | 130 +++++++ .../core/check/structure/DuplicateCode.java | 146 +------- .../core/integrated/DuplicateCodeFinder.java | 145 ++++++++ .../structure/StructuralEqualsVisitor.java | 9 +- .../exceptions/TestDuplicateCatchBlock.java | 349 ++++++++++++++++++ sample_config.yaml | 1 + 8 files changed, 661 insertions(+), 127 deletions(-) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestDuplicateCatchBlock.java diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/CodeModel.java b/autograder-core/src/main/java/de/firemage/autograder/core/CodeModel.java index 2872555f..3d6ae5a1 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/CodeModel.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/CodeModel.java @@ -1,6 +1,7 @@ package de.firemage.autograder.core; import de.firemage.autograder.core.file.SourceInfo; +import de.firemage.autograder.core.integrated.DuplicateCodeFinder; import de.firemage.autograder.core.integrated.MethodHierarchy; import de.firemage.autograder.core.integrated.MethodUtil; import de.firemage.autograder.core.integrated.ModelBuildException; @@ -249,6 +250,7 @@ public void process(CtType type) { MethodHierarchy.buildFor(model); UsesFinder.buildFor(model); + DuplicateCodeFinder.buildFor(model); // Only set the model at the end when everything has been initialized this.model = model; 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 2b02cdf2..9dbc90a5 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 @@ -51,6 +51,12 @@ public enum ProblemType implements AbstractProblemType { */ SIMPLIFY_ARRAYS_FILL, + /** + * Reports duplicate catch blocks that could be merged with a multi-catch block. + */ + @HasFalsePositives + DUPLICATE_CATCH_BLOCK, + /** * Reports unused assignments */ diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java new file mode 100644 index 00000000..83a3c77f --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java @@ -0,0 +1,130 @@ +package de.firemage.autograder.core.check.exceptions; + +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.ExecutableCheck; +import de.firemage.autograder.core.check.structure.DuplicateCode; +import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.VariableUtil; +import de.firemage.autograder.core.integrated.structure.StructuralEqualsVisitor; +import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtCatch; +import spoon.reflect.code.CtCatchVariable; +import spoon.reflect.code.CtStatement; +import spoon.reflect.code.CtTry; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.path.CtRole; +import spoon.reflect.reference.CtCatchVariableReference; + +import java.util.ArrayList; +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; + +@ExecutableCheck(reportedProblems = { ProblemType.DUPLICATE_CATCH_BLOCK }) +public class DuplicateCatchBlock extends IntegratedCheck { + private static boolean isDuplicateBlock(CtStatement left, CtStatement right, Predicate isAllowedDifference) { + StructuralEqualsVisitor visitor = new StructuralEqualsVisitor(); + + // don't emit a problem if the catch block was already a duplicate of another block flagged by the duplicate code check + if (DuplicateCode.isConsideredDuplicateCode(List.of(left), List.of(right))) { + return false; + } + + if (visitor.checkEquals(left, right)) { + return true; + } + + for (var difference : visitor.differences()) { + if (!isAllowedDifference.test(difference)) { + return false; + } + } + + return true; + } + + // The check allows things to differ if they are associated with the catch variable. + // For example between two catches, the caught exception type will differ, which is allowed. + // Additionally, simple variable renames are allowed. + private static boolean isAssociatedWithVariable(CtCatchVariable ctCatchVariable, CtElement ctElement) { + // this is executed if the type differs: + if (ctElement.getParent() instanceof CtCatchVariableReference ctVariableReference) { + return ctCatchVariable.equals(VariableUtil.getVariableDeclaration(ctVariableReference)); + } + + // this is when the variable names differ: + if (ctElement instanceof CtCatchVariableReference ctVariableReference) { + return ctCatchVariable.equals(VariableUtil.getVariableDeclaration(ctVariableReference)); + } + + return false; + } + + @Override + protected void check(StaticAnalysis staticAnalysis) { + staticAnalysis.processWith(new AbstractProcessor() { + @Override + public void process(CtTry ctTry) { + if (ctTry.isImplicit() || !ctTry.getPosition().isValidPosition()) { + return; + } + + // TODO: for large blocks this might trigger the duplicate code check, which would be two annotations for the same problem + List catchers = ctTry.getCatchers(); + // this likely happens for try-with blocks without a catch: + if (catchers.size() < 2) { + return; + } + + Set processedCatchers = Collections.newSetFromMap(new IdentityHashMap<>(catchers.size())); + for (CtCatch firstCatch : catchers) { + var ctVariable = firstCatch.getParameter(); + Collection exceptions = new ArrayList<>(List.of(firstCatch)); + + for (CtCatch ctCatch : catchers) { + Predicate isAllowedDifference = difference -> difference.role() == CtRole.NAME + && difference.left() instanceof CtElement leftElement + && difference.right() instanceof CtElement rightElement + && isAssociatedWithVariable(ctVariable, leftElement) + && isAssociatedWithVariable(ctCatch.getParameter(), rightElement); + + if (ctCatch == firstCatch + // don't emit a problem if the catch block was already a duplicate of another block + || processedCatchers.contains(ctCatch) + || !isDuplicateBlock(firstCatch.getBody(), ctCatch.getBody(), isAllowedDifference)) { + continue; + } + + exceptions.add(ctCatch); + } + + if (exceptions.size() > 1) { + processedCatchers.addAll(exceptions); + addLocalProblem( + ctTry, + new LocalizedMessage("common-reimplementation", Map.of( + "suggestion", "try { ... } catch (%s %s) { ... }".formatted( + exceptions.stream() + .map(CtCatch::getParameter) + .map(CtCatchVariable::getMultiTypes) + .flatMap(List::stream) + .map(CtElement::toString) + .collect(Collectors.joining(" | ")), + ctVariable.getSimpleName() + ) + )), + ProblemType.DUPLICATE_CATCH_BLOCK + ); + } + } + } + }); + } +} 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 2e919193..b2951d56 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 @@ -4,26 +4,20 @@ import de.firemage.autograder.core.ProblemType; import de.firemage.autograder.core.check.ExecutableCheck; import de.firemage.autograder.core.integrated.CoreUtil; +import de.firemage.autograder.core.integrated.DuplicateCodeFinder; 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.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.Arrays; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Iterator; +import java.util.Collection; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -44,148 +38,48 @@ private static String formatSourceRange(CtElement start, CtElement end) { ); } - 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 boolean isAnyStatementIn(DuplicateCodeFinder.DuplicateCode duplicate, Collection elements) { + return duplicate.left().stream().anyMatch(elements::contains) || duplicate.right().stream().anyMatch(elements::contains); } - 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(); - } + public static boolean isConsideredDuplicateCode(List left, List right) { + var duplicate = new DuplicateCodeFinder.DuplicateCode(left, right); - @Override - public Map.Entry next() { - return Map.entry(this.keyIterator.next(), this.valueIterator.next()); - } - }; - } - - private record CodeSegment(List statements) implements Iterable { - public CodeSegment { - statements = new ArrayList<>(statements); + if (duplicate.size() < MINIMUM_DUPLICATE_STATEMENT_SIZE) { + return false; } - public static CodeSegment of(CtStatement... statement) { - return new CodeSegment(Arrays.asList(statement)); - } - - public void add(CtStatement ctStatement) { - this.statements.add(ctStatement); - } + MethodUtil.UnnamedMethod leftMethod = MethodUtil.createMethodFrom(null, duplicate.left()); + MethodUtil.UnnamedMethod rightMethod = MethodUtil.createMethodFrom(null, duplicate.right()); - public CtStatement getFirst() { - return this.statements.get(0); - } - - public CtStatement getLast() { - return this.statements.get(this.statements.size() - 1); - } - - public List statements() { - return new ArrayList<>(this.statements); - } - - @Override - public Iterator iterator() { - return this.statements().iterator(); - } + return leftMethod.canBeMethod() && rightMethod.canBeMethod(); } @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); - } - - /* - 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<>(); + Set reported = Collections.newSetFromMap(new IdentityHashMap<>()); staticAnalysis.getModel().getRootPackage().accept(new CtScanner() { private void checkCtStatement(CtStatement ctStatement) { if (ctStatement.isImplicit() || !ctStatement.getPosition().isValidPosition()) { return; } - 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; - - CodeSegment leftCode = CodeSegment.of(ctStatement); - CodeSegment rightCode = CodeSegment.of(duplicate); - - for (var entry : zip(StatementUtil.getNextStatements(ctStatement), StatementUtil.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) { - continue; - } - - MethodUtil.UnnamedMethod leftMethod = MethodUtil.createMethodFrom(null, leftCode.statements()); - MethodUtil.UnnamedMethod rightMethod = MethodUtil.createMethodFrom(null, rightCode.statements()); - - if (!leftMethod.canBeMethod() || !rightMethod.canBeMethod()) { + for (var duplicate : DuplicateCodeFinder.findDuplicates(ctStatement)) { + if (isAnyStatementIn(duplicate, reported) || !isConsideredDuplicateCode(duplicate.left(), duplicate.right())) { continue; } // prevent duplicate reporting of the same code segments - reported.addAll(leftCode.statements()); - reported.addAll(rightCode.statements()); + reported.addAll(duplicate.left()); + reported.addAll(duplicate.right()); addLocalProblem( ctStatement, new LocalizedMessage( "duplicate-code", Map.of( - "left", formatSourceRange(leftCode.getFirst(), leftCode.getLast()), - "right", formatSourceRange(rightCode.getFirst(), rightCode.getLast()) + "left", formatSourceRange(duplicate.left().get(0), duplicate.left().get(duplicate.left().size() - 1)), + "right", formatSourceRange(duplicate.right().get(0), duplicate.right().get(duplicate.right().size() - 1)) ) ), ProblemType.DUPLICATE_CODE diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java new file mode 100644 index 00000000..e5041ab9 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java @@ -0,0 +1,145 @@ +package de.firemage.autograder.core.integrated; + +import de.firemage.autograder.core.integrated.structure.StructuralElement; +import de.firemage.autograder.core.integrated.structure.StructuralEqualsVisitor; +import spoon.processing.AbstractProcessor; +import spoon.processing.FactoryAccessor; +import spoon.reflect.CtModel; +import spoon.reflect.code.CtComment; +import spoon.reflect.code.CtStatement; +import spoon.reflect.code.CtStatementList; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.stream.StreamSupport; + +public final class DuplicateCodeFinder { + private static final String METADATA_KEY = "autograder_duplicate_code_uses"; + private final Map, List> occurrences; + + private DuplicateCodeFinder(CtModel model) { + this.occurrences = new HashMap<>(); + model.processWith(new AbstractProcessor() { + @Override + public void process(CtStatement ctStatement) { + if (ctStatement.isImplicit() || !ctStatement.getPosition().isValidPosition()) { + return; + } + + DuplicateCodeFinder.this.occurrences.computeIfAbsent( + new StructuralElement<>(ctStatement), + key -> new ArrayList<>() + ).add(ctStatement); + } + }); + + /*Map> collisions = new HashMap<>(); + for (var key : this.occurrences.keySet()) { + this.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");*/ + + } + + public static void buildFor(CtModel model) { + DuplicateCodeFinder uses = new DuplicateCodeFinder(model); + model.getRootPackage().putMetadata(METADATA_KEY, uses); + } + + private static DuplicateCodeFinder getFor(FactoryAccessor factoryAccessor) { + var uses = (DuplicateCodeFinder) ElementUtil.getRootPackage(factoryAccessor).getMetadata(METADATA_KEY); + if (uses == null) { + throw new IllegalArgumentException("No duplicate code uses information available for this model"); + } + return uses; + } + + private List findDuplicateStatements(CtStatement statement) { + return Collections.unmodifiableList(this.occurrences.get(new StructuralElement<>(statement))); + } + + 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()); + } + }; + } + + public record DuplicateCode(List left, List right) { + public int size() { + return this.left.stream().map(DuplicateCode::countStatements).mapToInt(Integer::intValue).sum(); + } + + 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); + } + + public List differences() { + return StreamSupport.stream(zip(this.left, this.right).spliterator(), false) + .flatMap(entry -> StructuralEqualsVisitor.findDifferences(entry.getKey(), entry.getValue()).stream()) + .toList(); + } + } + + /** + * Finds all duplicate code blocks with the given statement. + * @param start the first statement of the code block + * @return a list of all duplicate code blocks, the left will always contain the start statement and the right will be the duplicate + */ + public static List findDuplicates(CtStatement start) { + DuplicateCodeFinder finder = DuplicateCodeFinder.getFor(start); + + List result = new ArrayList<>(); + + // we start searching for duplicates from the given statement + for (CtStatement duplicate : finder.findDuplicateStatements(start)) { + if (duplicate == start) { + continue; + } + + List leftCode = new ArrayList<>(List.of(start)); + List rightCode = new ArrayList<>(List.of(duplicate)); + + for (var entry : zip(StatementUtil.getNextStatements(start), StatementUtil.getNextStatements(duplicate))) { + if (!StructuralEqualsVisitor.equals(entry.getKey(), entry.getValue())) { + break; + } + + leftCode.add(entry.getKey()); + rightCode.add(entry.getValue()); + } + + result.add(new DuplicateCode(leftCode, rightCode)); + } + + return result; + } +} 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 9dff8355..f57f65d3 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 @@ -3,6 +3,7 @@ import de.firemage.autograder.core.integrated.ExpressionUtil; import de.firemage.autograder.core.integrated.VariableUtil; import de.firemage.autograder.core.integrated.CoreUtil; +import spoon.reflect.code.CtCatchVariable; import spoon.reflect.code.CtExpression; import spoon.reflect.code.CtLocalVariable; import spoon.reflect.code.CtVariableRead; @@ -28,6 +29,12 @@ public final class StructuralEqualsVisitor extends EqualsVisitor { public record Difference(CtRole role, Object left, Object right) {} + public static Set findDifferences(CtElement left, CtElement right) { + var visitor = new StructuralEqualsVisitor(); + visitor.checkEquals(left, right); + return visitor.differences(); + } + public StructuralEqualsVisitor() { this.differences = new LinkedHashSet<>(); } @@ -78,7 +85,7 @@ public static boolean shouldSkip(CtRole role, Object element) { // NOTE: element might be a collection of CtElements - if (role == CtRole.NAME && CoreUtil.isInstanceOfAny(element, CtLocalVariable.class, CtField.class, CtParameter.class)) { + if (role == CtRole.NAME && CoreUtil.isInstanceOfAny(element, CtLocalVariable.class, CtField.class, CtParameter.class, CtCatchVariable.class)) { return true; } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestDuplicateCatchBlock.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestDuplicateCatchBlock.java new file mode 100644 index 00000000..589c7ed5 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/exceptions/TestDuplicateCatchBlock.java @@ -0,0 +1,349 @@ +package de.firemage.autograder.core.check.exceptions; + +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.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestDuplicateCatchBlock extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.DUPLICATE_CATCH_BLOCK); + + void assertDuplicateCatch(Problem problem, List exceptions, String variable) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage( + "common-reimplementation", Map.of( + "suggestion", "try { ... } catch (%s %s) { ... }".formatted( + String.join(" | ", exceptions), variable) + ) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testMotivation() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "IllegalPlayerNameException", + """ + public class IllegalPlayerNameException extends RuntimeException { + public IllegalPlayerNameException(String message) { + super(message); + } + } + """ + ), + Map.entry( + "IllegalGodsFavorException", + """ + public class IllegalGodsFavorException extends RuntimeException { + public IllegalGodsFavorException(String message) { + super(message); + } + } + """ + ), + Map.entry( + "IllegalHealthPointsException", + """ + public class IllegalHealthPointsException extends Exception { + public IllegalHealthPointsException(String message) { + super(message); + } + } + """ + ), + Map.entry( + "Main", + """ + public class Main { + private static void foo(int count) throws IllegalHealthPointsException { + if (count == 1) { + throw new IllegalPlayerNameException("Player name is illegal"); + } else if (count == 2) { + throw new IllegalGodsFavorException("God's favor is illegal"); + } else if (count == 3) { + throw new IllegalHealthPointsException("Health points are illegal"); + } + + System.out.println("Success"); + } + + public static void main(String[] args) { + try { + foo(1); + } catch (IllegalPlayerNameException e) { + e.printStackTrace(); + } catch (IllegalGodsFavorException e) { + e.printStackTrace(); + } catch (IllegalHealthPointsException e) { + e.printStackTrace(); + } + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + assertDuplicateCatch( + problems.next(), + List.of("IllegalPlayerNameException", "IllegalGodsFavorException", "IllegalHealthPointsException"), + "e" + ); + + problems.assertExhausted(); + } + + @Test + void testAccessesDifferentVariables() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + private static void foo(int count) { + System.out.println("Success" + count); + } + + public static void main(String[] args) { + int a = 1; + int b = 2; + + try { + foo(1); + } catch (IllegalArgumentException e) { + System.out.println(a); + } catch (IllegalStateException e) { + System.out.println(b); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testDifferentCatchBlocks() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + private static void foo(int count) { + System.out.println("Success" + count); + } + + public static void main(String[] args) { + try { + foo(1); + } catch (IllegalArgumentException e) { + System.out.println(e.getMessage()); + } catch (IllegalStateException e) { + e.printStackTrace(); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testSingleCatchBlock() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + private static void foo(int count) { + System.out.println("Success" + count); + } + + public static void main(String[] args) { + try { + foo(1); + } catch (IllegalArgumentException e) { + System.out.println(e.getMessage()); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testMergeMultiTypeBlock() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + private static void foo(int count) { + System.out.println("Success" + count); + } + + public static void main(String[] args) { + try { + foo(1); + } catch (IllegalArgumentException | IllegalStateException e) { + e.printStackTrace(); + } catch (NullPointerException e) { + e.printStackTrace(); + } + } + } + """ + ), PROBLEM_TYPES); + + assertDuplicateCatch( + problems.next(), + List.of("IllegalArgumentException", "IllegalStateException", "NullPointerException"), + "e" + ); + + problems.assertExhausted(); + } + + @Test + void testDifferingVariableNames() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + private static void foo(int count) { + System.out.println("Success" + count); + } + + public static void main(String[] args) { + try { + foo(1); + } catch (IllegalArgumentException e) { + e.printStackTrace(); + } catch (NullPointerException u) { + u.printStackTrace(); + } + } + } + """ + ), PROBLEM_TYPES); + + assertDuplicateCatch( + problems.next(), + List.of("IllegalArgumentException", "NullPointerException"), + "e" + ); + + problems.assertExhausted(); + } + + @Test + void testTryWithoutCatch() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + import java.util.Scanner; + + public class Main { + private static void foo(int count) { + System.out.println("Success" + count); + } + + public static void main(String[] args) { + try (Scanner scanner = new Scanner(System.in)){ + foo(1); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testMergeNotFirstCatch() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + private static void foo(int count) { + System.out.println("Success" + count); + } + + public static void main(String[] args) { + try { + foo(1); + } catch (IllegalArgumentException e) { + e.printStackTrace(); + System.out.println("Error"); + } catch (NullPointerException e) { + e.printStackTrace(); + } catch (IllegalStateException e) { + e.printStackTrace(); + } + } + } + """ + ), PROBLEM_TYPES); + + assertDuplicateCatch( + problems.next(), + List.of("NullPointerException", "IllegalStateException"), + "e" + ); + + problems.assertExhausted(); + } + + @Test + void testCodeDuplicate() throws IOException, LinterException { + // this tests that the DuplicateCatchBlock check does not report the same problems that DuplicateCode reports + + String largeDuplicateCode = "System.out.println(\"1\");%n".formatted().repeat(30); + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + private static void foo(int count) { + System.out.println("Success" + count); + } + + public static void main(String[] args) { + try { + foo(1); + } catch (IllegalArgumentException e) { + %s + } catch (NullPointerException e) { + %s + } + } + } + """.formatted(largeDuplicateCode, largeDuplicateCode) + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + +} diff --git a/sample_config.yaml b/sample_config.yaml index f123497b..805a1c1c 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -161,3 +161,4 @@ problemsToReport: - LOOP_SHOULD_BE_WHILE - USE_ARRAYS_COPY_OF - SIMPLIFY_STRING_SUBSTRING + - DUPLICATE_CATCH_BLOCK From eb5498131b8de2dbd455b083f6cc39bcd2981a53 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 1 Oct 2024 09:39:28 +0200 Subject: [PATCH 14/17] crash if there are too many hash collisions while testing --- .../check/exceptions/DuplicateCatchBlock.java | 1 - .../core/integrated/DuplicateCodeFinder.java | 28 +++++++++++-------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java index 83a3c77f..c319e66f 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java @@ -76,7 +76,6 @@ public void process(CtTry ctTry) { return; } - // TODO: for large blocks this might trigger the duplicate code check, which would be two annotations for the same problem List catchers = ctTry.getCatchers(); // this likely happens for try-with blocks without a catch: if (catchers.size() < 2) { diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java index e5041ab9..5e3b658e 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java @@ -37,19 +37,25 @@ public void process(CtStatement ctStatement) { } }); - /*Map> collisions = new HashMap<>(); - for (var key : this.occurrences.keySet()) { - this.collisions.computeIfAbsent(key.hashCode(), k -> new ArrayList<>()).add(key); - } + if (CoreUtil.isInDebugMode()) { + Map> collisions = new HashMap<>(); + for (var key : this.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();*/ - 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");*/ + int numberOfDuplicateHashCodes = this.occurrences.size() - collisions.size(); + if (numberOfDuplicateHashCodes > 20) { + throw new IllegalStateException("Too many hash collisions %d of %d elements.".formatted(numberOfDuplicateHashCodes, this.occurrences.size())); + } + } } public static void buildFor(CtModel model) { From 75b15ba671acbf7c9d09ea110a2d0de439a495a8 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 1 Oct 2024 09:43:47 +0200 Subject: [PATCH 15/17] improve count statements performance --- .../core/check/structure/DuplicateCode.java | 2 +- .../core/integrated/DuplicateCodeFinder.java | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) 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 b2951d56..144f0d64 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 @@ -45,7 +45,7 @@ private static boolean isAnyStatementIn(DuplicateCodeFinder.DuplicateCode duplic public static boolean isConsideredDuplicateCode(List left, List right) { var duplicate = new DuplicateCodeFinder.DuplicateCode(left, right); - if (duplicate.size() < MINIMUM_DUPLICATE_STATEMENT_SIZE) { + if (!duplicate.isMoreThanOrEqualTo(MINIMUM_DUPLICATE_STATEMENT_SIZE)) { return false; } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java index 5e3b658e..ba9fdf24 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java @@ -108,6 +108,19 @@ private static int countStatements(CtStatement ctStatement) { return Math.max(count, 1); } + public boolean isMoreThanOrEqualTo(int threshold) { + int size = 0; + // TODO: this could be optimized in the future by stopping getElements when the threshold is reached + for (var statement : this.left) { + size += countStatements(statement); + if (size >= threshold) { + return true; + } + } + + return Math.max(size, 1) >= threshold; + } + public List differences() { return StreamSupport.stream(zip(this.left, this.right).spliterator(), false) .flatMap(entry -> StructuralEqualsVisitor.findDifferences(entry.getKey(), entry.getValue()).stream()) From e0a23881283d30e414c6d4bd543a413fcd240b81 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 1 Oct 2024 10:11:38 +0200 Subject: [PATCH 16/17] disable debug mode crash for collisions --- .../autograder/core/integrated/DuplicateCodeFinder.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java index ba9fdf24..66fedb1d 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/DuplicateCodeFinder.java @@ -37,25 +37,26 @@ public void process(CtStatement ctStatement) { } }); + /* if (CoreUtil.isInDebugMode()) { Map> collisions = new HashMap<>(); for (var key : this.occurrences.keySet()) { collisions.computeIfAbsent(key.hashCode(), k -> new ArrayList<>()).add(key); } - /*var mostCommonCollisions = collisions.values() + var mostCommonCollisions = collisions.values() .stream() .filter(list -> list.size() > 1) .sorted((a, b) -> Integer.compare(b.size(), a.size())) .limit(10) - .toList();*/ + .toList(); int numberOfDuplicateHashCodes = this.occurrences.size() - collisions.size(); if (numberOfDuplicateHashCodes > 20) { throw new IllegalStateException("Too many hash collisions %d of %d elements.".formatted(numberOfDuplicateHashCodes, this.occurrences.size())); } - } + }*/ } public static void buildFor(CtModel model) { From 88104f2c4fc56275ea2809e26cc43ab0ebfd3d8b Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 1 Oct 2024 10:54:43 +0200 Subject: [PATCH 17/17] implement #363 (very experimental) --- .../firemage/autograder/core/ProblemType.java | 6 + .../check/exceptions/DuplicateCatchBlock.java | 2 +- .../check/structure/DuplicateIfBlock.java | 94 +++++++++ .../core/integrated/StatementUtil.java | 8 +- .../check/structure/TestDuplicateIfBlock.java | 191 ++++++++++++++++++ sample_config.yaml | 1 + 6 files changed, 300 insertions(+), 2 deletions(-) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateIfBlock.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateIfBlock.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 9dbc90a5..4d734e2d 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 @@ -57,6 +57,12 @@ public enum ProblemType implements AbstractProblemType { @HasFalsePositives DUPLICATE_CATCH_BLOCK, + /** + * Reports duplicate if blocks that could be merged by adjusting the condition. + */ + @HasFalsePositives + DUPLICATE_IF_BLOCK, + /** * Reports unused assignments */ diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java index c319e66f..1765112b 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/DuplicateCatchBlock.java @@ -29,7 +29,7 @@ @ExecutableCheck(reportedProblems = { ProblemType.DUPLICATE_CATCH_BLOCK }) public class DuplicateCatchBlock extends IntegratedCheck { - private static boolean isDuplicateBlock(CtStatement left, CtStatement right, Predicate isAllowedDifference) { + public static boolean isDuplicateBlock(CtStatement left, CtStatement right, Predicate isAllowedDifference) { StructuralEqualsVisitor visitor = new StructuralEqualsVisitor(); // don't emit a problem if the catch block was already a duplicate of another block flagged by the duplicate code check diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateIfBlock.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateIfBlock.java new file mode 100644 index 00000000..66da7c69 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/structure/DuplicateIfBlock.java @@ -0,0 +1,94 @@ +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.check.exceptions.DuplicateCatchBlock; +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.CtIf; +import spoon.reflect.code.CtStatement; +import spoon.reflect.declaration.CtElement; + +import java.util.ArrayList; +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; + +@ExecutableCheck(reportedProblems = { ProblemType.DUPLICATE_IF_BLOCK }) +public class DuplicateIfBlock extends IntegratedCheck { + private static List getDuplicates(CtIf ctIf, Predicate isDuplicate) { + List result = new ArrayList<>(); + + List followingStatements; + if (ctIf.getElseStatement() == null) { + // the if does not have an else, so check if the following statements are duplicate if blocks + followingStatements = StatementUtil.getNextStatements(ctIf); + + + } else { + // the if has an else, so check if the else has an if with a duplicate block + followingStatements = StatementUtil.getEffectiveStatements(ctIf.getElseStatement()); + } + + for (var statement : followingStatements) { + if (!(statement instanceof CtIf nextIf) || nextIf.getThenStatement() == null || !isDuplicate.test(nextIf.getThenStatement())) { + break; + } + + if (nextIf.getElseStatement() == null) { + result.add(nextIf); + } + } + + return result; + } + + @Override + protected void check(StaticAnalysis staticAnalysis) { + Set visited = Collections.newSetFromMap(new IdentityHashMap<>()); + staticAnalysis.processWith(new AbstractProcessor() { + @Override + public void process(CtIf ctIf) { + if (ctIf.isImplicit() || !ctIf.getPosition().isValidPosition() || visited.contains(ctIf)) { + return; + } + + // what we check: + // - if (condition) followed by another if (condition) + // - if (condition) with an else { if (condition) } + + // for the merging to work, the if block must be terminal (i.e. ends with return, throw, break, continue) + if (ctIf.getThenStatement() == null || !StatementUtil.isTerminal(ctIf.getThenStatement())) { + return; + } + + List duplicates = getDuplicates(ctIf, ctStatement -> DuplicateCatchBlock.isDuplicateBlock(ctIf.getThenStatement(), ctStatement, difference -> false)); + + if (!duplicates.isEmpty()) { + visited.add(ctIf); + visited.addAll(duplicates); + addLocalProblem( + ctIf, + new LocalizedMessage("common-reimplementation", Map.of( + "suggestion", "if (%s || %s) { ... }".formatted( + ctIf.getCondition(), + duplicates.stream() + .map(CtIf::getCondition) + .map(CtElement::toString) + .collect(Collectors.joining(" || ")) + ) + )), + ProblemType.DUPLICATE_IF_BLOCK + ); + } + } + }); + } +} diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/StatementUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/StatementUtil.java index bf34db7a..3137478c 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/StatementUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/StatementUtil.java @@ -18,7 +18,7 @@ import java.util.Optional; import java.util.stream.Stream; -public class StatementUtil { +public final class StatementUtil { private StatementUtil() { } @@ -140,6 +140,12 @@ public static Optional getSingleEffect(Collection return tryMakeEffect(statements.get(0)); } + public static boolean isTerminal(CtStatement ctStatement) { + List statements = StatementUtil.getEffectiveStatements(ctStatement); + + return !statements.isEmpty() && tryMakeEffect(statements.get(statements.size() - 1)).map(TerminalEffect.class::isInstance).orElse(false); + } + public static List getCasesEffects(Iterable> ctCases) { List effects = new ArrayList<>(); for (CtCase ctCase : ctCases) { diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateIfBlock.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateIfBlock.java new file mode 100644 index 00000000..7dc8f82d --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/structure/TestDuplicateIfBlock.java @@ -0,0 +1,191 @@ +package de.firemage.autograder.core.check.structure; + +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.api.Disabled; +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 TestDuplicateIfBlock extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.DUPLICATE_IF_BLOCK); + + void assertDuplicate(Problem problem, List conditions) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage( + "common-reimplementation", + Map.of( + "suggestion", "if (%s) { ... }".formatted(String.join(" || ", conditions)) + ) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testMissingThen() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i == 0); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testIfElse() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i == 0) { + System.out.println("zero"); + return; + } else { + if (i == 1) { + System.out.println("zero"); + return; + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertDuplicate(problems.next(), List.of("i == 0", "i == 1")); + + problems.assertExhausted(); + } + + @Test + void testIfElseExtraStatement() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i == 0) { + System.out.println("zero"); + return; + } else { + if (i == 1) { + System.out.println("zero"); + return; + } + + System.out.println("extra"); + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertDuplicate(problems.next(), List.of("i == 0", "i == 1")); + + problems.assertExhausted(); + } + + + @Test + void testMultipleNestedIf() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i == 0) { + System.out.println("zero"); + return; + } else { + if (i == 1) { + System.out.println("zero"); + return; + } else { + if (i == 2) { + System.out.println("zero"); + return; + } else { + if (i == 3) { + System.out.println("zero"); + return; + } + } + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertDuplicate(problems.next(), List.of("i == 2", "i == 3")); + + problems.assertExhausted(); + } + + @Test + void testFollowingIf() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + void foo(int i) { + if (i == 0) { + System.out.println("zero"); + return; + } + + if (i == 1) { + System.out.println("zero"); + return; + } + + if (i == 2) { + System.out.println("zero"); + return; + } + + if (i == 3) { + System.out.println("1"); + return; + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertDuplicate(problems.next(), List.of("i == 0", "i == 1", "i == 2")); + + problems.assertExhausted(); + } +} diff --git a/sample_config.yaml b/sample_config.yaml index 805a1c1c..64ad4af1 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -162,3 +162,4 @@ problemsToReport: - USE_ARRAYS_COPY_OF - SIMPLIFY_STRING_SUBSTRING - DUPLICATE_CATCH_BLOCK + - DUPLICATE_IF_BLOCK