From 72ff6b6e4f568090d937a29435836d56fa1cf1b7 Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Fri, 19 Aug 2022 13:25:17 +0200 Subject: [PATCH 1/9] Fix repair for S2164: Math should not be performed on floats --- .../processor/MathOnFloatProcessor.java | 63 +++++++++++++------ .../java/sorald/processor/ProcessorTest.java | 2 +- .../S2164_MathOnFloat/MathOnFloat.java | 6 +- .../MathOnFloat.java.expected | 28 +++++++++ 4 files changed, 77 insertions(+), 22 deletions(-) create mode 100644 sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java.expected diff --git a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java index 7a9959a09..cc3e7460d 100644 --- a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java +++ b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java @@ -2,41 +2,68 @@ 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; + CtElement parentOfCandidate = candidate.getParent(); + + if (parentOfCandidate instanceof CtTypedElement) { + 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 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()); + } + if (!binaryOperator.getType().equals(getFactory().Type().doublePrimitiveType())) { + binaryOperator + .getRightHandOperand() + .setTypeCasts(List.of(getFactory().Type().doublePrimitiveType())); + binaryOperator.setType(getFactory().Type().doublePrimitiveType()); + } + } + } } private boolean isArithmeticOperation(CtBinaryOperator ctBinaryOperator) { diff --git a/sorald/src/test/java/sorald/processor/ProcessorTest.java b/sorald/src/test/java/sorald/processor/ProcessorTest.java index 26b940e91..8fb27bbba 100644 --- a/sorald/src/test/java/sorald/processor/ProcessorTest.java +++ b/sorald/src/test/java/sorald/processor/ProcessorTest.java @@ -40,7 +40,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. 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..0c33da210 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 @@ -6,22 +6,22 @@ class MathOnFloat { 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 } } 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..f2a45a858 --- /dev/null +++ b/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java.expected @@ -0,0 +1,28 @@ + +// Test for rule s2164 + +class MathOnFloat { + + // 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); + } + +} From 79cc78f82ece14cf21eb944e4f0b76c42442b721 Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Fri, 19 Aug 2022 13:29:28 +0200 Subject: [PATCH 2/9] Update generated field --- sorald/src/main/java/sorald/sonar/SonarProcessorRepository.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) { From 6a2987ad8c4048beab41155dd6bdc1b39e22def6 Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Fri, 19 Aug 2022 13:35:48 +0200 Subject: [PATCH 3/9] Update repair description --- .../sorald/processor/MathOnFloatProcessor.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) 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. From 429bedd1ddde654ed5888cfc5676923787fc047d Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Fri, 19 Aug 2022 13:50:36 +0200 Subject: [PATCH 4/9] Verify repair for field --- .../processor_test_files/S2164_MathOnFloat/MathOnFloat.java | 4 ++++ .../S2164_MathOnFloat/MathOnFloat.java.expected | 4 ++++ 2 files changed, 8 insertions(+) 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 0c33da210..024026550 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,6 +2,10 @@ 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; 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 index f2a45a858..3202cb1d8 100644 --- 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 @@ -3,6 +3,10 @@ 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; From 8f4ce5c052b7537ce43ed898e69701835a389413 Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Fri, 19 Aug 2022 13:56:33 +0200 Subject: [PATCH 5/9] Improve coverage --- .../main/java/sorald/processor/MathOnFloatProcessor.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java index cc3e7460d..1b5b9dd0f 100644 --- a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java +++ b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java @@ -56,12 +56,8 @@ protected void repairInternal(CtBinaryOperator element) { */ binaryOperator.setType(getFactory().Type().doublePrimitiveType()); } - if (!binaryOperator.getType().equals(getFactory().Type().doublePrimitiveType())) { - binaryOperator - .getRightHandOperand() - .setTypeCasts(List.of(getFactory().Type().doublePrimitiveType())); - binaryOperator.setType(getFactory().Type().doublePrimitiveType()); - } + // We do not need to cast the type of the right hand operand as it is already a + // double } } } From 901c897c712c36ac4a856fc351a1d9df19e222a1 Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Mon, 22 Aug 2022 14:39:15 +0200 Subject: [PATCH 6/9] tests: add configuration to verify that MathOnFloatProcessor does skip repairs for Incomplete cases (#868) * Add test to verify that INCOMPLETE processor skip repairs * Assert that the repaired file compiles * Add docstrings * Add information about `INCOMPLETE` processor tests * Strengthen test --- docs/CONTRIBUTING.md | 9 ++++ .../java/sorald/processor/ProcessorTest.java | 49 ++++++++++++++++++- .../sorald/processor/ProcessorTestHelper.java | 9 ++-- .../INCOMPLETE_DoNotCastFloat.java | 5 ++ 4 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/INCOMPLETE_DoNotCastFloat.java 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/test/java/sorald/processor/ProcessorTest.java b/sorald/src/test/java/sorald/processor/ProcessorTest.java index 8fb27bbba..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; @@ -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; +} From 7468bbdb448d751095fab68b0644989d936021b9 Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Mon, 22 Aug 2022 14:51:26 +0200 Subject: [PATCH 7/9] Assume that the parent of CtBinaryOperator will always be a CtTypedElement --- .../java/sorald/processor/MathOnFloatProcessor.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java index 1b5b9dd0f..1afcb3415 100644 --- a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java +++ b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java @@ -20,12 +20,10 @@ public class MathOnFloatProcessor extends SoraldAbstractProcessor) { - if (((CtTypedElement) parentOfCandidate) - .getType() - .equals(getFactory().Type().floatPrimitiveType())) { - return false; - } + if (((CtTypedElement) parentOfCandidate) + .getType() + .equals(getFactory().Type().floatPrimitiveType())) { + return false; } return isArithmeticOperation(candidate) From 8d25a9caff589c5d7c7bbcc075ca3e8690dca179 Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Mon, 22 Aug 2022 18:13:25 +0200 Subject: [PATCH 8/9] Remove redundant methods --- .../processor/MathOnFloatProcessor.java | 45 +++---------------- .../S2164_MathOnFloat/MathOnFloat.java | 2 + .../MathOnFloat.java.expected | 4 +- 3 files changed, 10 insertions(+), 41 deletions(-) diff --git a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java index 1afcb3415..28c67c6bb 100644 --- a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java +++ b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java @@ -1,10 +1,8 @@ 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.declaration.CtElement; import spoon.reflect.declaration.CtTypedElement; @@ -20,15 +18,9 @@ public class MathOnFloatProcessor extends SoraldAbstractProcessor) parentOfCandidate) + return !((CtTypedElement) parentOfCandidate) .getType() - .equals(getFactory().Type().floatPrimitiveType())) { - return false; - } - - return isArithmeticOperation(candidate) - && isOperationBetweenFloats(candidate) - && !withinStringConcatenation(candidate); + .equals(getFactory().Type().floatPrimitiveType()); } @Override @@ -42,7 +34,7 @@ protected void repairInternal(CtBinaryOperator element) { if (binaryOperator.getRightHandOperand() instanceof CtBinaryOperator) { repairInternal((CtBinaryOperator) binaryOperator.getRightHandOperand()); } else { - if (!binaryOperator.getType().equals(getFactory().Type().doublePrimitiveType())) { + if (isOperationBetweenFloats(binaryOperator)) { binaryOperator .getLeftHandOperand() .setTypeCasts(List.of(getFactory().Type().doublePrimitiveType())); @@ -60,41 +52,14 @@ protected void repairInternal(CtBinaryOperator element) { } } - 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; - } - 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/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java b/sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java index 024026550..a330feb85 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 @@ -21,6 +21,8 @@ void myMethod() { double d7 = a + a / b; // Noncompliant + double d8 = a + b + e * pi; + int i = 16777216; int j = 1; int k = i + j; 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 index 3202cb1d8..0bd199e99 100644 --- 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 @@ -16,12 +16,14 @@ class MathOnFloat { double d2 = ((double) (a)) - b; double d3 = ((double) (a)) * b; double d4 = ((double) (a)) / b; - double d5 = ((double) (((double) (a)) / b)) + 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; From a0768e2864f87e821183e2456621f1d839dc732a Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Mon, 22 Aug 2022 18:16:08 +0200 Subject: [PATCH 9/9] Simplify canRepairInternal --- .../src/main/java/sorald/processor/MathOnFloatProcessor.java | 5 +---- .../processor_test_files/S2164_MathOnFloat/MathOnFloat.java | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java index 28c67c6bb..72b643f78 100644 --- a/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java +++ b/sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java @@ -4,7 +4,6 @@ import sorald.annotations.IncompleteProcessor; import sorald.annotations.ProcessorAnnotation; import spoon.reflect.code.CtBinaryOperator; -import spoon.reflect.declaration.CtElement; import spoon.reflect.declaration.CtTypedElement; import spoon.reflect.visitor.filter.TypeFilter; @@ -16,9 +15,7 @@ public class MathOnFloatProcessor extends SoraldAbstractProcessor) parentOfCandidate) + return !((CtTypedElement) candidate.getParent()) .getType() .equals(getFactory().Type().floatPrimitiveType()); } 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 a330feb85..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 @@ -21,7 +21,7 @@ void myMethod() { double d7 = a + a / b; // Noncompliant - double d8 = a + b + e * pi; + double d8 = a + b + e * pi; // Noncompliant int i = 16777216; int j = 1;