From a5d93aa27f76b9b45d4c50448867630695fceda4 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Fri, 15 Nov 2024 13:21:43 +0100 Subject: [PATCH 01/12] update messages --- .../complexity/RedundantBooleanEqual.java | 2 +- .../complexity/UseOperatorAssignment.java | 8 +- .../core/check/debug/AssertCheck.java | 2 +- .../check/general/StringCompareCheck.java | 7 +- .../core/check/general/UseGuardClauses.java | 111 ---------------- .../src/main/resources/strings.de.ftl | 121 +++++++++--------- .../src/main/resources/strings.en.ftl | 108 ++++++++-------- .../UseGuardClauses/code/Test.java | 89 ------------- .../check_tests/UseGuardClauses/config.txt | 8 -- 9 files changed, 130 insertions(+), 326 deletions(-) delete mode 100644 autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseGuardClauses.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseGuardClauses/code/Test.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseGuardClauses/config.txt diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantBooleanEqual.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantBooleanEqual.java index ac09ffdf..ae53acea 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantBooleanEqual.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantBooleanEqual.java @@ -35,7 +35,7 @@ private void reportProblem(CtBinaryOperator ctBinaryOperator, boolean literal addLocalProblem( ctBinaryOperator, new LocalizedMessage( - "redundant-boolean-equal", + "common-reimplementation", Map.of( "suggestion", suggestion ) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/UseOperatorAssignment.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/UseOperatorAssignment.java index 69958075..ec5f0a88 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/UseOperatorAssignment.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/UseOperatorAssignment.java @@ -5,6 +5,7 @@ import de.firemage.autograder.core.check.ExecutableCheck; import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.StaticAnalysis; +import de.firemage.autograder.core.integrated.TypeUtil; import de.firemage.autograder.core.integrated.evaluator.OperatorHelper; import spoon.processing.AbstractProcessor; import spoon.reflect.code.BinaryOperatorKind; @@ -39,8 +40,7 @@ public class UseOperatorAssignment extends IntegratedCheck { private boolean isCommutativeType(CtTypedElement ctTypedElement) { return ctTypedElement.getType() == null || NON_COMMUTATIVE_TYPES.stream() - .map(ty -> ctTypedElement.getFactory().Type().createReference(ty)) - .noneMatch(ty -> ty.equals(ctTypedElement.getType())); + .noneMatch(ty -> TypeUtil.isTypeEqualTo(ctTypedElement.getType(), ty)); } private boolean isCommutative(BinaryOperatorKind binaryOperatorKind) { @@ -106,8 +106,8 @@ public void process(CtAssignment assignment) { addLocalProblem( assignment, new LocalizedMessage( - "use-operator-assignment-exp", - Map.of("simplified", simplifiedExpr) + "common-reimplementation", + Map.of("suggestion", simplifiedExpr) ), ProblemType.USE_OPERATOR_ASSIGNMENT ); diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/debug/AssertCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/debug/AssertCheck.java index cceec3e1..84ae76af 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/debug/AssertCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/debug/AssertCheck.java @@ -15,7 +15,7 @@ protected void check(StaticAnalysis staticAnalysis) { staticAnalysis.processWith(new AbstractProcessor>() { @Override public void process(CtAssert element) { - addLocalProblem(element, new LocalizedMessage("assert-used-exp"), ProblemType.ASSERT); + addLocalProblem(element, new LocalizedMessage("assert-used"), ProblemType.ASSERT); } }); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/StringCompareCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/StringCompareCheck.java index 56f1ee98..ba5f1270 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/StringCompareCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/StringCompareCheck.java @@ -35,8 +35,11 @@ public void process(CtBinaryOperator operator) { if (isStringComparison(lhs, rhs)) { addLocalProblem(operator, new LocalizedMessage( - "string-cmp-exp", - Map.of("lhs", lhs, "rhs", rhs) + "suggest-replacement", + Map.of( + "original", operator.toString(), + "replacement", "%s.equals(%s)".formatted(lhs, rhs) + ) ), ProblemType.STRING_COMPARE_BY_REFERENCE ); diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseGuardClauses.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseGuardClauses.java deleted file mode 100644 index 10b737d5..00000000 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseGuardClauses.java +++ /dev/null @@ -1,111 +0,0 @@ -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.FactoryUtil; -import de.firemage.autograder.core.integrated.IntegratedCheck; -import de.firemage.autograder.core.integrated.StatementUtil; -import de.firemage.autograder.core.integrated.StaticAnalysis; -import de.firemage.autograder.core.integrated.effects.Effect; -import de.firemage.autograder.core.integrated.effects.TerminalEffect; -import spoon.processing.AbstractProcessor; -import spoon.reflect.code.BinaryOperatorKind; -import spoon.reflect.code.CtExpression; -import spoon.reflect.code.CtIf; -import spoon.reflect.code.CtStatement; -import spoon.reflect.code.UnaryOperatorKind; - -import java.util.List; -import java.util.Optional; - -@ExecutableCheck(reportedProblems = { ProblemType.USE_GUARD_CLAUSES }) -public class UseGuardClauses extends IntegratedCheck { - private void reportProblem(CtStatement ctStatement, CtExpression condition) { - addLocalProblem( - ctStatement, - new LocalizedMessage("use-guard-clauses"), - ProblemType.USE_GUARD_CLAUSES - ); - } - - private boolean isTerminal(CtStatement ctStatement) { - List ctStatements = StatementUtil.getEffectiveStatements(ctStatement); - - if (ctStatements.isEmpty()) return false; - - - Optional optionalEffect = StatementUtil.tryMakeEffect(ctStatements.get(ctStatements.size() - 1)); - - return optionalEffect.map(TerminalEffect.class::isInstance).orElse(false); - } - - private void checkCtIf(CtIf ctIf, CtExpression condition) { - // if the condition != null, then the ctIf is an else if - if (condition != null) { - CtExpression ifCondition = FactoryUtil.createBinaryOperator( - condition, - ctIf.getCondition(), - BinaryOperatorKind.AND - ); - - if (this.isTerminal(ctIf.getThenStatement())) { - reportProblem(ctIf.getThenStatement(), ifCondition); - } - } - - // the condition to reach the else statement - CtExpression elseCondition = FactoryUtil.createUnaryOperator( - UnaryOperatorKind.NOT, - ctIf.getCondition() - ); - - // check the else statement - CtStatement ctStatement = ctIf.getElseStatement(); - // if there is no else, return - if (ctStatement == null) return; - - List ctStatements = StatementUtil.getEffectiveStatements(ctStatement); - - if (condition != null) { - elseCondition = FactoryUtil.createBinaryOperator( - condition, - elseCondition, - BinaryOperatorKind.AND - ); - } - - if (ctStatements.size() == 1 && ctStatements.get(0) instanceof CtIf ctElseIf) { - CtExpression elseIfCondition = FactoryUtil.createBinaryOperator( - elseCondition, - ctElseIf.getCondition(), - BinaryOperatorKind.AND - ); - - checkCtIf(ctElseIf, elseIfCondition); - // check the else statement - } else if (this.isTerminal(ctStatement)) { - reportProblem(ctStatement, elseCondition); - } - } - - @Override - protected void check(StaticAnalysis staticAnalysis) { - staticAnalysis.processWith(new AbstractProcessor() { - @Override - public void process(CtIf ctIf) { - if (ctIf.isImplicit() || !ctIf.getPosition().isValidPosition()) return; - - CtIf parentIf = ctIf.getParent(CtIf.class); - if (parentIf != null && parentIf.getElseStatement() != null) { - List ctStatements = StatementUtil.getEffectiveStatements(parentIf.getElseStatement()); - if (ctStatements.size() == 1 && ctStatements.get(0).equals(ctIf)) { - return; - } - } - - checkCtIf(ctIf, null); - } - }); - } -} diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index 838667a3..1727da12 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -24,18 +24,18 @@ common-reimplementation = Der Code kann vereinfacht werden zu '{$suggestion}'. use-string-formatted = '{$formatted}' ist schöner zu lesen. use-format-string = '{$formatted}' ist schöner zu lesen. -optional-tri-state = Statt einem Optional boolean, sollte man ein enum verwenden. +optional-tri-state = Statt einem Optional boolean, sollte ein enum verwendet werden. equals-hashcode-comparable-contract = Es müssen immer equals und hashCode zusammen überschrieben werden. Genauso muss wenn Comparable implementiert wird equals und hashCode überschrieben werden. -use-enum-collection = Bei Maps wenn ein Enum als Key ist und bei Sets als Wert, sollte man EnumMap/EnumSet verwenden. +use-enum-collection = Verwende bei Maps mit enum als Key oder bei sets als Wert EnumMap/EnumSet. compare-to-zero = Das Ergebnis von compareTo oder compare sollte nur mit 0 verglichen werden. - Es ist eine Implementierungsdetail, ob ein gegebener Typ streng die Werte + Es ist ein Implementierungsdetail, ob ein gegebener Typ die Werte '-1, 0, +1' oder andere zurückgibt. equals-using-hashcode = Equals nur mit hashCode zu implementieren ist fehleranfällig. - Hashes kollidieren häufig, was zu falschen Ergebnissen in equals führt. -equals-unsafe-cast = Im Javadoc von equals steht, dass es false für inkompatible Typen zurückgeben soll. + Hashes können kollidieren, wodurch es zu falschen Ergebnissen in equals kommen könnte. +equals-unsafe-cast = Im JavaDoc von equals steht, dass es false für inkompatible Typen zurückgeben soll. Diese Implementierung kann eine ClassCastException werfen. equals-incompatible-type = Ein Vergleich zwischen Objekten mit inkompatiblen Typen gibt immer false zurück inconsistent-hashcode = Das hashCode-Verhalten ist inkonsistent. Es wird in equals nicht verglichen, aber in hashCode verwendet. @@ -47,7 +47,7 @@ equals-reference = == sollte in equals verwendet werden, um Gleichheit zu sich s array-as-key-of-set-or-map = Arrays überschreiben weder equals noch hashCode. Dementsprechend werden Vergleiche basierend auf der Referenz gemacht und nicht auf dem Inhalt. Verwende stattdessen eine Liste. -implement-comparable = Der Typ '{$name}' sollte `Comparable<{$name}>` implementieren, dann kann man sich den `Comparator` sparen. +implement-comparable = Der Typ '{$name}' sollte 'Comparable<{$name}>' implementieren, dann kann man sich den 'Comparator' sparen. # Comment commented-out-code = Dieser auskommentierte Code sollte entfernt werden @@ -58,24 +58,24 @@ comment-language-exp-german = Der Code enthält deutsche und englische Kommentar 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 +javadoc-method-exp-param-missing = Der Parameter '{$param}' wird im JavaDoc nicht dokumentiert. +javadoc-method-exp-param-unknown = Der JavaDoc dokumentiert den nicht existierenden Parameter '{$param}'. javadoc-unexpected-tag = Der JavaDoc-Kommentar sollte keinen '@{$tag}'-Tag haben. -javadoc-type-exp-invalid-author = Im @author-Tag sollte dein u-Kürzel stehen: {$authors} +javadoc-type-exp-invalid-author = Im @author-Tag sollte ein u-Kürzel stehen: {$authors} -javadoc-stub-exp-desc = Die Beschreibung des Javadoc-Kommentars ist leer +javadoc-stub-exp-desc = Die Beschreibung des JavaDoc-Kommentars ist leer javadoc-stub-exp-param = Nichtssagende Beschreibung für den Parameter '{$param}' javadoc-stub-exp-return = Nichtssagende Beschreibung für den Rückgabewert -javadoc-stub-exp-throws = Nichtssagende Beschreibung für die Exception {$exp} +javadoc-stub-exp-throws = Nichtssagende Beschreibung für die Exception '{$exp}' -javadoc-undocumented-throws = Die Exception {$exp} wird geworfen, aber nicht im Javadoc-Kommentar erwähnt. +javadoc-undocumented-throws = Die Exception '{$exp}' wird geworfen, aber nicht im JavaDoc-Kommentar erwähnt. todo-comment = TODOs sollten nicht in der finalen Abgabe vorhanden sein. # Complexity -use-diamond-operator = Du kannst die Typen in '< A, B, ... >' entfernen und stattdessen '<>' verwenden, siehe https://docs.oracle.com/javase/tutorial/java/generics/genTypeInference.html und https://stackoverflow.com/a/16352848/7766117 +use-diamond-operator = Die Typen in '< A, B, ... >' können entfernt werden, schreibe stattdessen '<>'. extends-object = Explizit von Object zu erben ist unnötig @@ -87,8 +87,6 @@ redundant-modifier = Die folgenden Modifier sind redundant und sollten deswegen redundant-return-exp = Unnötiges return -redundant-boolean-equal = Es ist unnötig explizit zu überprüfen, ob eine Bedingung gleich true oder false ist. Schreibe stattdessen '{$suggestion}'. - self-assignment-exp = Nutzlose Zuweisung von '{$rhs}' zu '{$lhs}' too-many-exceptions = Das Projekt definiert {$count} Exceptions. Das sind zu viele. @@ -97,30 +95,27 @@ redundant-variable = Die Variable '{$name}' ist unnötig, verwende stattdessen d unused-import = Der Import '{$import}' wird nicht verwendet und sollte deswegen entfernt werden. -use-operator-assignment-exp = Zuweisung kann zu '{$simplified}' vereinfacht werden - complex-regex = Nichttriviale Regex brauchen einen erklärenden Kommentar (Score ist {$score}, maximal erlaubt ist {$max}) -redundant-catch = Eine exception sollte nicht gefangen werden, um sie dann direkt wieder zu werfen. +redundant-catch = Eine Exception sollte nicht gefangen werden, um sie dann direkt wieder zu werfen. redundant-uninitialized-variable = Die Variable '{$variable}' wurde deklariert, aber der Wert '{$value}' wird nicht direkt zugewiesen. Schreibe stattdessen '{$suggestion}'. -multiple-inline-statements = Es sollten nicht mehrere Aussagen in einer Zeile stehen. Also keine Deklarationen von mehreren Variablen oder Zuweisungen in einer Zeile. +multiple-inline-statements = Es sollten nicht mehrere Aussagen in einer Zeile stehen. -multi-threading = Multithreading ist nicht Teil der Vorlesung. Code der nur einem Thread ausgeführt wird, thread-safe zu schreiben, macht den Code unnötig komplex. +multi-threading = Multithreading wird nicht verwendet. Code der nur auf einem Thread ausgeführt wird, thread-safe zu schreiben, macht den Code unnötig komplex. -try-catch-complexity = Die Komplexität von try-catch-Blöcken sollte möglichst gering gehalten werden. Es sollten weniger als {$max} Statements vorhanden sein. Versuche den Code in mehrere Methoden aufzuteilen bzw. nicht dringend benötigte Statements aus dem try-Block zu entfernen. +try-catch-complexity = Die Komplexität von try-catch-Blöcken sollte möglichst gering gehalten werden. + Es sollten weniger als {$max} Statements vorhanden sein. Versuche den Code in + mehrere Methoden aufzuteilen bzw. nicht dringend benötigte Statements aus dem + try-Block zu entfernen. redundant-else = Die 'else' ist unnötig. Schreibe '{$expected}' statt '{$given}'. redundant-assignment = Der Variable '{$variable}' wird hier ein Wert zugewiesen, der dann nie verwendet wird. Deswegen kann die Zuweisung entfernt werden. # Debug -assert-used-exp = Assertions lassen das gesamte Programm abstürzen, wenn sie false sind. - Außerdem können sie deaktiviert werden, weswegen man sich nicht darauf verlassen kann, - dass bestimmte Bedingungen zutreffen. Sie sind super für Testzwecke, sollten aber nicht - Teil der finalen Lösung sein. Wenn du eine Invariante dokumentieren willst, verwende - einen Kommentar. +assert-used = Assertions sollten nicht in der finalen Abgabe vorhanden sein. Verwende stattdessen Exceptions. print-stack-trace = Stack Traces sollten in der Abgabe nicht ausgegeben werden @@ -130,18 +125,18 @@ custom-exception-inheritance-runtime = Die selbstdefinierte Exception '{$name}' empty-catch-block = Leerer catch-Block -exception-controlflow-caught = {$exception} wird geworfen und im umgebenden Block sofort wieder gefangen -exception-controlflow-should-not-be-caught = {$exception} sollte man niemals fangen +exception-controlflow-caught = '{$exception}' wird geworfen und im umgebenden Block sofort wieder gefangen +exception-controlflow-should-not-be-caught = '{$exception}' sollte man niemals fangen -runtime-exception-caught = RuntimeExceptions '{$exception}' sollten nicht gefangen werden +runtime-exception-caught = RuntimeException '{$exception}' sollten nicht gefangen werden exception-message = Eine Exception sollte immer eine Nachricht dabei haben, die erklärt was der Fehler ist und im Idealfall wie es zu dem Fehler kam. -number-format-exception-ignored = NumberFormatException sollte gefangen werden und entweder behandelt oder durch eine eigene Exception ersetzt werden. +number-format-exception-ignored = 'NumberFormatException' sollte gefangen werden und entweder behandelt oder durch eine eigene Exception ersetzt werden. # General -compare-objects-exp = Implementiere eine equals-Methode für den Typ {$type} und verwende sie zum Vergleichen +compare-objects-exp = Der Typ '{$type}' sollte 'equals' implementieren und nicht über 'toString' verglichen werden. variable-should-be = Die Variable '{$variable}' sollte '{$suggestion}' sein. @@ -149,29 +144,25 @@ reassigned-parameter = Dem Parameter '{$name}' sollte kein neuer Wert zugewiesen double-brace-init = Die obskure 'Double Brace'-Syntax sollte vermieden werden -missing-override = '{$name}' sollte eine '@Override'-Annotation haben, siehe https://stackoverflow.com/a/94411/7766117. +missing-override = '{$name}' sollte eine '@Override'-Annotation haben. -system-specific-linebreak = Systemabhängiger Zeilenumbruch (\n) benutzt. Besser ist System.lineSeparator() oder (falls es sich um einen format-String handelt) '%n'. +system-specific-linebreak = Systemabhängiger Zeilenumbruch (\n) benutzt. Besser ist 'System.lineSeparator()' oder (falls es sich um einen format-String handelt) '%n'. field-should-be-final = Das Attribut '{$name}' sollte final sein. -string-cmp-exp = Strings sollten nicht per Referenz, sonder mit der 'equals'-Methode verglichen werden: '{$lhs}.equals({$rhs})' statt '{$lhs} == {$rhs}' +do-not-use-raw-types-exp = Generische Typen sollten immer mit Typparameter angegeben werden und nie als Raw Type. -do-not-use-raw-types-exp = Generische Typen sollten immer mit Typparameter angegeben werden und nie als Raw Type, siehe https://stackoverflow.com/a/2770692/7766117 +avoid-labels = Labels sollten vermieden werden. -avoid-labels = Labels sollten vermieden werden. Siehe https://stackoverflow.com/a/33689582/7766117. +avoid-shadowing = Die Variable '{$name}' verdeckt ein Attribut mit dem selben Namen. Abgesehen vom Konstruktor und settern, sollte man das vermeiden. -avoid-shadowing = Die Variable '{$name}' verdeckt ein Attribut mit dem selben Namen. Abgesehen vom Konstruktor, sollte man das vermeiden. - -suppress-warnings = @SuppressWarnings unterdrückt Warnungen des Compilers oder von Checkstyle, anstatt das unterliegende Problem zu beheben. +suppress-warnings = '@SuppressWarnings' unterdrückt Warnungen des Compilers oder Checkstyle, anstatt das unterliegende Problem zu beheben. 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 = 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 +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}'. import-types = Statt den Pfad zum Typ anzugeben, sollte '{$type}' importiert werden. Datentypen aus dem selben Paket oder 'java.lang' müssen nicht explizit importiert werden. @@ -186,7 +177,7 @@ binary-operator-on-boolean = Statt '|' und '&' sollte man '||' und '&&' verwende object-datatype = Statt dem Datentyp 'Object', sollte die Variable '{$variable}' einen konkreten oder generischen Datentyp haben. -magic-string = Der String '{$value}' sollte in eine Konstante ausgelagert werden. Siehe Wiki Artikel zu Magic Strings. +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} @@ -196,7 +187,7 @@ loop-should-be-while = Diese Schleife sollte eine while Schleife sein: {$suggest # Naming bool-getter-name = Für boolean getter bietet es sich an ein Verb als Präfix zu verwenden. Zum Beispiel '{$newName}' statt '{$oldName}'. -constants-name-exp = Der Name '{$name}' ist nicht aussagekräftig gegeben den Wert '{$value}' +constants-name-exp = Der Name '{$name}' ist nicht aussagekräftig gegeben dem Wert '{$value}' constants-name-exp-value = Der Wert '{$value}' der Konstante '{$name}' sollte nicht im Namen vorkommen linguistic-naming-boolean = Der Name von '{$name}' deutet an, dass es vom Typ boolean ist oder diesen zurückgibt, stattdessen ist der Typ '{$type}'. @@ -207,16 +198,16 @@ linguistic-naming-setter = Der Name von '{$name}' deutet an, dass es ein setter variable-name-single-letter = Der Bezeichner '{$name}' ist nicht aussagekräftig variable-is-abbreviation = Unnötige Abkürzung '{$name}' variable-name-type-in-name = Der Bezeichner '{$name}' sollte nicht den Typ im Namen haben -similar-identifier = Der Bezeichner '{$left}' ist sehr ähnlich zu '{$right}'. Das kann zu Verwechslungen und Tippfehlern führen, weswegen man diesen umbenennen sollte. +similar-identifier = Der Bezeichner '{$left}' ist sehr ähnlich zu '{$right}'. type-has-descriptive-name-pre-suffix = Der Name enthält unnötige Präfixe oder Suffixe -type-has-descriptive-name-exception = Eine Klasse die von Exception erbt, sollte 'Exception' am Ende ihres Namens haben +type-has-descriptive-name-exception = Eine Klasse die von 'Exception' erbt, sollte 'Exception' am Ende ihres Namens haben package-naming-convention = Der Name eines Pakets sollte am besten ein Wort sein und alle Buchstaben sollten nach Konvention klein sein. Außer dem Zeichen '_' sollten zudem keine Sonderzeichen auftreten. An folgenden Stellen wird das nicht eingehalten: '{$positions}' -variable-redundant-number-suffix = Der Bezeichner '{$name}' enthält eine redundante Zahl am Ende. +variable-redundant-number-suffix = Der Bezeichner '{$name}' enthält eine unnötige Zahl am Ende. # OOP concrete-collection = Der Typ '{$type}' sollte durch eine Schnittstelle wie zum Beispiel 'List' oder 'Set' ersetzt werden. @@ -225,7 +216,7 @@ leaked-collection-return = Die Methode '{$method}' gibt eine Referenz zu dem Fel leaked-collection-constructor = Der Konstruktor '{$signature}' weißt dem Feld '{$field}' eine Referenz zu, dadurch ist es möglich das Feld von außerhalb zu verändern. Weise stattdessen eine Kopie dem Feld zu. leaked-collection-assign = Die Methode '{$method}' weißt dem Feld '{$field}' eine Referenz zu, dadurch ist es möglich das Feld von außerhalb zu verändern. Weise stattdessen eine Kopie dem Feld zu. -method-should-be-abstract = {$type}::{$method} sollte abstrakt sein, anstatt eine Platzhalter-Implementierung anzugeben +method-should-be-abstract = '{$type}::{$method}' sollte abstrakt sein, anstatt eine Platzhalter-Implementierung anzugeben. method-should-be-static = Die Methode '{$name}' sollte statisch sein, da sie auf keine Instanzattribute oder Methoden zugreift. @@ -234,38 +225,48 @@ utility-exp-constructor = Utility-Klassen müssen genau einen privaten und param static-field-should-be-instance = Das statische Attribut '{$name}' sollte ein Instanzattribut sein. -constants-class-exp = Konstanten sollten in der Klasse gespeichert werden in der sie auch verwendet werden und nicht in einer separaten Klasse. Siehe https://stackoverflow.com/a/15056462/7766117 +constants-class-exp = Konstanten sollten in der Klasse gespeichert werden in der sie auch verwendet werden und + nicht in einer separaten Klasse. empty-interface-exp = Interfaces sollten nicht leer sein. ui-input-separation = Eingaben sollten nicht im Programm verteilt sein. Wurde auch verwendet in {$first}. ui-output-separation = Ausgaben sollten nicht im Programm verteilt sein. Wurde auch verwendet in {$first}. -do-not-use-system-exit = System.exit() darf nicht verwendet werden. Strukturiere deinen Code so, dass er sich natürlich beendet. +do-not-use-system-exit = 'System.exit()' darf nicht verwendet werden. Strukturiere deinen Code so, dass er sich natürlich beendet. avoid-inner-classes = Jede Klasse sollte in einer eigenen Datei sein. Innere-Klassen sollten vermieden werden. -mutable-enum = Enums sollten nicht veränderbar sein. Siehe https://stackoverflow.com/a/41199773/7766117 +mutable-enum = Enums sollten nicht veränderbar sein. should-be-enum-attribute = Die Werte vom switch sollten Attribute des enums sein. Alternativ sollte man eine Map verwenden. -closed-set-of-values-switch = Der Switch hat nur endlich viele cases. Dabei handelt es sich um eine abgeschlossene Menge, die als enum modelliert werden sollte. Die Werte sind: '{$values}'. -closed-set-of-values-list = Die Auflistung hat nur endlich viele Werte und sollte deswegen als enum modelliert werden. Die Werte sind: '{$values}'. -closed-set-of-values-method = Die Methode gibt nur die Konstanten Werte '{$values}' zurück. Dabei handelt es sich um endlich viele, weswegen man das als enum modellieren sollte. +closed-set-of-values-switch = Der Switch hat nur endlich viele cases. Dabei handelt es sich um eine abgeschlossene Menge, + die als enum modelliert werden sollte. Die Werte sind: '{$values}'. +closed-set-of-values-list = Die Auflistung hat nur endlich viele Werte und sollte deswegen als enum modelliert werden. + Die Werte sind: '{$values}'. +closed-set-of-values-method = Die Methode gibt nur die Konstanten Werte '{$values}' zurück. Dabei handelt es sich um + endlich viele, weswegen man das als enum modellieren sollte. -do-not-use-instanceof = instanceof sollte nicht verwendet werden. Siehe Ilias Wiki. -do-not-use-instanceof-emulation = instanceof sollte nicht verwendet werden und auch nicht durch getClass oder ClassCastException emuliert werden. Siehe Ilias Wiki. +do-not-use-instanceof = 'instanceof' sollte nicht verwendet werden. +do-not-use-instanceof-emulation = 'instanceof' sollte nicht verwendet werden und auch nicht durch 'getClass' oder + 'ClassCastException' emuliert werden. abstract-class-without-abstract-method = Abstrakte Klassen sollten mindestens eine abstrakte Methode haben. -composition-over-inheritance = Die Oberklasse hat nur Felder. Statt Vererbung sollte hier Komposition verwendet werden. Zum Beispiel ein interface mit dem getter: '{$suggestion}'. -should-be-interface = Die Klasse hat nur Methoden und keine Felder. Statt Vererbung sollte hier ein Interface mit Standard-Implementierungen verwendet werden. +composition-over-inheritance = Die Oberklasse hat nur Felder. Statt Vererbung sollte hier Komposition verwendet werden. + Zum Beispiel ein interface mit dem getter: '{$suggestion}'. +should-be-interface = Die Klasse hat nur Methoden und keine Felder. Statt Vererbung sollte hier ein Interface mit + Standard-Implementierungen verwendet werden. -avoid-static-blocks = Statische Blöcke sollten vermieden werden, da sie keine objekt-orientierte Lösung darstellen und schlecht erweiterbar sind. Statische Blöcke sollten durch eine objektorientierte Lösung ersetzt werden (bspw. Konstruktoren). +avoid-static-blocks = Statische Blöcke sollten vermieden werden, da sie keine objekt-orientierte Lösung darstellen und + schlecht erweiterbar sind. Statische Blöcke sollten durch eine objektorientierte Lösung ersetzt + werden (bspw. Konstruktoren). # Structure default-package = Das default-Paket sollte nicht verwendet werden. Die folgenden Klassen sind im default-Paket: {$positions} -too-few-packages = Das Projekt hat mehr als {$max} Klassen, aber nur ein Paket. Du solltest dir eine sinnvolle Einteilung der Klassen in mehrere Pakete überlegen (bspw. nach den Kriterien commands, logic, ui, ...). +too-few-packages = Das Projekt hat mehr als {$max} Klassen, aber nur ein Paket. Verwende mehrere Pakete um die + Klassen sinvoll zu gruppieren. # Unnecessary diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl index 4e334258..f40c6b73 100644 --- a/autograder-core/src/main/resources/strings.en.ftl +++ b/autograder-core/src/main/resources/strings.en.ftl @@ -31,7 +31,7 @@ equals-hashcode-comparable-contract = Equals and hashCode must always be overrid use-enum-collection = For maps where an enum is used as a key and for sets as a value, one should use EnumMap/EnumSet. compare-to-zero = The result of #compareTo or #compare should only be compared to 0. - It is an implementation detail whether a given type returns strictly the values + It is an implementation detail whether a given type returns the values '-1, 0, +1' or others. equals-using-hashcode = Implementing equals by just comparing hashCodes is fragile. Hashes collide frequently, and this will lead to false positives in equals. @@ -64,7 +64,7 @@ javadoc-method-exp-param-unknown = The JavaDoc comment mentions the parameter '{ javadoc-unexpected-tag = The JavaDoc comment should not have an '@{$tag}' tag. -javadoc-type-exp-invalid-author = The @author tag should contain your u-shorthand: {$authors} +javadoc-type-exp-invalid-author = The @author tag should contain a u-shorthand: {$authors} javadoc-stub-exp-desc = Javadoc has an empty description javadoc-stub-exp-param = Stub description for parameter {$param} @@ -76,9 +76,9 @@ javadoc-undocumented-throws = The exception {$exp} is thrown, but not mentioned todo-comment = TODOs should not be in the final submission. # Complexity -use-diamond-operator = You can remove the types specified in the '< A, B, ... >' and just use '<>' instead, see https://docs.oracle.com/javase/tutorial/java/generics/genTypeInference.html and https://stackoverflow.com/a/16352848/7766117 +use-diamond-operator = The types specified in the '< A, B, ... >' can be remove, use '<>' instead. -extends-object = Explicitly extending Object is unnecessary +extends-object = Explicitly extending 'Object' is unnecessary for-loop-var = Each for-loop should have exactly one control variable @@ -88,7 +88,7 @@ redundant-modifier = The following modifiers are redundant and should be removed redundant-return-exp = Unnecessary return -self-assignment-exp = Useless assignment of {$rhs} to {$lhs} +self-assignment-exp = Useless assignment of '{$rhs}' to '{$lhs}' too-many-exceptions = The project defines {$count} exceptions. Those are too many. @@ -96,35 +96,33 @@ redundant-variable = The variable '{$name}' is redundant, the value can be used unused-import = The import '{$import}' is unused and should therefore be removed. -redundant-boolean-equal = It is redundant to explicitly check if a condition equals true or false. Write instead '{$suggestion}'. - -use-operator-assignment-exp = Assignment can be simplified to '{$simplified}' - complex-regex = Nontrivial regex need an explanation (score is {$score}, max allowed score is {$max}) redundant-catch = An exception should not be caught and then rethrown immediately. -redundant-uninitialized-variable = The variable '{$variable}' has been declared, but the value '{$value}' is not directly assigned. Instead you should write '{$suggestion}'. +redundant-uninitialized-variable = The variable '{$variable}' has been declared, but the value '{$value}' is not directly assigned. Instead write '{$suggestion}'. merge-nested-if = The nested if can be combined with the outer if. The condition for the outer if should then be '{$suggestion}'. -multiple-inline-statements = There should not be multiple statements in a single line. So no declarations of multiple variables or assignments in the same line. +multiple-inline-statements = There should not be multiple statements in a single line. -multi-threading = Multithreading is out of scope for this lecture. The code is only executed on a single thread. Writing it thread-safe makes the code unnecessarily complex. +multi-threading = Multithreading is not used. The code is only executed on a single thread. + Writing it thread-safe makes the code unnecessarily complex. -try-catch-complexity = The complexity of try-catch-blocks should be kept low. There should be less than {$max} statements in the try-block. You should refactor the code into multiple methods or extract unnecessary statements out of the try-block. +try-catch-complexity = The complexity of try-catch-blocks should be kept low. + There should be less than {$max} statements in the try-block. + Refactor the code into multiple methods or extract unnecessary + statements out of the try-block. redundant-else = The 'else' is redundant. Write '{$expected}' instead of '{$given}'. -redundant-assignment = The variable '{$variable}' is assigned a value that is never read, therefore the assignment can be removed. +redundant-assignment = The variable '{$variable}' is assigned a value that is never read, + therefore the assignment can be removed. # Debug -assert-used-exp = Assertions crash the entire program if they evaluate to false. - Also they can be disabled, so never rely on them to e.g. check user input. - They are great for testing purposes, but should not be part of your final solution. - If you want to document an invariant, consider a comment. +assert-used = Assertions should not be present in the final submission. Use exceptions instead. -print-stack-trace = Don't print stack traces in your final solution +print-stack-trace = Stack traces should not be printed in the final submission. # Exceptions custom-exception-inheritance-error = The custom exception '{$name}' should not extend 'Error'. @@ -132,18 +130,18 @@ custom-exception-inheritance-runtime = The custom exception '{$name}' should ext empty-catch-block = Empty catch block -exception-controlflow-caught = {$exception} thrown and immediately caught in a surrounding block -exception-controlflow-should-not-be-caught = {$exception} should never be caught +exception-controlflow-caught = '{$exception}' thrown and immediately caught in a surrounding block +exception-controlflow-should-not-be-caught = '{$exception}' should never be caught -runtime-exception-caught = RuntimeExceptions '{$exception}' should not be caught +runtime-exception-caught = RuntimeException '{$exception}' should not be caught exception-message = An exception should always have a message that explains what the problem is and ideally how it occurred. -number-format-exception-ignored = NumberFormatException should be caught or replaced through your own exception. +number-format-exception-ignored = 'NumberFormatException' should be caught and/or replaced by self-defined exception. # General -compare-objects-exp = Implement an equals method for type {$type} and use it for comparisons +compare-objects-exp = Implement an equals method for type '{$type}' and use it for comparisons variable-should-be = The variable '{$variable}' should be '{$suggestion}'. @@ -151,29 +149,25 @@ reassigned-parameter = The parameter '{$name}' should not be assigned a new valu double-brace-init = Don't use the obscure 'double brace initialization' syntax -missing-override = '{$name}' should have an '@Override'-annotation, see https://stackoverflow.com/a/94411/7766117. +missing-override = '{$name}' should have an '@Override'-annotation. -system-specific-linebreak = Always use system-independent line breaks such as the value obtained from System.lineSeparator() or %n in format strings +system-specific-linebreak = Always use system-independent line breaks such as the value obtained from 'System.lineSeparator()' or '%n' in format strings field-should-be-final = The attribute '{$name}' should be final. -string-cmp-exp = Use the equals method: '{$lhs}.equals({$rhs})' instead of '{$lhs} == {$rhs}' +do-not-use-raw-types-exp = Generic Types should always have generics and never be used as raw types. -do-not-use-raw-types-exp = Generic Types should always have generics and never be used as raw types, see https://stackoverflow.com/a/2770692/7766117 +avoid-labels = Labels should be avoided. -avoid-labels = Labels should be avoided. See https://stackoverflow.com/a/33689582/7766117. +avoid-shadowing = The variable '{$name}' hides an attribute with the same name. Except for in the constructor/setters, this should be avoided. -avoid-shadowing = The variable '{$name}' hides an attribute with the same name. Except for in the constructor, this should be avoided. - -suppress-warnings = @SuppressWarnings suppresses warnings from the compiler or Checkstyle, without fixing the underlying problem of the code. +suppress-warnings = '@SuppressWarnings' suppresses warnings from the compiler or Checkstyle, without fixing the underlying problem of the code. 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 = 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 +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}'. import-types = Instead of qualifying the type, '{$type}' should be imported. Types from the same package or 'java.lang' do not have to be imported explicitly. @@ -186,7 +180,7 @@ 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. -magic-string = The string '{$value}' should be in a constant. See the wiki article for magic strings. +magic-string = The string '{$value}' should be in a constant. 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} @@ -220,11 +214,17 @@ variable-redundant-number-suffix = The identifier '{$name}' has a redundant numb # OOP concrete-collection = The type '{$type}' should be replaced by an interface like 'List' or 'Set'. -leaked-collection-return = The method '{$method}' returns a reference to the field '{$field}'. This allows the field to be modified from the outside. Instead, a copy should be returned. -leaked-collection-constructor = The constructor '{$signature}' assigns a reference to the field '{$field}'. This allows the field to be modified from the outside. Instead, a copy should be made before setting the field. -leaked-collection-assign = The method '{$method}' assigns a reference to the field '{$field}'. This allows the field to be modified from the outside. Instead, a copy should be made before setting the field. +leaked-collection-return = The method '{$method}' returns a reference to the field '{$field}'. + This allows the field to be modified from the outside. + Instead, a copy should be returned. +leaked-collection-constructor = The constructor '{$signature}' assigns a reference to the field '{$field}'. + This allows the field to be modified from the outside. Instead, + a copy should be made before setting the field. +leaked-collection-assign = The method '{$method}' assigns a reference to the field '{$field}'. + This enables modification of the field from the outside. Instead, + a copy should be made before setting the field. -method-should-be-abstract = {$type}::{$method} should be abstract and not provide a default implementation +method-should-be-abstract = '{$type}::{$method}' should be abstract and not provide a default implementation method-should-be-static = The method '{$name}' should be static, because it does not access instance attributes or methods. @@ -233,18 +233,18 @@ utility-exp-constructor = Utility classes must have a single private no-arg cons static-field-should-be-instance = The static field '{$name}' must not be static. -constants-class-exp = Constants should be saved in the class they are used in and not in a separate class. See https://stackoverflow.com/a/15056462/7766117 +constants-class-exp = Constants should be saved in the class they are used in and not in a dedicated class. empty-interface-exp = Interfaces should not be empty. ui-input-separation = Input should not be spread over the program. Other use in {$first}. ui-output-separation = Output should not be spread over the program. Other use in {$first}. -do-not-use-system-exit = System.exit() must not be used. Structure your code in so that it exits naturally. +do-not-use-system-exit = 'System.exit()' must not be used. Structure code so that it exits naturally. avoid-inner-classes = Every class should be in its own file. Inner-Classes should be avoided. -mutable-enum = Enums should be immutable. See https://stackoverflow.com/a/41199773/7766117 +mutable-enum = Enums should be immutable. should-be-enum-attribute = The values of the switch should be associated attributes of the enum. Alternatively, one should use a Map. @@ -252,19 +252,27 @@ closed-set-of-values-switch = The switch has only finitely many cases. This is a closed-set-of-values-list = The listing only has finitely many values and should therefore be modeled as an enum. The values are: '{$values}'. closed-set-of-values-method = The method only returns the constant values '{$values}'. There are only finitely many, which is why one should model it as an enum. -do-not-use-instanceof = instanceof should not be used. See Ilias Wiki. -do-not-use-instanceof-emulation = instanceof should not be used and also not be emulated through getClass or ClassCastException. See Ilias Wiki. +do-not-use-instanceof = 'instanceof' should not be used. +do-not-use-instanceof-emulation = 'instanceof' should not be used and not be emulated through + 'getClass' or 'ClassCastException'. abstract-class-without-abstract-method = Abstract classes should have at least one abstract method. -composition-over-inheritance = The parent class has only fields. Instead of inheritance, composition should be used. For example through an interface with the getter: '{$suggestion}'. -should-be-interface = The parent class has only methods without fields. Instead of inheritance an interface with default-implementations should be used. +composition-over-inheritance = The parent class has only fields. Instead of inheritance, composition + should be used. For example through an interface with the getter: + '{$suggestion}'. +should-be-interface = The parent class has only methods without fields. Instead of inheritance an + interface with default-implementations should be used. -avoid-static-blocks = Static blocks should be avoided because they are not expandable and object-oriented. Static blocks should be replaced by an object-oriented solution (e.g. constructor). +avoid-static-blocks = Static blocks should be avoided because they are not expandable and + object-oriented. Static blocks should be replaced by an object-oriented + solution (e.g. constructor). # Structure -default-package = The default-package should not be used. The following classes are in the default-package: {$positions} -too-few-packages = The project has more than {$max} classes, but only one package is used. You should think about a better classification for the different classes to distribute the classes over multiple packages (e.g. commands, logic, ui, ...). +default-package = The default-package should not be used. The following classes are in the + default-package: {$positions} +too-few-packages = The project has more than {$max} classes, but only one package is used. + Use multiple packages to structure the code. # Unnecessary empty-block = Empty blocks should be removed or have a comment explaining why they are empty. diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseGuardClauses/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseGuardClauses/code/Test.java deleted file mode 100644 index 6ee7f565..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseGuardClauses/code/Test.java +++ /dev/null @@ -1,89 +0,0 @@ -package de.firemage.autograder.core.check_tests.UseGuardClauses.code; - -public class Test { - public static void main(String[] args) { - } - - void run(Timer timer) { - if (!timer.isEnabled()) - return; - - if (!timer.isValid()) - throw new IllegalArgumentException(); - - timer.run(); - } - - void run2(Timer timer) { - if (timer.isEnabled() && timer.isValid()) { - timer.run(); - } else if (!timer.isEnabled()) { /*# not ok #*/ - return; - } else if (!timer.isValid()) { /*# not ok #*/ - throw new IllegalArgumentException(); - } - } - - void example(boolean a) { - if (a) { /*# ok #*/ - System.out.println("a"); - return; // early return - } else { - System.out.println("b"); - // normal code path - } - } - - void isOkay(boolean a, boolean b) { - if (a) { - // do something - } else if (b) { - // do some other thing - } else { - // do some third thing - } - - // something that is always done - } - - void withLoop(boolean a, boolean b) { - while (a) { - if (b) { - System.out.println("hello"); - } else { /*# not ok #*/ - continue; - } - } - } - - void bigExample(boolean a, boolean b, boolean c, boolean d) { - if (a) { - if (b) { - if (c) { - // do something - } else if (d) { - // do something - } else { /*# not ok #*/ - throw new IllegalArgumentException(); - } - } else { /*# not ok #*/ - throw new IllegalArgumentException(); - } - } else { /*# not ok #*/ - throw new IllegalArgumentException(); - } - } -} - -class Timer { - boolean isEnabled() { - return true; - } - - boolean isValid() { - return true; - } - - void run() { - } -} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseGuardClauses/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseGuardClauses/config.txt deleted file mode 100644 index 90f75e7b..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/UseGuardClauses/config.txt +++ /dev/null @@ -1,8 +0,0 @@ -general.UseGuardClauses -Use guard clauses to simplify code and make it more readable. -Test.java:20-22 -Test.java:22-24 -Test.java:53-55 -Test.java:66-68 -Test.java:69-71 -Test.java:72-74 From 4c6170b092891d55e83f331ef040979412dfccbc Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sun, 24 Nov 2024 12:18:15 +0100 Subject: [PATCH 02/12] revert #567 (update to java 21) and some minor adjustments --- .../core/check/api/SequentialAddAll.java | 8 +++--- .../check/comment/UnnecessaryComment.java | 8 ++---- .../core/check/oop/LeakedCollectionCheck.java | 18 ++++++------- .../autograder/core/check/utils/Option.java | 8 +++--- .../core/integrated/ExpressionUtil.java | 3 --- .../autograder/core/integrated/TypeUtil.java | 4 +++ .../core/integrated/UsesFinder.java | 25 ++++++++++--------- .../structure/StructuralElement.java | 4 +-- .../structure/StructuralEqualsVisitor.java | 5 ++-- pom.xml | 2 +- 10 files changed, 42 insertions(+), 43 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SequentialAddAll.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SequentialAddAll.java index d7db6796..0a92d558 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SequentialAddAll.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SequentialAddAll.java @@ -62,7 +62,7 @@ static Optional of(CtStatement ctStatement) { return Optional.empty(); } - return Optional.of(new AddInvocation(collection, executableReference, ctInvocation.getArguments().get(0))); + return Optional.of(new AddInvocation(collection, executableReference, ctInvocation.getArguments().getFirst())); } } @@ -91,7 +91,7 @@ private void reportProblem(CtVariableReference ctVariable, List ctVariable, List SOME_GOOD_NAME = List.of(%s); /* ... */ %s.addAll(SOME_GOOD_NAME)".formatted( - ExpressionUtil.getExpressionType(values.get(0)), + ExpressionUtil.getExpressionType(values.getFirst()), values.stream() .map(CtElement::toString) .collect(Collectors.joining(", ")), 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 index 27e74d7c..7be14144 100644 --- 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 @@ -42,11 +42,7 @@ private void checkComments(Collection comments) { } private static boolean isStandaloneComment(CtComment ctComment) { - var parent = ctComment.getParent(); - if (parent == null) { - return false; - } - return !parent.getComments().contains(ctComment); + return ctComment.getParent() instanceof CtElement ctElement && !ctElement.getComments().contains(ctComment); } @Override @@ -75,7 +71,7 @@ public void process(CtElement element) { .map(CtComment.class::cast) .collect(Collectors.toCollection(ArrayList::new)); - followingComments.add(0, ctComment); + followingComments.addFirst(ctComment); checkComments(followingComments); return; 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 c23265df..8f748cda 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 @@ -241,9 +241,7 @@ private static List> findPreviousAssignee(CtVariableRead ctVa boolean foundPreviousAssignment = false; CtStatement currentStatement = ctVariableRead.getParent(CtStatement.class); - var reversedStatements = new ArrayList<>(StatementUtil.getEffectiveStatements(ctExecutable.getBody())); - Collections.reverse(reversedStatements); - for (CtStatement ctStatement : reversedStatements) { + for (CtStatement ctStatement : StatementUtil.getEffectiveStatements(ctExecutable.getBody()).reversed()) { if (!foundPreviousAssignment) { if (ctStatement == currentStatement) { foundPreviousAssignment = true; @@ -282,7 +280,7 @@ && isParameterOf(ctVariableDeclaration, ctExecutable)) { List> previousAssignees = findPreviousAssignee(ctVariableRead); if (!previousAssignees.isEmpty()) { - return findParameterReference(previousAssignees.get(0), ctExecutable); + return findParameterReference(previousAssignees.getFirst(), ctExecutable); } return Option.some((CtParameter) ctVariableDeclaration); @@ -445,11 +443,13 @@ private void checkCtType(CtType ctType) { ctTypeMember = fixRecordAccessor(ctRecord, ctMethod); } - if (ctTypeMember instanceof CtConstructor ctConstructor) { - checkCtExecutableAssign(ctConstructor); - } else if (ctTypeMember instanceof CtMethod ctMethod) { - checkCtExecutableReturn(ctMethod); - checkCtExecutableAssign(ctMethod); + switch (ctTypeMember) { + case CtConstructor ctConstructor -> checkCtExecutableAssign(ctConstructor); + case CtMethod ctMethod -> { + checkCtExecutableReturn(ctMethod); + checkCtExecutableAssign(ctMethod); + } + default -> {} } } } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java index ba0598cf..edb044bc 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java @@ -33,8 +33,8 @@ default boolean isSome() { } default Option map(Function function) { - if (this instanceof Some someValue) { - return new Some<>(function.apply(someValue.value)); + if (this instanceof Some(T value)) { + return new Some<>(function.apply(value)); } else if (this instanceof None) { return new None<>(); } @@ -56,8 +56,8 @@ default T nullable() { } default Stream stream() { - if (this instanceof Some someValue) { - return Stream.of(someValue.value); + if (this instanceof Some(T value)) { + return Stream.of(value); } else if (this instanceof None) { return Stream.empty(); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExpressionUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExpressionUtil.java index ef3784ee..bda24efa 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExpressionUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExpressionUtil.java @@ -10,7 +10,6 @@ import spoon.reflect.code.CtBinaryOperator; import spoon.reflect.code.CtConditional; import spoon.reflect.code.CtExpression; -import spoon.reflect.code.CtFieldRead; import spoon.reflect.code.CtInvocation; import spoon.reflect.code.CtLiteral; import spoon.reflect.code.CtTextBlock; @@ -20,8 +19,6 @@ import spoon.reflect.code.LiteralBase; import spoon.reflect.code.UnaryOperatorKind; import spoon.reflect.declaration.CtElement; -import spoon.reflect.declaration.CtEnum; -import spoon.reflect.declaration.CtEnumValue; import spoon.reflect.declaration.CtTypedElement; import spoon.reflect.declaration.CtVariable; import spoon.reflect.eval.PartialEvaluator; 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 56557f0d..3129d3ad 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 @@ -137,6 +137,10 @@ public static boolean isTypeEqualTo(CtTypeReference ctType, CtTypeReference ctTypeReference, Class expected) { CtType expectedType = ctTypeReference.getFactory().Type().get(expected); + return isSubtypeOf(ctTypeReference, expectedType); + } + + public static boolean isSubtypeOf(CtTypeReference ctTypeReference, CtType expectedType) { if (ctTypeReference.getTypeDeclaration() == null || ctTypeReference instanceof CtTypeParameterReference) { return ctTypeReference.isSubtypeOf(expectedType.getReference()); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java index 3be22dbb..32a83697 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java @@ -38,6 +38,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.SequencedSet; import java.util.Set; import java.util.stream.Stream; @@ -78,16 +79,13 @@ private static UsesFinder getFor(FactoryAccessor factoryAccessor) { */ @SuppressWarnings("rawtypes") public static CtElementStream getAllUses(CtNamedElement element) { - if (element instanceof CtVariable variable) { - return UsesFinder.variableUses(variable).asUntypedStream(); - } else if (element instanceof CtTypeParameter typeParameter) { - return UsesFinder.typeParameterUses(typeParameter).asUntypedStream(); - } else if (element instanceof CtExecutable executable) { - return UsesFinder.executableUses(executable).asUntypedStream(); - } else if (element instanceof CtType type) { - return UsesFinder.typeUses(type).asUntypedStream(); - } - throw new IllegalArgumentException("Unsupported element: " + element.getClass().getName()); + return switch (element) { + case CtVariable variable -> UsesFinder.variableUses(variable).asUntypedStream(); + case CtTypeParameter typeParameter -> UsesFinder.typeParameterUses(typeParameter).asUntypedStream(); + case CtExecutable executable -> UsesFinder.executableUses(executable).asUntypedStream(); + case CtType type -> UsesFinder.typeUses(type).asUntypedStream(); + default -> throw new IllegalArgumentException("Unsupported element: " + element.getClass().getName()); + }; } public static CtElementStream> variableUses(CtVariable variable) { @@ -183,7 +181,10 @@ public static boolean isAccessingVariable(CtVariable ctVariable, CtVariableAc } public static CtVariable getDeclaredVariable(CtVariableAccess ctVariableAccess) { - return UsesFinder.getFor(ctVariableAccess).scanner.variableAccessDeclarations.getOrDefault(ctVariableAccess, null); + return UsesFinder.getFor(ctVariableAccess).scanner.variableAccessDeclarations.getOrDefault( + ctVariableAccess, + VariableUtil.getVariableDeclaration(ctVariableAccess.getVariable()) + ); } /** * The scanner searches for uses of supported code elements in a single pass over the entire model. @@ -198,7 +199,7 @@ private static class UsesScanner extends CtScanner { private final Map> typeParameterUses = new IdentityHashMap<>(); private final Map> executableUses = new IdentityHashMap<>(); private final Map> typeUses = new IdentityHashMap<>(); - private final Map> subtypes = new IdentityHashMap<>(); + private final Map> subtypes = new IdentityHashMap<>(); // Caches the current instanceof pattern variables, since Spoon doesn't track them yet // We are conservative: A pattern introduces a variable until the end of the current block diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java index 3d066483..4d8dc244 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java @@ -12,11 +12,11 @@ public boolean equals(Object otherObject) { if (this == otherObject) { return true; } - if (!(otherObject instanceof StructuralElement otherStructuralElement)) { + if (!(otherObject instanceof StructuralElement(var otherElement))) { return false; } - return StructuralEqualsVisitor.equals(this.element, otherStructuralElement.element); + return StructuralEqualsVisitor.equals(this.element, otherElement); } @Override 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 f57f65d3..9de6e57a 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 @@ -15,6 +15,7 @@ import spoon.support.visitor.equals.EqualsVisitor; import java.util.LinkedHashSet; +import java.util.SequencedSet; import java.util.Set; public final class StructuralEqualsVisitor extends EqualsVisitor { @@ -25,7 +26,7 @@ public final class StructuralEqualsVisitor extends EqualsVisitor { CtRole.COMMENT, CtRole.COMMENT_CONTENT, CtRole.COMMENT_TAG, CtRole.COMMENT_TYPE ); - private final Set differences; + private final SequencedSet differences; public record Difference(CtRole role, Object left, Object right) {} @@ -117,7 +118,7 @@ protected boolean fail(CtRole role, Object element, Object other) { * * @return the differences */ - public Set differences() { + public SequencedSet differences() { return new LinkedHashSet<>(this.differences); } } diff --git a/pom.xml b/pom.xml index 72fa78be..b82893b4 100644 --- a/pom.xml +++ b/pom.xml @@ -38,7 +38,7 @@ UTF-8 - 17 + 21 ${java.version} ${java.version} ${java.version} From 294210a34b3f015b17e91ac0f171c7d9180d2b63 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 9 Dec 2024 09:29:31 +0100 Subject: [PATCH 03/12] fix spelling mistake --- autograder-core/src/main/resources/strings.de.ftl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl index 1727da12..94754cc6 100644 --- a/autograder-core/src/main/resources/strings.de.ftl +++ b/autograder-core/src/main/resources/strings.de.ftl @@ -213,8 +213,8 @@ variable-redundant-number-suffix = Der Bezeichner '{$name}' enthält eine unnöt concrete-collection = Der Typ '{$type}' sollte durch eine Schnittstelle wie zum Beispiel 'List' oder 'Set' ersetzt werden. leaked-collection-return = Die Methode '{$method}' gibt eine Referenz zu dem Feld '{$field}' zurück. Dadurch ist es möglich das Feld von außerhalb zu verändern. Gebe stattdessen eine Kopie zurück. -leaked-collection-constructor = Der Konstruktor '{$signature}' weißt dem Feld '{$field}' eine Referenz zu, dadurch ist es möglich das Feld von außerhalb zu verändern. Weise stattdessen eine Kopie dem Feld zu. -leaked-collection-assign = Die Methode '{$method}' weißt dem Feld '{$field}' eine Referenz zu, dadurch ist es möglich das Feld von außerhalb zu verändern. Weise stattdessen eine Kopie dem Feld zu. +leaked-collection-constructor = Der Konstruktor '{$signature}' weist dem Feld '{$field}' eine Referenz zu, dadurch ist es möglich das Feld von außerhalb zu verändern. Weise stattdessen eine Kopie dem Feld zu. +leaked-collection-assign = Die Methode '{$method}' weist dem Feld '{$field}' eine Referenz zu, dadurch ist es möglich das Feld von außerhalb zu verändern. Weise stattdessen eine Kopie dem Feld zu. method-should-be-abstract = '{$type}::{$method}' sollte abstrakt sein, anstatt eine Platzhalter-Implementierung anzugeben. From 60ff2a011ed9f9f5ee088d4ba54c08d96c4a8e4e Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 9 Dec 2024 09:52:08 +0100 Subject: [PATCH 04/12] fix chained if check, improve tests and improve suggestion #631 #649 --- .../core/check/complexity/ChainedIfCheck.java | 49 ++- .../check/complexity/TestChainedIfCheck.java | 372 ++++++++++++++++++ .../check_tests/ChainedIfCheck/code/Test.java | 104 ----- .../check_tests/ChainedIfCheck/config.txt | 8 - 4 files changed, 416 insertions(+), 117 deletions(-) create mode 100644 autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/code/Test.java delete mode 100644 autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/config.txt diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/ChainedIfCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/ChainedIfCheck.java index 75fdb9b0..919a34dd 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/ChainedIfCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/ChainedIfCheck.java @@ -9,6 +9,7 @@ import de.firemage.autograder.core.integrated.StaticAnalysis; import spoon.processing.AbstractProcessor; import spoon.reflect.code.CtBlock; +import spoon.reflect.code.CtExpression; import spoon.reflect.code.CtIf; import spoon.reflect.code.CtStatement; @@ -27,12 +28,48 @@ public void process(CtIf ctIf) { return; } + // We can combine a nested if with the parent if, when the code looks like this: + // if (a) { + // if (b) { + // ... + // } + // } + // + // ... + // + // Because there is + // - no code before/after the if(b) + // - there is no explicit else branch + // + // If there is an else or else if, we cannot merge the if-statements: + // if (a) { + // if (b) { + // ... + // } + // } else { + // ... + // } + // + // Because the code would behave differently if the condition a is true and b is false. + // + // Technically, one could solve this by adjusting the else to have a condition: + // } else if (a && !b || !a) { + // + // but that is not obvious, which is why it is not suggested. + // check if the if-statement has a nested if: List thenStatements = StatementUtil.getEffectiveStatements(ctIf.getThenStatement()); - if (thenStatements.size() == 1 + if (// a nested if is exactly one statement + thenStatements.size() == 1 + // the statement must be an if-statement && thenStatements.get(0) instanceof CtIf nestedIf + // and that nested if must not have an else branch && (nestedIf.getElseStatement() == null - || StatementUtil.getEffectiveStatements(nestedIf.getElseStatement()).isEmpty())) { + // or if it has one, it should be effectively empty + || StatementUtil.getEffectiveStatements(nestedIf.getElseStatement()).isEmpty()) + // and like described above, there must not be an else branch in the parent if + && ctIf.getElseStatement() == null + ) { addLocalProblem( ctIf.getCondition(), new LocalizedMessage( @@ -49,6 +86,7 @@ public void process(CtIf ctIf) { ); } + // Now check if the else branch has a nested if, which could be merged with the parent if: CtStatement elseStatement = ctIf.getElseStatement(); if (!(elseStatement instanceof CtBlock ctBlock) || ctBlock.getStatements().isEmpty()) { return; @@ -60,11 +98,12 @@ public void process(CtIf ctIf) { } if (statements.get(0) instanceof CtIf ctElseIf && !elseStatement.isImplicit()) { + CtExpression condition = ctElseIf.getCondition(); + addLocalProblem( ctElseIf.getCondition(), - new LocalizedMessage("suggest-replacement", Map.of( - "original", "else {\"{\"} if (...) {\"{\"} ... {\"}\"} {\"}\"}", - "suggestion", "else if (...) {\"{\"} ... {\"}\"}" + new LocalizedMessage("common-reimplementation", Map.of( + "suggestion", "} else if (%s) { ... }".formatted(condition) )), ProblemType.UNMERGED_ELSE_IF ); diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java new file mode 100644 index 00000000..a295b96b --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java @@ -0,0 +1,372 @@ +package de.firemage.autograder.core.check.complexity; + +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 TestChainedIfCheck extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.MERGE_NESTED_IF, ProblemType.UNMERGED_ELSE_IF); + + private void assertEqualsNested(Problem problem, String suggestion) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + "merge-nested-if", + Map.of("suggestion", suggestion) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + private void assertEqualsElseIf(Problem problem, String suggestion) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage("common-reimplementation", Map.of( + "suggestion", "} else if (%s) { ... }".formatted(suggestion) + )) + ), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testNestedIfsWithImplicitElse() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + if (true) { + if (false) { + } + } + } + } + """ + ), PROBLEM_TYPES); + + + assertEqualsNested(problems.next(), "true && false"); + problems.assertExhausted(); + } + + @Test + void testNestedIfsWithOuterElse() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + if (true) { + if (false) { + System.out.println("A"); + } + } else { + System.out.println("B"); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testElseIfWithNestedIfNoElse() throws LinterException, IOException { + // TODO: again think of when this will be a problem with control flow? + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x, int y) { + if (x == 0) { + System.out.println("A"); + } else if (x == 1) { + if (y == 0) { + System.out.println("B"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsNested(problems.next(), "x == 1 && y == 0"); + + problems.assertExhausted(); + } + + @Test + void testElseIfWithNestedIfWithElse() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x, int y) { + if (x == 0) { + System.out.println("A"); + } else if (x == 1) { + if (y == 0) { + System.out.println("B"); + } + } else { + System.out.println("C"); + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testRegularIfElseIfElseChain() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x, int y) { + if (x == 0) { + System.out.println("A"); + } else if (x == 1) { + System.out.println("B"); + } else { + System.out.println("C"); + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + problems.assertExhausted(); + } + + @Test + void testElseWithNestedIfAndCode() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x, int y) { + if (x == 0) { + } else if (x == 1) { + } else { + System.out.println("A"); + if (x == 2) { + System.out.println("B"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testElseWithNestedIf() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x, int y) { + if (x == 0) { + } else if (x == 1) { + } else { + if (x == 2) { + System.out.println("B"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsElseIf(problems.next(), "x == 2"); + + problems.assertExhausted(); + } + + @Test + void testCombineNestedIf() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x) { + if (x > 0) { + if (x < 10) { + System.out.println("A"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsNested(problems.next(), "x > 0 && x < 10"); + + problems.assertExhausted(); + } + + @Test + void testNestedIfWithReturn() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int a) { + if (a <= 0) { + if (a == -3) { + System.out.println("a is -3"); + } + return; + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + + @Test + void testComplicatedIfWithNested() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int a, int b) { + if (a == 1) { + if (b == 2) { + System.out.println("a is 1"); + } + } else if (a == 2) { + if (b == 3) { + throw new IllegalStateException("an error occurred"); + } + System.out.println("a is 2"); + } else if (a == 3) { + if (b == 3) { + System.out.println("a is 3 and b is 3"); + } else { + throw new IllegalStateException("should never happen"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testCombineNestedIfWithCode() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x) { + if (x > 0) { + System.out.println("A"); + if (x < 10) { + System.out.println("B"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testCombineNestedIfTrailingCode() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x) { + if (x > 0) { + if (x < 10) { + System.out.println("B"); + } + System.out.println("A"); + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + + @Test + void testIfWithoutBlock() throws LinterException, IOException { + // this resulted in a crash in the past + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x) { + if (x > 0); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/code/Test.java deleted file mode 100644 index b067f80b..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/code/Test.java +++ /dev/null @@ -1,104 +0,0 @@ -public class Test { - public static void main(String[] args) { - if (true) { /*# not ok #*/ - if (false) { - - } - } - - if (true) { /*# not ok #*/ - if (false) { - - } - } else { - - } - - if (true) { - - } else if (true) { /*# not ok #*/ - if (false) { - - } - } - - if (true) { - - } else if (true) { - - } else { - - } - - if (true) { - - } else if (true) { - - } else { - foo(); /*# ok #*/ - } - - if (true) { - - } else if (true) { - - } else { - foo(); /*# ok #*/ - if (true) { - - } - } - - if (true) { - - } else if (true) { - - } else { - if (true) { /*# not ok #*/ - } - } - - if (true) { - - } else if (true) { - - } else { - if (true) { /*# not ok #*/ - } else { - - } - } - } - - private static void foo() { - - } -} - -// Tests from https://github.com/pmd/pmd/blob/e8dbb54cb5ed682d9dfd4f54895f4bbd73be728e/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/CollapsibleIfStatements.xml#L4 -class PmdTests { - void callable(boolean x, boolean y) { - if (x) { /*# not ok #*/ - if (y) { - } - } - - if (x) { /*# ok #*/ - int z = 5; - if (y) { - } - } - - if (x) { /*# ok #*/ - if (y) { - } - int z = 5; - } - } -} - -class CrashChainedIf { - void isTrue(boolean a) { - if (a); /*# ok #*/ - } -} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/config.txt deleted file mode 100644 index e79ff561..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/config.txt +++ /dev/null @@ -1,8 +0,0 @@ -complexity.ChainedIfCheck -Avoid else { if () { } } -Test.java:3 -Test.java:9 -Test.java:19 -Test.java:57 -Test.java:66 -Test.java:81 From 8e608be094547d79a73e98e8edb71e7920aa8bdd Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 9 Dec 2024 09:52:30 +0100 Subject: [PATCH 05/12] remove TODO --- .../autograder/core/check/complexity/TestChainedIfCheck.java | 1 - 1 file changed, 1 deletion(-) diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java index a295b96b..2f6c9f6f 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java @@ -87,7 +87,6 @@ public static void main(String[] args) { @Test void testElseIfWithNestedIfNoElse() throws LinterException, IOException { - // TODO: again think of when this will be a problem with control flow? ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( JavaVersion.JAVA_17, "Test", From 796ec7147d34e783729e2b5c3a452d04a633e802 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:31:03 +0100 Subject: [PATCH 06/12] Add test for spoon bug --- .../TestRedundantIfForBooleanCheck.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantIfForBooleanCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantIfForBooleanCheck.java index 792dadab..606c81cb 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantIfForBooleanCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantIfForBooleanCheck.java @@ -7,6 +7,7 @@ import de.firemage.autograder.core.check.AbstractCheckTest; import de.firemage.autograder.api.JavaVersion; import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import java.io.IOException; @@ -378,4 +379,28 @@ public boolean doA(int a) { problems.assertExhausted(); } + + @Disabled("This is a bug in Spoon, should be fixed by the PR spoon#6094") + @Test + void testMissingParensAroundStringConcatSuggestion() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public boolean isRotation(String word, String rotation) { + if (word.length() != rotation.length()) { + return false; + } + + return (word + word).contains(rotation); + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsRedundant(problems.next(), "return word.length() == rotation.length() && (word + word).contains(rotation)"); + + problems.assertExhausted(); + } } From 306549c92ca75f875c5645f97c05c838f2b3229e Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:34:36 +0100 Subject: [PATCH 07/12] fix #626 --- .../core/check/naming/ConstantsHaveDescriptiveNamesCheck.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/ConstantsHaveDescriptiveNamesCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/ConstantsHaveDescriptiveNamesCheck.java index 8d749d2c..9feafeee 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/ConstantsHaveDescriptiveNamesCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/naming/ConstantsHaveDescriptiveNamesCheck.java @@ -114,6 +114,8 @@ private static List listCharOptions(char c) { case '_' -> List.of("underscore", "dash", "line"); case '/', '\\' -> List.of("slash", "backslash"); case '[', ']' -> List.of("bracket"); + case '*' -> List.of("star", "asterisk"); + case '+' -> List.of("plus"); default -> Character.isAlphabetic(c) ? List.of(String.valueOf(Character.toLowerCase(c))) : null; }; } From 9b0d76a5a86464cd50c8aba7255303e3e36d1576 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:41:08 +0100 Subject: [PATCH 08/12] suggest entrySet only if get is used frequently #634 --- .../de/firemage/autograder/core/check/api/UseEntrySet.java | 5 ++++- .../firemage/autograder/core/check/api/TestUseEntrySet.java | 6 +++++- 2 files changed, 9 insertions(+), 2 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 ef5b49dd..65985171 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 @@ -19,6 +19,9 @@ @ExecutableCheck(reportedProblems = {ProblemType.USE_ENTRY_SET}) public class UseEntrySet extends IntegratedCheck { + // If a map is iterated over and the key is used to get the value more than MINIMUM_GET_CALLS times, it suggests using entrySet + private static final int MINIMUM_GET_CALLS = 3; + private static boolean hasInvokedKeySet(CtInvocation ctInvocation) { return ctInvocation.getTarget() != null && ctInvocation.getExecutable() != null @@ -66,7 +69,7 @@ public void process(CtForEach ctForEach) { && ctVariableAccess.getVariable().equals(loopVariable.getReference())) .toList(); - if (!invocations.isEmpty()) { + if (invocations.size() >= MINIMUM_GET_CALLS) { addLocalProblem( ctForEach.getExpression(), new LocalizedMessage( 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 fb3ac4b5..2bfdbb0f 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 @@ -56,6 +56,7 @@ public static void main(String[] args) { if (storedColors.get(color) > 0) { result.put(color, storedColors.get(color)); } + System.out.println(storedColors.get(color)); } } } @@ -132,10 +133,12 @@ private java.util.List list() { public void method() { for (var mapKey : %s) { %s + %s + %s } } } - """.formatted(iterable, String.join("", body)) + """.formatted(iterable, String.join("", body), String.join("", body), String.join("", body)) ), PROBLEM_TYPES); if (suggestion != null) { @@ -158,6 +161,7 @@ private static > void execute(T map) { for (var mapKey : map.keySet()) { System.out.println(map.get(mapKey)); System.out.println(map.get(mapKey)); + System.out.println(map.get(mapKey)); } } } From 1f1a0a8526a3541650a72c341d795903936224e8 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:49:25 +0100 Subject: [PATCH 09/12] make exception for special case with raw types #636 --- .../autograder/core/check/general/DoNotUseRawTypes.java | 8 +++++++- .../test/java/de/firemage/autograder/core/CheckTest.java | 2 +- .../core/check_tests/DoNotUseRawTypes/code/Test.java | 4 +++- .../core/check_tests/DoNotUseRawTypes/config.txt | 1 - 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/DoNotUseRawTypes.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/DoNotUseRawTypes.java index 4a0a93ba..195f08ab 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/general/DoNotUseRawTypes.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/general/DoNotUseRawTypes.java @@ -6,6 +6,7 @@ import de.firemage.autograder.core.integrated.IntegratedCheck; import de.firemage.autograder.core.integrated.StaticAnalysis; import spoon.processing.AbstractProcessor; +import spoon.reflect.code.CtExpression; import spoon.reflect.declaration.CtType; import spoon.reflect.path.CtRole; import spoon.reflect.reference.CtTypeReference; @@ -27,8 +28,13 @@ private boolean isRawType(CtTypeReference ctTypeReference) { return false; } + // ignore types in expressions like 'new ArrayList()' + if (ctTypeReference.getParent(CtExpression.class) != null) { + return false; + } + return declaration.getFormalCtTypeParameters().size() != ctTypeReference.getActualTypeArguments().size(); - } + } // @Override protected void check(StaticAnalysis staticAnalysis) { diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/CheckTest.java b/autograder-core/src/test/java/de/firemage/autograder/core/CheckTest.java index 3a1784db..83e0e01d 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/CheckTest.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/CheckTest.java @@ -29,7 +29,7 @@ public class CheckTest { // this is useful for debugging/executing only relevant tests // // example: List.of("oop.ShouldBeEnumAttribute") - private static final List ONLY_TEST = List.of(); + private static final List ONLY_TEST = List.of("general.DoNotUseRawTypes"); public record Config(List lines) { public static Config fromPath(Path path) throws IOException { diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/DoNotUseRawTypes/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/DoNotUseRawTypes/code/Test.java index 6f3e258b..978c15d4 100644 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/DoNotUseRawTypes/code/Test.java +++ b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/DoNotUseRawTypes/code/Test.java @@ -16,10 +16,12 @@ public static void main(String[] args) { } void example1() { - List /*# not ok #*/ aList = new ArrayList /*# not ok #*/(); + List /*# not ok #*/ aList = new ArrayList(); String s = "Hello World!"; aList.add(s); String c = (String)aList.get(0); for (Map.Entry entry : l4.entrySet()) {} /*# ok #*/ } + + private Map subnets = new HashMap(); /*# ok #*/ } diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/DoNotUseRawTypes/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/DoNotUseRawTypes/config.txt index feefbec1..2975730c 100644 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/DoNotUseRawTypes/config.txt +++ b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/DoNotUseRawTypes/config.txt @@ -10,4 +10,3 @@ Test.java:14 Test.java:14 Test.java:19 -Test.java:19 From c2d06ded1120d2293dbd58b3899eef48860054c7 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:55:10 +0100 Subject: [PATCH 10/12] improve redundant variable check #635 --- .../check/complexity/RedundantVariable.java | 22 +++++++++---------- .../complexity/TestRedundantVariable.java | 20 ----------------- 2 files changed, 11 insertions(+), 31 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantVariable.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantVariable.java index a3654380..dc1aa9ad 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantVariable.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantVariable.java @@ -38,7 +38,7 @@ private boolean isComplexExpression(CtExpression ctExpression) { return ctExpression instanceof CtSwitchExpression || ctExpression.toString().length() > MAX_EXPRESSION_SIZE; } - private void checkVariableRead(CtStatement ctStatement, CtVariableRead ctVariableRead, CodeModel model) { + private void checkVariableRead(CtStatement ctStatement, CtVariableRead ctVariableRead) { if (// the variable must be a local variable !(ctVariableRead.getVariable().getDeclaration() instanceof CtLocalVariable ctLocalVariable) // it should not have any annotations (e.g. @SuppressWarnings("unchecked")) @@ -82,19 +82,19 @@ private void checkVariableRead(CtStatement ctStatement, CtVariableRead ctVari protected void check(StaticAnalysis staticAnalysis) { staticAnalysis.getModel().getRootPackage().accept(new CtScanner() { @Override - public void visitCtInvocation(CtInvocation ctInvocation) { - if (!ctInvocation.getPosition().isValidPosition() - || ctInvocation.isImplicit() - // only check invocations with a single variable - || ctInvocation.getArguments().size() != 1 - || !(ctInvocation.getArguments().get(0) instanceof CtVariableRead ctVariableRead)) { - super.visitCtInvocation(ctInvocation); + public void visitCtLocalVariable(CtLocalVariable ctLocalVariable) { + if (!ctLocalVariable.getPosition().isValidPosition() + || ctLocalVariable.isImplicit() + // only check local variables with a default expression + || ctLocalVariable.getDefaultExpression() == null + || !(ctLocalVariable.getDefaultExpression() instanceof CtVariableRead ctVariableRead)) { + super.visitCtLocalVariable(ctLocalVariable); return; } - checkVariableRead(ctInvocation, ctVariableRead, staticAnalysis.getCodeModel()); + checkVariableRead(ctLocalVariable, ctVariableRead); - super.visitCtInvocation(ctInvocation); + super.visitCtLocalVariable(ctLocalVariable); } @Override @@ -107,7 +107,7 @@ public void visitCtReturn(CtReturn ctReturn) { return; } - checkVariableRead(ctReturn, ctVariableRead, staticAnalysis.getCodeModel()); + checkVariableRead(ctReturn, ctVariableRead); super.visitCtReturn(ctReturn); } diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantVariable.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantVariable.java index bae8155b..6c245bfd 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantVariable.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantVariable.java @@ -355,26 +355,6 @@ public String get() { problems.assertExhausted(); } - @Test - void testRedundantPrintln() throws IOException, LinterException { - ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( - JavaVersion.JAVA_17, - "Test", - """ - public class Test { - public static void main(String[] args) { - int a = 5; - System.out.println(a); - } - } - """ - ), PROBLEM_TYPES); - - assertEqualsRedundant(problems.next(), "a", "System.out.println(5)"); - - problems.assertExhausted(); - } - @Test void testRedundantSwitchExpression() throws IOException, LinterException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( From b613b1182c3da76b0c9592c68b2a345c3dfe808c Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Mon, 9 Dec 2024 12:44:02 +0100 Subject: [PATCH 11/12] detect `Map`s for leaked collections #625 --- .../core/check/oop/LeakedCollectionCheck.java | 29 +---- .../declaration/CtRecordComponentImpl.java | 117 ------------------ .../check/general/TestFieldShouldBeFinal.java | 1 + .../check/oop/TestLeakedCollectionCheck.java | 70 +++++++++++ pom.xml | 10 +- 5 files changed, 83 insertions(+), 144 deletions(-) delete mode 100644 autograder-core/src/main/java/spoon/support/reflect/declaration/CtRecordComponentImpl.java 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 8f748cda..127cb835 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 @@ -37,7 +37,6 @@ import spoon.reflect.declaration.CtTypeMember; import spoon.reflect.declaration.CtTypedElement; import spoon.reflect.declaration.CtVariable; -import spoon.reflect.factory.Factory; import spoon.reflect.reference.CtTypeReference; import spoon.reflect.visitor.CtScanner; import spoon.reflect.visitor.filter.TypeFilter; @@ -81,7 +80,7 @@ }) public class LeakedCollectionCheck extends IntegratedCheck { private static boolean isMutableType(CtTypedElement ctTypedElement) { - return ctTypedElement.getType().isArray() || TypeUtil.isSubtypeOf(ctTypedElement.getType(), java.util.Collection.class); + return ctTypedElement.getType().isArray() || TypeUtil.isSubtypeOf(ctTypedElement.getType(), java.util.Collection.class) || TypeUtil.isSubtypeOf(ctTypedElement.getType(), java.util.Map.class); } /** @@ -98,7 +97,7 @@ private static boolean canBeMutated(CtField ctVariable) { return true; } - if (!TypeUtil.isSubtypeOf(ctVariable.getType(), java.util.Collection.class)) { + if (!TypeUtil.isSubtypeOf(ctVariable.getType(), java.util.Collection.class) && !TypeUtil.isSubtypeOf(ctVariable.getType(), java.util.Map.class)) { // not a collection return false; } @@ -130,7 +129,7 @@ private boolean isMutableExpression(CtExpression ctExpression) { } // we only care about arrays and collections for now - if (!TypeUtil.isSubtypeOf(ctExpression.getType(), java.util.Collection.class)) { + if (!TypeUtil.isSubtypeOf(ctExpression.getType(), java.util.Collection.class) && !TypeUtil.isSubtypeOf(ctExpression.getType(), java.util.Map.class)) { // not a collection return false; } @@ -412,24 +411,6 @@ private static CtReturn createCtReturn(CtExpression ctExpression) { return ctReturn.setReturnedExpression((CtExpression) ctExpression); } - // The generated accessor method of a record does not have a real return statement. - // This method fixes that by creating the return statement. - private static CtMethod fixRecordAccessor(CtRecord ctRecord, CtMethod ctMethod) { - // TODO: remove when https://github.com/INRIA/spoon/pull/5801 is merged. - Factory factory = ctMethod.getFactory(); - CtMethod result = ctMethod.clone(); - CtFieldRead ctFieldRead = factory.createFieldRead(); - - ctFieldRead.setTarget(null); - ctFieldRead.setVariable(ctRecord.getField(ctMethod.getSimpleName()).getReference()); - ctFieldRead.setType(result.getType()); - - result.setBody(createCtReturn(ctFieldRead)); - result.setParent(ctRecord); - - return result; - } - @Override protected void check(StaticAnalysis staticAnalysis) { staticAnalysis.getModel().getRootPackage().accept(new CtScanner() { @@ -439,10 +420,6 @@ private void checkCtType(CtType ctType) { } for (CtTypeMember ctTypeMember : ctType.getTypeMembers()) { - if (ctType instanceof CtRecord ctRecord && ctTypeMember instanceof CtMethod ctMethod && ctMethod.isImplicit()) { - ctTypeMember = fixRecordAccessor(ctRecord, ctMethod); - } - switch (ctTypeMember) { case CtConstructor ctConstructor -> checkCtExecutableAssign(ctConstructor); case CtMethod ctMethod -> { diff --git a/autograder-core/src/main/java/spoon/support/reflect/declaration/CtRecordComponentImpl.java b/autograder-core/src/main/java/spoon/support/reflect/declaration/CtRecordComponentImpl.java deleted file mode 100644 index beabafb2..00000000 --- a/autograder-core/src/main/java/spoon/support/reflect/declaration/CtRecordComponentImpl.java +++ /dev/null @@ -1,117 +0,0 @@ -/* - * SPDX-License-Identifier: (MIT OR CECILL-C) - * - * Copyright (C) 2006-2023 INRIA and contributors - * - * Spoon is available either under the terms of the MIT License (see LICENSE-MIT.txt) or the Cecill-C License (see LICENSE-CECILL-C.txt). You as the user are entitled to choose the terms under which to adopt Spoon. - */ -package spoon.support.reflect.declaration; - -import java.util.Collections; -import java.util.HashSet; -import java.util.Set; -import spoon.JLSViolation; -import spoon.reflect.annotations.MetamodelPropertyField; -import spoon.reflect.declaration.CtField; -import spoon.reflect.declaration.CtMethod; -import spoon.reflect.declaration.CtNamedElement; -import spoon.reflect.declaration.CtRecordComponent; -import spoon.reflect.declaration.CtShadowable; -import spoon.reflect.declaration.CtTypedElement; -import spoon.reflect.declaration.ModifierKind; -import spoon.reflect.path.CtRole; -import spoon.reflect.reference.CtTypeReference; -import spoon.reflect.visitor.CtVisitor; -import spoon.support.reflect.CtExtendedModifier; - -public class CtRecordComponentImpl extends CtNamedElementImpl implements CtRecordComponent { - - private static final Set forbiddenNames = createForbiddenNames(); - @MetamodelPropertyField(role = CtRole.TYPE) - private CtTypeReference type; - @MetamodelPropertyField(role = CtRole.IS_SHADOW) - boolean isShadow; - - @Override - public CtMethod toMethod() { - CtMethod method = this.getFactory().createMethod(); - method.setSimpleName(getSimpleName()); - method.setType((CtTypeReference) getType().clone()); - method.setExtendedModifiers(Collections.singleton(new CtExtendedModifier(ModifierKind.PUBLIC, true))); - method.setImplicit(true); - method.setBody(getFactory().createCodeSnippetStatement("return " + getSimpleName())); - return method; - } - - @Override - public CtField toField() { - CtField field = this.getFactory().createField(); - field.setSimpleName(getSimpleName()); - field.setType((CtTypeReference) getType()); - Set modifiers = new HashSet<>(); - modifiers.add(new CtExtendedModifier(ModifierKind.PRIVATE, true)); - modifiers.add(new CtExtendedModifier(ModifierKind.FINAL, true)); - field.setExtendedModifiers(modifiers); - field.setImplicit(true); - return field; - } - - @Override - public boolean isImplicit() { - return true; - } - - @Override - public CtTypeReference getType() { - return type; - } - - @Override - public C setType(CtTypeReference type) { - if (type != null) { - type.setParent(this); - } - getFactory().getEnvironment().getModelChangeListener().onObjectUpdate(this, CtRole.TYPE, type, this.type); - this.type = type; - return (C) this; - } - - @Override - public void accept(CtVisitor visitor) { - visitor.visitCtRecordComponent(this); - } - - @Override - public T setSimpleName(String simpleName) { - checkName(simpleName); - return super.setSimpleName(simpleName); - } - - private void checkName(String simpleName) { - if (forbiddenNames.contains(simpleName)) { - JLSViolation.throwIfSyntaxErrorsAreNotIgnored(this, "The name '" + simpleName + "' is not allowed as record component name."); - } - } - private static Set createForbiddenNames() { - return Set.of("clone", "finalize", "getClass", "notify", "notifyAll", "equals", "hashCode", "toString", "wait"); - } - - @Override - public CtRecordComponent clone() { - return (CtRecordComponent) super.clone(); - } - - - - @Override - public boolean isShadow() { - return isShadow; - } - - @Override - public E setShadow(boolean isShadow) { - getFactory().getEnvironment().getModelChangeListener().onObjectUpdate(this, CtRole.IS_SHADOW, isShadow, this.isShadow); - this.isShadow = isShadow; - return (E) this; - } -} 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 b5da5364..3fdad8b9 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 @@ -627,6 +627,7 @@ public static void main(String[] args) {} } @Test + @Disabled("Some spoon bug in 11.1.1-SNAPSHOT") void testRecordStaticField() throws LinterException, IOException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( JavaVersion.JAVA_17, 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 e026b4c0..f476c7d6 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 @@ -935,6 +935,27 @@ public record Zoo(String name, Collection animals) { problems.assertExhausted(); } + @Test + void testCompactConstructorImmutableMap() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Zoo", + """ + import java.util.Map; + import java.util.Collection; + import java.util.HashMap; + + public record Zoo(String name, Map animals) { + public Zoo { + animals = Map.copyOf(animals); + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + @Test void testCompactConstructorLeakedAssign() throws IOException, LinterException { ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( @@ -962,6 +983,32 @@ public Collection animals() { problems.assertExhausted(); } + @Test + void testCompactConstructorLeakedAssignMap() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Zoo", + """ + import java.util.Map; + import java.util.Collection; + import java.util.HashMap; + + public record Zoo(String name, Map animals) { + public Zoo { + animals = animals; + } + + public Map animals() { + return new HashMap<>(animals); + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedConstructor(problems.next(), "Zoo(String, Map)", "animals"); + + problems.assertExhausted(); + } @Test void testCompactConstructorCopied() throws IOException, LinterException { @@ -986,6 +1033,29 @@ public record Zoo(String name, Collection animals) { problems.assertExhausted(); } + @Test + void testCompactConstructorCopiedMap() throws IOException, LinterException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Zoo", + """ + import java.util.Map; + import java.util.Collection; + import java.util.HashMap; + + public record Zoo(String name, Map animals) { + public Zoo { + animals = new HashMap<>(animals); + } + } + """ + ), PROBLEM_TYPES); + + assertEqualsLeakedReturn(problems.next(), "animals", "animals"); + + problems.assertExhausted(); + } + @Test void testSelfAssignment() throws IOException, LinterException { diff --git a/pom.xml b/pom.xml index b82893b4..38b6134c 100644 --- a/pom.xml +++ b/pom.xml @@ -35,6 +35,14 @@ https://github.com/Feuermagier/autograder/tree/main + + + spoon-snapshot + Maven Repository for Spoon Snapshots + https://oss.sonatype.org/content/repositories/snapshots/ + + + UTF-8 @@ -45,7 +53,7 @@ 2.0.16 2.18.0 - 11.1.0 + 11.1.1-SNAPSHOT 0.70 0.10.2 From d0067bccc77fd6f83cc4aba33585761bf2233eea Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Tue, 10 Dec 2024 09:47:27 +0100 Subject: [PATCH 12/12] Revert "revert #567 (update to java 21) and some minor adjustments" This reverts commit 4c6170b092891d55e83f331ef040979412dfccbc. --- .../core/check/api/SequentialAddAll.java | 8 +++--- .../check/comment/UnnecessaryComment.java | 8 ++++-- .../core/check/oop/LeakedCollectionCheck.java | 19 +++++++------- .../autograder/core/check/utils/Option.java | 8 +++--- .../core/integrated/ExpressionUtil.java | 3 +++ .../autograder/core/integrated/TypeUtil.java | 4 --- .../core/integrated/UsesFinder.java | 25 +++++++++---------- .../structure/StructuralElement.java | 4 +-- .../structure/StructuralEqualsVisitor.java | 5 ++-- pom.xml | 2 +- 10 files changed, 44 insertions(+), 42 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SequentialAddAll.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SequentialAddAll.java index 0a92d558..d7db6796 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SequentialAddAll.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/SequentialAddAll.java @@ -62,7 +62,7 @@ static Optional of(CtStatement ctStatement) { return Optional.empty(); } - return Optional.of(new AddInvocation(collection, executableReference, ctInvocation.getArguments().getFirst())); + return Optional.of(new AddInvocation(collection, executableReference, ctInvocation.getArguments().get(0))); } } @@ -91,7 +91,7 @@ private void reportProblem(CtVariableReference ctVariable, List ctVariable, List SOME_GOOD_NAME = List.of(%s); /* ... */ %s.addAll(SOME_GOOD_NAME)".formatted( - ExpressionUtil.getExpressionType(values.getFirst()), + ExpressionUtil.getExpressionType(values.get(0)), values.stream() .map(CtElement::toString) .collect(Collectors.joining(", ")), 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 index 7be14144..27e74d7c 100644 --- 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 @@ -42,7 +42,11 @@ private void checkComments(Collection comments) { } private static boolean isStandaloneComment(CtComment ctComment) { - return ctComment.getParent() instanceof CtElement ctElement && !ctElement.getComments().contains(ctComment); + var parent = ctComment.getParent(); + if (parent == null) { + return false; + } + return !parent.getComments().contains(ctComment); } @Override @@ -71,7 +75,7 @@ public void process(CtElement element) { .map(CtComment.class::cast) .collect(Collectors.toCollection(ArrayList::new)); - followingComments.addFirst(ctComment); + followingComments.add(0, ctComment); checkComments(followingComments); return; 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 127cb835..8ca12338 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 @@ -37,6 +37,7 @@ import spoon.reflect.declaration.CtTypeMember; import spoon.reflect.declaration.CtTypedElement; import spoon.reflect.declaration.CtVariable; +import spoon.reflect.factory.Factory; import spoon.reflect.reference.CtTypeReference; import spoon.reflect.visitor.CtScanner; import spoon.reflect.visitor.filter.TypeFilter; @@ -240,7 +241,9 @@ private static List> findPreviousAssignee(CtVariableRead ctVa boolean foundPreviousAssignment = false; CtStatement currentStatement = ctVariableRead.getParent(CtStatement.class); - for (CtStatement ctStatement : StatementUtil.getEffectiveStatements(ctExecutable.getBody()).reversed()) { + var reversedStatements = new ArrayList<>(StatementUtil.getEffectiveStatements(ctExecutable.getBody())); + Collections.reverse(reversedStatements); + for (CtStatement ctStatement : reversedStatements) { if (!foundPreviousAssignment) { if (ctStatement == currentStatement) { foundPreviousAssignment = true; @@ -279,7 +282,7 @@ && isParameterOf(ctVariableDeclaration, ctExecutable)) { List> previousAssignees = findPreviousAssignee(ctVariableRead); if (!previousAssignees.isEmpty()) { - return findParameterReference(previousAssignees.getFirst(), ctExecutable); + return findParameterReference(previousAssignees.get(0), ctExecutable); } return Option.some((CtParameter) ctVariableDeclaration); @@ -420,13 +423,11 @@ private void checkCtType(CtType ctType) { } for (CtTypeMember ctTypeMember : ctType.getTypeMembers()) { - switch (ctTypeMember) { - case CtConstructor ctConstructor -> checkCtExecutableAssign(ctConstructor); - case CtMethod ctMethod -> { - checkCtExecutableReturn(ctMethod); - checkCtExecutableAssign(ctMethod); - } - default -> {} + if (ctTypeMember instanceof CtConstructor ctConstructor) { + checkCtExecutableAssign(ctConstructor); + } else if (ctTypeMember instanceof CtMethod ctMethod) { + checkCtExecutableReturn(ctMethod); + checkCtExecutableAssign(ctMethod); } } } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java index edb044bc..ba0598cf 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/utils/Option.java @@ -33,8 +33,8 @@ default boolean isSome() { } default Option map(Function function) { - if (this instanceof Some(T value)) { - return new Some<>(function.apply(value)); + if (this instanceof Some someValue) { + return new Some<>(function.apply(someValue.value)); } else if (this instanceof None) { return new None<>(); } @@ -56,8 +56,8 @@ default T nullable() { } default Stream stream() { - if (this instanceof Some(T value)) { - return Stream.of(value); + if (this instanceof Some someValue) { + return Stream.of(someValue.value); } else if (this instanceof None) { return Stream.empty(); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExpressionUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExpressionUtil.java index bda24efa..ef3784ee 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExpressionUtil.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/ExpressionUtil.java @@ -10,6 +10,7 @@ import spoon.reflect.code.CtBinaryOperator; import spoon.reflect.code.CtConditional; import spoon.reflect.code.CtExpression; +import spoon.reflect.code.CtFieldRead; import spoon.reflect.code.CtInvocation; import spoon.reflect.code.CtLiteral; import spoon.reflect.code.CtTextBlock; @@ -19,6 +20,8 @@ import spoon.reflect.code.LiteralBase; import spoon.reflect.code.UnaryOperatorKind; import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtEnum; +import spoon.reflect.declaration.CtEnumValue; import spoon.reflect.declaration.CtTypedElement; import spoon.reflect.declaration.CtVariable; import spoon.reflect.eval.PartialEvaluator; 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 3129d3ad..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 @@ -137,10 +137,6 @@ public static boolean isTypeEqualTo(CtTypeReference ctType, CtTypeReference ctTypeReference, Class expected) { CtType expectedType = ctTypeReference.getFactory().Type().get(expected); - return isSubtypeOf(ctTypeReference, expectedType); - } - - public static boolean isSubtypeOf(CtTypeReference ctTypeReference, CtType expectedType) { if (ctTypeReference.getTypeDeclaration() == null || ctTypeReference instanceof CtTypeParameterReference) { return ctTypeReference.isSubtypeOf(expectedType.getReference()); } diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java index 32a83697..3be22dbb 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/UsesFinder.java @@ -38,7 +38,6 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.SequencedSet; import java.util.Set; import java.util.stream.Stream; @@ -79,13 +78,16 @@ private static UsesFinder getFor(FactoryAccessor factoryAccessor) { */ @SuppressWarnings("rawtypes") public static CtElementStream getAllUses(CtNamedElement element) { - return switch (element) { - case CtVariable variable -> UsesFinder.variableUses(variable).asUntypedStream(); - case CtTypeParameter typeParameter -> UsesFinder.typeParameterUses(typeParameter).asUntypedStream(); - case CtExecutable executable -> UsesFinder.executableUses(executable).asUntypedStream(); - case CtType type -> UsesFinder.typeUses(type).asUntypedStream(); - default -> throw new IllegalArgumentException("Unsupported element: " + element.getClass().getName()); - }; + if (element instanceof CtVariable variable) { + return UsesFinder.variableUses(variable).asUntypedStream(); + } else if (element instanceof CtTypeParameter typeParameter) { + return UsesFinder.typeParameterUses(typeParameter).asUntypedStream(); + } else if (element instanceof CtExecutable executable) { + return UsesFinder.executableUses(executable).asUntypedStream(); + } else if (element instanceof CtType type) { + return UsesFinder.typeUses(type).asUntypedStream(); + } + throw new IllegalArgumentException("Unsupported element: " + element.getClass().getName()); } public static CtElementStream> variableUses(CtVariable variable) { @@ -181,10 +183,7 @@ public static boolean isAccessingVariable(CtVariable ctVariable, CtVariableAc } public static CtVariable getDeclaredVariable(CtVariableAccess ctVariableAccess) { - return UsesFinder.getFor(ctVariableAccess).scanner.variableAccessDeclarations.getOrDefault( - ctVariableAccess, - VariableUtil.getVariableDeclaration(ctVariableAccess.getVariable()) - ); + return UsesFinder.getFor(ctVariableAccess).scanner.variableAccessDeclarations.getOrDefault(ctVariableAccess, null); } /** * The scanner searches for uses of supported code elements in a single pass over the entire model. @@ -199,7 +198,7 @@ private static class UsesScanner extends CtScanner { private final Map> typeParameterUses = new IdentityHashMap<>(); private final Map> executableUses = new IdentityHashMap<>(); private final Map> typeUses = new IdentityHashMap<>(); - private final Map> subtypes = new IdentityHashMap<>(); + private final Map> subtypes = new IdentityHashMap<>(); // Caches the current instanceof pattern variables, since Spoon doesn't track them yet // We are conservative: A pattern introduces a variable until the end of the current block diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java index 4d8dc244..3d066483 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/structure/StructuralElement.java @@ -12,11 +12,11 @@ public boolean equals(Object otherObject) { if (this == otherObject) { return true; } - if (!(otherObject instanceof StructuralElement(var otherElement))) { + if (!(otherObject instanceof StructuralElement otherStructuralElement)) { return false; } - return StructuralEqualsVisitor.equals(this.element, otherElement); + return StructuralEqualsVisitor.equals(this.element, otherStructuralElement.element); } @Override 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 9de6e57a..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 @@ -15,7 +15,6 @@ import spoon.support.visitor.equals.EqualsVisitor; import java.util.LinkedHashSet; -import java.util.SequencedSet; import java.util.Set; public final class StructuralEqualsVisitor extends EqualsVisitor { @@ -26,7 +25,7 @@ public final class StructuralEqualsVisitor extends EqualsVisitor { CtRole.COMMENT, CtRole.COMMENT_CONTENT, CtRole.COMMENT_TAG, CtRole.COMMENT_TYPE ); - private final SequencedSet differences; + private final Set differences; public record Difference(CtRole role, Object left, Object right) {} @@ -118,7 +117,7 @@ protected boolean fail(CtRole role, Object element, Object other) { * * @return the differences */ - public SequencedSet differences() { + public Set differences() { return new LinkedHashSet<>(this.differences); } } diff --git a/pom.xml b/pom.xml index 38b6134c..df7417d5 100644 --- a/pom.xml +++ b/pom.xml @@ -46,7 +46,7 @@ UTF-8 - 21 + 17 ${java.version} ${java.version} ${java.version}