test: Verify that compilable Java files still compile after repair

[slarse]

Fix #563

This PR adds a test case that checks that files that are compilable before repair, are also compilable after. There are two test cases that fail, CastArithmeticOperand.java and MathOnFloat.java. Both fail due to "potentially lossy conversion from double to float".

Link to failures.

[slarse]

@fermadeiral I'm thinking I'll rebrand this PR as a fix for files that fail to compile after repair, and just add in the test case as the verifier that the fix actually did something. Thoughts on that? Will probably make the PR semi-big but it seems inconvenient to try to split it.

[fermadeiral]

I'm thinking I'll rebrand this PR as a fix for files that fail to compile after repair

It seems good to me.

[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?

[fermadeiral]

@slarse, sorry, I'm very busy today and my mind can't focus on what you are saying :|

@khaes-kth, could you take this PR?

[slarse]

@slarse, sorry, I'm very busy today and my mind can't focus on what you are saying :|

@khaes-kth, could you take this PR?

That's okay, this can wait for another day if no one has time right now. I have >5 other projects that require attention :)

[khaes-kth]

@fermadeiral & @slarse : I hope it is not late. I just read the PR code changes and comments.

I think @slarse 's suggestion to fix this specific issue of "lossy conversion" is the best solution (converting the whole thing to float). But I still think adding the test case that Simon has added makes a lot of sense. @slarse : shouldn't we add the test case in this PR and fix the processor in a separate PR?

[slarse]

I think @slarse 's suggestion to fix this specific issue of "lossy conversion" is the best solution (converting the whole thing to float). But I still think adding the test case that Simon has added makes a lot of sense. @slarse : shouldn't we add the test case in this PR and fix the processor in a separate PR?

We could do that, yes. I can ignore the failing tests for the moment. I'll do that sometime tomorrow.

[lyxell]

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]

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.

Let's continue the discussion in #570, it's beyond the scope of this PR. Could you copy your comment over there?

[slarse]

@fermadeiral @khaes-kth Whoever has time, this is ready for merge.

[khaes-kth]

@fermadeiral @khaes-kth Whoever has time, this is ready for merge.

Thanks @slarse