Fix unchecked conversion not failed compilation#4961
Conversation
|
Hello, Thank you! |
|
Thanks for submitting. A few preliminary remarks:
Of course causality is much more indirect: detecting raw types in that position should not (and does not) directly cause compilation to fail. Did you understand how the change triggers the compile error in this particular case? Perhaps adding another similar test which passes helps (now and for posterity) to understand the actual effect of the change?
It would help to be more specific, stating which part of that section is not correctly implemented in ecj. Actually my own first reaction was: isn't all this already handled in type inference? (no, it is not).
In many cases of type checking it is not obvious, which way a compiler should behave. Comparing ecj to javac gives a hint but not a proof. Ideally, when a compiler accepts a program wrongly, this can be proven by crafting a program that is accepted by the compiler but throws an unexpected exception at run time. Do you think this kind of proof might be possible for this example? Is anything obviously wrong about the byte code that ecj generates?
The exact error messages are not standardized. If you look closer, do both error messages tell different stories or the same story just with different words? |
|
Thanks @stephan-herrmann I need to wrap my around this again. Will take some time, but do my best. |
Detect when any explicit type argument is a raw type Adds regression test testGH4957 reproducing the Comparator.comparing + raw Comparable scenario (issue eclipse-jdt#4957) and asserting the expected warnings/errors. With this fix testGH4957 passes.
|
I did had a look at it and tried to understand your comment.
I think I meant the following section of the spec:
maybe....maybe not... My interpretation is this:
Before the change the But javac seems to be stricter.
I tried to construct a runtime-failing example. import java.util.Comparator;
import java.util.Map;
public class Snippet2 {
public static void main(String[] args) {
Comparator raw = Comparator
.<Map<String, Object>, Comparable>comparing(
e -> (Comparable) e.get("event_date"),
Comparator.nullsLast(Comparator.naturalOrder())).thenComparing(
e -> (String) e.get("event_type"));
System.out.println(raw.compare("not a map", "also not a map"));
}
}Javac gives: I am not sure this is what you were looking for. This is not a good proof of the original bug in isolation, because the runtime failure also depends on explicit raw usage at the call site. But it does show that the unchecked/raw behavior in this area can lead to concrete runtime problems. What do you think is a valid next step. Would you like to take over and just use my beginner thoughts as input? |
|
Thanks for your response @chrisrueger Let me put the cards on the table: What I've seen so far looks very good. Still we need to build trust that the fix really fixes the core problem, and does so without causing grief elsewhere. Several ways how this trust can be established:
Now, let's see where we stand with your response:
The weak part here is "I think I meant", the rest is good.
You chose no simple code area, and hence no need to be shy if you are not 100% certain. Let's work it out together.
All that follows looks good. I'll still verify it in the debugger, but so far no complaints.
This was a tremendous challenge, actually (I had briefly tried and failed).
The effect looks pretty similar to what I was looking for, but here the ClassCastException happens "before" the location where compilers disagree. See: import java.util.Comparator;
import java.util.Map;
public class Snippet3 {
public static void main(String[] args) {
Comparator raw = Comparator
.<Map<String, Integer>, Comparable>comparing(
e -> e.get("event_date"));
System.out.println(raw.compare("not a map", "also not a map"));
}
}This is accepted by both compilers and still throws the exact CCE you also observed. Who is to blame? Right, any use of raw types can produce CCE in location where the user didn't even write a cast. So, the example did not succeed to distinguish between correct and incorrect compilation, which only means, we should probably focus on evidence by reasoning, rather then evidence by a witness.
I'll take a closer look at the change both in the debugger and in relation to JLS. From there I'll either come back with more questions / requests, or I may find all to be in good shape and merge it right away. Stay tuned. |
Closes #4957
What it does
How to test
org.eclipse.jdt.core.tests.compiler.regression.GenericsRegressionTest_9.testGH4957()without the fix inorg.eclipse.jdt.internal.compiler.lookup.ParameterizedGenericMethodBinding.computeCompatibleMethod(MethodBinding, TypeBinding[], Scope, InvocationSite)or just use e.g. Eclipse 2025-03 (or even 2026-06 (4.40) Build id: I20260319-1800 master) and paste the the following class:
Expected Result: Compilation should fail in Eclipse
Actual result: Compiliation succeeds.
But in javacc compilation fails.
With this PR compilation fails also in Eclipse.
But note: Eclipse seems to error with a different message than javac. Not sure this is a deeper problem. But at least it fails at the same place.
ECJ fails with:
while javac failed with:
Disclaimer:
I used AI to help me understand the issue and spec and find the places in the code. I have manually tested and debugged via latest JDT Dev environment via Oomph and ran my test in Eclipse.
This is my first step in the compiler space. Feel free to reject it or just use it as a base for a better fix if needed.
Author checklist
thoroughlytested my changes as good as I can