fix: mark MathOnFloatProcessor as incomplete

docs/CONTRIBUTING.md

@@ -237,3 +237,12 @@ for an example.
 > capture formatting that's of absolute relevance to Sorald itself and its
 > transformations. Overspecifying exact matches leads to unnecessary work down
 > the line.
+
+#### Testing partially fixable rules
+
+Some processors cannot repair all cases of a violation, and they are called
+`IncompleteProcessor`. To keep track of which cases we do not attempt to
+repair, one can create test files prefixed with `INCOMPLETE`. The test suite
+will ensure that the violation persists after Sorald is run. See
+[INCOMPLETE_DoNotCastFloat.java](sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/INCOMPLETE_DoNotCastFloat.java)
+for an example.

sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java

@@ -2,41 +2,62 @@
 
 import java.util.List;
 import sorald.Constants;
+import sorald.annotations.IncompleteProcessor;
 import sorald.annotations.ProcessorAnnotation;
 import spoon.reflect.code.BinaryOperatorKind;
 import spoon.reflect.code.CtBinaryOperator;
-import spoon.reflect.code.CtCodeSnippetExpression;
 import spoon.reflect.declaration.CtElement;
+import spoon.reflect.declaration.CtTypedElement;
 import spoon.reflect.visitor.filter.TypeFilter;
 
+@IncompleteProcessor(
+        description =
+                "does not cast the operands to double when the expected type of the result is float.")
 @ProcessorAnnotation(key = "S2164", description = "Math should not be performed on floats")
 public class MathOnFloatProcessor extends SoraldAbstractProcessor<CtBinaryOperator> {
 
     @Override
     protected boolean canRepairInternal(CtBinaryOperator candidate) {
-        List<CtBinaryOperator> binaryOperatorChildren =
-                candidate.getElements(new TypeFilter<>(CtBinaryOperator.class));
-        if (binaryOperatorChildren.size()
-                == 1) { // in a nested binary operator expression, only one will be processed.
-            if (isArithmeticOperation(candidate)
-                    && isOperationBetweenFloats(candidate)
-                    && !withinStringConcatenation(candidate)) {
-                return true;
-            }
+        CtElement parentOfCandidate = candidate.getParent();
+
+        if (((CtTypedElement<?>) parentOfCandidate)
+                .getType()
+                .equals(getFactory().Type().floatPrimitiveType())) {
+            return false;
         }
-        return false;
+
+        return isArithmeticOperation(candidate)
+                && isOperationBetweenFloats(candidate)
+                && !withinStringConcatenation(candidate);
     }
 
     @Override
     protected void repairInternal(CtBinaryOperator element) {
-        CtCodeSnippetExpression newLeftHandOperand =
-                element.getFactory()
-                        .createCodeSnippetExpression("(double) " + element.getLeftHandOperand());
-        element.setLeftHandOperand(newLeftHandOperand);
-        CtCodeSnippetExpression newRightHandOperand =
-                element.getFactory()
-                        .createCodeSnippetExpression("(double) " + element.getRightHandOperand());
-        element.setRightHandOperand(newRightHandOperand);
+        List<CtBinaryOperator> binaryOperatorChildren =
+                element.getElements(new TypeFilter<>(CtBinaryOperator.class));
+        for (CtBinaryOperator binaryOperator : binaryOperatorChildren) {
+            if (binaryOperator.getLeftHandOperand() instanceof CtBinaryOperator<?>) {
+                repairInternal((CtBinaryOperator<?>) binaryOperator.getLeftHandOperand());
+            }
+            if (binaryOperator.getRightHandOperand() instanceof CtBinaryOperator<?>) {
+                repairInternal((CtBinaryOperator<?>) binaryOperator.getRightHandOperand());
+            } else {
+                if (!binaryOperator.getType().equals(getFactory().Type().doublePrimitiveType())) {
+                    binaryOperator
+                            .getLeftHandOperand()
+                            .setTypeCasts(List.of(getFactory().Type().doublePrimitiveType()));
+
+                    /**
+                     * We also set the type so that the other operand is not explicitly cast as JVM
+                     * implicitly does that For example, `(double) a + (double) b` is equivalent to
+                     * `(double) a + b`. Thus, we provide a cleaner repair.
+                     */
+                    binaryOperator.setType(getFactory().Type().doublePrimitiveType());
+                }
+                // We do not need to cast the type of the right hand operand as it is already a
+                // double
+            }
+        }
     }
 
     private boolean isArithmeticOperation(CtBinaryOperator ctBinaryOperator) {

sorald/src/main/java/sorald/processor/MathOnFloatProcessor.md

@@ -1,9 +1,23 @@
-In arithmetic expressions between two `float`s, both left and right operands are cast to `double`.
+In arithmetic expressions between two `float`s, one of the operands are cast to `double`.
 
 Example:
+```diff
+        float a = 16777216.0f;
+        float b = 1.0f;
+-       double d1 = a + b;
++       double d1 = (double) a + b;
+```
+
+Note that this processor is incomplete as it does not perform the following
+repair even though it is recommended by SonarSource in their
+[documentation](https://rules.sonarsource.com/java/RSPEC-2164):
 ```diff
          float a = 16777216.0f;
          float b = 1.0f;
 -        float c = a + b; // Noncompliant, yields 1.6777216E7 not 1.6777217E7
 +        float c = (double) a + (double) b;
 ```
+
+The reason we do not perform this repair is that it produces a non-compilable
+code. See [#570](https://github.com/SpoonLabs/sorald/issues/570) for more
+details.

sorald/src/main/java/sorald/sonar/SonarProcessorRepository.java

@@ -48,7 +48,7 @@ public class SonarProcessorRepository implements ProcessorRepository {
 
     // GENERATED FIELD
     public static final String RULE_DESCRIPTIONS =
-            "S1068: Unused \"private\" fields should be removed\nS1118: Utility classes should not have public constructors\n\t(incomplete: Only handles implicit public constructor)\nS1132: Strings literals should be placed on the left side when checking for equality\nS1155: Collection.isEmpty() should be used to test for emptiness\nS1217: \"Thread.run()\" should not be called directly\nS1444: \"public static\" fields should be constant\n\t(incomplete: does not fix variable naming)\nS1481: Unused local variables should be removed\nS1596: \"Collections.EMPTY_LIST\", \"EMPTY_MAP\", and \"EMPTY_SET\" should not be used\nS1656: Variables should not be self-assigned\nS1854: Unused assignments should be removed\nS1860: Synchronization should not be based on Strings or boxed primitives\nS1948: Fields in a \"Serializable\" class should either be transient or serializable\nS2057: Every class implementing Serializable should declare a static final serialVersionUID.\n\t(incomplete: This processor does not address the case where the class already has a serialVersionUID with a non long type.)\nS2095: Resources should be closed\nS2097: \"equals(Object obj)\" should test argument type\nS2111: \"BigDecimal(double)\" should not be used\nS2116: \"hashCode\" and \"toString\" should not be called on array instances\nS2142: \"InterruptedException\" should not be ignored\nS2164: Math should not be performed on floats\nS2167: \"compareTo\" should not return \"Integer.MIN_VALUE\"\nS2184: Math operands should be cast before assignment\nS2204: \".equals()\" should not be used to test the values of \"Atomic\" classes\nS2225: \"toString()\" and \"clone()\" methods should not return null\n\t(incomplete: does not fix null returning clone())\nS2272: \"Iterator.next()\" methods should throw \"NoSuchElementException\"\nS2755: XML parsers should not be vulnerable to XXE attacks\n\t(incomplete: This processor is a WIP and currently supports a subset of rule 2755. See Sorald\'s documentation for details.)\nS3032: JEE applications should not \"getClassLoader\"\nS3067: \"getClass\" should not be used for synchronization\nS3984: Exception should not be created without being thrown\nS4065: \"ThreadLocal.withInitial\" should be preferred\nS4973: Strings and Boxed types should be compared using \"equals()\"";
+            "S1068: Unused \"private\" fields should be removed\nS1118: Utility classes should not have public constructors\n\t(incomplete: Only handles implicit public constructor)\nS1132: Strings literals should be placed on the left side when checking for equality\nS1155: Collection.isEmpty() should be used to test for emptiness\nS1217: \"Thread.run()\" should not be called directly\nS1444: \"public static\" fields should be constant\n\t(incomplete: does not fix variable naming)\nS1481: Unused local variables should be removed\nS1596: \"Collections.EMPTY_LIST\", \"EMPTY_MAP\", and \"EMPTY_SET\" should not be used\nS1656: Variables should not be self-assigned\nS1854: Unused assignments should be removed\nS1860: Synchronization should not be based on Strings or boxed primitives\nS1948: Fields in a \"Serializable\" class should either be transient or serializable\nS2057: Every class implementing Serializable should declare a static final serialVersionUID.\n\t(incomplete: This processor does not address the case where the class already has a serialVersionUID with a non long type.)\nS2095: Resources should be closed\nS2097: \"equals(Object obj)\" should test argument type\nS2111: \"BigDecimal(double)\" should not be used\nS2116: \"hashCode\" and \"toString\" should not be called on array instances\nS2142: \"InterruptedException\" should not be ignored\nS2164: Math should not be performed on floats\n\t(incomplete: does not cast the operands to double when the expected type of the result is float.)\nS2167: \"compareTo\" should not return \"Integer.MIN_VALUE\"\nS2184: Math operands should be cast before assignment\nS2204: \".equals()\" should not be used to test the values of \"Atomic\" classes\nS2225: \"toString()\" and \"clone()\" methods should not return null\n\t(incomplete: does not fix null returning clone())\nS2272: \"Iterator.next()\" methods should throw \"NoSuchElementException\"\nS2755: XML parsers should not be vulnerable to XXE attacks\n\t(incomplete: This processor is a WIP and currently supports a subset of rule 2755. See Sorald\'s documentation for details.)\nS3032: JEE applications should not \"getClassLoader\"\nS3067: \"getClass\" should not be used for synchronization\nS3984: Exception should not be created without being thrown\nS4065: \"ThreadLocal.withInitial\" should be preferred\nS4973: Strings and Boxed types should be compared using \"equals()\"";
 
     @Override
     public Class<? extends SoraldAbstractProcessor<?>> getProcessor(String key) {

sorald/src/test/java/sorald/processor/ProcessorTest.java

@@ -31,6 +31,7 @@
 import sorald.TestHelper;
 import sorald.event.StatsMetadataKeys;
 import sorald.rule.Rule;
+import sorald.sonar.ProjectScanner;
 import sorald.sonar.SonarRule;
 import spoon.Launcher;
 import spoon.reflect.CtModel;
@@ -40,7 +41,7 @@
 public class ProcessorTest {
 
     private static final Set<String> FILES_THAT_DONT_COMPILE_AFTER_REPAIR =
-            Set.of("MathOnFloat.java", "CastArithmeticOperand.java");
+            Set.of("CastArithmeticOperand.java");
 
     /**
      * Parameterized test that processes a single Java file at a time with a single processor.
@@ -75,7 +76,49 @@ private static class NonCompliantJavaFileProvider implements ArgumentsProvider {
         @Override
         public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext)
                 throws IOException {
-            return ProcessorTestHelper.getTestCasesInTemporaryDirectory().map(Arguments::of);
+            return ProcessorTestHelper.getTestCasesInTemporaryDirectory()
+                    .filter(
+                            testCase ->
+                                    !ProcessorTestHelper.hasCasesThatMakeProcessorIncomplete(
+                                            testCase))
+                    .map(Arguments::of);
+        }
+    }
+
+    /**
+     * Test cases that process non-repairable cases in partially fixable rules. This ensures that
+     * the violations exist even after the repair is performed.
+     */
+    @ParameterizedTest
+    @ArgumentsSource(IncompleteProcessorCaseFileProvider.class)
+    void testProcessNonRepairableCases(ProcessorTestHelper.ProcessorTestCase testCase) {
+        // act
+        var violationsBefore =
+                ProjectScanner.scanProject(
+                        testCase.nonCompliantFile,
+                        testCase.nonCompliantFile.getParentFile(),
+                        testCase.getRule());
+        ProcessorTestHelper.runSorald(testCase);
+
+        // assert
+        assertCompiles(testCase.repairedFilePath().toFile());
+
+        var violationsAfter =
+                ProjectScanner.scanProject(
+                        testCase.repairedFilePath().toFile(),
+                        testCase.repairedFilePath().toFile().getParentFile(),
+                        testCase.getRule());
+        assertThat(violationsBefore, equalTo(violationsAfter));
+    }
+
+    private static class IncompleteProcessorCaseFileProvider implements ArgumentsProvider {
+
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext)
+                throws Exception {
+            return ProcessorTestHelper.getTestCasesInTemporaryDirectory()
+                    .filter(ProcessorTestHelper::hasCasesThatMakeProcessorIncomplete)
+                    .map(Arguments::of);
         }
     }
 
@@ -232,6 +275,10 @@ private static class NonCompliantJavaFileWithExpectedProvider implements Argumen
         public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext)
                 throws IOException {
             return ProcessorTestHelper.getTestCasesInTemporaryDirectory()
+                    .filter(
+                            testCase ->
+                                    !ProcessorTestHelper.hasCasesThatMakeProcessorIncomplete(
+                                            testCase))
                     .filter(testCase -> testCase.expectedOutfile().isPresent())
                     .map(Arguments::of);
         }

sorald/src/test/java/sorald/processor/ProcessorTestHelper.java

@@ -153,6 +153,10 @@ public static boolean isStandaloneCompilableTestFile(File testFile) {
         return !testFile.getName().startsWith("NOCOMPILE");
     }
 
+    public static boolean hasCasesThatMakeProcessorIncomplete(ProcessorTestCase testCase) {
+        return testCase.nonCompliantFile.getName().startsWith("INCOMPLETE");
+    }
+
     /**
      * Return a stream of all valid test cases, based on the tests files in {@link
      * ProcessorTestHelper#TEST_FILES_ROOT}. The test case source files are put in a temporary
@@ -164,14 +168,13 @@ public static Stream<ProcessorTestCase> getTestCasesInTemporaryDirectory() throw
     }
 
     /** Run sorald on the given test case. */
-    public static void runSorald(ProcessorTestCase testCase, String... extraArgs) throws Exception {
+    public static void runSorald(ProcessorTestCase testCase, String... extraArgs) {
         Assertions.assertHasRuleViolation(testCase.nonCompliantFile, testCase.getRule());
         runSorald(testCase.nonCompliantFile, testCase.getRule(), extraArgs);
     }
 
     /** Run sorald on the given file with the given checkClass * */
-    public static void runSorald(File originaFilesPath, Rule rule, String... extraArgs)
-            throws Exception {
+    public static void runSorald(File originaFilesPath, Rule rule, String... extraArgs) {
         String originalFileAbspath = originaFilesPath.getAbsolutePath();
 
         boolean brokenWithSniper = BROKEN_WITH_SNIPER.contains(rule);

sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/INCOMPLETE_DoNotCastFloat.java

@@ -0,0 +1,5 @@
+class DoNoCastFloat {
+    float a = 16777216.0f;
+    float b = 1.0f;
+    final float c = a + b;
+}

sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java

@@ -2,26 +2,30 @@
 
 class MathOnFloat {
 
+    float e = 2.71f;
+    float pi = 3.14f;
+    double c = e * pi; // Noncompliant
+
     // Tests from https://github.com/SonarSource/sonar-java/blob/master/java-checks-test-sources/src/main/java/checks/MathOnFloatCheck.java
     void myMethod() {
         float a = 16777216.0f;
         float b = 1.0f;
-        float c = a + b; // Noncompliant {{Use a "double" or "BigDecimal" instead.}} yields 1.6777216E7 not 1.6777217E7
 
         double d1 = a + b; // Noncompliant ; addition is still between 2 floats
         double d2 = a - b; // Noncompliant
         double d3 = a * b; // Noncompliant
         double d4 = a / b; // Noncompliant
-        double d5 = a / b + b; // Noncompliant, only one issue should be reported
+        double d5 = a / b + b; // Noncompliant
 
         double d6 = a + d1;
 
+        double d7 = a + a / b; // Noncompliant
+
         int i = 16777216;
         int j = 1;
         int k = i + j;
         System.out.println("[Max time to retrieve connection:"+(a/1000f/1000f)+" ms.");
         System.out.println("[Max time to retrieve connection:"+a/1000f/1000f);
-        float foo = 12 + a/1000f/1000f; // Noncompliant
     }
 
 }

sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java.expected

@@ -0,0 +1,32 @@
+
+// Test for rule s2164
+
+class MathOnFloat {
+
+    float e = 2.71f;
+    float pi = 3.14f;
+    double c = (double) e * pi;
+
+    // Tests from https://github.com/SonarSource/sonar-java/blob/master/java-checks-test-sources/src/main/java/checks/MathOnFloatCheck.java
+    void myMethod() {
+        float a = 16777216.0f;
+        float b = 1.0f;
+
+        double d1 = ((double) (a)) + b;
+        double d2 = ((double) (a)) - b;
+        double d3 = ((double) (a)) * b;
+        double d4 = ((double) (a)) / b;
+        double d5 = ((double) (((double) (a)) / b)) + b;
+
+        double d6 = a + d1;
+
+        double d7 = a + ((double) (a)) / b;
+
+        int i = 16777216;
+        int j = 1;
+        int k = i + j;
+        System.out.println("[Max time to retrieve connection:"+(a/1000f/1000f)+" ms.");
+        System.out.println("[Max time to retrieve connection:"+a/1000f/1000f);
+    }
+
+}