Community
Participate
Working Groups
Build Identifier: 20110218-0911 public class Test { public static void initPattern(String p, Character escapeChar) { int len = p.length(); for (int i = 0; i < len; i++) { char c = p.charAt(i); if (escapeChar != null && escapeChar == c) { // << warning on escapeChar c = p.charAt(++i); } } } } Reproducible: Always Steps to Reproduce: 1. Put the above into the node 2. Notice the compiler warning. 3.
Does not happen when the for loop is commented out. This means we're recording an incorrect null reference to be checked during the deferred null check.
Created attachment 192941 [details] proposed fix v1.0 + tests In the given case, the problem occurs because of incorrectly recording a null reference for escapeChar at the condition escapeChar == c. The status of escapeChar here in flowInfo is "protected non null" because of the earlier escapeChar != null condition. We should make sure that null reference for such cases are not recorded for deferred null checks in loops.
Stephan, can you please review? Its a small patch. Thanks!
Created attachment 192964 [details] alternative patch While analyzing the patch I found an alternative solution. I'm not saying this one is better, just: it seems to fix the same issue. I was asking myself, what exactly are those methods in FlowInfo testing. Specifically, UnconditionalFlowInfo.isPotentiallyNonNull() answers true for these states: 0010 pot. non null 0011 pot. nn & pot. un 0110 pot. n & pot. nn 0111 pot. n & pot. nn & pot. un 1010 def. non null 1011 pot. nn & prot. nn but not for: 1111 prot. non null I see no reason why it answers true for def. non null but false for prot. non null. The patch changes this and with this all of NullReferenceTest actually pass (without the patch in comment 2).
Regardless of my alternative patch (which would still require more scrutiny) the patch in comment 2 looks good to me. It has only limited impact and for that reason it may be better suitable at this point in the cycle. Given the code comment I found the patch easy to understand and attempts to find a counter example failed. I'll leave it up to you to either release your patch as is or follow up on my sketch.
Thanks Stephan! Infact, I had already considered the approach that you have mentioned above. But, I wasn't 100% sure why pot. non null (or null) does not have protected non null(or null) under its gambit. Maybe its intentional, maybe not. Anyhow since I was sure of the implications of my patch atleast I did not go the more adventurous way. So, lets stick to my patch as of now, and I'll keep the ambiguity in mind. I'll add a TODO comment to that effect.
Released in HEAD for 3.7M7
Verified for 3.7 M7 using build I20110421-1800
