Community
Participate
Working Groups
The null analysis doesn't find issues in code like this: void foo() { final String s1 = null; for (int i=0; i<4; i++) new Runnable() { public void run() { s1.toString(); } }.run(); } This is because information is dropped at two locations in the implementation: When analysing the body of a loop the flowInfo is set to the flowInfo.nullInfoLessUnconditionalCopy() because null-analysis inside loops must be deferred. However, inside TypeDeclaration.internalAnalyseCode(..) we also cut the connection among flow contexts that would otherwise do the deferred checking: method.analyseCode(this.scope, null, flowInfo.copy()); the second argument (null) means that the method is analysed within a parent-less flow context. Both details together imply that no connection can be made between the state outside the loop and insdide the method in the local class, although this could easily be made given that the local in question (s1) must be final by construction.
Created attachment 204806 [details] test & proposed fix I was briefly considering special treatment for final varialbles inside FlowInfo.nullInfoLessUnconditionalCopy(). This would probably work OK since there's no reason to defer analysis of final variables, however, the change would be non-trivial (respecting extraBits inside FlowInfo, avoiding double reporting regarding final variables both immediately *and* deferred etc.) Changing the other location in question looks much easier and does achieve the desired outcome: by passing down the enclosing flowContext of a local type into analysing its methods we can indeed establish the missing link. I'm currently running the tests.
Created attachment 204814 [details] more tests & improved fix This patch includes tests also for - constructors - initializers - types nested in a try-finally With only small changes the fix also covers these issues. Passes all tests in compiler.regression (except for the unrelated regressions mentioned in bug 359943 comment 17) and AllJavaModelTests
Released as commit f103afa2b75b8198880ca7616628c69d234642fd for 3.8 M3.
Reopening as this caused regressions in org.eclipse.jdt.core.tests.eval. Investigating.
Created attachment 204838 [details] additional test & fix Here's what happened in VariableTest et al: We were analysing a deeply nested return statement: class declaration - static initializer - try-catch - local class declaration - method - return statement Inside ReturnStatement.analyseCode we have a loop that traverses the parent chain of flow contexts, which would normally iterate until a null parent is found. If an initializer context is found, we raise an error. With the change from this bug we were simply traversing too far, finding the initializer as a parent of the return statement. The new patch fixes this by simply breaking from the loop when we hit the (innermost) enclosing method. I checked for similar loops, but could only find BreakStatement and ContinueStatement which already properly stop traversing when the targetContext is found. No action required here. Thus the fix consists of one more break condition inside the affected loop. I added a test that checks the three statements (return/break/continue) in the given nesting structure. Currently rerunning the tests.
Released additional fix: commit 2d0b70991183be97a67695fa28c23b0cb3fbbdbf for 3.8 M3.
This fix introduced a regression, it causes ECJ to compile the following successfully, while javac rejects it package p; class F { void f () { while (true) { class Inner { { if (true) break; } } new Inner(); } } }
Created attachment 205072 [details] test & additional fix (In reply to comment #7) > This fix introduced a regression, it causes ECJ to compile the following > successfully, while javac rejects it I'm sorry! I failed to see that many other loops over the parent chain, which exist inside the flow package, should not cross the newly introduced link either. I believe it's safest to let all these loops use the same check (inside new method FlowContext.getLocalParent()) and rather return null than an out-of-scope parent. The effect should be that most analyses are performed exactly as before this bug, except for deferred null analysis which needs to see null status even from outside the current class. This patch actually reverts part of the patch from comment 5 and replaces it with the more general approach.
Thanks for the prompt follow up Stephan. :) The fix looks good.
Released additional fix from comment 8: commit 1adff7e346cbd79e4672d3b7b0f319fb8170d9e9 for 3.8 M3
Verified for 3.8 M3 using build id: N20111022-2000
