From dd9c8af7dbb5b6f06a343339ecf4079c5dc130e6 Mon Sep 17 00:00:00 2001 From: luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 13 Jan 2024 09:26:15 +0100 Subject: [PATCH 01/10] hide contributors block for releases --- jreleaser.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/jreleaser.yml b/jreleaser.yml index 4f3a84da..c6c9768d 100644 --- a/jreleaser.yml +++ b/jreleaser.yml @@ -25,11 +25,7 @@ release: preset: conventional-commits format: '- {{commitShortHash}} {{commitTitle}}' contributors: - format: '- {{contributorName}}{{#contributorUsernameAsLink}} ({{.}}){{/contributorUsernameAsLink}}' - hide: - contributors: - - '[bot]' - - 'GitHub' + enabled: false distributions: helloworld: From 86e190b907ba600e888eb8b328be1a230839555a Mon Sep 17 00:00:00 2001 From: luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 13 Jan 2024 10:29:55 +0100 Subject: [PATCH 02/10] =?UTF-8?q?detect=20encoding=20correctly=20for=20fil?= =?UTF-8?q?es=20with=20'=C2=A7'=20#368?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../autograder/core/file/FileSourceInfo.java | 16 +++++++++ .../core/file/TestFileSourceInfo.java | 34 +++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/file/TestFileSourceInfo.java diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/file/FileSourceInfo.java b/autograder-core/src/main/java/de/firemage/autograder/core/file/FileSourceInfo.java index 8a388f21..cc4f0430 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/file/FileSourceInfo.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/file/FileSourceInfo.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -54,9 +55,24 @@ public class FileSourceInfo implements SourceInfo, Serializable { } private SerializableCharset detectCharset(File file, SourcePath sourcePath) { + // There is an issue where it detects TIS-620 for a file that contains a '§', even though it is UTF-8. + // See https://github.com/Feuermagier/autograder/issues/368. + // + // According to this issue https://github.com/albfernandez/juniversalchardet/issues/22 it is impossible + // for the detector to find the correct charset in some cases. + // + // The workaround is to use a list of charsets that are likely to be used in a submission and if one + // has been detected that is not in the list, use UTF-8 by default. + Set supportedCharsets = Set.of( + StandardCharsets.UTF_8, + StandardCharsets.US_ASCII, + StandardCharsets.ISO_8859_1 + ); + try { return new SerializableCharset(Optional.ofNullable(UniversalDetector.detectCharset(file)) .map(Charset::forName) + .filter(supportedCharsets::contains) .orElse(StandardCharsets.UTF_8)); } catch (IOException e) { throw new IllegalArgumentException("Failed to read file '%s' for detecting charset".formatted(sourcePath), e); diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/file/TestFileSourceInfo.java b/autograder-core/src/test/java/de/firemage/autograder/core/file/TestFileSourceInfo.java new file mode 100644 index 00000000..fba5702e --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/file/TestFileSourceInfo.java @@ -0,0 +1,34 @@ +package de.firemage.autograder.core.file; + +import de.firemage.autograder.core.compiler.JavaVersion; +import de.firemage.autograder.core.errorprone.TempLocation; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestFileSourceInfo { + private final TempLocation tempLocation = TempLocation.random(); + + // See https://github.com/Feuermagier/autograder/issues/368 + @Test + void testDetectThaiEncoding() throws IOException { + try (TempLocation folder = tempLocation.createTempDirectory("test")) { + Path folderPath = folder.tempLocation().toPath(); + Path filePath = Paths.get(folderPath.toString(), "Test.java"); + Files.write(filePath, "public class Test { char symbol = '§'; }".getBytes()); + + FileSourceInfo sourceInfo = new FileSourceInfo(folderPath, JavaVersion.JAVA_17); + + assertEquals(1, sourceInfo.compilationUnits().size()); + for (CompilationUnit unit : sourceInfo.compilationUnits()) { + assertEquals(StandardCharsets.UTF_8, unit.charset()); + } + } + } +} From c8fefabce44029835d9e9428613ffb9b2d38ac58 Mon Sep 17 00:00:00 2001 From: luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 13 Jan 2024 11:12:07 +0100 Subject: [PATCH 03/10] improve compare char value check #367 --- .../core/check/general/CompareCharValue.java | 60 ++++-- .../src/main/resources/strings.de.ftl | 2 +- .../src/main/resources/strings.en.ftl | 2 +- .../check/general/TestCompareCharValue.java | 180 ++++++++++++++++++ .../CompareCharValue/code/Test.java | 9 - .../check_tests/CompareCharValue/config.txt | 4 - 6 files changed, 230 insertions(+), 27 deletions(-) create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestCompareCharValue.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/CompareCharValue/code/Test.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/CompareCharValue/config.txt diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/CompareCharValue.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/CompareCharValue.java index 31068514..5916929e 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/CompareCharValue.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/CompareCharValue.java @@ -12,14 +12,17 @@ import spoon.reflect.code.CtBinaryOperator; import spoon.reflect.code.CtExpression; import spoon.reflect.code.CtLiteral; -import spoon.reflect.reference.CtTypeReference; import java.util.EnumSet; +import java.util.Map; +import java.util.Optional; import java.util.Set; -@ExecutableCheck(reportedProblems = { ProblemType.COMPARE_CHAR_VALUE }) +@ExecutableCheck(reportedProblems = {ProblemType.COMPARE_CHAR_VALUE}) public class CompareCharValue extends IntegratedCheck { + private static final int MIN_ASCII_VALUE = 1; private static final int MAX_ASCII_VALUE = 127; + private static final Set COMPARISON_OPERATORS = EnumSet.of( BinaryOperatorKind.EQ, BinaryOperatorKind.NE, @@ -29,32 +32,65 @@ public class CompareCharValue extends IntegratedCheck { BinaryOperatorKind.LT ); + private static Optional getComparedIntegerValue(CtExpression left, CtExpression right) { + if (!SpoonUtil.isTypeEqualTo(left.getType(), char.class) + || !(SpoonUtil.resolveConstant(right) instanceof CtLiteral literal && literal.getValue() instanceof Integer value)) { + return Optional.empty(); + } + + if (value < MIN_ASCII_VALUE || value > MAX_ASCII_VALUE) { + return Optional.empty(); + } + + return Optional.of(value); + } + @Override protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) { staticAnalysis.processWith(new AbstractProcessor>() { @Override public void process(CtBinaryOperator ctBinaryOperator) { - if (!COMPARISON_OPERATORS.contains(ctBinaryOperator.getKind())) return; + if (ctBinaryOperator.isImplicit() + || !ctBinaryOperator.getPosition().isValidPosition() + || !COMPARISON_OPERATORS.contains(ctBinaryOperator.getKind())) { + return; + } - CtExpression lhs = SpoonUtil.resolveCtExpression(ctBinaryOperator.getLeftHandOperand()); - CtExpression rhs = SpoonUtil.resolveCtExpression(ctBinaryOperator.getRightHandOperand()); + CtExpression left = ctBinaryOperator.getLeftHandOperand(); + CtExpression right = ctBinaryOperator.getRightHandOperand(); - CtTypeReference charType = lhs.getFactory().Type().createReference(char.class); + CtExpression charExpression = left; + Optional number = getComparedIntegerValue(left, right); - if (lhs.getType().equals(charType) && rhs instanceof CtLiteral ctLiteral && ctLiteral.getValue() instanceof Integer intValue) { - if (intValue > MAX_ASCII_VALUE) return; - } else if (lhs instanceof CtLiteral ctLiteral && ctLiteral.getValue() instanceof Integer intValue && rhs.getType().equals(charType)) { - if (intValue > MAX_ASCII_VALUE) return; - } else { + if (number.isEmpty()) { + charExpression = right; + number = getComparedIntegerValue(right, left); + } + + if (number.isEmpty()) { return; } + int asciiValue = number.get(); + addLocalProblem( ctBinaryOperator, - new LocalizedMessage("compare-char-value"), + new LocalizedMessage( + "compare-char-value", + Map.of( + "expression", charExpression.prettyprint(), + "intValue", asciiValue, + "charValue", "" + (char) asciiValue + ) + ), ProblemType.COMPARE_CHAR_VALUE ); } }); } + + @Override + public Optional maximumProblems() { + return Optional.of(3); + } } diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index 41cfca97..a6b5772a 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -171,7 +171,7 @@ scanner-closed = Scanner sollte geschlossen werden unchecked-type-cast = Es muss sicher gestellt werden, dass der Typ des Objekts mit dem Typ des Casts übereinstimmt. Ansonsten kann der Code abstürzen. -compare-char-value = char-Werte im ASCII Bereich sollten als char-Werte verglichen werden, nicht als int-Werte. +compare-char-value = Hier wird '{$expression}' vom Typ char mit dem Wert {$intValue} verglichen. Es ist nicht offensichtlich für welchen Buchstabe der Wert steht, schreibe stattdessen '{$charValue}'. use-guard-clauses = Der Code bricht den normalen Kontrollfluss durch zum Beispiel ein return ab. if-else-Blöcke mit solchen Abbrüchen kann man mithilfe von sogenannten guard-clauses schöner schreiben. Das hat unter anderem den Vorteil, dass man doppelten Code leichter erkennt. Siehe für eine detaillierte Erklärung https://medium.com/@scadge/if-statements-design-guard-clauses-might-be-all-you-need-67219a1a981a oder https://deviq.com/design-patterns/guard-clause diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl index ad3a4d91..1c8da81f 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -173,7 +173,7 @@ scanner-closed = Scanner should be closed unchecked-type-cast = It has to be ensured that the type of the object is the same as that of the cast. Otherwise, the code might crash. -compare-char-value = char values in the ASCII range should be compared as char values, not as int values. +compare-char-value = Here '{$expression}' of type char is compared with the value {$intValue}. It is not obvious which letter the value represents, therefore write '{$charValue}'. use-guard-clauses = The code cancels the normal control-flow through for example a return. if-else-blocks with those conditions can be written more beautifully using so called guard-clauses. This has the advantage that you can better recognize duplicate code. See for a detailed explanation https://medium.com/@scadge/if-statements-design-guard-clauses-might-be-all-you-need-67219a1a981a or https://deviq.com/design-patterns/guard-clause diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestCompareCharValue.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestCompareCharValue.java new file mode 100644 index 00000000..03cc0bf3 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestCompareCharValue.java @@ -0,0 +1,180 @@ +package de.firemage.autograder.core.check.general; + +import de.firemage.autograder.core.LinterException; +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.Problem; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.AbstractCheckTest; +import de.firemage.autograder.core.compiler.JavaVersion; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestCompareCharValue extends AbstractCheckTest { + private static final String LOCALIZED_MESSAGE_KEY = "compare-char-value"; + private static final List PROBLEM_TYPES = List.of(ProblemType.COMPARE_CHAR_VALUE); + + private void assertEqualsCompare(Problem problem, String expression, int intValue, char charValue) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + LOCALIZED_MESSAGE_KEY, + Map.of( + "expression", expression, + "intValue", intValue, + "charValue", charValue + ) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testEqualsCompare() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + char c = 'a'; + if (c == 97) { + System.out.println("a"); + } + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsCompare(problems.next(), "c", 97, 'a'); + + problems.assertExhausted(); + } + + @Test + void testChainedComparison() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + char symbol = 'a'; + boolean isValid = (symbol > 47 && symbol < 58) || symbol == '*' || symbol == '-' || symbol == '+'; + isValid = (symbol > '/' && symbol < ':') || symbol == '*' || symbol == '-' || symbol == '+'; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsCompare(problems.next(), "symbol", 47, '/'); + assertEqualsCompare(problems.next(), "symbol", 58, ':'); + + problems.assertExhausted(); + } + + @Test + void testCompareWithZero() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + char symbol = 'a'; + boolean isValid = symbol != 0; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testCompareWithIntConstant() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private static final int EXPECTED_VALUE = 47; + + public static void main(String[] args) { + char symbol = 'a'; + boolean isValid = symbol == EXPECTED_VALUE; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsCompare(problems.next(), "symbol", 47, '/'); + + problems.assertExhausted(); + } + + @Test + void testCompareWithBoxedInt() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private static final Integer EXPECTED_VALUE = 47; + + public static void main(String[] args) { + char symbol = 'a'; + boolean isValid = symbol == EXPECTED_VALUE; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsCompare(problems.next(), "symbol", 47, '/'); + + problems.assertExhausted(); + } + + + @Test + void testCompareWithComplexCharExpression() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + boolean isValid = args[0].charAt(args.length) == 47; + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsCompare(problems.next(), "args[0].charAt(args.length)", 47, '/'); + + problems.assertExhausted(); + } + + @Test + void testCompareTooLargeInt() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + char symbol = 'a'; + boolean isValid = symbol <= 1000; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/CompareCharValue/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/CompareCharValue/code/Test.java deleted file mode 100644 index 90cce395..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/CompareCharValue/code/Test.java +++ /dev/null @@ -1,9 +0,0 @@ -package de.firemage.autograder.core.check_tests.CompareCharValue.code; - -public class Test { - public static void main(String[] args) { - char symbol = 'a'; - boolean isValid = (symbol > 47 /*# not ok #*/ && symbol < 58 /*# not ok #*/) || symbol == '*' || symbol == '-' || symbol == '+'; - isValid = (symbol > '/' && symbol < ':') || symbol == '*' || symbol == '-' || symbol == '+'; /*# ok #*/ - } -} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/CompareCharValue/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/CompareCharValue/config.txt deleted file mode 100644 index 1da0772c..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/CompareCharValue/config.txt +++ /dev/null @@ -1,4 +0,0 @@ -general.CompareCharValue -Character should be compared to chars and not int values -Test.java:6 -Test.java:6 From d9218c98eac9c33475a6954979be860b364b0fa4 Mon Sep 17 00:00:00 2001 From: luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 13 Jan 2024 11:34:41 +0100 Subject: [PATCH 04/10] detect uses of `String#concat` #364 --- .../firemage/autograder/core/ProblemType.java | 1 + .../core/check/api/AvoidStringConcat.java | 59 +++++++++++++++ .../core/check/api/TestAvoidStringConcat.java | 74 +++++++++++++++++++ sample_config.yaml | 1 + 4 files changed, 135 insertions(+) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/api/AvoidStringConcat.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestAvoidStringConcat.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 178a5bfa..0a76520c 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 @@ -145,5 +145,6 @@ public enum ProblemType { ARRAY_AS_KEY_OF_SET_OR_MAP, MULTIPLE_INLINE_STATEMENTS, UNNECESSARY_BOXING, + AVOID_STRING_CONCAT, REDUNDANT_UNINITIALIZED_VARIABLE } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/AvoidStringConcat.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/AvoidStringConcat.java new file mode 100644 index 00000000..12f93eaf --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/AvoidStringConcat.java @@ -0,0 +1,59 @@ +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.dynamic.DynamicAnalysis; +import de.firemage.autograder.core.integrated.CtRange; +import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.SpoonUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import org.apache.commons.lang3.Range; +import spoon.processing.AbstractProcessor; +import spoon.reflect.code.BinaryOperatorKind; +import spoon.reflect.code.CtBinaryOperator; +import spoon.reflect.code.CtExpression; +import spoon.reflect.code.CtInvocation; +import spoon.reflect.factory.Factory; +import spoon.reflect.reference.CtTypeReference; + +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +@ExecutableCheck(reportedProblems = { ProblemType.AVOID_STRING_CONCAT }) +public class AvoidStringConcat extends IntegratedCheck { + @Override + protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) { + 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 + || !SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), String.class, "concat", String.class)) { + return; + } + + addLocalProblem( + ctInvocation, + new LocalizedMessage( + "common-reimplementation", + Map.of( + "suggestion", "%s + %s".formatted( + ctInvocation.getTarget().prettyprint(), + ctInvocation.getArguments().get(0).prettyprint() + ) + ) + ), + ProblemType.AVOID_STRING_CONCAT + ); + } + }); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestAvoidStringConcat.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestAvoidStringConcat.java new file mode 100644 index 00000000..aa37f542 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestAvoidStringConcat.java @@ -0,0 +1,74 @@ +package de.firemage.autograder.core.check.api; + +import de.firemage.autograder.core.LinterException; +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.Problem; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.AbstractCheckTest; +import de.firemage.autograder.core.compiler.JavaVersion; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestAvoidStringConcat extends AbstractCheckTest { + private static final String LOCALIZED_MESSAGE_KEY = "common-reimplementation"; + private static final List PROBLEM_TYPES = List.of(ProblemType.AVOID_STRING_CONCAT); + + private void assertEqualsConcat(Problem problem, String suggestion) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + LOCALIZED_MESSAGE_KEY, + Map.of("suggestion", suggestion) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testConcat() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static String join(String left, String right) { + return left.concat(right); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsConcat(problems.next(), "left + right"); + + problems.assertExhausted(); + } + + @Test + void testConcatStream() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + + public class Test { + public static String join(List values, String right) { + return values.stream().map(right::concat).reduce(String::concat).orElse(""); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } +} diff --git a/sample_config.yaml b/sample_config.yaml index 177c27b4..b2da12df 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -142,3 +142,4 @@ - TOO_FEW_PACKAGES - TRY_CATCH_COMPLEXITY - AVOID_STATIC_BLOCKS +- AVOID_STRING_CONCAT From 9845fbc278f5ec2e8a88977f80521dff8e2fad6c Mon Sep 17 00:00:00 2001 From: luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 13 Jan 2024 11:41:14 +0100 Subject: [PATCH 05/10] ignore unused `serialVersionUID` #360 --- .../unnecessary/UnusedCodeElementCheck.java | 5 ++++ .../TestUnusedCodeElementCheck.java | 29 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java index 0b462fab..90ef2bcf 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java @@ -125,6 +125,11 @@ public void visitCtTypeParameter(CtTypeParameter ctTypeParameter) { @Override public void visitCtField(CtField ctField) { + if (ctField.getSimpleName().equals("serialVersionUID")) { + super.visitCtField(ctField); + return; + } + checkUnused(staticAnalysis, ctField); super.visitCtField(ctField); } 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 be056d9b..11e9759c 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 @@ -566,4 +566,33 @@ public static void main(String[] args) { problems.assertExhausted(); } + + @Test + void testSerialVersionUID() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings( + JavaVersion.JAVA_17, + Map.ofEntries( + Map.entry( + "Main", + """ + public class Main { + public static void main(String[] args) { + throw new MyException(); + } + } + """ + ), + Map.entry( + "MyException", + """ + public class MyException extends RuntimeException { + private static final long serialVersionUID = 1L; + } + """ + ) + ) + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } } From 65adc8862f6470e9801b404f323985588a1fa051 Mon Sep 17 00:00:00 2001 From: luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 13 Jan 2024 11:42:24 +0100 Subject: [PATCH 06/10] only highlight first line for loops #361 --- .../main/java/de/firemage/autograder/core/CodePosition.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/CodePosition.java b/autograder-core/src/main/java/de/firemage/autograder/core/CodePosition.java index 225f3410..6730e147 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/CodePosition.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/CodePosition.java @@ -2,6 +2,7 @@ import de.firemage.autograder.core.file.SourceInfo; import de.firemage.autograder.core.file.SourcePath; +import spoon.reflect.code.CtLoop; import spoon.reflect.cu.SourcePosition; import spoon.reflect.declaration.CtElement; import spoon.reflect.declaration.CtMethod; @@ -32,7 +33,7 @@ public static CodePosition fromSourcePosition( // Instead of highlighting all lines of a class or method, only highlight the first line. // // Someone might explicitly specify a source position, in which case it will differ from the element's position. - if ((ctElement instanceof CtType || ctElement instanceof CtMethod) && ctElement.getPosition().equals(sourcePosition)) { + if ((ctElement instanceof CtType || ctElement instanceof CtMethod || ctElement instanceof CtLoop) && ctElement.getPosition().equals(sourcePosition)) { return new CodePosition( sourceInfo, relativePath, From b42579267698ada1b75493e2e9771f850462b8f8 Mon Sep 17 00:00:00 2001 From: luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 13 Jan 2024 11:56:29 +0100 Subject: [PATCH 07/10] detect empty comments #366 --- .../firemage/autograder/core/ProblemType.java | 1 + .../check/comment/UnnecessaryComment.java | 42 +++++++++++ .../src/main/resources/strings.de.ftl | 2 + .../src/main/resources/strings.en.ftl | 2 + .../check/comment/TestUnnecessaryComment.java | 70 +++++++++++++++++++ sample_config.yaml | 1 + 6 files changed, 118 insertions(+) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/comment/UnnecessaryComment.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/comment/TestUnnecessaryComment.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 0a76520c..74b12f60 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 @@ -146,5 +146,6 @@ public enum ProblemType { MULTIPLE_INLINE_STATEMENTS, UNNECESSARY_BOXING, AVOID_STRING_CONCAT, + UNNECESSARY_COMMENT, REDUNDANT_UNINITIALIZED_VARIABLE } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/comment/UnnecessaryComment.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/comment/UnnecessaryComment.java new file mode 100644 index 00000000..75b0848d --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/comment/UnnecessaryComment.java @@ -0,0 +1,42 @@ +package de.firemage.autograder.core.check.comment; + +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.ExecutableCheck; +import de.firemage.autograder.core.dynamic.DynamicAnalysis; +import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.SpoonUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtComment; +import spoon.reflect.code.CtStatement; + +@ExecutableCheck(reportedProblems = { ProblemType.UNNECESSARY_COMMENT }) +public class UnnecessaryComment extends IntegratedCheck { + @Override + protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) { + staticAnalysis.processWith(new AbstractProcessor() { + @Override + public void process(CtComment ctComment) { + if (ctComment.isImplicit() + || !ctComment.getPosition().isValidPosition() + || ctComment.getCommentType() != CtComment.CommentType.INLINE) { + return; + } + + CtStatement previous = SpoonUtil.getPreviousStatement(ctComment).orElse(null); + if (previous instanceof CtComment) { + return; + } + + if (ctComment.getContent().isBlank()) { + addLocalProblem( + ctComment, + new LocalizedMessage("unnecessary-comment-empty"), + ProblemType.UNNECESSARY_COMMENT + ); + } + } + }); + } +} diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index a6b5772a..a604264f 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -60,6 +60,8 @@ comment-language-exp-invalid = Dieser Kommentar ist weder auf Deutsch noch auf E comment-language-exp-english = Der Code enthält deutsche und englische Kommentare. Dieser Kommentar ist auf Englisch. Ein deutscher Kommentar befindet sich bei {$path}:{$line} comment-language-exp-german = Der Code enthält deutsche und englische Kommentare. Dieser Kommentar ist auf Deutsch. Ein englischer Kommentar befindet sich bei {$path}:{$line} +unnecessary-comment-empty = Dieser Kommentar ist leer und sollte daher entfernt werden + javadoc-method-exp-param-missing = Der Parameter '{$param}' wird im Javadoc-Kommentar nicht erwähnt javadoc-method-exp-param-unknown = Der JavaDoc-Kommentar erwähnt den Parameter '{$param}', dieser wird allerdings nicht deklariert diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl index 1c8da81f..b51aadfc 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -61,6 +61,8 @@ comment-language-exp-invalid = The language of this comment is neither English n comment-language-exp-english = The code contains comments in German and in English. This comment is in English. A German comment can be found at {$path}:{$line} comment-language-exp-german = The code contains comments in German and in English. This comment is in German. An English comment can be found at {$path}:{$line} +unnecessary-comment-empty = This comment is empty and should therefore be removed + javadoc-method-exp-param-missing = The parameter '{$param}' is not mentioned in the JavaDoc comment javadoc-method-exp-param-unknown = The JavaDoc comment mentions the parameter '{$param}', but the parameter doesn't exist diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/comment/TestUnnecessaryComment.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/comment/TestUnnecessaryComment.java new file mode 100644 index 00000000..0201bfc6 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/comment/TestUnnecessaryComment.java @@ -0,0 +1,70 @@ +package de.firemage.autograder.core.check.comment; + +import de.firemage.autograder.core.LinterException; +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.Problem; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.AbstractCheckTest; +import de.firemage.autograder.core.compiler.JavaVersion; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestUnnecessaryComment extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.UNNECESSARY_COMMENT); + + void assertEqualsEmpy(Problem problem) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage("unnecessary-comment-empty")), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testPlaceholder() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator( + StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public Test() { + // + } + } + """ + ), + PROBLEM_TYPES + ); + + assertEqualsEmpy(problems.next()); + + problems.assertExhausted(); + } + + @Test + void testCommentSpacer() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator( + StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public Test() { + // Here is a very long explanation + // + // ^^ here the line is empty and valid for spacing + } + } + """ + ), + PROBLEM_TYPES + ); + + problems.assertExhausted(); + } +} diff --git a/sample_config.yaml b/sample_config.yaml index b2da12df..ca791d3f 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -143,3 +143,4 @@ - TRY_CATCH_COMPLEXITY - AVOID_STATIC_BLOCKS - AVOID_STRING_CONCAT +- UNNECESSARY_COMMENT From 05b1bc9788b0e6ec10dd87c3ff64701ec9ec1b8b Mon Sep 17 00:00:00 2001 From: luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 13 Jan 2024 12:26:10 +0100 Subject: [PATCH 08/10] detect uses of `Object` as type #365 --- .../firemage/autograder/core/ProblemType.java | 1 + .../core/check/general/ObjectDatatype.java | 49 +++++++ .../src/main/resources/strings.de.ftl | 2 + .../src/main/resources/strings.en.ftl | 3 + .../check/general/TestObjectDatatype.java | 129 ++++++++++++++++++ sample_config.yaml | 1 + 6 files changed, 185 insertions(+) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/general/ObjectDatatype.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestObjectDatatype.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 74b12f60..ef9d4a3b 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 @@ -147,5 +147,6 @@ public enum ProblemType { UNNECESSARY_BOXING, AVOID_STRING_CONCAT, UNNECESSARY_COMMENT, + OBJECT_DATATYPE, REDUNDANT_UNINITIALIZED_VARIABLE } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/ObjectDatatype.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/ObjectDatatype.java new file mode 100644 index 00000000..febc8172 --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/ObjectDatatype.java @@ -0,0 +1,49 @@ +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.dynamic.DynamicAnalysis; +import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.SpoonUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import spoon.processing.AbstractProcessor; +import spoon.reflect.declaration.CtVariable; +import spoon.reflect.reference.CtTypeReference; + +import java.util.Map; + +@ExecutableCheck(reportedProblems = { ProblemType.OBJECT_DATATYPE }) +public class ObjectDatatype extends IntegratedCheck { + private static boolean hasObjectType(CtTypeReference ctTypeReference) { + return !ctTypeReference.isGenerics() && SpoonUtil.isTypeEqualTo(ctTypeReference, java.lang.Object.class) + || ctTypeReference.getActualTypeArguments().stream().anyMatch(ObjectDatatype::hasObjectType); + } + + @Override + protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) { + staticAnalysis.processWith(new AbstractProcessor>() { + @Override + public void process(CtVariable ctVariable) { + if (ctVariable.isImplicit() || !ctVariable.getPosition().isValidPosition()) { + return; + } + + if (SpoonUtil.isInOverriddenMethod(ctVariable) || ctVariable.getType().isArray()) { + return; + } + + if (hasObjectType(ctVariable.getType())) { + addLocalProblem( + ctVariable, + new LocalizedMessage( + "object-datatype", + Map.of("variable", ctVariable.getSimpleName()) + ), + ProblemType.OBJECT_DATATYPE + ); + } + } + }); + } +} diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index a604264f..ec006a0f 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -187,6 +187,8 @@ merge-nested-if = Die Verschachtelte if kann mit der äußeren if kombiniert wer binary-operator-on-boolean = Statt '|' und '&' sollte man '||' und '&&' verwenden. +object-datatype = Statt dem Datentyp 'Object', sollte die Variable '{$variable}' einen konkreten oder generischen Datentyp haben. + # 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 b51aadfc..bda9affd 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -187,6 +187,9 @@ avoid-recompiling-regex = The constant is only used with 'Pattern.compile' or 'P binary-operator-on-boolean = Instead of '|' and '&' one should use '||' and '&&'. +object-datatype = Instead of the datatype 'Object', the variable '{$variable}' should have a concrete or generic datatype. + + # 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/TestObjectDatatype.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestObjectDatatype.java new file mode 100644 index 00000000..17d25330 --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestObjectDatatype.java @@ -0,0 +1,129 @@ +package de.firemage.autograder.core.check.general; + +import de.firemage.autograder.core.LinterException; +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.Problem; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.AbstractCheckTest; +import de.firemage.autograder.core.compiler.JavaVersion; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestObjectDatatype extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.OBJECT_DATATYPE); + + void assertHasObject(Problem problem, String variable) { + assertEquals( + this.linter.translateMessage(new LocalizedMessage( + "object-datatype", + Map.of("variable", variable) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testObjectVariable() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + Object field; + + void method() { + Object local = "abc"; + } + } + """ + ), PROBLEM_TYPES); + + assertHasObject(problems.next(), "field"); + assertHasObject(problems.next(), "local"); + problems.assertExhausted(); + } + + @Test + void testObjectVariableArray() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + Object[] field1; + Object[][] field2; + + void method() { + Object[][] local = null; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testObjectEquals() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + class Test { + @Override + public boolean equals(Object o) { + return false; + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testGenerics() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + + class Test { + List list; + List erased; + T field; + + List objects; + } + """ + ), PROBLEM_TYPES); + + assertHasObject(problems.next(), "objects"); + + problems.assertExhausted(); + } + + @Test + void testRawTypes() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + import java.util.List; + + class Test { + List list; + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } +} diff --git a/sample_config.yaml b/sample_config.yaml index ca791d3f..b4186886 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -144,3 +144,4 @@ - AVOID_STATIC_BLOCKS - AVOID_STRING_CONCAT - UNNECESSARY_COMMENT +- OBJECT_DATATYPE From 3569e69b2eb9d666d2574ebf8335f360a340bb0e Mon Sep 17 00:00:00 2001 From: luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 13 Jan 2024 12:28:06 +0100 Subject: [PATCH 09/10] fix tests --- .../firemage/autograder/core/check_tests/AvoidLabels/config.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/AvoidLabels/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/AvoidLabels/config.txt index 892bc4d1..242a9e99 100644 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/AvoidLabels/config.txt +++ b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/AvoidLabels/config.txt @@ -2,4 +2,4 @@ general.AvoidLabels Avoid Labels Test.java:5-7 Test.java:8-9 -Test.java:11-13 +Test.java:11 From d71ec100864c04abb76b50d0f43f1bda0b910bf9 Mon Sep 17 00:00:00 2001 From: luro02 <24826124+Luro02@users.noreply.github.com> Date: Sat, 13 Jan 2024 13:14:52 +0100 Subject: [PATCH 10/10] detect common reimplementations of `Math` api #311 --- .../firemage/autograder/core/ProblemType.java | 2 + .../check/api/CommonReimplementation.java | 105 ------- .../core/check/api/MathReimplementation.java | 241 ++++++++++++++++ .../check/api/TestCommonReimplementation.java | 167 ------------ .../check/api/TestMathReimplementation.java | 257 ++++++++++++++++++ sample_config.yaml | 2 + 6 files changed, 502 insertions(+), 272 deletions(-) create mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/api/MathReimplementation.java create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestMathReimplementation.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 ef9d4a3b..ad42bfd2 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 @@ -90,6 +90,8 @@ public enum ProblemType { COMMON_REIMPLEMENTATION_ARRAY_COPY, COMMON_REIMPLEMENTATION_STRING_REPEAT, COMMON_REIMPLEMENTATION_MAX_MIN, + COMMON_REIMPLEMENTATION_SQRT, + COMMON_REIMPLEMENTATION_HYPOT, COMMON_REIMPLEMENTATION_ADD_ALL, COMMON_REIMPLEMENTATION_ADD_ENUM_VALUES, COMMON_REIMPLEMENTATION_ARRAYS_FILL, diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CommonReimplementation.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CommonReimplementation.java index 55c005ce..97c08d13 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CommonReimplementation.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/CommonReimplementation.java @@ -150,110 +150,6 @@ private void checkArrayCopy(CtFor ctFor) { } } - private void checkMaxMin(CtIf ctIf) { - Set maxOperators = Set.of(BinaryOperatorKind.LT, BinaryOperatorKind.LE); - Set minOperators = Set.of(BinaryOperatorKind.GT, BinaryOperatorKind.GE); - - // ensure that in the if block there is only one assignment to a variable - // and the condition is a binary operator with <, <=, > or >= - List thenBlock = SpoonUtil.getEffectiveStatements(ctIf.getThenStatement()); - if (thenBlock.size() != 1 - || !(thenBlock.get(0) instanceof CtAssignment thenAssignment) - || !(thenAssignment.getAssigned() instanceof CtVariableWrite ctVariableWrite) - || !(ctIf.getCondition() instanceof CtBinaryOperator ctBinaryOperator) - || (!maxOperators.contains(ctBinaryOperator.getKind()) && !minOperators.contains(ctBinaryOperator.getKind()))) { - return; - } - - // keep track of the assigned variable (must be the same in the else block) - CtVariableReference assignedVariable = ctVariableWrite.getVariable(); - - // this is the value that is assigned if the then-block is not executed - // The variable is not changed without an else-Block (this would be equivalent to an else { variable = variable; }) - CtExpression elseValue = ctIf.getFactory().createVariableRead( - assignedVariable.clone(), - assignedVariable.getModifiers().contains(ModifierKind.STATIC) - ); - if (ctIf.getElseStatement() != null) { - List elseBlock = SpoonUtil.getEffectiveStatements(ctIf.getElseStatement()); - if (elseBlock.size() != 1 - || !(elseBlock.get(0) instanceof CtAssignment elseAssignment) - || !(elseAssignment.getAssigned() instanceof CtVariableAccess elseAccess) - // ensure that the else block assigns to the same variable - || !elseAccess.getVariable().equals(assignedVariable)) { - return; - } - - elseValue = elseAssignment.getAssignment(); - } - - CtBinaryOperator condition = ctBinaryOperator; - // ensure that the else value is on the left side of the condition - if (ctBinaryOperator.getRightHandOperand().equals(elseValue)) { - condition = SpoonUtil.swapCtBinaryOperator(condition); - } - - // if it is not on either side of the condition, return - if (!condition.getLeftHandOperand().equals(elseValue)) { - return; - } - - // max looks like this: - // if (variable < max) { - // variable = max; - // } - // - // or with an explicit else block: - // - // if (max > expr) { - // v = max; - // } else { - // v = expr; - // } - - if (maxOperators.contains(condition.getKind())) { - addLocalProblem( - ctIf, - new LocalizedMessage( - "common-reimplementation", - Map.of( - "suggestion", "%s = Math.max(%s, %s)".formatted( - ctVariableWrite.prettyprint(), - elseValue.prettyprint(), - condition.getRightHandOperand().prettyprint() - ) - ) - ), - ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN - ); - - return; - } - - // if (variable > min) { - // variable = min; - // } - - if (minOperators.contains(condition.getKind())) { - addLocalProblem( - ctIf, - new LocalizedMessage( - "common-reimplementation", - Map.of( - "suggestion", "%s = Math.min(%s, %s)".formatted( - ctVariableWrite.prettyprint(), - elseValue.prettyprint(), - condition.getRightHandOperand().prettyprint() - ) - ) - ), - ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN - ); - - return; - } - } - private void checkAddAll(CtForEach ctFor) { List statements = SpoonUtil.getEffectiveStatements(ctFor.getBody()); if (statements.size() != 1) { @@ -654,7 +550,6 @@ public void visitCtIf(CtIf ctIf) { return; } - checkMaxMin(ctIf); checkModulo(ctIf); super.visitCtIf(ctIf); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/MathReimplementation.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/MathReimplementation.java new file mode 100644 index 00000000..f373da2b --- /dev/null +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/MathReimplementation.java @@ -0,0 +1,241 @@ +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.dynamic.DynamicAnalysis; +import de.firemage.autograder.core.integrated.IntegratedCheck; +import de.firemage.autograder.core.integrated.SpoonUtil; +import de.firemage.autograder.core.integrated.StaticAnalysis; +import spoon.reflect.code.BinaryOperatorKind; +import spoon.reflect.code.CtAssignment; +import spoon.reflect.code.CtBinaryOperator; +import spoon.reflect.code.CtExpression; +import spoon.reflect.code.CtIf; +import spoon.reflect.code.CtInvocation; +import spoon.reflect.code.CtLiteral; +import spoon.reflect.code.CtStatement; +import spoon.reflect.code.CtTypeAccess; +import spoon.reflect.code.CtVariableAccess; +import spoon.reflect.code.CtVariableWrite; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.ModifierKind; +import spoon.reflect.reference.CtVariableReference; +import spoon.reflect.visitor.CtScanner; + +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +@ExecutableCheck(reportedProblems = { + ProblemType.COMMON_REIMPLEMENTATION_SQRT, + ProblemType.COMMON_REIMPLEMENTATION_HYPOT, + ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN +}) +public class MathReimplementation extends IntegratedCheck { + private static boolean isMathPow(CtInvocation ctInvocation) { + return ctInvocation.getTarget() instanceof CtTypeAccess ctTypeAccess + && SpoonUtil.isTypeEqualTo(ctTypeAccess.getAccessedType(), Math.class) + && SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), double.class, "pow", double.class, double.class); + } + + private static boolean isMathSqrt(CtInvocation ctInvocation) { + return ctInvocation.getTarget() instanceof CtTypeAccess ctTypeAccess + && SpoonUtil.isTypeEqualTo(ctTypeAccess.getAccessedType(), Math.class) + && SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), double.class, "sqrt", double.class); + } + + private static Optional> getPow2(CtExpression ctExpression) { + if (ctExpression instanceof CtBinaryOperator ctBinaryOperator + && ctBinaryOperator.getLeftHandOperand().equals(ctBinaryOperator.getRightHandOperand()) + && ctBinaryOperator.getKind() == BinaryOperatorKind.MUL) { + return Optional.of(ctBinaryOperator.getLeftHandOperand()); + } + + if (ctExpression instanceof CtInvocation ctInvocation + && isMathPow(ctInvocation) + && ctInvocation.getArguments().get(1) instanceof CtLiteral ctLiteral + && ctLiteral.getValue() instanceof Number value + && value.doubleValue() == 2.0) { + return Optional.of(ctInvocation.getArguments().get(0)); + } + + return Optional.empty(); + } + + private void checkHypot(CtExpression ctExpression) { + if (!(ctExpression instanceof CtInvocation ctInvocation) + || !isMathSqrt(ctInvocation) + || !(ctInvocation.getArguments().get(0) instanceof CtBinaryOperator ctBinaryOperator) + || ctBinaryOperator.getKind() != BinaryOperatorKind.PLUS) { + return; + } + + Optional> left = getPow2(ctBinaryOperator.getLeftHandOperand()); + Optional> right = getPow2(ctBinaryOperator.getRightHandOperand()); + + if (left.isPresent() && right.isPresent()) { + addLocalProblem( + ctExpression, + new LocalizedMessage( + "common-reimplementation", + Map.of("suggestion", "Math.hypot(%s, %s)".formatted(left.get().prettyprint(), right.get().prettyprint())) + ), + ProblemType.COMMON_REIMPLEMENTATION_HYPOT + ); + } + } + + private void checkSqrt(CtExpression ctExpression) { + if (!(ctExpression instanceof CtInvocation ctInvocation) || !isMathPow(ctInvocation)) { + return; + } + + if (SpoonUtil.resolveCtExpression(ctInvocation.getArguments().get(1)) instanceof CtLiteral ctLiteral + && ctLiteral.getValue() instanceof Double doubleValue + && doubleValue == 0.5) { + addLocalProblem( + ctExpression, + new LocalizedMessage( + "common-reimplementation", + Map.of("suggestion", "Math.sqrt(%s)".formatted(ctInvocation.getArguments().get(0).prettyprint())) + ), + ProblemType.COMMON_REIMPLEMENTATION_SQRT + ); + } + } + + + private void checkMaxMin(CtIf ctIf) { + Set maxOperators = Set.of(BinaryOperatorKind.LT, BinaryOperatorKind.LE); + Set minOperators = Set.of(BinaryOperatorKind.GT, BinaryOperatorKind.GE); + + // ensure that in the if block there is only one assignment to a variable + // and the condition is a binary operator with <, <=, > or >= + List thenBlock = SpoonUtil.getEffectiveStatements(ctIf.getThenStatement()); + if (thenBlock.size() != 1 + || !(thenBlock.get(0) instanceof CtAssignment thenAssignment) + || !(thenAssignment.getAssigned() instanceof CtVariableWrite ctVariableWrite) + || !(ctIf.getCondition() instanceof CtBinaryOperator ctBinaryOperator) + || (!maxOperators.contains(ctBinaryOperator.getKind()) && !minOperators.contains(ctBinaryOperator.getKind()))) { + return; + } + + // keep track of the assigned variable (must be the same in the else block) + CtVariableReference assignedVariable = ctVariableWrite.getVariable(); + + // this is the value that is assigned if the then-block is not executed + // The variable is not changed without an else-Block (this would be equivalent to an else { variable = variable; }) + CtExpression elseValue = ctIf.getFactory().createVariableRead( + assignedVariable.clone(), + assignedVariable.getModifiers().contains(ModifierKind.STATIC) + ); + if (ctIf.getElseStatement() != null) { + List elseBlock = SpoonUtil.getEffectiveStatements(ctIf.getElseStatement()); + if (elseBlock.size() != 1 + || !(elseBlock.get(0) instanceof CtAssignment elseAssignment) + || !(elseAssignment.getAssigned() instanceof CtVariableAccess elseAccess) + // ensure that the else block assigns to the same variable + || !elseAccess.getVariable().equals(assignedVariable)) { + return; + } + + elseValue = elseAssignment.getAssignment(); + } + + CtBinaryOperator condition = ctBinaryOperator; + // ensure that the else value is on the left side of the condition + if (ctBinaryOperator.getRightHandOperand().equals(elseValue)) { + condition = SpoonUtil.swapCtBinaryOperator(condition); + } + + // if it is not on either side of the condition, return + if (!condition.getLeftHandOperand().equals(elseValue)) { + return; + } + + // max looks like this: + // if (variable < max) { + // variable = max; + // } + // + // or with an explicit else block: + // + // if (max > expr) { + // v = max; + // } else { + // v = expr; + // } + + if (maxOperators.contains(condition.getKind())) { + addLocalProblem( + ctIf, + new LocalizedMessage( + "common-reimplementation", + Map.of( + "suggestion", "%s = Math.max(%s, %s)".formatted( + ctVariableWrite.prettyprint(), + elseValue.prettyprint(), + condition.getRightHandOperand().prettyprint() + ) + ) + ), + ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN + ); + + return; + } + + // if (variable > min) { + // variable = min; + // } + + if (minOperators.contains(condition.getKind())) { + addLocalProblem( + ctIf, + new LocalizedMessage( + "common-reimplementation", + Map.of( + "suggestion", "%s = Math.min(%s, %s)".formatted( + ctVariableWrite.prettyprint(), + elseValue.prettyprint(), + condition.getRightHandOperand().prettyprint() + ) + ) + ), + ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN + ); + + return; + } + } + + @Override + protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) { + staticAnalysis.getModel().getRootPackage().accept(new CtScanner() { + @Override + protected void enter(CtElement ctElement) { + if (ctElement instanceof CtExpression ctExpression + && !ctExpression.isImplicit() + && ctExpression.getPosition().isValidPosition()) { + checkSqrt(ctExpression); + checkHypot(ctExpression); + } + + super.enter(ctElement); + } + + @Override + public void visitCtIf(CtIf ctIf) { + if (ctIf.isImplicit() || !ctIf.getPosition().isValidPosition() || ctIf.getThenStatement() == null) { + super.visitCtIf(ctIf); + return; + } + + checkMaxMin(ctIf); + super.visitCtIf(ctIf); + } + }); + } +} diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCommonReimplementation.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCommonReimplementation.java index ef91ffcc..50ab96a8 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCommonReimplementation.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCommonReimplementation.java @@ -181,173 +181,6 @@ public static int[][] copyMatrix(int[][] matrix) { problems.assertExhausted(); } - @Test - void testMax() throws LinterException, IOException { - ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( - JavaVersion.JAVA_17, - "Main", - """ - public class Main { - public static void foo(int a, int b) { - int left = a; - int right = b; - - if (left < right) { - left = right; - } - - if (left <= right) { - left = right; - } - - if (right > left) { - left = right; - } - - if (right >= left) { - left = right; - } - - if (0 >= left) { - left = 0; - } - - if (1 > left) { - left = 1; - } - } - } - """ - ), List.of(ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN)); - - - List expectedProblems = List.of( - "left = Math.max(left, right)", - "left = Math.max(left, right)", - "left = Math.max(left, right)", - "left = Math.max(left, right)", - "left = Math.max(left, 0)", - "left = Math.max(left, 1)" - ); - - for (String expectedProblem : expectedProblems) { - assertEqualsReimplementation(problems.next(), expectedProblem); - } - - problems.assertExhausted(); - } - - @Test - void testMin() throws LinterException, IOException { - ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( - JavaVersion.JAVA_17, - "Main", - """ - public class Main { - public static void foo(int a, int b) { - int left = a; - int right = b; - - if (right < left) { - left = right; - } - - if (right <= left) { - left = right; - } - - if (left > right) { - left = right; - } - - if (left >= right) { - left = right; - } - - if (left >= 0) { - left = 0; - } - - if (left > 1) { - left = 1; - } - } - } - """ - ), List.of(ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN)); - - - List expectedProblems = List.of( - "left = Math.min(left, right)", - "left = Math.min(left, right)", - "left = Math.min(left, right)", - "left = Math.min(left, right)", - "left = Math.min(left, 0)", - "left = Math.min(left, 1)" - ); - - for (String expectedProblem : expectedProblems) { - assertEqualsReimplementation(problems.next(), expectedProblem); - } - - problems.assertExhausted(); - } - - - @Test - void testMinMaxWithElse() throws LinterException, IOException { - ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( - JavaVersion.JAVA_17, - "Main", - """ - public class Main { - public static void foo(int a, int b) { - int result = 0; - - if (a < b) { - result = a; - } else { - result = b; - } - - if (a <= b) { - result = a; - } else { - result = b; - } - - if (a < b) { - result = b; - } else { - result = a; - } - - if (a <= b) { - result = b; - } else { - result = a; - } - - } - } - """ - ), List.of(ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN)); - - List expectedProblems = List.of( - "result = Math.min(b, a)", - "result = Math.min(b, a)", - "result = Math.max(a, b)", - "result = Math.max(a, b)" - ); - - for (String expectedProblem : expectedProblems) { - assertEqualsReimplementation(problems.next(), expectedProblem); - } - - problems.assertExhausted(); - } - - @Test void testAddAllArray() throws LinterException, IOException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestMathReimplementation.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestMathReimplementation.java new file mode 100644 index 00000000..68f9c57b --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestMathReimplementation.java @@ -0,0 +1,257 @@ +package de.firemage.autograder.core.check.api; + +import de.firemage.autograder.core.LinterException; +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.Problem; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.AbstractCheckTest; +import de.firemage.autograder.core.compiler.JavaVersion; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestMathReimplementation extends AbstractCheckTest { + private static final String LOCALIZED_MESSAGE_KEY = "common-reimplementation"; + private static final List PROBLEM_TYPES = List.of( + ProblemType.COMMON_REIMPLEMENTATION_SQRT, + ProblemType.COMMON_REIMPLEMENTATION_HYPOT, + ProblemType.COMMON_REIMPLEMENTATION_MAX_MIN + ); + + private void assertEqualsReimplementation(Problem problem, String suggestion) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + LOCALIZED_MESSAGE_KEY, + Map.of( + "suggestion", suggestion + ) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testSqrt() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private int sqrt(int x) { + return (int) Math.pow(x, 0.5); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsReimplementation(problems.next(), "Math.sqrt(x)"); + + problems.assertExhausted(); + } + + @Test + void testHypot() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + private double exampleA(int x, int y) { + return Math.sqrt(x * x + y * y); + } + + private double exampleB(int x, int y) { + return Math.sqrt(Math.pow(x, 2) + y * y); + } + + private double exampleC(int x, int y) { + return Math.sqrt(Math.pow(x, 2) + Math.pow(y, 2)); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsReimplementation(problems.next(), "Math.hypot(x, y)"); + assertEqualsReimplementation(problems.next(), "Math.hypot(x, y)"); + assertEqualsReimplementation(problems.next(), "Math.hypot(x, y)"); + + problems.assertExhausted(); + } + + + @Test + void testMax() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void foo(int a, int b) { + int left = a; + int right = b; + + if (left < right) { + left = right; + } + + if (left <= right) { + left = right; + } + + if (right > left) { + left = right; + } + + if (right >= left) { + left = right; + } + + if (0 >= left) { + left = 0; + } + + if (1 > left) { + left = 1; + } + } + } + """ + ), PROBLEM_TYPES); + + + List expectedProblems = List.of( + "left = Math.max(left, right)", + "left = Math.max(left, right)", + "left = Math.max(left, right)", + "left = Math.max(left, right)", + "left = Math.max(left, 0)", + "left = Math.max(left, 1)" + ); + + for (String expectedProblem : expectedProblems) { + assertEqualsReimplementation(problems.next(), expectedProblem); + } + + problems.assertExhausted(); + } + + @Test + void testMin() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void foo(int a, int b) { + int left = a; + int right = b; + + if (right < left) { + left = right; + } + + if (right <= left) { + left = right; + } + + if (left > right) { + left = right; + } + + if (left >= right) { + left = right; + } + + if (left >= 0) { + left = 0; + } + + if (left > 1) { + left = 1; + } + } + } + """ + ), PROBLEM_TYPES); + + + List expectedProblems = List.of( + "left = Math.min(left, right)", + "left = Math.min(left, right)", + "left = Math.min(left, right)", + "left = Math.min(left, right)", + "left = Math.min(left, 0)", + "left = Math.min(left, 1)" + ); + + for (String expectedProblem : expectedProblems) { + assertEqualsReimplementation(problems.next(), expectedProblem); + } + + problems.assertExhausted(); + } + + + @Test + void testMinMaxWithElse() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Main", + """ + public class Main { + public static void foo(int a, int b) { + int result = 0; + + if (a < b) { + result = a; + } else { + result = b; + } + + if (a <= b) { + result = a; + } else { + result = b; + } + + if (a < b) { + result = b; + } else { + result = a; + } + + if (a <= b) { + result = b; + } else { + result = a; + } + + } + } + """ + ), PROBLEM_TYPES); + + List expectedProblems = List.of( + "result = Math.min(b, a)", + "result = Math.min(b, a)", + "result = Math.max(a, b)", + "result = Math.max(a, b)" + ); + + for (String expectedProblem : expectedProblems) { + assertEqualsReimplementation(problems.next(), expectedProblem); + } + + problems.assertExhausted(); + } +} diff --git a/sample_config.yaml b/sample_config.yaml index b4186886..a1073087 100644 --- a/sample_config.yaml +++ b/sample_config.yaml @@ -145,3 +145,5 @@ - AVOID_STRING_CONCAT - UNNECESSARY_COMMENT - OBJECT_DATATYPE +- COMMON_REIMPLEMENTATION_SQRT +- COMMON_REIMPLEMENTATION_HYPOT