Community
Participate
Working Groups
Found this while playing around with a particular test case. In the following snippet: public static void main(String[] args) { int i; String str = null; if (str != null) i++; } we don't warn "variable i may not have been initialized" on i++, while javac does. On investigating, I found that this is because all uninitialized warnings in unreachable code are suppressed in org.eclipse.jdt.internal.compiler.flow.UnconditionalFlowInfo.isDefinitelyAssigned(LocalVariableBinding). This is clearly wrong, because deadcode is an ecj-specific feature and javac will always raise the uninitialized warning in such scenarios. We should make sure this warning is not suppressed in code which is flagged as UNREACHABLE because of being analysed as dead code by ecj, but is suppressed in those cases that are analysed as unreachable as mandated by the JLS. Sigh!
This is against the JLS. This is serious and should be fixed asap. null analysis should not hide any errors that are mandatory from the JLS.
This is related to what was done in bug 166641.
(In reply to comment #2) > This is related to what was done in bug 166641. Not exactly, i saw the patch attached there and it seems we were always hiding that warning for unreachable code. With that bug fix, Maxime only 'unlocked' this warning for cases where the declaration itself was inside unreachable code
The bad news is, we need to still make sure that in the following case, even though i++ occurs in deadcode, we still cannot raise the uninitialized warning public static void main(String[] args) { int i; l : { if (false) break l; return; } i++; }
This is a regression over 3.5.2. This is a must fix for 3.7M6.
(In reply to comment #5) > This is a regression over 3.5.2. This is a must fix for 3.7M6. This was introduced by the fix for bug 293917. Commenting line 624 in FlowContext (HEAD) makes the required warning re-appear. I'm doing some background research on this.
(In reply to comment #6) > This was introduced by the fix for bug 293917. > Commenting line 624 in FlowContext (HEAD) makes the required warning re-appear. Yes, we agree on that. I got to the same conclusion :-).
Created attachment 189898 [details] sketch towards a possible solution This sketch introduces a fresh new tagBit (UNREACHABLE_BY_NULLANALYSIS) to signal when null analysis finds that a branch is unreachable. The trick is: the normal analysis completely ignores this bit, only during UnconditionalFlowInfo.mergedWith(..) do I check for this bit to avoid merging-in irrelevant null info. The sketch is still incomplete, it causes a few relevant regressions in NullReferenceTests, but it already fixes the issue at hand. It also causes many of the dead code warnings introduced in bug 293917 to disappear again. Is that good or bad?
(In reply to comment #8) > Created attachment 189898 [details] [diff] > sketch towards a possible solution I think the approach of having a separate bit for "dead code" is the best option we have here. I'll look into how we can still keep the current good dead code analysis intact while still using this approach.
Created attachment 189936 [details] proposed fix v1.0 This follows from the above patch, to make sure we separate the unreachability due to null analysis from that of the assignment analysis, and still keep the deadcode analysis untouched. All NullRefTests pass. Yet to run the complete suite.
(In reply to comment #10) > Created attachment 189936 [details] > proposed fix v1.0 > > [...] All NullRefTests pass. [...] Congrats! Quick questions: do we really *want* the dead code warnings after we already signal a redundant check? How does your patch affect optimization? I assume sub-classes of FlowContext should be changed, too?
(In reply to comment #11) > (In reply to comment #10) > > Created attachment 189936 [details] [diff] [details] > > proposed fix v1.0 > Quick questions: do we really *want* the dead code warnings after we already > signal a redundant check? I would say yes because its an existing feature, and it should either be completely removed or be consistent across all cases. With just the patch in comment 8, we report dead code warnings in fewer cases, but we still do report it in some cases. This ambiguity is not good. >How does your patch affect optimization? It doesn't touch optimization at all. I plan to handle that separately for bug 326950, using the same new bit we're introducing here. The only place that'll need to change will be the Statement#complainIfUnreachable() method, where we'll make sure we dont reset the IsReachable bit when only UNREACHABLE_BY_NULLANALYSIS is set. > I assume sub-classes of FlowContext should be changed, too? Yup!
Another case to be covered is for fields: public class X { public final int fi; { int i; String str = null; if (str != null) i = fi; } } Here javac gives 2 errors: variable fi might not have been initialized i = fi; ^ variable fi might not have been initialized public class Wtf { ^ 2 errors while ecj only gives 1. (for the class and not for the assignment). Anyway, this is trivial and the fix for isDefinitelyAssigned(LocalVariableBinding) needs to be extended to isDefinitelyAssigned(FieldBinding)
Created attachment 190035 [details] proposed fix v2.0 + tests This patch creates a new bit in FlowInfo, UNREACHABLE_OR_DEAD in addition to UNREACHABLE_BY_NULLANALYSIS, so that the bit UNREACHABLE can be defined as UNREACHABLE = UNREACHABLE_OR_DEAD | UNREACHABLE_BY_NULLANALYSIS. This is done so that at all places where we currently check for UNREACHABLE to be set we don't have to check for UNREACHABLE|UNREACHABLE_BY_NULLANALYSIS instead. The patch also extends the fix to FinallyFlowContext and LoopingFlowContext. In the isDefinitelyAssigned(..) methods, we now make sure we only skip the warning if the UNREACHABLE flag is set, but not because of null analysis. All tests pass.
Stephan, can you please do a final review? Thanks!
Small typo in the doc of FlowInfo.reachMode(). Will correct it when the patch is reviewed.
The test org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest.testBug338234() needs to be updated.
Created attachment 190073 [details] proposed fix v2.0 + tests updated Same patch with changes mentioned above.
(In reply to comment #18) > Created attachment 190073 [details] > proposed fix v2.0 + tests updated > > Same patch with changes mentioned above. I'm now looking into this. BTW, I can't directly use your patch with the Apply Patch wizard. Maybe you missed to set patch root to "Workspace"? (no need to create a new patch, though).
I believe we have a viable solution here, yet I'd like to take a minute to improve the comprehensibility. The comment explaining UNREACHABLE_OR_DEAD doesn't seem to match the usage of this flag: "unreachable code as defined in the language spec". The JLS explicitely excludes "if (false) { x=3; }" from recognition of unreachable code. Furthermore, for loop conditions the JLS only considers the constants true/false, not optimized booleans. I believe, the representation of JLS-unreachable in our code is FlowInfo.DEAD_END. Our UNREACHABLE flag already includes optional compiler specific analysis. All clients calling flowInfo.setReachMode(UNREACHABLE) now implicitly set (UNREACHABLE_OR_DEAD|UNREACHABLE_BY_NULLANALYSIS), which isn't quite the intention, I believe. This only works because of the two flags only three combinations are actually used: 0 = REACHABLE 4 = UNREACHABLE_BY_NULLANALYSIS 5 = UNREACHABLE_OR_DEAD|UNREACHABLE_BY_NULLANALYSIS I don't think 1 (UNREACHABLE_OR_DEAD) is ever used in isolation, right? And more specifically, no code checks for (tagBits & UNREACHABLE) == UNREACHABLE_OR_DEAD To challenge my understanding I made a quick experiment: replace all occurrences of setReachMode(UNREACHABLE) with setReachMode(UNREACHABLE_OR_DEAD). Still NullReferenceTests and FlowInfoTests pass 100%. After this replacement we use these combinations: 0 = REACHABLE 1 = UNREACHABLE_OR_DEAD 4 = UNREACHABLE_BY_NULLANALYSIS i.e., we never set both flags at the same time, but use the combined UNREACHABLE only for tests. To me this looks cleaner. Two further adjustments are required to make the above statement fully true: Both implementations of setReachMode currently contain this line this.tagBits |= UNREACHABLE; which I'd change to this.tagBits |= reachMode; I think our future selves working on future changes will prefer the 0 1 4 encoding over the 0 4 5 one, what do you think? BTW: I'd also move NULL_FLAG_MASK to some higher value so we can use subsequent bits 0 1 2. Some more cosmetic points: - indentation in UnconditionalFlowInfo.mergedWith (sorry, that's from my minimal-changing sketch) - update doc comment of setReachMode() - also the comment of reachMode() should mention the new flags, no?
Thanks Stephan for reviewing! (In reply to comment #20) > I believe we have a viable solution here, yet I'd like to take a minute > to improve the comprehensibility. > This only works because of the two flags only three > combinations are actually used: > 0 = REACHABLE > 4 = UNREACHABLE_BY_NULLANALYSIS > 5 = UNREACHABLE_OR_DEAD|UNREACHABLE_BY_NULLANALYSIS > I don't think 1 (UNREACHABLE_OR_DEAD) is ever used in isolation, right? Yes, you're right. Either both or only UNREACHABLE_BY_NULL analysis is set at any point. > And more specifically, no code checks for > (tagBits & UNREACHABLE) == UNREACHABLE_OR_DEAD Not in this patch, but the patch in bug 326950 does. > 0 = REACHABLE > 1 = UNREACHABLE_OR_DEAD > 4 = UNREACHABLE_BY_NULLANALYSIS > i.e., we never set both flags at the same time, but use the combined > UNREACHABLE only for tests. To me this looks cleaner. Ok. This can be done. > BTW: I'd also move NULL_FLAG_MASK to some higher value so we can use > subsequent bits 0 1 2. > Some more cosmetic points: I'll take care of these
Created attachment 190298 [details] proposed fix v2.1 + tests Conceptually same fix with the bits changed as suggested above. Also, UnconditionalFlowInfo.mergedWith() doesnt require any change now, because the UNREACHABLE bit is already checked in the starting of the method for both flow infos. Stephan, can you give it a final once over and i'll release it. Thanks!
(In reply to comment #22) > Created attachment 190298 [details] > proposed fix v2.1 + tests > > Conceptually same fix with the bits changed as suggested above. Looks good now, but ... > Also, > UnconditionalFlowInfo.mergedWith() doesnt require any change now, because the > UNREACHABLE bit is already checked in the starting of the method for both flow > infos. I disagree. Try this: void foo(boolean b) { int i; String str = null; if (b) { if (str == null) return; } else { i = 2; } // <- watch this merge of branches! i++; } Do you see the warning "The local variable i may not have been initialized"? Shouldn't we? I believe an updated version of the change in mergedWith() is required in order to achieve this, no?
(In reply to comment #23) > Do you see the warning "The local variable i may not have been initialized"? > Shouldn't we? Yes, we must report an error in this case.
Created attachment 190355 [details] proposed fix v2.2 + tests Same fix with correction in mergedWith(..) method.
I agree with the last patch. +1
Released in HEAD for 3.7M6
Verified for 3.7M6 using build I20110307-0800
Verified for 3.7RC0 using I20110501-0200 (4.1 I-build)
