Summary: | [compiler][null] bogus warning "redundant null check" in condition of do {} while() loop | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Stephan Herrmann <stephan.herrmann> | ||||||||
Component: | Core | Assignee: | Stephan Herrmann <stephan.herrmann> | ||||||||
Status: | VERIFIED FIXED | QA Contact: | |||||||||
Severity: | normal | ||||||||||
Priority: | P3 | CC: | amj87.iitr, Olivier_Thomann | ||||||||
Version: | 3.7 | ||||||||||
Target Milestone: | 3.7 M6 | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Whiteboard: | |||||||||||
Attachments: |
|
Description
Stephan Herrmann
2011-02-05 16:55:28 EST
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. |