modernize field should be final check and fix #401

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

@@ -0,0 +1,74 @@
+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.dynamic.DynamicAnalysis;
+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.CtFieldWrite;
+import spoon.reflect.declaration.CtConstructor;
+import spoon.reflect.declaration.CtField;
+
+import java.util.Map;
+
+@ExecutableCheck(reportedProblems = {ProblemType.FIELD_SHOULD_BE_FINAL})
+public class FieldShouldBeFinal extends IntegratedCheck {
+    @Override
+    protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) {
+        staticAnalysis.processWith(new AbstractProcessor<CtField<?>>() {
+            @Override
+            public void process(CtField<?> ctField) {
+                if (ctField.isImplicit() || !ctField.getPosition().isValidPosition() || ctField.isFinal()) {
+                    return;
+                }
+
+                // check if the field is written to outside of constructors
+                boolean hasWrite = SpoonUtil.hasAnyUses(
+                    ctField,
+                    ctElement -> ctElement instanceof CtFieldWrite<?> ctFieldWrite
+                        && ctFieldWrite.getParent(CtConstructor.class) == null
+                );
+
+                // 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 = SpoonUtil.hasAnyUses(
+                    ctField,
+                    ctElement -> ctElement instanceof CtFieldWrite<?> ctFieldWrite
+                        && ctFieldWrite.getParent(CtConstructor.class) != null
+                );
+
+                // 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) {
+                    addLocalProblem(
+                        ctField,
+                        new LocalizedMessage("field-should-be-final", Map.of("name", ctField.getSimpleName())),
+                        ProblemType.FIELD_SHOULD_BE_FINAL
+                    );
+                }
+            }
+        });
+    }
+}

autograder-core/src/main/resources/strings.de.ftl

@@ -161,7 +161,7 @@ missing-override = '{$name}' sollte eine '@Override'-Annotation haben, siehe htt
 
 system-specific-linebreak = Systemabhängiger Zeilenumbruch (\n) benutzt. Besser ist System.lineSeparator() oder (falls es sich um einen format-String handelt) '%n'.
 
-field-final-exp = Das Attribut '{$name}' sollte final sein
+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}'
 

autograder-core/src/main/resources/strings.en.ftl

@@ -163,7 +163,7 @@ missing-override = '{$name}' should have an '@Override'-annotation, see https://
 
 system-specific-linebreak = Always use system-independent line breaks such as the value obtained from System.lineSeparator() or %n in format strings
 
-field-final-exp = The attribute '{$name}' should be final
+field-should-be-final = The attribute '{$name}' should be final.
 
 string-cmp-exp = Use the equals method: '{$lhs}.equals({$rhs})' instead of '{$lhs} == {$rhs}'
 

autograder-core/src/test/java/de/firemage/autograder/core/check/general/TestFieldShouldBeFinal.java

@@ -0,0 +1,224 @@
+package de.firemage.autograder.core.check.general;
+
+import de.firemage.autograder.core.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.compiler.JavaVersion;
+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 TestFieldShouldBeFinal extends AbstractCheckTest {
+    private static final List<ProblemType> PROBLEM_TYPES = List.of(ProblemType.FIELD_SHOULD_BE_FINAL);
+
+    void assertFinal(Problem problem, String name) {
+        assertEquals(
+            this.linter.translateMessage(new LocalizedMessage(
+                "field-should-be-final",
+                Map.of("name", name)
+            )),
+            this.linter.translateMessage(problem.getExplanation())
+        );
+    }
+
+    @Test
+    void testMultipleAssignments() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "Test",
+            """
+                class Test {
+                    String value;
+
+                    Test() {
+                        this.value = "Hello World";
+                    }
+
+                    void foo() {
+                        this.value = "Value";
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testAssignedOnlyInConstructor() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "Test",
+            """
+                class Test {
+                    String value;
+
+                    Test() {
+                        this.value = "Hello World";
+                    }
+
+                    @Override
+                    public String toString() {
+                        return this.value;
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        assertFinal(problems.next(), "value");
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testInitializedAndAssignedInConstructor() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "Test",
+            """
+                class Test {
+                    int value = 1;
+
+                    Test() {
+                        this.value = 2;
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testAlreadyFinal() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "Test",
+            """
+                class Test {
+                    final int value;
+
+                    Test() {
+                        this.value = 2;
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testPartialConstructorInit() throws LinterException, IOException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "A",
+            """
+                abstract class A {
+                    private String value = null;
+
+                    protected A() {
+
+                    }
+
+                    protected A(String value) {
+                        this.value = value;
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testStaticVariableInlineAssignedInConstructor() throws LinterException, IOException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "User",
+            """
+                class User {
+                    static int nextId;
+                    final int id;
+
+                    User() {
+                        this.id = nextId++;
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testStaticVariableOnlyInitialized() throws LinterException, IOException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "User",
+            """
+                class User {
+                    static int nextId = 1;
+                    final int id;
+
+                    User() {
+                        this.id = nextId;
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        assertFinal(problems.next(), "nextId");
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testStaticVariableAssignedConstantInConstructor() throws LinterException, IOException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "User",
+            """
+                class User {
+                    static int nextId;
+                    final int id;
+
+                    User() {
+                        nextId = 1;
+                        this.id = nextId;
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testStaticVariableDynamicAssignment() throws LinterException, IOException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "User",
+            """
+                class User {
+                    static int nextId;
+                    final int id;
+
+                    User(int next) {
+                        nextId = next;
+                        this.id = next;
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+}