Merge pull request #503 from Luro02/main

Implement some things

autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java

@@ -73,7 +73,7 @@ public enum ProblemType {
     TOO_FEW_PACKAGES,
     TRY_CATCH_COMPLEXITY,
     AVOID_STATIC_BLOCKS,
-
+    METHOD_SHOULD_BE_STATIC,
     REASSIGNED_PARAMETER,
     DOUBLE_BRACE_INITIALIZATION,
     FOR_CAN_BE_FOREACH,
@@ -89,7 +89,8 @@ public enum ProblemType {
     IDENTIFIER_CONTAINS_TYPE_NAME,
     USE_GUARD_CLAUSES,
     CONCRETE_COLLECTION_AS_FIELD_OR_RETURN_VALUE,
-    LIST_NOT_COPIED_IN_GETTER,
+    LEAKED_COLLECTION_RETURN,
+    LEAKED_COLLECTION_ASSIGN,
     METHOD_USES_PLACEHOLDER_IMPLEMENTATION,
     UTILITY_CLASS_NOT_FINAL,
     UTILITY_CLASS_INVALID_CONSTRUCTOR,

autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseArraysFill.java

@@ -43,7 +43,7 @@ private void checkArraysFill(CtFor ctFor) {
 
         CtElement loopVariable = SpoonUtil.getReferenceDeclaration(forLoopRange.loopVariable());
         // return if the for loop uses the loop variable (would not be a simple repetition)
-        if (SpoonUtil.hasAnyUsesIn(loopVariable, ctAssignment.getAssignment(), e -> true)) {
+        if (SpoonUtil.hasAnyUsesIn(loopVariable, ctAssignment.getAssignment())) {
             return;
         }
 

autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseModuloOperator.java

@@ -70,17 +70,23 @@ private void checkModulo(CtIf ctIf) {
         // is equal to
         //
         // variable %= 3;
-        if (Set.of(BinaryOperatorKind.GE, BinaryOperatorKind.EQ).contains(condition.getKind())) {
-            CtExpression<?> right = condition.getRightHandOperand();
+        CtExpression<?> checkedValue = condition.getRightHandOperand();
 
+        // for boxed types, one could check if the value is null,
+        // for which the suggestion `a %= null` would not make sense
+        if (SpoonUtil.isNullLiteral(checkedValue)) {
+            return;
+        }
+
+        if (Set.of(BinaryOperatorKind.GE, BinaryOperatorKind.EQ).contains(condition.getKind())) {
             addLocalProblem(
                 ctIf,
                 new LocalizedMessage(
                     "common-reimplementation",
                     Map.of(
                         "suggestion", "%s %%= %s".formatted(
                             assignedVariable,
-                            right
+                            checkedValue
                         )
                     )
                 ),

autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseStringFormatted.java

@@ -37,6 +37,11 @@ private void checkCtInvocation(CtInvocation<?> ctInvocation) {
         }
 
         CtExpression<?> format = args.remove(0);
+        // skip if the format string is not a string literal (e.g. a complex concatenation)
+        if (SpoonUtil.tryGetStringLiteral(SpoonUtil.resolveConstant(format)).isEmpty()) {
+            return;
+        }
+
         String output = "%s.formatted(%s)".formatted(
             format,
             args.stream().map(CtExpression::toString).reduce((a, b) -> a + ", " + b).orElse("")

autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantAssignment.java

@@ -7,6 +7,7 @@
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.uses.UsesFinder;
 import spoon.processing.AbstractProcessor;
 import spoon.reflect.code.CtAssignment;
 import spoon.reflect.code.CtLocalVariable;
@@ -50,12 +51,7 @@ public void process(CtAssignment<?, ?> ctAssignment) {
                     return;
                 }
 
-
-                if (followingStatements.stream().noneMatch(statement -> SpoonUtil.hasAnyUsesIn(
-                    ctLocalVariable,
-                    statement,
-                    element -> element instanceof CtVariableRead<?>
-                ))) {
+                if (followingStatements.stream().noneMatch(statement -> UsesFinder.ofVariableRead(ctLocalVariable).in(statement).hasAny())) {
                     addLocalProblem(
                         ctAssignment,
                         new LocalizedMessage(

autograder-core/src/main/java/de/firemage/autograder/core/check/general/AvoidShadowing.java

@@ -6,9 +6,8 @@
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.uses.UsesFinder;
 import spoon.processing.AbstractProcessor;
-import spoon.reflect.code.CtLocalVariable;
-import spoon.reflect.code.CtVariableRead;
 import spoon.reflect.declaration.CtConstructor;
 import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtMethod;
@@ -48,18 +47,6 @@ private static Collection<CtFieldReference<?>> getAllVisibleFields(CtTypeInforma
         return result;
     }
 
-    /**
-     * Searches for a variable read of the given variable in the given element.
-     *
-     * @param ctVariable the variable that should be read
-     * @param in the element to search in
-     * @return true if a variable read was found, false otherwise
-     * @param <T> the type of the variable
-     */
-    private static <T> boolean hasVariableReadIn(CtVariable<T> ctVariable, CtElement in) {
-        return SpoonUtil.findUsesIn(ctVariable, in).stream().anyMatch(ctElement -> ctElement instanceof CtVariableRead<?>);
-    }
-
     @Override
     protected void check(StaticAnalysis staticAnalysis) {
         staticAnalysis.processWith(new AbstractProcessor<CtVariable<?>>() {
@@ -89,7 +76,9 @@ public void process(CtVariable<?> ctVariable) {
                 Collection<CtFieldReference<?>> visibleFields = getAllVisibleFields(parent);
 
                 List<CtFieldReference<?>> hiddenFields = visibleFields.stream()
+                    // ignore fields that are allowed to be hidden
                     .filter(ctFieldReference -> !ALLOWED_FIELDS.contains(ctFieldReference.getSimpleName()))
+                    // only keep fields that have the same name as the variable, but are not the same field
                     .filter(ctFieldReference -> ctFieldReference.getSimpleName().equals(ctVariable.getSimpleName())
                         && !ctFieldReference.equals(ctVariable.getReference()))
                     .toList();
@@ -102,11 +91,11 @@ public void process(CtVariable<?> ctVariable) {
                 CtElement variableParent = ctVariable.getParent();
 
                 // there might be multiple fields hidden by the variable (e.g. subclass hides superclass field)
-                boolean isFieldRead = hiddenFields.stream().anyMatch(ctFieldReference -> hasVariableReadIn(ctFieldReference.getFieldDeclaration(), variableParent));
+                boolean isFieldRead = hiddenFields.stream().anyMatch(ctFieldReference -> UsesFinder.ofVariableRead(ctFieldReference.getFieldDeclaration()).in(variableParent).hasAny());
 
                 // to reduce the number of annotations, we only report a problem if the variable AND the hidden field are read in
                 // the same context
-                if (hasVariableReadIn(ctVariable, variableParent) && isFieldRead) {
+                if (UsesFinder.ofVariableRead(ctVariable).in(variableParent).hasAny() && isFieldRead) {
                     addLocalProblem(
                         ctVariable,
                         new LocalizedMessage("avoid-shadowing", Map.of("name", ctVariable.getSimpleName())),

autograder-core/src/main/java/de/firemage/autograder/core/check/general/FieldShouldBeFinal.java

@@ -6,6 +6,7 @@
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.uses.UsesFinder;
 import spoon.processing.AbstractProcessor;
 import spoon.reflect.code.CtFieldWrite;
 import spoon.reflect.declaration.CtConstructor;
@@ -38,11 +39,8 @@ public void process(CtField<?> ctField) {
                 }
 
                 // check if the field is written to in constructors, which is fine if it does not have an explicit value
-                boolean hasWriteInConstructor = SpoonUtil.hasAnyUses(
-                    ctField,
-                    ctElement -> ctElement instanceof CtFieldWrite<?> ctFieldWrite
-                        && ctFieldWrite.getParent(CtConstructor.class) != null
-                );
+                boolean hasWriteInConstructor = UsesFinder.ofVariableWrite(ctField)
+                    .hasAnyMatch(ctFieldWrite -> ctFieldWrite.getParent(CtConstructor.class) != null);
 
                 // we need to check if the field is explicitly initialized, because this is not allowed:
                 //

autograder-core/src/main/java/de/firemage/autograder/core/check/general/ForToForEachLoop.java

@@ -8,6 +8,7 @@
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.uses.UsesFinder;
 import spoon.processing.AbstractProcessor;
 import spoon.reflect.code.CtArrayRead;
 import spoon.reflect.code.CtBodyHolder;
@@ -64,20 +65,15 @@ public class ForToForEachLoop extends IntegratedCheck {
     public static Optional<CtVariable<?>> getForEachLoopVariable(
         CtBodyHolder ctBodyHolder,
         ForLoopRange forLoopRange,
-        Function<CtVariableAccess<?>, Optional<CtVariableAccess<?>>> getPotentialLoopVariableAccess
+        Function<? super CtVariableAccess<?>, Optional<CtVariableAccess<?>>> getPotentialLoopVariableAccess
     ) {
         CtVariable<?> loopVariable = forLoopRange.loopVariable().getDeclaration();
 
-        // if a ForLoopRange exists, it is guranteed that the variable is incremented by 1 for each step
+        // if a ForLoopRange exists, it is guaranteed that the variable is incremented by 1 for each step
 
         CtVariable<?> ctVariable = null;
         CtVariableAccess<?> expectedAccess = null;
-        for (CtElement use : SpoonUtil.findUsesIn(loopVariable, ctBodyHolder.getBody())) {
-            if (!(use instanceof CtVariableAccess<?> ctVariableAccess)) {
-                throw new IllegalStateException(
-                    "SpoonUtil.findUsesIn returned non-variable access for '%s' as input".formatted(loopVariable));
-            }
-
+        for (CtVariableAccess<?> ctVariableAccess : UsesFinder.of(loopVariable).in(ctBodyHolder.getBody()).all()) {
             // We need to get the variable on which the access is performed (e.g. in a[i] we need to get a)
             CtVariableAccess<?> potentialLoopVariableAccess = getPotentialLoopVariableAccess.apply(ctVariableAccess)
                 .orElse(null);

autograder-core/src/main/java/de/firemage/autograder/core/check/general/LoopShouldBeFor.java

@@ -6,6 +6,7 @@
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.uses.UsesFinder;
 import spoon.processing.AbstractProcessor;
 import spoon.reflect.code.BinaryOperatorKind;
 import spoon.reflect.code.CtAssignment;
@@ -102,8 +103,7 @@ private static LoopSuggestion getCounter(CtLoop ctLoop) {
         CtStatement finalLastStatement = lastStatement;
         boolean isUpdatedMultipleTimes = statements.stream()
             .filter(statement -> statement != finalLastStatement)
-            .anyMatch(
-                statement -> SpoonUtil.hasAnyUsesIn(ctLocalVariable, statement, ctElement -> ctElement instanceof CtVariableWrite<?>));
+            .anyMatch(statement -> UsesFinder.ofVariableWrite(ctLocalVariable).in(statement).hasAny());
 
         if (isUpdatedMultipleTimes) {
             return null;

autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseDifferentVisibility.java

@@ -6,6 +6,7 @@
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
+import de.firemage.autograder.core.integrated.uses.UsesFinder;
 import spoon.processing.AbstractProcessor;
 import spoon.reflect.CtModel;
 import spoon.reflect.declaration.CtElement;
@@ -59,7 +60,7 @@ public String toString() {
     private static Visibility getVisibility(CtTypeMember ctTypeMember) {
         CtModel ctModel = ctTypeMember.getFactory().getModel();
 
-        List<CtElement> references = SpoonUtil.findUsesOf(ctTypeMember);
+        List<CtElement> references = UsesFinder.of(ctTypeMember).all();
 
         CtElement commonParent = SpoonUtil.findCommonParent(ctTypeMember, references);
         CtType<?> declaringType = ctTypeMember.getDeclaringType();
@@ -124,7 +125,7 @@ public void process(CtTypeMember ctTypeMember) {
                         .getFields()
                         .stream()
                         // filter out the field itself and those that do not reference the field
-                        .filter(field -> field != ctField && !SpoonUtil.findUsesIn(ctField, field).isEmpty())
+                        .filter(field -> field != ctField && SpoonUtil.hasAnyUsesIn(ctField, field))
                         .map(UseDifferentVisibility::getVisibility)
                         .max(Visibility::compareTo);