Merge pull request #376 from Luro02/main

Fix some bugs

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

@@ -14,6 +14,7 @@ public enum ProblemType {
     EQUALS_HASHCODE_COMPARABLE_CONTRACT,
     UNCHECKED_TYPE_CAST,
     TOO_MANY_EXCEPTIONS,
+    CONSTANT_NAME_CONTAINS_VALUE,
     DEPRECATED_COLLECTION_USED,
     COLLECTION_IS_EMPTY_REIMPLEMENTED,
     STRING_IS_EMPTY_REIMPLEMENTED,

autograder-core/src/main/java/de/firemage/autograder/core/check/naming/ConstantsHaveDescriptiveNamesCheck.java

@@ -12,14 +12,13 @@
 import spoon.reflect.code.CtLiteral;
 import spoon.reflect.code.CtTypeAccess;
 import spoon.reflect.declaration.CtField;
-import spoon.reflect.declaration.CtVariable;
 
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Stream;
 
-@ExecutableCheck(reportedProblems = {ProblemType.MEANINGLESS_CONSTANT_NAME})
+@ExecutableCheck(reportedProblems = {ProblemType.MEANINGLESS_CONSTANT_NAME, ProblemType.CONSTANT_NAME_CONTAINS_VALUE})
 public class ConstantsHaveDescriptiveNamesCheck extends IntegratedCheck {
     private static final List<String> NUMBER_PRE_SUFFIXES =
             List.of("index", "number", "value", "argument", "element", "param", "parameter", "arg", "group", "constant", "value_of");
@@ -145,20 +144,6 @@ private static boolean containsValueInName(String name, CtLiteral<?> value) {
         return lowerCaseName.contains(valueString);
     }
 
-    private void reportProblem(String key, CtVariable<?> ctVariable) {
-        this.addLocalProblem(
-            ctVariable,
-            new LocalizedMessage(
-                key,
-                Map.of(
-                    "name", ctVariable.getSimpleName(),
-                    "value", ctVariable.getDefaultExpression().prettyprint()
-                )
-            ),
-            ProblemType.MEANINGLESS_CONSTANT_NAME
-        );
-    }
-
     @Override
     protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) {
         staticAnalysis.processWith(new AbstractProcessor<CtField<?>>() {
@@ -189,9 +174,29 @@ public void process(CtField<?> field) {
 
                 if (literal.getValue() instanceof Integer v1 && isNonDescriptiveIntegerName(fieldName, v1)
                     || literal.getValue() instanceof String v2 && isNonDescriptiveStringName(fieldName, v2)) {
-                    reportProblem("constants-name-exp", field);
+                    addLocalProblem(
+                        field,
+                        new LocalizedMessage(
+                            "constants-name-exp",
+                            Map.of(
+                                "name", field.getSimpleName(),
+                                "value", field.getDefaultExpression().prettyprint()
+                            )
+                        ),
+                        ProblemType.MEANINGLESS_CONSTANT_NAME
+                    );
                 } else if (containsValueInName(fieldName, literal)) {
-                    reportProblem("constants-name-exp-value", field);
+                    addLocalProblem(
+                        field,
+                        new LocalizedMessage(
+                            "constants-name-exp-value",
+                            Map.of(
+                                "name", field.getSimpleName(),
+                                "value", field.getDefaultExpression().prettyprint()
+                            )
+                        ),
+                        ProblemType.CONSTANT_NAME_CONTAINS_VALUE
+                    );
                 }
             }
         });

autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java

@@ -50,6 +50,7 @@
 import spoon.reflect.factory.Factory;
 import spoon.reflect.factory.TypeFactory;
 import spoon.reflect.reference.CtExecutableReference;
+import spoon.reflect.reference.CtFieldReference;
 import spoon.reflect.reference.CtReference;
 import spoon.reflect.reference.CtTypeParameterReference;
 import spoon.reflect.reference.CtTypeReference;
@@ -61,9 +62,13 @@
 import spoon.reflect.visitor.filter.SameFilter;
 import spoon.reflect.visitor.filter.VariableAccessFilter;
 
+import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
+import java.util.Deque;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -809,14 +814,11 @@ public static boolean isStaticCallTo(CtInvocation<?> invocation, String typeName
     }
 
     public static boolean isEffectivelyFinal(CtVariable<?> ctVariable) {
-        CtModel ctModel = ctVariable.getFactory().getModel();
-
         if (ctVariable.getModifiers().contains(ModifierKind.FINAL)) {
             return true;
         }
 
-        List<CtVariableAccess<?>> variableUses = ctModel
-            .getElements(new BetterVariableAccessFilter<>(ctVariable));
+        List<? extends CtVariableAccess<?>> variableUses = SpoonUtil.findUsesOf(ctVariable);
 
         return variableUses.isEmpty() || variableUses.stream().noneMatch(variableAccess -> variableAccess instanceof CtVariableWrite<?>);
     }
@@ -829,23 +831,59 @@ public static <T> Optional<CtExpression<T>> getEffectivelyFinalExpression(CtVari
         return Optional.ofNullable(ctVariable.getDefaultExpression());
     }
 
+    /**
+     * Checks if the given element is guaranteed to be immutable.
+     * <p>
+     * Note that when this method returns {@code false}, the type might still be immutable.
+     *
+     * @param ctTypeReference the type to check
+     * @return true if the given element is guaranteed to be immutable, false otherwise
+     * @param <T> the type of the element
+     */
     public static <T> boolean isImmutable(CtTypeReference<T> ctTypeReference) {
-        CtType<T> ctType = ctTypeReference.getTypeDeclaration();
-        if (ctType == null) {
-            return false;
-        }
+        Deque<CtTypeReference<?>> queue = new ArrayDeque<>(Collections.singletonList(ctTypeReference));
+        Collection<CtType<?>> visited = new HashSet<>();
 
-        if (ctTypeReference.unbox().isPrimitive() || SpoonUtil.isTypeEqualTo(ctTypeReference, java.lang.String.class)) {
-            return true;
-        }
+        while (!queue.isEmpty()) {
+            CtType<?> ctType = queue.removeFirst().getTypeDeclaration();
 
-        if (ctType.isShadow()) {
-            return false;
+            // if the type is not in the classpath, null is returned
+            // in those cases, assume that the type is not immutable
+            if (ctType == null) {
+                return false;
+            }
+
+            // skip types that have been checked (those are guaranteed to be immutable)
+            if (visited.contains(ctType)) {
+                continue;
+            }
+
+            // primitive types and strings are immutable as well:
+            if (ctType.getReference().unbox().isPrimitive()
+                || SpoonUtil.isTypeEqualTo(ctType.getReference(), java.lang.String.class)) {
+                continue;
+            }
+
+            // types that are not in the classpath like java.util.ArrayList are shadow types.
+            // the source code for those is missing, so it is impossible to check if they are immutable.
+            // => assume they are not immutable
+            if (ctType.isShadow()) {
+                return false;
+            }
+
+            // for a type to be immutable, all of its fields must be final and immutable as well:
+            for (CtFieldReference<?> ctFieldReference : ctType.getAllFields()) {
+                if (!SpoonUtil.isEffectivelyFinal(ctFieldReference.getFieldDeclaration())) {
+                    return false;
+                }
+
+                queue.add(ctFieldReference.getType());
+            }
+
+            visited.add(ctType);
         }
 
-        return ctType.getAllFields().stream()
-            .allMatch(ctFieldReference -> SpoonUtil.isEffectivelyFinal(ctFieldReference.getFieldDeclaration())
-                && SpoonUtil.isImmutable(ctFieldReference.getType()));
+        return true;
     }
 
     public static boolean isTypeEqualTo(CtTypeReference<?> ctType, Class<?>... expected) {
@@ -885,7 +923,7 @@ public static boolean isMainMethod(CtMethod<?> method) {
      * @param ctElement the element to get the parents of
      * @return an iterable over all parents, the given element is not included
      */
-    public static Iterable<CtElement> parents(CtElement ctElement) {
+    private static Iterable<CtElement> parents(CtElement ctElement) {
         return () -> new Iterator<>() {
             private CtElement current = ctElement;
 
@@ -942,10 +980,6 @@ public static boolean isInnerClass(CtTypeMember type) {
         return type.getDeclaringType() != null;
     }
 
-    public static boolean isInnerClass(CtTypeReference<?> ctTypeReference) {
-        return ctTypeReference.getDeclaringType() != null;
-    }
-
     /**
      * Checks if the given method is overriding another method.
      * <p>
@@ -1036,7 +1070,6 @@ public boolean matches(CtAbstractInvocation<?> invocation) {
             CtExecutableReference<?> invocationExecutable = invocation.getExecutable();
             return invocationExecutable.equals(this.executable.getReference())
                 || this.executable.equals(invocationExecutable.getExecutableDeclaration())
-                // TODO: consider removing this?
                 || invocationExecutable.isOverriding(this.executable.getReference());
         }
     }
@@ -1047,7 +1080,6 @@ public boolean matches(CtExecutableReferenceExpression<?, ?> expression) {
             CtExecutableReference<?> invocationExecutable = expression.getExecutable();
             return invocationExecutable.equals(this.executable.getReference())
                 || this.executable.equals(invocationExecutable.getExecutableDeclaration())
-                // TODO: consider removing this?
                 || invocationExecutable.isOverriding(this.executable.getReference());
         }
     }
@@ -1124,9 +1156,12 @@ private static Filter<CtElement> buildExecutableFilter(CtExecutable<?> ctExecuta
     // - CtVariable<?>
     // - CtExecutable<?>
     // - CtTypeMember
-
-    public static <T> List<CtElement> findUsesOf(CtVariable<T> ctVariable) {
-        return SpoonUtil.findUses(ctVariable);
+    @SuppressWarnings("unchecked")
+    public static <T> List<CtVariableAccess<T>> findUsesOf(CtVariable<T> ctVariable) {
+        return SpoonUtil.findUses(ctVariable)
+            .stream()
+            .map(ctElement -> (CtVariableAccess<T>) ctElement)
+            .collect(Collectors.toList());
     }
 
     public static List<CtElement> findUsesOf(CtTypeMember ctTypeMember) {
@@ -1137,7 +1172,6 @@ public static <T> List<CtElement> findUsesOf(CtExecutable<T> ctExecutable) {
         return SpoonUtil.findUses(ctExecutable);
     }
 
-
     public static List<CtElement> findUses(CtElement ctElement) {
         return new ArrayList<>(ctElement.getFactory().getModel().getElements(new UsesFilter(ctElement)));
     }

autograder-core/src/test/java/de/firemage/autograder/core/check/api/TestCommonReimplementation.java

@@ -342,6 +342,37 @@ public static Cell[] createCells(int n) {
         problems.assertExhausted();
     }
 
+    @Test
+    void testArraysFillRecursiveType() throws LinterException, IOException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings(
+            JavaVersion.JAVA_17,
+            Map.ofEntries(
+                Map.entry(
+                    "Main",
+                    """
+                    class PlayingFieldEntry {
+                        static final PlayingFieldEntry FREE = new PlayingFieldEntry();
+                    }
+
+                    public class Main {
+                        public static void main(String[] args) {
+                            PlayingFieldEntry[] field = new PlayingFieldEntry[1];
+
+                            for (int i = 0; i < field.length; i++) {
+                                field[i] = PlayingFieldEntry.FREE;
+                            }
+                        }
+                    }
+                    """
+                )
+            )
+        ), List.of(ProblemType.COMMON_REIMPLEMENTATION_ARRAYS_FILL));
+
+        assertEqualsReimplementation(problems.next(), "Arrays.fill(field, 0, field.length, PlayingFieldEntry.FREE)");
+
+        problems.assertExhausted();
+    }
+
     @Test
     void testEnumValuesAddAllUnorderedSet() throws LinterException, IOException {
         ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(

sample_config.yaml

@@ -147,3 +147,4 @@
 - OBJECT_DATATYPE
 - COMMON_REIMPLEMENTATION_SQRT
 - COMMON_REIMPLEMENTATION_HYPOT
+- CONSTANT_NAME_CONTAINS_VALUE