Community
Participate
Working Groups
The JDT/Core code is currently compiled with org.eclipse.jdt.core.compiler.problem.potentialNullReference=ignore I would like to see us eating our own dog food, here. However, currently this would bring us as many as 362 additional warnings. Browsing some of the problems I see a clear majority resulting from the following coding pattern: void foo (Type[] array) { int length = array == null ? 0 : array.length; for (int i = 0; i < length; i++) process(array[i]); } I suggest to make the connection between null checking and shortcutting the loop explicit as in void foo (Type[] array) { if (array != null) { int length = array.length; for (int i = 0; i < length; i++) process(array[i]); } } I don't see sufficient advantage in the former pattern, and by using the second the compiler will understand our reasoning and not flag potential null. Of course it would be cool if the analysis would understand the original pattern, too, but this requires correlation analysis, which we currently don't do (and even: correlation involving an int!). Once this pattern is changed it will be much easier to browse the remaining warnings for real problems. Perhaps this will unveil further patterns to either avoid or improve analysis for.
See bug 127575 for another reason that makes it hard to enable the preference (speaking for JDT UI here).
Created attachment 189975 [details] two samples Here are two samples of what I'm talking about: Inside ConstructorDeclaration it's the pure form of the pattern. In class Main a little more hand-crafting is needed.
The same can be said for fields as well. Turning on the option to include fields will show up similar warnings.
I see two action items: 1. clean up our code so it can be understood by our null analysis (this bug) 2. make tutorials of some kind disseminating to users what we learned from (1)
Getting experience with this kind of cleanup is getting more and more relevant.
I agree that in terms of efficiency of the code, we can find fault with both patterns mentioned in comment #0. However, what is worrying me is the 362 warnings. To top it, we don't have a good explanation for the user as to why he should change the code.
Too much on my plate for 4.6. Bulk deferral to 4.7
Ran out of time for 4.7. Bulk move to 4.8.
New Gerrit change created: https://git.eclipse.org/r/131529
New Gerrit change created: https://git.eclipse.org/r/131530
Maybe it is necessary to repeat the purpose of this bug: The idea is not to "fix" actual potential bugs, but to help the compiler so it doesn't report potential null references. There are some strategies besides Stephan‘s suggestion in comment #0: - Early param null checks if(array==null) return; - Map null arguments to empty, preallocated instances static final Type[] EMPTY_TYPE_ARRAY={}; if(array==null) array=EMPTY_TYPE_ARRAY; - Expect params to be null-null by changing invocations: Never start with null in the first place, but use preallocated empty arrays (NOTE: only for internal methods possilbe, not for existing APIs). (It would be good if we could already add annotations to document the expectations, even if annotation based null analysis is still of by default) - add asserts (doesnt work in stephans example, but in similar situations) if(length != 0) { assert array != null; ... } So sometimes the code may get a bit more ugly by these added checks. But note that once arrays are guaranteed to be non-null, for loops can often be replaced by enhanced loops, which actually increases readability.
(In reply to Till Brychcy from comment #11) +1
I do support the provided patches but suggest to keep them back until we are branch-free in JDT (i.e., right after merging BETA_JAVA_12 into master).
