Merge pull request #522 from Luro02/main

Implement some things

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

@@ -2,6 +2,7 @@
 
 import de.firemage.autograder.core.file.SourceInfo;
 import de.firemage.autograder.core.file.SourcePath;
+import spoon.reflect.code.CtAbstractSwitch;
 import spoon.reflect.code.CtLoop;
 import spoon.reflect.cu.SourcePosition;
 import spoon.reflect.declaration.CtElement;
@@ -33,7 +34,7 @@ public static CodePosition fromSourcePosition(
         // Instead of highlighting all lines of a class or method, only highlight the first line.
         //
         // Someone might explicitly specify a source position, in which case it will differ from the element's position.
-        if ((ctElement instanceof CtType<?> || ctElement instanceof CtMethod<?> || ctElement instanceof CtLoop) && ctElement.getPosition().equals(sourcePosition)) {
+        if ((ctElement instanceof CtType<?> || ctElement instanceof CtMethod<?> || ctElement instanceof CtLoop || ctElement instanceof CtAbstractSwitch<?>) && ctElement.getPosition().equals(sourcePosition)) {
             return new CodePosition(
                 sourceInfo,
                 relativePath,

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

@@ -6,7 +6,6 @@ public enum ProblemType {
     AVOID_INNER_CLASSES,
     USE_STRING_FORMATTED,
     OPTIONAL_TRI_STATE,
-    OPTIONAL_ARGUMENT,
     AVOID_LABELS,
     SIMPLIFY_ARRAYS_FILL,
     REDUNDANT_ASSIGNMENT,
@@ -65,9 +64,7 @@ public enum ProblemType {
     RUNTIME_EXCEPTION_CAUGHT,
     OBJECTS_COMPARED_VIA_TO_STRING,
     FIELD_SHOULD_BE_CONSTANT,
-    CONSTANT_IN_INTERFACE,
     DO_NOT_HAVE_CONSTANTS_CLASS,
-    STATIC_METHOD_IN_INTERFACE,
     DO_NOT_USE_RAW_TYPES,
     DUPLICATE_CODE,
     TOO_FEW_PACKAGES,

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

@@ -76,7 +76,7 @@ private static <T> boolean isOrderedCollection(CtTypeReference<T> ctTypeReferenc
 
     private void reportProblem(CtVariable<?> ctVariable, List<? extends CtExpression<?>> addedValues) {
         String values = "List.of(%s)".formatted(addedValues.stream()
-            .map(CtElement::prettyprint)
+            .map(CtElement::toString)
             .collect(Collectors.joining(", "))
         );
 

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

@@ -7,12 +7,11 @@
 import de.firemage.autograder.core.integrated.StaticAnalysis;
 import spoon.processing.AbstractProcessor;
 import spoon.reflect.declaration.CtMethod;
-import spoon.reflect.declaration.CtParameter;
 import spoon.reflect.declaration.CtTypedElement;
 import spoon.reflect.declaration.CtVariable;
 import spoon.reflect.reference.CtTypeReference;
 
-@ExecutableCheck(reportedProblems = { ProblemType.OPTIONAL_TRI_STATE, ProblemType.OPTIONAL_ARGUMENT })
+@ExecutableCheck(reportedProblems = { ProblemType.OPTIONAL_TRI_STATE })
 public class OptionalBadPractices extends IntegratedCheck {
     private void checkCtVariable(CtTypedElement<?> ctTypedElement) {
         CtTypeReference<?> ctTypeReference = ctTypedElement.getType();
@@ -24,15 +23,6 @@ private void checkCtVariable(CtTypedElement<?> ctTypedElement) {
             return;
         }
 
-        // Check if the variable is a function parameter:
-        if (ctTypedElement instanceof CtParameter<?>) {
-            this.addLocalProblem(
-                ctTypeReference,
-                new LocalizedMessage("optional-argument"),
-                ProblemType.OPTIONAL_ARGUMENT
-            );
-        }
-
         // Check if the Optional is used as a tri-state:
         boolean isTriState =
             ctTypeReference.getActualTypeArguments().stream()

autograder-core/src/main/java/de/firemage/autograder/core/check/comment/ThrowsJavadocCheck.java

@@ -6,6 +6,8 @@
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
+import spoon.reflect.code.CtAbstractSwitch;
+import spoon.reflect.code.CtCase;
 import spoon.reflect.code.CtConstructorCall;
 import spoon.reflect.code.CtJavaDoc;
 import spoon.reflect.code.CtJavaDocTag;
@@ -39,6 +41,12 @@ private void checkCtExecutable(CtExecutable<?> ctExecutable) {
         List<CtThrow> ctThrows = ctExecutable.filterChildren(CtThrow.class::isInstance)
                 .map(CtThrow.class::cast).list();
         for (CtThrow ctThrow : ctThrows) {
+            CtCase<?> ctParentCase = ctThrow.getParent(CtCase.class);
+            // skip default cases in switch statements
+            if (ctParentCase != null && ctParentCase.getCaseExpressions().isEmpty()) {
+                continue;
+            }
+
             if (ctThrow.getThrownExpression() instanceof CtConstructorCall<?> ctConstructorCall) {
                 String name = ctConstructorCall.getType().getSimpleName();
 

autograder-core/src/main/java/de/firemage/autograder/core/check/exceptions/ExceptionMessageCheck.java

@@ -4,41 +4,57 @@
 import de.firemage.autograder.core.ProblemType;
 import de.firemage.autograder.core.check.ExecutableCheck;
 
-import de.firemage.autograder.core.integrated.ExceptionUtil;
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
 import spoon.processing.AbstractProcessor;
 import spoon.reflect.code.CtCase;
 import spoon.reflect.code.CtConstructorCall;
 import spoon.reflect.code.CtExpression;
+import spoon.reflect.code.CtInvocation;
 import spoon.reflect.code.CtThrow;
 import spoon.reflect.declaration.CtConstructor;
 import spoon.reflect.declaration.CtElement;
 
-import java.util.List;
-
 @ExecutableCheck(reportedProblems = ProblemType.EXCEPTION_WITHOUT_MESSAGE)
 public class ExceptionMessageCheck extends IntegratedCheck {
     private static boolean isExceptionWithoutMessage(CtExpression<?> expression) {
-        return expression instanceof CtConstructorCall<?> ctorCall
-            && ExceptionUtil.isRuntimeException(ctorCall.getType())
-            && !hasMessage(ctorCall.getArguments());
-    }
+        if (!(expression instanceof CtConstructorCall<?> ctConstructorCall) ||
+            !(SpoonUtil.isSubtypeOf(expression.getType(), java.lang.Exception.class))) {
+            return false;
+        }
 
-    private static boolean hasMessage(List<? extends CtExpression<?>> arguments) {
-        if (arguments.isEmpty()) {
+        // check if the invoked constructor passes a message to the exception
+        if (ctConstructorCall.getExecutable().getExecutableDeclaration() instanceof CtConstructor<?> ctConstructor
+            && ctConstructor.getBody().filterChildren(ctElement -> ctElement instanceof CtInvocation<?> ctInvocation
+                // we just check if there is any invocation with a message, because this is easier and might be enough
+                // for most cases.
+                //
+                // this way will not result in false positives, only in false negatives
+                && ctInvocation.getExecutable().isConstructor()
+                && hasMessage(ctInvocation.getArguments())
+            ).first() != null) {
             return false;
         }
 
-        CtExpression<?> ctExpression = arguments.get(0);
-        String literal = SpoonUtil.tryGetStringLiteral(ctExpression).orElse(null);
+        return !hasMessage(ctConstructorCall.getArguments());
+    }
 
-        if (literal != null) {
-            return !literal.isBlank();
+    private static boolean hasMessage(Iterable<? extends CtExpression<?>> arguments) {
+        for (CtExpression<?> ctExpression : arguments) {
+            // consider a passed throwable as having message
+            if (SpoonUtil.isSubtypeOf(ctExpression.getType(), java.lang.Throwable.class)) {
+                return true;
+            }
+
+            String literal = SpoonUtil.tryGetStringLiteral(ctExpression).orElse(null);
+
+            if (literal != null) {
+                return !literal.isBlank();
+            }
         }
 
-        return true;
+        return false;
     }
 
     private static boolean isInAllowedContext(CtElement ctElement) {

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

@@ -10,6 +10,8 @@
 import spoon.reflect.path.CtRole;
 import spoon.reflect.reference.CtTypeReference;
 
+import java.util.Optional;
+
 @ExecutableCheck(reportedProblems = { ProblemType.DO_NOT_USE_RAW_TYPES })
 public class DoNotUseRawTypes extends IntegratedCheck {
     private boolean isRawType(CtTypeReference<?> ctTypeReference) {
@@ -48,4 +50,9 @@ public void process(CtTypeReference<?> ctTypeReference) {
             }
         });
     }
+
+    @Override
+    public Optional<Integer> maximumProblems() {
+        return Optional.of(4);
+    }
 }

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

@@ -2,12 +2,15 @@
 
 import de.firemage.autograder.core.LocalizedMessage;
 import de.firemage.autograder.core.ProblemType;
-import de.firemage.autograder.core.integrated.UsesFinder;
 import de.firemage.autograder.core.check.ExecutableCheck;
 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.UsesFinder;
 import spoon.processing.AbstractProcessor;
-import spoon.reflect.code.CtFieldWrite;
+import spoon.reflect.code.CtAssignment;
+import spoon.reflect.code.CtStatement;
+import spoon.reflect.declaration.CtClass;
 import spoon.reflect.declaration.CtConstructor;
 import spoon.reflect.declaration.CtField;
 
@@ -16,6 +19,100 @@
 
 @ExecutableCheck(reportedProblems = {ProblemType.FIELD_SHOULD_BE_FINAL})
 public class FieldShouldBeFinal extends IntegratedCheck {
+    /**
+     * Checks if a field can be final.
+     * <p>
+     * A returned value of false does not guarantee that the field can not be final.
+     *
+     * @param ctField the field to check
+     * @return true if the field can be final, false otherwise
+     * @param <T> the type of the field
+     */
+    private static <T> boolean canBeFinal(CtField<T> ctField) {
+        if (ctField.isFinal()) {
+            return true;
+        }
+
+        // if the field has any write that is not in a constructor, it can not be final
+        if (!(ctField.getDeclaringType() instanceof CtClass<?> ctClass)
+            || UsesFinder.variableWrites(ctField).hasAnyMatch(ctFieldWrite -> ctFieldWrite.getParent(CtConstructor.class) == null)) {
+            return false;
+        }
+
+        if (ctField.isProtected() && SpoonUtil.hasSubtype(ctClass)) {
+            return false;
+        }
+
+        // we need to check if the field is explicitly initialized, because this is not allowed:
+        //
+        // final String a = "hello";
+        // a = "world"; // error
+        //
+        // but this is allowed:
+        //
+        // final String a;
+        // a = "hello";
+        boolean hasExplicitValue = ctField.getDefaultExpression() != null && !ctField.getDefaultExpression().isImplicit();
+
+        // Static fields and final is complicated.
+        //
+        // The check must not be able to recognize any edge case. It is enough when it is able
+        // to recognize the most important cases.
+        //
+        // For static fields, this would be an explicit value and no write in the whole program.
+        if (ctField.isStatic()) {
+            return hasExplicitValue && !UsesFinder.variableWrites(ctField).hasAny();
+        }
+
+        // for a field to be final, it must be written to exactly once in each code path of each constructor.
+        //
+        // if the field is not written to, an implicit value is used, which is fine
+        // if the field is written to more than once, it can not be final
+        int allowedWrites = 1;
+        if (hasExplicitValue) {
+            // if the field has an explicit value, it must not be written to more than once to be final
+            allowedWrites = 0;
+        }
+
+        // NOTE: I did not implement the code path checking, e.g.
+        //
+        // if (condition) {
+        //   this.field = 1;
+        // } else {
+        //   this.field = 2;
+        // }
+        //
+        // that is complicated to implement correctly.
+
+        for (CtConstructor<?> ctConstructor : ctClass.getConstructors()) {
+            // an implicit constructor is the default one
+            // -> that constructor does not write to any of the fields
+            if (ctConstructor.isImplicit() && allowedWrites != 0) {
+                return false;
+            }
+
+            int mainPathWrites = 0;
+            int otherPathWrites = 0;
+            for (CtStatement ctStatement : SpoonUtil.getEffectiveStatementsOf(ctConstructor)) {
+                if (ctStatement instanceof CtAssignment<?,?> ctAssignment && UsesFinder.variableWrites(ctField).nestedIn(ctAssignment).hasAny()) {
+                    mainPathWrites += 1;
+                } else if (UsesFinder.variableWrites(ctField).nestedIn(ctStatement).hasAny()) {
+                    otherPathWrites += 1;
+                }
+            }
+
+            // we have the main path, e.g. the constructor body where each statement is guaranteed to be executed
+            // and the other path, statements that are only executed under certain conditions
+
+            if (mainPathWrites != allowedWrites || otherPathWrites != 0) {
+                return false;
+            }
+        }
+
+        // we reached here -> the field has exactly one write in each constructor
+        return true;
+    }
+
     @Override
     protected void check(StaticAnalysis staticAnalysis) {
         staticAnalysis.processWith(new AbstractProcessor<CtField<?>>() {
@@ -25,42 +122,13 @@ public void process(CtField<?> ctField) {
                     return;
                 }
 
-                // check if the field is written to outside of constructors
-                boolean hasWrite = UsesFinder.variableUses(ctField)
-                        .ofType(CtFieldWrite.class)
-                        .notNestedIn(CtConstructor.class).hasAny();
-
-                // a field can not be final if it is written to from outside of constructors
-                if (hasWrite) {
-                    return;
-                }
-
-                // check if the field is written to in constructors, which is fine if it does not have an explicit value
-                boolean hasWriteInConstructor = UsesFinder.variableUses(ctField)
-                        .ofType(CtFieldWrite.class)
-                        .nestedIn(CtConstructor.class).hasAny();
-
-                // we need to check if the field is explicitly initialized, because this is not allowed:
-                //
-                // final String a = "hello";
-                // a = "world"; // error
-                //
-                // but this is allowed:
-                //
-                // final String a;
-                // a = "hello";
-                boolean hasExplicitValue = ctField.getDefaultExpression() != null && !ctField.getDefaultExpression().isImplicit();
-
-                // a static field can only be final if it has an explicit value and is never written to
-                if (ctField.isStatic() && (hasWriteInConstructor || !hasExplicitValue)) {
-                    return;
-                }
-
-                // a field can be final if it is only written to in the constructor or it is never assigned a new value and has an explicit value
-                if (!hasWriteInConstructor || !hasExplicitValue) {
+                if (canBeFinal(ctField)) {
                     addLocalProblem(
                         ctField,
-                        new LocalizedMessage("field-should-be-final", Map.of("name", ctField.getSimpleName())),
+                        new LocalizedMessage(
+                            "field-should-be-final",
+                            Map.of("name", ctField.getSimpleName())
+                        ),
                         ProblemType.FIELD_SHOULD_BE_FINAL
                     );
                 }

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

@@ -11,6 +11,7 @@
 import spoon.reflect.reference.CtTypeReference;
 
 import java.util.Map;
+import java.util.Optional;
 
 @ExecutableCheck(reportedProblems = { ProblemType.OBJECT_DATATYPE })
 public class ObjectDatatype extends IntegratedCheck {
@@ -45,4 +46,9 @@ public void process(CtVariable<?> ctVariable) {
             }
         });
     }
+
+    @Override
+    public Optional<Integer> maximumProblems() {
+        return Optional.of(4);
+    }
 }