Community
Participate
Working Groups
Build Identifier: I20100513-1500 See attached test case Reproducible: Always Steps to Reproduce: 1. Compile test case. You can see one false warnings if the compiler setting "Redundant null check" is on.
Created attachment 169480 [details] Contains test case
Ayush, please follow up.
I have essentially the same problem, with essentially the same reproduction test case. If the initial line = "" is changed to line= null then the warning goes away. It does not change based on whether the initial doRead is set to true or false. package net.wagland.paul.test; import java.io.BufferedReader; import java.io.IOException; public class NullWarningTest { public void testNullWarning(BufferedReader bufReader) throws IOException { String line = ""; boolean doRead = false; while (true) { if (doRead) line = bufReader.readLine(); if (line == null) return; doRead = true; } } } I am getting this warning on eclipse Version: 3.6.2 Build id: M20110204-1030 (aka 3.6.3 RC3) Bug 332713 should be marked as a duplicate of this bug.
(In reply to comment #3) > Bug 332713 should be marked as a duplicate of this bug. I don't think its a dup.
(In reply to comment #4) > (In reply to comment #3) > > Bug 332713 should be marked as a duplicate of this bug. > > I don't think its a dup. I disagree. If you change my "while" to "for" then you get pretty much identical code. The only difference is that in the attachment, the boolean does not change value through the loop, however that is not required to produce this false warning. And this, in turn, is conceptually identical to the first test case on bug 332713.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Bug 332713 should be marked as a duplicate of this bug. > > > > I don't think its a dup. Sorry, I was looking at the wrong bug. These 2 indeed are dups
*** Bug 332713 has been marked as a duplicate of this bug. ***
Created attachment 188566 [details] possible fix Fix for bug 291418 should've also checked for the variable declared outside a loop being possibly assigned inside one of the branches of the loop, since that assignment can change the null status, even if it doesnt mark the variable as null or non null per se.
Created attachment 188605 [details] proposed fix v1.0 + regression tests same fix as above with tests added. The fix basically just adds a check to make sure that the null status has not been affected inside a loop by means of an assignment, before assuming that the null status can be borrowed from upstream info.
Stephan can you please review the changes when you have time? Thanks!
(In reply to comment #10) > Stephan can you please review the changes when you have time? Thanks! I tried to break the patch but couldn't, so it looks good. I wondered why that condition only asks the flowInfo.isPotentially.. bits, but flowInfo.isDefinitely... is already asked earlier in the same method so it looks complete now. Next I checked for possible interaction with the pending fix for bug 336428 since it operates in the same neighborhood, but the issues are actually independent. My final concern was trying to understand why this is relevant: this.upstreamNullFlowInfo.isDefinitelyNonNull(local) whereas the opposite is not this.upstreamNullFlowInfo.isDefinitelyNull(local) Trying to understand these in context led me to puzzle about this comment: // prot. non null + prot. null => pot. null // PREMATURE use tainted instead (in NullReferenceImplTransformations). This makes the semantics of mergedWith(..) asymmetric which is surprising. However, in that larger context I couldn't see any easy answers (do you see why these things are so asymmetric?) and your patch is surely the best we can do right now, hence +1. (You might just want to add some linebreak so the comment starts somewhere before column 162 :) )
(In reply to comment #11) > Trying to understand these in context led me to puzzle about this comment: > // prot. non null + prot. null => pot. null // PREMATURE use tainted instead > (in NullReferenceImplTransformations). > > This makes the semantics of mergedWith(..) asymmetric which is surprising. I tried hard to find a test that breaks due to a missing pot.-non-null bit. I found that pot.-non-null is only used in combination with definitely-null in order stop us from reporting "can only be null". If we also have pot.null in the mix that warning will never occur so pot.-non-null can be safely dropped in the mentioned merge. Thus all looks fine to me, no reason to worry about asymmetry here.
(In reply to comment #12) > Thus all looks fine to me, no reason to worry about asymmetry here. Thanks Stephan, I'll release the fix today. (In reply to comment #11) > (You might just want to add some linebreak so the comment starts somewhere > before column 162 :) ) Will do!
Released in HEAD for 3.7M6
Is there any chance of getting this in for the 3.6.2 release?
(In reply to comment #15) > Is there any chance of getting this in for the 3.6.2 release? Unfortunately no. We're too close to shipping 3.6.2 to make any more changes. Besides, this is not really a blocking issue.
Verified for 3.7M6.
