-
Notifications
You must be signed in to change notification settings - Fork 27
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: mark
MathOnFloatProcessor as incomplete (#866)
* Fix repair for S2164: Math should not be performed on floats * Update generated field * Update repair description * Verify repair for field * Improve coverage * 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 * Assume that the parent of CtBinaryOperator will always be a CtTypedElement * Remove redundant methods * Simplify canRepairInternal
- Loading branch information
1 parent
2d6b983
commit c04ae77
Showing
9 changed files
with
162 additions
and
61 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
85 changes: 34 additions & 51 deletions
85
sorald/src/main/java/sorald/processor/MathOnFloatProcessor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<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; | ||
| } | ||
| } | ||
| 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<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 (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()); | ||
| } | ||
| } |
16 changes: 15 additions & 1 deletion
16
sorald/src/main/java/sorald/processor/MathOnFloatProcessor.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 changes: 5 additions & 0 deletions
5
.../src/test/resources/processor_test_files/S2164_MathOnFloat/INCOMPLETE_DoNotCastFloat.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| class DoNoCastFloat { | ||
| float a = 16777216.0f; | ||
| float b = 1.0f; | ||
| final float c = a + b; | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
34 changes: 34 additions & 0 deletions
34
sorald/src/test/resources/processor_test_files/S2164_MathOnFloat/MathOnFloat.java.expected
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
|
|
||
| } |