improve suggestion for redundant negation

Feuermagier · Apr 6, 2024 · 3dd9653 · 3dd9653
1 parent 122f967
commit 3dd9653
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 16 deletions.
diff --git a/...re/src/main/java/de/firemage/autograder/core/check/complexity/RedundantNegationCheck.java b/...re/src/main/java/de/firemage/autograder/core/check/complexity/RedundantNegationCheck.java
@@ -8,7 +8,10 @@
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
 
+import de.firemage.autograder.core.integrated.evaluator.Evaluator;
+import de.firemage.autograder.core.integrated.evaluator.fold.Fold;
 import spoon.processing.AbstractProcessor;
+import spoon.reflect.code.CtBinaryOperator;
 import spoon.reflect.code.CtExpression;
 import spoon.reflect.code.CtUnaryOperator;
 import spoon.reflect.code.UnaryOperatorKind;
@@ -17,33 +20,42 @@
 
 @ExecutableCheck(reportedProblems = { ProblemType.REDUNDANT_NEGATION })
 public class RedundantNegationCheck extends IntegratedCheck {
+    private record NegationFolder() implements Fold {
+        @Override
+        @SuppressWarnings("unchecked")
+        public <T> CtExpression<T> foldCtUnaryOperator(CtUnaryOperator<T> ctUnaryOperator) {
+            // only check negations !(operand)
+            if (ctUnaryOperator.getKind() != UnaryOperatorKind.NOT) {
+                return ctUnaryOperator;
+            }
+
+            CtExpression<?> operand = ctUnaryOperator.getOperand();
+
+            // this negates the operand and optimizes it if possible
+            return (CtExpression<T>) SpoonUtil.negate(operand);
+        }
+    }
+
     @Override
     protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) {
-        staticAnalysis.processWith(new AbstractProcessor<CtUnaryOperator<?>>() {
+        staticAnalysis.processWith(new AbstractProcessor<CtExpression<?>>() {
             @Override
-            public void process(CtUnaryOperator<?> ctUnaryOperator) {
-                if (ctUnaryOperator.isImplicit() || !ctUnaryOperator.getPosition().isValidPosition()) {
+            public void process(CtExpression<?> ctExpression) {
+                if (ctExpression.isImplicit()
+                    || !ctExpression.getPosition().isValidPosition()
+                    || ctExpression.getParent(CtExpression.class) != null) {
                     return;
                 }
 
-                // only check negations !(operand)
-                if (ctUnaryOperator.getKind() != UnaryOperatorKind.NOT) {
-                    return;
-                }
-
-                CtExpression<?> operand = ctUnaryOperator.getOperand();
-
                 // this negates the operand and optimizes it if possible
-                CtExpression<?> negated = SpoonUtil.negate(operand);
+                CtExpression<?> negated = new Evaluator(new NegationFolder()).evaluate(ctExpression);
                 // if they are equal, the negation is not redundant
-                if (ctUnaryOperator.equals(negated)) {
+                if (ctExpression.equals(negated)) {
                     return;
                 }
 
-                // TODO: do the same as in RepeatedMathOperationCheck
-
                 addLocalProblem(
-                    ctUnaryOperator,
+                    ctExpression,
                     new LocalizedMessage(
                         "common-reimplementation",
                         Map.of("suggestion", negated.prettyprint())

diff --git a/...rc/test/java/de/firemage/autograder/core/check/complexity/TestRedundantNegationCheck.java b/...rc/test/java/de/firemage/autograder/core/check/complexity/TestRedundantNegationCheck.java
@@ -53,7 +53,7 @@ void testRedundantNegation(String expression, String arguments, String expected)
         ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
             JavaVersion.JAVA_17,
             "Test",
-            "public class Test { public void foo(%s) { System.out.println(%s); } }".formatted(
+            "public class Test { public void foo(%s) { var value = %s; } }".formatted(
                 arguments,
                 expression
             )
@@ -84,4 +84,27 @@ public void foo(List<Object> list) {
 
         problems.assertExhausted();
     }
+
+    @Test
+    void testNestedUnnecessaryNegation() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "Test",
+            """
+                import java.util.List;
+
+                public class Test {
+                    public void foo(int a, int b, int c) {
+                        if (a == 0 || a == 1 && !(b == 0 || b == 1)) {
+                            System.out.println("Hello");
+                        }
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        assertEqualsRedundant(problems.next(), "(a == 0) || ((a == 1) && ((b != 0) && (b != 1)))");
+
+        problems.assertExhausted();
+    }
 }