Community
Participate
Working Groups
Build Identifier: M20100909-0800 See attached source code. javac produces the following output: Canceled after 3 times through the loop number = 3 -1 Eclipse produces the following output: Canceled after 3 times through the loop -1 Eclipse is removing the if (number != null) block in the catch when it's not dead code. Reproducible: Always Steps to Reproduce: 1. Run attached code compiled with javac. 2. Run attached code compiled with Eclipse.
Created attachment 185225 [details] Source Code
Reproduced using HEAD. Ayushman, please investigate. This is a candidate for 3.6.2.
Seems like the return statement in the try block is causing a problem. Removing it corrects the false warning. Investigating.
Created attachment 185260 [details] draft patch Basically the return statement erases the null info captured in the try block with tryInfo, and that info instead gets captured in handlingContext.initsOnReturn. So when we try to calculate the catch block's catchInfo, using tryInfo turns out futile in the presence of return statement. I attempted to fix that using this patch, but this broke org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest.test1009(). This is because test1009() is yet another special case where the nullness of a variable is getting affected by an exception throwing method, and the return statement guarantees that the control can only go beyond the try block when the variable has been properly assigned. So the above fix seems insufficient. I'll continue investigating this tomorrow. Stephan, meanwhile if you happen to look at the patch and have a better idea of fixing it, let me know.
Created attachment 185280 [details] proposed patch I do have a proposal :) The difficulty comes from the difference between definite assignment analysis (must be conservative as per JLS 16.2.15) and null-analysis (which we want to perform with more precision). The existing methods like FlowInfo.addPotentialInitializationsFrom(..) cannot fully handle both analyses at the desired levels. My patch does two things: - Introduce a new method FlowInfo.addNullInfoFrom(..) which only handles null info. - Change null-analysis for catch blocks of declared exceptions: Use nullInfoLessUnconditionalInfo for most cases and use null info *only* from the corresponding initsOnException, as this is the only way to enter the catch block. The effect: the current problem is fixed and additionally three existing tests now produce the errors that were originally intended.
(In reply to comment #5) > Created an attachment (id=185280) [details] [diff] > proposed patch > > I do have a proposal :) Thanks Stephan! The patch looks good to me and works for all cases I tried. Please go ahead and release it, once you've added the copyrights. I'm running the complete test suite once on my machine as well.
(In reply to comment #6) > [..] I'm running the complete test suite once on my machine as well. All compiler tests pass.
Released for 3.7M5
Will this also be in 3.6.2?
(In reply to comment #9) > Will this also be in 3.6.2? I was just checking the behavior with older versions: - the null-analysis has been wrong since day 1 (within 3.2.x) - the generated code was still correct until 3.5.2 - only starting from 3.6.0 the compiler performs the wrong optimization -> IMO we should backport the fix for 3.6.2 as this is a regression over 3.5.2. The fix is small so backporting shouldn't be hard. Olivier?
Daniel, +1 for 3.6.2? I am in favor of a backport as this is a regression (see comment 10) and there is no workaround.
> Daniel, +1 for 3.6.2? > I am in favor of a backport as this is a regression (see comment 10) and there > is no workaround. Removing code is not good. +1 for 3.6.2.
.
Created attachment 185336 [details] same patch, but for 3.6.x Here's the same patch for R3.6_maintenance.
Ayush, could you please review for 3.6.2? I'm also currently running compiler.regression.TestAll.
Created attachment 185401 [details] rectified fix for 3.7 While reviewing this for 3.6.2 I noticed that we had missed out fixing a related issue in the presence of finally block. So in the example code, we still get bad code if we add a finally{} after the catch. This patch takes care of this and also adds a corresponding test. I'll also add this in the fix for 3.6.2 and submit another patch. We should run the compiler tests for 3.7 and 3.6.2 again then.
Created attachment 185402 [details] rectified fix for 3.6.2 Fix for R3_6_maintenance.
(In reply to comment #17) > Created an attachment (id=185401) [details] > rectified fix for 3.7 > > While reviewing this for 3.6.2 I noticed that we had missed out fixing a > related issue in the presence of finally block. So in the example code, we > still get bad code if we add a finally{} after the catch. Good point. > This patch takes care of this and also adds a corresponding test. > I'll also add this in the fix for 3.6.2 and submit another patch. We should run > the compiler tests for 3.7 and 3.6.2 again then. Patches look good to me.
(In reply to comment #17) > Created an attachment (id=185401) [details] [diff] > rectified fix for 3.7 All compiler tests pass for 3.7, and all regression tests pass for 3.6.2. Stephan, i'll let you also look at the changes once and release both the patches. Thanks!
After final scrutiny I agree with the latest patches. I've released the additional fix from comment 17 into HEAD.
Released to R3_6_maintenance for 3.6.2
Verified for 3.6.2 using build M20110119-0834
Verified for 3.8M2
This also affects null annotation-analysis in lines marked B and C, while line A is accepted (I assume as per the fix to bug 384380) Version: 4.3.0 Build id: I20130605-2000 BTW: These errors can't be silenced by @SuppressWarnings("null") on method test()! import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; public abstract class Test { @NonNull Object nonNull = new Object(); public void test() { final boolean debug = false; SEARCH: while (someCondition()) { final Object next = getNext(); if (next == null) { continue SEARCH; } if (debug) { // this is accepted next.toString(); // A // Null type mismatch: required '@NonNull Object' but the provided value is inferred // as @Nullable nonNull = next; // B expectNonNull(next); // C } } } abstract void expectNonNull(@NonNull Object param); @Nullable abstract Object getNext(); abstract boolean someCondition(); }
(In reply to comment #25) > This also affects null annotation-analysis in lines marked B and C, while > line A is accepted (I assume as per the fix to bug 384380) If this is supposed to report a bug, then please open a new bug. This one here is closed. Also, I don't see the relation to dead code.
As the variable "debug" is constant false and never changes, the lines in question are effectively dead code. But you're right, I copy my comment to the still open bug 406160 to represent the latest state on this group of three related bugs: bug 406160 bug 384380 this bug 332637 I was reading them in parallel and typed my comment in the wrong place.
