diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 1a335f259..d4b188412 100644 --- a/docs/CONTRIBUTING.md +++ b/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. diff --git a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java index 7a9959a09..72b643f78 100644 --- a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java +++ b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java @@ -1,79 +1,62 @@ package sorald.processor; 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 { @Override protected boolean canRepairInternal(CtBinaryOperator candidate) { - List 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; - } - } - return false; + return !((CtTypedElement) candidate.getParent()) + .getType() + .equals(getFactory().Type().floatPrimitiveType()); } @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 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 (isOperationBetweenFloats(binaryOperator)) { + binaryOperator + .getLeftHandOperand() + .setTypeCasts(List.of(getFactory().Type().doublePrimitiveType())); - private boolean isArithmeticOperation(CtBinaryOperator ctBinaryOperator) { - return ctBinaryOperator.getKind().compareTo(BinaryOperatorKind.PLUS) == 0 - || ctBinaryOperator.getKind().compareTo(BinaryOperatorKind.MINUS) == 0 - || ctBinaryOperator.getKind().compareTo(BinaryOperatorKind.MUL) == 0 - || ctBinaryOperator.getKind().compareTo(BinaryOperatorKind.DIV) == 0; + /** + * 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 isOperationBetweenFloats(CtBinaryOperator ctBinaryOperator) { return ctBinaryOperator .getLeftHandOperand() .getType() - .getSimpleName() - .equals(Constants.FLOAT) + .equals(getFactory().Type().floatPrimitiveType()) && ctBinaryOperator .getRightHandOperand() .getType() - .getSimpleName() - .equals(Constants.FLOAT); - } - - private boolean withinStringConcatenation(CtBinaryOperator ctBinaryOperator) { - CtElement parent = ctBinaryOperator; - while (parent.getParent() instanceof CtBinaryOperator) { - parent = parent.getParent(); - } - return ((CtBinaryOperator) parent).getKind().compareTo(BinaryOperatorKind.PLUS) == 0 - && (((CtBinaryOperator) parent) - .getLeftHandOperand() - .getType() - .getQualifiedName() - .equals(Constants.STRING_QUALIFIED_NAME) - || ((CtBinaryOperator) parent) - .getRightHandOperand() - .getType() - .getQualifiedName() - .equals(Constants.STRING_QUALIFIED_NAME)); + .equals(getFactory().Type().floatPrimitiveType()); } } diff --git a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.md b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.md index 9f64de0a4..4b11cb1e4 100644 --- a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.md +++ b/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. diff --git a/sorald/src/main/java/sorald/sonar/SonarProcessorRepository.java b/sorald/src/main/java/sorald/sonar/SonarProcessorRepository.java index 36eba4547..7ce5c7ab7 100644 --- a/sorald/src/main/java/sorald/sonar/SonarProcessorRepository.java +++ b/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> getProcessor(String key) { diff --git a/sorald/src/test/java/sorald/processor/ProcessorTest.java b/sorald/src/test/java/sorald/processor/ProcessorTest.java index 26b940e91..263395f6c 100644 --- a/sorald/src/test/java/sorald/processor/ProcessorTest.java +++ b/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 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 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 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 provideArguments(ExtensionContext extensionContext) throws IOException { return ProcessorTestHelper.getTestCasesInTemporaryDirectory() + .filter( + testCase -> + !ProcessorTestHelper.hasCasesThatMakeProcessorIncomplete( + testCase)) .filter(testCase -> testCase.expectedOutfile().isPresent()) .map(Arguments::of); } diff --git a/sorald/src/test/java/sorald/processor/ProcessorTestHelper.java b/sorald/src/test/java/sorald/processor/ProcessorTestHelper.java index 2da3a9505..d91c1ee96 100644 --- a/sorald/src/test/java/sorald/processor/ProcessorTestHelper.java +++ b/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 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); diff --git a/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/INCOMPLETE_DoNotCastFloat.java b/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/INCOMPLETE_DoNotCastFloat.java new file mode 100644 index 000000000..428f2e6db --- /dev/null +++ b/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; +} diff --git a/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java b/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java index 3a613ebe6..92b6a8968 100644 --- a/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java +++ b/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java @@ -2,26 +2,32 @@ 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 + + double d8 = a + b + e * pi; // 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 } } diff --git a/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java.expected b/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java.expected new file mode 100644 index 000000000..0bd199e99 --- /dev/null +++ b/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java.expected @@ -0,0 +1,34 @@ + +// 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) (a)) / b)) + b; + + double d6 = a + d1; + + double d7 = a + ((double) (a)) / b; + + double d8 = (((double) (a)) + b) + (((double) (e)) * pi); + + 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); + } + +}