Community
Participate
Working Groups
Compile the following class with warnings for redundant null checks enabled: public class DoWhileBug { void test(boolean b1, Object o1) { Object o2 = new Object(); do { if (b1) o1 = null; } while ((o2 = o1) != null); } } The compiler reports a bogus warning: 1. ERROR in DoWhileBug.java (at line 7)\n } while ((o2 = o1) != null);\n ^^^^^^^^^\n Redundant null check: The variable o2 cannot be null at this location\n
Created attachment 188390 [details] test The same example as a regression test. I'm investigating.
This is a regression that was introduced in 3.7 M2a
I believe this bug was unveiled by the fix for bug 292478: prior to that bug the assignment (o2 = o1) simply had no null status at all, but since that bug the info for o1 is transfered to o2. However, the bug is not in the transfer itself, but the improved analysis brings us into code paths that have not been executed before.
Created attachment 188391 [details] test & preliminary fix The same test plus a fix. It works (NullReferenceTests are green) but I'm 100% not sure why. Hints: this is the exact code location where the failing test and very similar passing tests differ. Second, the code had some odd asymmetry: Handling pot.nn. and not pot.n. is very likely a bug. I still want to analyse whether the checks against HIDE_NULL_COMPARISON_WARNING are correctly located.
(In reply to comment #4) > It works (NullReferenceTests are green) but I'm 100% not sure why. Should read: "I'm not 100% sure why." :)
Created attachment 189794 [details] slightly improved patch with more tests Here's basically the same patch with one small improvement and more tests. After more scrutiny I can now say *why* the patch helps: The check for the while condition is performed in two steps: 1. recordUsingNullReference evaluates what is known from the loop body only 2. complainOnDeferredNullChecks performs final checks based on flow info combined from entry and exit(s) of the loop. For the particular test case, during (1) we know that o2 can be null, hence it is relevant for a check in phase (2). During phase (2) we only know about paths where o2 is def non-null. Hence the bogus warning "cannot be null". The patch fixes this by recordUsingNullReference in the following way: when recording the assignment of o2 for deferred check, we need to change the checkType from CAN_ONLY_NULL_NON_NULL to CAN_ONLY_NULL. By this we remember that pot null was already observed, and warnings should only be issued for "can only be null" not the inverse. Thus I'm confident the patch makes sense (it did fix the issue to begin with :) ) The patch also contains two disabled test cases, with the following analysis of why they (must) fail in the current strategy: testBug336428a: - phase(1): o2 is pot null, requires check for CAN_ONLY_NULL - phase(2): at loop start/exit o2 is def null -> bogus warning "can only be null", should be silent Analysis is missing the branch where o2=o1 will bring in the 'unknown' state of o1 from outside the loop. testBug336428b: - phase(1): without considering the upstreamNullInfo analysis believes o2 is def unknown, doesn't even consider raising a warning. This unknown state is an artifact of analysing the loop body in isolation. The last test case testBug336428e demonstrates that checking of HIDE_NULL_COMPARISON_WARNING is indeed relevant in this code branch. So here're my suggestions: First, release the current patch as is (i.e. with disabled tests) Aysush, would you have a minute to double check the patch? (The patch itself is really smal :) ) Second, collect the findings about incomplete loop analysis for investigation past 3.7. I thought we already had a bug for this but can't find it right now.
(In reply to comment #6) > Created attachment 189794 [details] [diff] > slightly improved patch with more tests > > First, release the current patch as is (i.e. with disabled tests) > Aysush, would you have a minute to double check the patch? > (The patch itself is really smal :) ) Patch looks good. The tests c,d and e are green without the fix as well though. Is that intended?
(In reply to comment #7) > Patch looks good. The tests c,d and e are green without the fix as well though. > Is that intended? Tests c & d mainly document how close to the disabled case b we get without getting burned. Let me know if you prefer to omit or disable those for now. Test e protects us against a possible bug in the patch itself: is the checks against HIDE_NULL_COMPARISON_WARNING correctly placed? I know that the assert issue is generally covered by other tests, but in this (white box) test I wanted to be really sure we actually trigger this exact control flow. This test I definitely want to keep.
(In reply to comment #8) > Tests c & d mainly document how close to the disabled case b we get without > getting burned. Let me know if you prefer to omit or disable those for now. > > Test e protects us against a possible bug in the patch itself: > is the checks against HIDE_NULL_COMPARISON_WARNING correctly placed? > I know that the assert issue is generally covered by other tests, but in > this (white box) test I wanted to be really sure we actually trigger this > exact control flow. This test I definitely want to keep. Ok. Sounds good. The patch is good to go then. The tests are good to have.
Released for 3.7M6
Verified for 3.7M6.
