MathOnFloatProcessor and CastArithmeticOperandProcessor can produce non-compilable output

[slarse]

This is rather tricky. Sorald is technically doing exactly what Sonar says, namely casting to double. It's just that in a few cases, that's not the right thing to do (even though Sonar says so). For example:

// original, sonar says cast operands to double
float c = a + b;
// repaired
float c = (double) a + (double) b;

The problem is of course that the right hand side is now a double, but you can't assign a double to a float due to lossy conversion. Sonar only says to cast to double, so from that perspective, Sorald is doing the right thing.

It's the same case for all compilation failures. If the variables are changed to double instead of float, the compile error disappears. My only suggestion for solving this is that, in the case that whatever reads the result of the expression expects a float, then cast the entire expression to float. E.g. like so:

// original, sonar says cast operands to double
float c = a + b;
// repaired
float c = (float) ((double) a + (double) b);

It makes for some really ugly code, though. @fermadeiral thoughts? Any other ideas?

Originally posted by @slarse in #569 (comment)

[slarse]

According to discussion during meeting, we don't attempt to repair if we can tell before hand that there will be a compile error.

[lyxell]

@lyxell #569 (comment)

Does it really fix the problem though? Couldn't it be the case that this approach actually just supresses a potentially important warning for a bug?

public class Test {
    public static void main(String[] args) {
         float a = 16777216.0f;
         float b = 1.0f;
         float c1 = a + b; // Sonar warning, yields 1.6777216E7 not 1.6777217E7
         System.out.println(c1 == a); // true

         float c2 = (float) ((double) a + (double) b); // No Sonar warning, also yields 1.6777216E7 not 1.6777217E7
         System.out.println(c2 == a); // true

         double c3 = (double) a + (double) b; // Yields 1.6777217E7
         System.out.println(c3 == a); // false
    }
}

@slarse #569 (comment)

Does it really fix the problem though? Couldn't it be the case that this approach actually just supresses a potentially important warning for a bug?

That would of course be situational. The accuracy of the actual mathematical computation is improved, but in some cases (like this one) the end result is the same.

Even if the end result is the same, I guess one could still argue that the code is less efficient, less readable and a potentially important warning is suppressed.

There is also two roundings going on so I think the end result could actually have worse accuracy. Even though this paper claims that it is safe for addition, subtraction, multiplication, division and square root.

Wouldn't it be a better idea to skip the repair if the result must be cast to float? Or would that make the processor more complex?

[slarse]

Wouldn't it be a better idea to skip the repair if the result must be cast to float? Or would that make the processor more complex?

During a meeting today, we agreed that we do not attempt to repair these warnings if we can determine that doing so will cause a compile error. This amounts to implementing canRepairInternal to analyze the expression to be repaired.

[slarse]

To be clear, we don't cast to float.

[lyxell]

During a meeting today, we agreed that we do not attempt to repair these warnings if we can determine that doing so will cause a compile error. This amounts to implementing canRepairInternal to analyze the expression to be repaired.

Cool, this is achieved with Spoon?

[slarse]

During a meeting today, we agreed that we do not attempt to repair these warnings if we can determine that doing so will cause a compile error. This amounts to implementing canRepairInternal to analyze the expression to be repaired.

Cool, this is achieved with Spoon?

Yeah, the analysis is implemented with Spoon. The related methods can be found in SoraldAbstractProcessor, the most important one being this one:

https://github.com/SpoonLabs/sorald/blob/158988d62cfbaacc53ff8b2693c86c37e91ea308/src/main/java/sorald/processor/SoraldAbstractProcessor.java#L95-L119

We need to define for example MathOnFloatProcessor as incomplete (see description of what an incomplete processor is in the javadoc above), and implement canRepairInternal to return false if we can determine that the expression being analyzed is used in a context that expects a float.