rewrite final check #508

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/integrated/CtElementStream.java

@@ -164,6 +164,10 @@ public boolean hasAny() {
         return baseStream.findAny().isPresent();
     }
 
+    public boolean hasAnyMatch(Predicate<? super T> predicate) {
+        return new CtElementStream<>(baseStream.filter(predicate)).hasAny();
+    }
+
     /**
      * Checks whether the stream contains no elements.
      * @return

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

@@ -14,6 +14,7 @@
 import spoon.reflect.code.BinaryOperatorKind;
 import spoon.reflect.code.CtBinaryOperator;
 import spoon.reflect.code.CtBlock;
+import spoon.reflect.code.CtBodyHolder;
 import spoon.reflect.code.CtBreak;
 import spoon.reflect.code.CtCase;
 import spoon.reflect.code.CtComment;
@@ -620,6 +621,19 @@ public static List<CtStatement> getEffectiveStatements(CtStatement ctStatement)
         return getEffectiveStatements(List.of(ctStatement));
     }
 
+    public static List<CtStatement> getEffectiveStatementsOf(CtBodyHolder ctBodyHolder) {
+        if (ctBodyHolder == null) {
+            return List.of();
+        }
+
+        CtStatement body = ctBodyHolder.getBody();
+        if (body == null) {
+            return List.of();
+        }
+
+        return getEffectiveStatements(body);
+    }
+
     public static <T> CtExpression<T> resolveCtExpression(CtExpression<T> ctExpression) {
         if (ctExpression == null) return null;
 
@@ -722,6 +736,17 @@ public static boolean isSignatureEqualTo(
         return true;
     }
 
+    private record SubtypeFilter(CtTypeReference<?> ctTypeReference) implements Filter<CtType<?>> {
+        @Override
+        public boolean matches(CtType<?> element) {
+            return element != this.ctTypeReference.getTypeDeclaration() && element.isSubtypeOf(this.ctTypeReference);
+        }
+    }
+
+    public static boolean hasSubtype(CtType<?> ctType) {
+        return ctType.getFactory().getModel().filterChildren(new SubtypeFilter(ctType.getReference())).first() != null;
+    }
+
     /**
      * Creates a static invocation of the given method on the given target type.
      *

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

@@ -86,6 +86,16 @@ public static CtElementStream<CtVariableAccess<?>> variableUses(CtVariable<?> va
         return CtElementStream.of(UsesFinder.getFor(variable).scanner.variableUses.getOrDefault(variable, List.of())).assumeElementType();
     }
 
+    @SuppressWarnings("unchecked")
+    public static CtElementStream<CtVariableWrite<?>> variableWrites(CtVariable<?> variable) {
+        return (CtElementStream<CtVariableWrite<?>>) (Object) CtElementStream.of(UsesFinder.getFor(variable).scanner.variableUses.getOrDefault(variable, List.of())).assumeElementType().ofType(CtVariableWrite.class);
+    }
+
+    @SuppressWarnings("unchecked")
+    public static CtElementStream<CtVariableRead<?>> variableReads(CtVariable<?> variable) {
+        return (CtElementStream<CtVariableRead<?>>) (Object) CtElementStream.of(UsesFinder.getFor(variable).scanner.variableUses.getOrDefault(variable, List.of())).assumeElementType().ofType(CtVariableRead.class);
+    }
+
     public static CtElementStream<CtTypeParameterReference> typeParameterUses(CtTypeParameter typeParameter) {
         return CtElementStream.of(UsesFinder.getFor(typeParameter).scanner.typeParameterUses.getOrDefault(typeParameter, List.of()));
     }