detect redundant assignments #362

Feuermagier · Mar 18, 2024 · c10212b · c10212b
1 parent f4f5c81
commit c10212b
Show file tree

Hide file tree

Showing 8 changed files with 203 additions and 15 deletions.
diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java b/autograder-core/src/main/java/de/firemage/autograder/core/ProblemType.java
@@ -9,6 +9,7 @@ public enum ProblemType {
     OPTIONAL_ARGUMENT,
     AVOID_LABELS,
     SIMPLIFY_ARRAYS_FILL,
+    REDUNDANT_ASSIGNMENT,
     AVOID_SHADOWING,
     COLLECTIONS_N_COPIES,
     DO_NOT_USE_SYSTEM_EXIT,

diff --git a/...-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantAssignment.java b/...-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantAssignment.java
@@ -0,0 +1,72 @@
+package de.firemage.autograder.core.check.complexity;
+
+import de.firemage.autograder.core.LocalizedMessage;
+import de.firemage.autograder.core.ProblemType;
+import de.firemage.autograder.core.check.ExecutableCheck;
+import de.firemage.autograder.core.check.unnecessary.UnusedCodeElementCheck;
+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.CtAssignment;
+import spoon.reflect.code.CtLocalVariable;
+import spoon.reflect.code.CtLoop;
+import spoon.reflect.code.CtStatement;
+import spoon.reflect.code.CtStatementList;
+import spoon.reflect.code.CtVariableRead;
+import spoon.reflect.code.CtVariableWrite;
+import spoon.reflect.declaration.CtMethod;
+import spoon.reflect.reference.CtLocalVariableReference;
+
+
+import java.util.List;
+import java.util.Map;
+
+@ExecutableCheck(reportedProblems = { ProblemType.REDUNDANT_ASSIGNMENT })
+public class RedundantAssignment extends IntegratedCheck {
+    @Override
+    protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) {
+        staticAnalysis.processWith(new AbstractProcessor<CtAssignment<?, ?>>() {
+            @Override
+            public void process(CtAssignment<?, ?> ctAssignment) {
+                if (ctAssignment.isImplicit()
+                    || !ctAssignment.getPosition().isValidPosition()
+                    || !(ctAssignment.getAssigned() instanceof CtVariableWrite<?> ctVariableWrite)
+                    || !(ctVariableWrite.getVariable() instanceof CtLocalVariableReference<?> ctLocalVariableReference)) {
+                    return;
+                }
+
+                if (!(ctAssignment.getParent() instanceof CtStatementList)
+                    || ctLocalVariableReference.getDeclaration() == null
+                    || !(ctAssignment.getParent().getParent() instanceof CtMethod<?>)) {
+                    return;
+                }
+
+                List<CtStatement> followingStatements = SpoonUtil.getNextStatements(ctAssignment);
+
+                CtLocalVariable<?> ctLocalVariable = ctLocalVariableReference.getDeclaration();
+
+                if (UnusedCodeElementCheck.isUnused(ctLocalVariable, true)) {
+                    return;
+                }
+
+
+                if (followingStatements.stream().noneMatch(statement -> SpoonUtil.hasAnyUsesIn(
+                    ctLocalVariable,
+                    statement,
+                    element -> element instanceof CtVariableRead<?>
+                ))) {
+                    addLocalProblem(
+                        ctAssignment,
+                        new LocalizedMessage(
+                            "redundant-assignment",
+                            Map.of("variable", ctLocalVariable.getSimpleName())
+                        ),
+                        ProblemType.REDUNDANT_ASSIGNMENT
+                    );
+                }
+            }
+        });
+    }
+}
diff --git a/...e/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java b/...e/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java
@@ -26,19 +26,15 @@
 
 @ExecutableCheck(reportedProblems = { ProblemType.UNUSED_CODE_ELEMENT, ProblemType.UNUSED_CODE_ELEMENT_PRIVATE })
 public class UnusedCodeElementCheck extends IntegratedCheck {
-    private void checkUnused(StaticAnalysis staticAnalysis, CtNamedElement ctElement) {
-        if (ctElement.isImplicit() || !ctElement.getPosition().isValidPosition()) {
-            return;
-        }
-
+    public static boolean isUnused(CtNamedElement ctElement, boolean hasMainMethod) {
         // ignore exception constructors and params in those constructors
         CtConstructor<?> parentConstructor = ctElement.getParent(CtConstructor.class);
         if (parentConstructor == null && ctElement instanceof CtConstructor<?> ctConstructor) {
             parentConstructor = ctConstructor;
         }
 
         if (parentConstructor != null && SpoonUtil.isSubtypeOf(parentConstructor.getType(), java.lang.Throwable.class)) {
-            return;
+            return false;
         }
 
         Predicate<CtElement> shouldVisit = element -> true;
@@ -47,30 +43,43 @@ private void checkUnused(StaticAnalysis staticAnalysis, CtNamedElement ctElement
             shouldVisit = shouldVisit.and(Predicate.not(reference -> method.equals(reference.getParent(CtMethod.class))));
         }
 
-        ProblemType problemType = ProblemType.UNUSED_CODE_ELEMENT;
-        if (ctElement instanceof CtModifiable ctModifiable && ctModifiable.isPrivate()) {
-            problemType = ProblemType.UNUSED_CODE_ELEMENT_PRIVATE;
-        }
-
         if (!SpoonUtil.hasAnyUses(ctElement, shouldVisit)) {
             // do not report unused elements if there is no main method in the model and the element is accessible
             // (i.e. not private)
             if (ctElement instanceof CtParameter<?>
                 && ctElement.getParent() instanceof CtTypeMember ctTypeMember
                 && !ctTypeMember.getDeclaringType().isPrivate()
                 // check if there is no main method in the model
-                && !staticAnalysis.getCodeModel().hasMainMethod()) {
-                return;
+                && !hasMainMethod) {
+                return false;
             }
 
             if (ctElement instanceof CtModifiable ctModifiable
                 && !ctModifiable.isPrivate()
                 && ctModifiable instanceof CtTypeMember ctTypeMember
                 && !ctTypeMember.getDeclaringType().isPrivate()
                 // check if there is no main method in the model
-                && !staticAnalysis.getCodeModel().hasMainMethod()
+                && !hasMainMethod
             ) {
-                return;
+                return false;
+            }
+
+            return true;
+        }
+
+        return false;
+    }
+
+    private void checkUnused(StaticAnalysis staticAnalysis, CtNamedElement ctElement) {
+        if (ctElement.isImplicit() || !ctElement.getPosition().isValidPosition()) {
+            return;
+        }
+
+
+        if (isUnused(ctElement, staticAnalysis.getCodeModel().hasMainMethod())) {
+            ProblemType problemType = ProblemType.UNUSED_CODE_ELEMENT;
+            if (ctElement instanceof CtModifiable ctModifiable && ctModifiable.isPrivate()) {
+                problemType = ProblemType.UNUSED_CODE_ELEMENT_PRIVATE;
             }
 
             String name = ctElement.getSimpleName();

diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java
@@ -1327,6 +1327,12 @@ public static boolean hasAnyUses(CtElement ctElement, Predicate<? super CtElemen
             .first(CtElement.class) != null;
     }
 
+    public static boolean hasAnyUsesIn(CtElement ctElement, CtElement toSearchIn, Predicate<? super CtElement> predicate) {
+        return toSearchIn
+            .filterChildren(new CompositeFilter<>(FilteringOperator.INTERSECTION, predicate::test, new UsesFilter(ctElement)))
+            .first(CtElement.class) != null;
+    }
+
     public static List<CtElement> findUses(CtElement ctElement) {
         return new ArrayList<>(ctElement.getFactory().getModel().getElements(new UsesFilter(ctElement)));
     }

diff --git a/autograder-core/src/main/resources/strings.de.ftl b/autograder-core/src/main/resources/strings.de.ftl
@@ -121,6 +121,8 @@ try-catch-complexity = Die Komplexität von try-catch-Blöcken sollte möglichst
 
 redundant-else = Die 'else' ist unnötig. Schreibe '{$expected}' statt '{$given}'.
 
+redundant-assignment = Der Variable '{$variable}' wird hier ein Wert zugewiesen, der dann nie verwendet wird. Deswegen kann die Zuweisung entfernt werden.
+
 # Debug
 assert-used-exp = Assertions lassen das gesamte Programm abstürzen, wenn sie false sind.
     Außerdem können sie deaktiviert werden, weswegen man sich nicht darauf verlassen kann,

diff --git a/autograder-core/src/main/resources/strings.en.ftl b/autograder-core/src/main/resources/strings.en.ftl
@@ -124,6 +124,8 @@ try-catch-complexity = The complexity of try-catch-blocks should be kept low. Th
 
 redundant-else = The 'else' is redundant. Write '{$expected}' instead of '{$given}'.
 
+redundant-assignment = The variable '{$variable}' is assigned a value that is never read, therefore the assignment can be removed.
+
 # Debug
 assert-used-exp = Assertions crash the entire program if they evaluate to false.
               Also they can be disabled, so never rely on them to e.g. check user input.

diff --git a/...e/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantAssignment.java b/...e/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantAssignment.java
@@ -0,0 +1,95 @@
+package de.firemage.autograder.core.check.complexity;
+
+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 TestRedundantAssignment extends AbstractCheckTest {
+    private static final List<ProblemType> PROBLEM_TYPES = List.of(ProblemType.REDUNDANT_ASSIGNMENT);
+
+    void assertEqualsRedundant(Problem problem, String variable) {
+        assertEquals(
+            this.linter.translateMessage(new LocalizedMessage(
+                "redundant-assignment",
+                Map.of("variable", variable)
+            )),
+            this.linter.translateMessage(problem.getExplanation())
+        );
+    }
+
+    @Test
+    void testMotivation() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "Test",
+            """
+                public class Test {
+                    public void test() {
+                        int a = 5;
+                        
+                        System.out.println(a);
+                        
+                        a = 3;
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+
+        assertEqualsRedundant(problems.next(), "a");
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testInLoop() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "Test",
+            """
+                public class Test {
+                    public void test() {
+                        int a = 5;
+                        for (int i = 0; i < 5; i++) {
+                            System.out.println(a);
+                            a = i * 2;
+                        }
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testAllowedAssignment() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "Test",
+            """
+                public class Test {
+                    public void test() {
+                        int a = 5;
+                        a = 3;
+
+                        System.out.println(a);
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+}
diff --git a/sample_config.yaml b/sample_config.yaml
@@ -156,3 +156,4 @@
 - COLLECTION_ADD_ALL
 - USE_MODULO_OPERATOR
 - NUMBER_FORMAT_EXCEPTION_IGNORED
+- REDUNDANT_ASSIGNMENT