Bugzilla – Full Text Bug Listing
| Summary: | [compiler][null] Compiler misidentifies a case of "variable can only be null" | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Mike Schrag <mschrag> | ||||
| Component: | Core | Assignee: | Stephan Herrmann <stephan.herrmann> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | Olivier_Thomann, satyam.kandula, srikanth_sankaran, stephan.herrmann | ||||
| Version: | 3.7 | ||||||
| Target Milestone: | 4.3 M2 | ||||||
| Hardware: | PC | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 385415, 386181 | ||||||
| Attachments: |
|
||||||
Thanks for the test.
It shows that our null analysis uses a similar flow analysis as is required
for analysing definite assignment - even in cases where this is not optimal.
Consider:
public class Test {
public static void main(String[] args) {
String s;
while (true) {
try {
s = "hi";
}
finally {
s.length();
}
}
}
}
----------
1. ERROR in Test.java (at line 9)
s.length();
^
The local variable s may not have been initialized
----------
1 problem (1 error)
I believe this error is correct, because flow analysis for try-finally is quite
pessimistic (as per the JLS - may want to double check).
However, for null analysis we're not bound to the JLS and should do better.
Documenting negative result of search for dups: It a first look one might think this issue be somewhat related to bug 150082 (and bug 158000 ?). See NullReferenceTest#test0530_try_finally(), which was introduced by the combined fix for both bugs. But in that case the assignment could indeed be skipped by an unchecked exception. In the example of this bug nothing can prevent the assignment from being executed, so we have different situations. This will be a good item to fix for 3.8. Too late for 3.7. I suspect that some of the false positives in bug 368546 are related to this issue. I'll report back any findings once I have a grip on bug 368546. If you plan to include a fix for this in 3.8 M7, please adjust the target suitably, so it becomes easier to track. I don't think we are in the state to make any promise right now, but we may want to assess this shortly after EclipseCon, OK? Stephan, do you want to investigate this further and see if we can come up with a safe fix for RC1? Otherwise we can move it to 4.3 Moving to 4.3 Created attachment 219137 [details]
rough sketch of a possible fix
Here's a rough sketch of how this could possibly be addressed by decoupling null-analysis for try-finally from def-assign analysis.
- more logic to collect null info in FlowContext.initsOnFinally as we go, so that in the end we need less algebraic magic (new: FlowContext.markFinallyNullStatus(), mergeFinallyNullInfo()).
- additionally record null-effects towards finally block after each statement in a try block.
- record at the FlowContext when we see an expression that could possibly through an exception (incl. undeclared exception). From that point onward use any null info only conditionally (incomplete list: MessageSend, AllocationExpression + Expression.checkNPE() - record by setting exceptionlessInScope to null).
- inside TryStatement completely replace magic of NullInfoRegistry with a regular FlowInfo, replace mitigateNullInfoFrom with explicitly adding the null info collected in initsOnFinally.
- changes in UFI avoid NPEs seen along the way (extra[0] was null, while extra[2] was not).
- additional test in TryStatement captures an intermediate regression (which rendered VerifyTests uncompilable - fixed)
- changes in NullReferenceTest:
- two new tests for this bug
- started to adapt test results to improved analysis (needs verification)
Status: 26 failures in NullReferenceTest (sum of all compliance levels).
After fixing said regressions I've pushed the patch to https://git.eclipse.org/r/#/c/6991/ Significant new change: removal of NullInfoRegistry and all classes used for testing this :) After some experimentation the concept from comment 9 works well with just a few improvements: When recording null info into FlowContext.initsOnFinally, we need to now how to combine the info. This is controlled by the new field FlowContext.conditionalLevel using the following encoding: -1: not inside a try-finally, don't collect info into initsOnFinally. 0: analyzing a top-level statement inside a try-block: effects will be seen unconditionally inside the corresponding finally-block > 0: analyzing a statement/expression that may or may not be executed: effects will affect the finally-block only conditionally Field conditionalLevel is maintained like this: - ExceptionHandlingFlowContext.<init> initializes to 0, the fun begins. - flow contexts inherit value from their parent - flow contexts of conditional structures (switch/loop) add 1 to the inherited value, i.e., their entire scope can only conditionally affect the finally-block, except: DoStatement starts with a clean 0, it is not a pre-test loop. - if statement and ternary expression temporarily increment the conditionalLevel and reset at their end - abrupt exits are signaled (FlowContext.recordAbruptExit()) by incrementing the conditionalLevel (with no matching decrement): -- return, break, continue, throw -- all expressions that may throw any exception incl. unchecked exceptions The entire class NullInfoRegistry is indeed obsolete now (I never was very comfortable with this class anyway). Required changes in NullReferenceTest look OK to me (i.e., demonstrate more precise analysis). Changes in UnconditionalFlowInfo fix two bugs: - AIOOBE (see comment 9) - proposed fix - wrong bit arithmetic (see bug 386181) - workaround for now Advance experiments show that bug 385415 can be easily fixed on top of this patch. Please mark comments on details in https://git.eclipse.org/r/#/c/6991/, thanks. Ayush: OK to assign to me? :) Stephan, sorry I couldn't find time to review this for M1. Ok to move out to M2? (This has a bunch of changes so I don't want to rush). (In reply to comment #12) > Stephan, sorry I couldn't find time to review this for M1. Ok to move out to > M2? (This has a bunch of changes so I don't want to rush). Sure, this is not the kind of patch to be released in a hurry :) During self-review I made one significant change: I had some changes in UnconditionalFlowInfo to prevent an AIOOBE. With the latest version of the patch I can no longer reproduce the AIOOBE and reverting that part of the change has the advantage that we continue to consistently play by the following rule: - all individual sub-arrays of FlowInfo.extra have the same length (or are null) If this rule is violated we want to be informed, so we can fix the code. Additionally, I filed two follow-up issues: bug 388049 and bug 388050. With these changes / fups the patch has been released for 4.3 M2 via commit http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=6d141275326cf4caf65ec5dca68b565e2e9b1360 Verified for 4.3 M2 using Build id: I20120916-2000 |
Build Identifier: 3.6.1 M20100909-0800 public class Test { public static void main(String[] args) { String s = null; while (true) { try { s = "hi"; } finally { s.length(); s = null; } } } } The compiler warns on the s.length() line saying that s can only be null at this point, which is not true -- in fact it can NEVER be null. This test case is totally contrived, but distilled from something more real :) Reproducible: Always