Community
Participate
Working Groups
I20081104-0916 public class Unboxing { public static void main(String[] args) { a(); b(); } private static void a() { Integer i= null; if (i == 1) // wrong problem: "Null comparison always yields // false: The variable i can only be null at this location" System.out.println("a"); } private static void b() { Integer i= null; if (i.intValue() == 1) // correct: "Null pointer access: The // variable i can only be null at this location" System.out.println("b"); } } The problem reported in a() should be the same problem as in b(), since both methods are effectively the same and result in an NPE.
Created attachment 156203 [details] proposed fix v0.5 + regression tests Modified FlowContext#recordUsingNullReference, to check for unboxing. If it is taking place, then report the null pointer access warning instead of redundant null check warning.
The patch does not look good to me. There a specific field on Expression for boxing/unboxing (implicitConversion) which has to be used instead calling lookup methods to know whether an unboxing has been done or not. Hence the condition can be reduced to: if ((checkType & CHECK_MASK) == CAN_ONLY_NULL && (reference.implicitConversion & TypeIds.UNBOXING) != 0) { scope.problemReporter().localVariableNullReference(local, reference); return; } Which will be less time consuming as there won't be any extra methods calls...
Created attachment 156255 [details] proposed fix v1.0 + regression tests fixed the check for autounboxing as per comment 2.
Patch looks good to me
Oops! I just noticed that for two cases the old message is still shown- for loops and finally blocks. I'll append the fix for these 2 cases also, and submit a fresh patch.
Created attachment 158215 [details] updated fix + regression tests The fix is the same, but its now applied to the places that I earlier missed - precisely looping context and finally context. Also added two new regression tests for these cases, and updated the previously supplied tests with the new dead code warnings.
Frederic can you please re-review? Thanks!
(In reply to comment #7) > Frederic can you please re-review? > Thanks! Can you update the patch as it's no longer applicable since fix for bug 250056 has been released, thanks
Created attachment 158695 [details] updated patch for HEAD patch updated for HEAD.
(In reply to comment #9) > Created an attachment (id=158695) [details] > updated patch for HEAD > The patch looks good to me.
Released for 3.6M6. Regression tests added in: org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#testBug253896a org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#testBug253896b org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#testBug253896c org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#testBug253896d
Verified for 3.6M6 using I20100307-2000.
Here is another case of the same problem. Map< String, Integer > map = new HashMap<>(); ############################# Integer test = ( map == null ) ? 0 : map.get( "foo" ); if ( test == null ) // Null comparison always yields false... test = 0; ############################# If I write it using regular if selection instead of inline-if, it works as expected. And it also works, if I use String and "" instead of Integer and 0. This is for Eclipse 4.5.2. So, I would suggest to reopen this bug.
