Community
Participate
Working Groups
The JDT Core currently has a single compiler option (Null reference) to control the error/warning/ignore setting of both 1. the potential programming problem of null dereferences and 2. the unnecessary code of some checks against null. This is unfortunate because one generally wants to have the former reported as errors and the latter as warnings (or even ignored because many are false positives).
Created attachment 56990 [details] patch for jdt.core The attached patches address this issue by splitting the "Null reference" compiler option into two: * Null dereference * Null reference All unit tests in org.eclipse.jdt.core.tests.RunJDTCoreTests (project org.eclipse.jdt.core.tests.model) pass both before and after all 3 patches are applied to CVS HEAD. A discussion of the changes follows. project org.eclipse.jdt.core: org.eclipse.jdt.internal.compiler.batch.Main.java: added handling for the warning token "nullDereference" modified handling of the "null" warning token to enable both null references and dereferences org.eclipse.jdt.core.compiler.IProblem.java: added 2 new entries for null dereferences. '''Can the values 417 & 418 be used?''' org.eclipse.jdt.internal.compiler.ast.Expression.java: in method checkNPE: added parameter isDereference, which is passed to flowContext.recordUsingNullReference provided a version of checkNPE with the original signature that passes false for isDereference org.eclipse.jdt.internal.compiler.ast.ArrayReference.java: FieldReference.java: ForeachStatement.java: MessageSend.java: QualifiedNameReference.java: replaced calls to the original checkNPE method with calls to the new one with true passed for isDereference org.eclipse.jdt.internal.compiler.flow.FlowContext.java: FinallyFlowContext.java: LoopingFlowContext.java: in method recordUsingNullReference: added parameter isDereference in the case where checkType is MayNull, the only one possible when called from checkNPE the method called on the problem reporter is called with the new isDereference parameter org.eclipse.jdt.internal.compiler.impl.CompilerOptions.java: added new String constant for "report null dereference" added new long constant, bit 51. Can this one be used? added null dereferences to the get & set map methods added null dereferences to the value returned by the method warningOptionNames added handling of new warning token to methods warningTokenFromIrritant & warningTokenToIrritant added null dereferences to warningTokens org.eclipse.jdt.internal.compiler.problem.ProblemReporter.java: added entries for null dereference to the method getIrritant & getProblemCategory for methods localVariableCanOnlyBeNull & localVariableMayBeNull added parameter isDereference the problem (and thus the severity) are determined by isDereference added version of methods with original signatures that pass false for isDereference org.eclipse.jdt.internal.compiler.problem.messages.properties: entries added for the two new IProblems project org.eclipse.jdt.ui: ProblemSeverities.java: added a Key for for Null Dereferences added a Label and ComboBox for Null Dereferences PreferencesMessages.properties PreferencesMessages.java: added entries for Null Dereferences project org.eclipse.jdt.core.tests.compiler: NullReferenceTest.java : added the NullDereference option to the value returned by getCompilerOptions() .
Created attachment 56991 [details] patch for jdt.ui
Created attachment 56992 [details] patch for jdt.tests.compiler 3 of 3 patches
Severity changed to major. The current compiler option is practically unusable as it stands because too many false positives are being reported: e.g. in org.eclipse.jdt.core v_731 alone, there are over 300 false positives. As a consequence, it seems that the compiler option is generally set to Ignore. This is unfortunate because the feature can be useful at detecting potential null dereferences. The given patches help take a step in the direction of gaining control / reducing the false positives.
See also bug 138009 that asks for the separation of tunings on slightly different criteria (for one part, the same as here; for the other part, a separation of the cases in which we are certain of the null value). This would leave us with a matrix which would track level of certainty on one of its axis, and dereference vs reference on the other axis - not making any comment on the proper way to cope with this, yet.
If I try to start from the UI, what about adding two check boxes under the 'Null reference:' heading in the preferences: - 'Check potential null values'; would raise the warning level of 'may be null' messages up to the one chosen for the whole category if true, else would ignore them and only retain 'can only be (non) null' ones; - 'Check comparisons to null'; would raise the warning level of messages raised upon 'a == null' cases to the one chosen for the whole category if true, else would ignore them and only check null dereferences. This is less complete than the same design with three levels for each of the categories above, but seems to cover the most interesting combinations. It also clearly subordinates all null checks to a single main option and minimizes UI and conceptual clutter.
(In reply to comment #6) Interesting proposal which may indeed reduced clutter and offer some flexibility. Just to be sure, can you confirm that the following is correct: if the compiler option is set to Error, and the two check boxes are unchecked, then only null dereferences (exp.field or exp.m()) in which exp was known to be null (by flow analysis (FA)) would be reported as an error, all other cases would be ignored. Right? As currently implemented, FA also checks the case where null is being cast to a type; would reporting of this error also be controlled by the 'Check comparisons to null' check box? Might it make sense to have a separate check box -- particularly since the JLS allows casts of null to any type? In fact, instead of an 'Check comparisons to null' being disabled implying that dereference errors are reported, why not have multiple check boxes named with the checks they enable: 'Check dereferences' 'Check comparisons to null' and possibly 'Check null casts'
(In reply to comment #7) > (In reply to comment #6) > > Interesting proposal which may indeed reduced clutter and offer some > flexibility. Just to be sure, can you confirm that the following is > correct: if the compiler option is set to Error, and the two check > boxes are unchecked, then only null dereferences (exp.field or exp.m()) > in which exp was known to be null (by flow analysis (FA)) would be reported as > an error, all other cases would be ignored. Right? Right. > > As currently implemented, FA also checks the case where null is being > cast to a type; would reporting of this error also be controlled by the > 'Check comparisons to null' check box? Might it make sense to have a > separate check box -- particularly since the JLS allows casts of null to > any type? I would avoid adding yet another case, to keep some balance with the other options (I mean, options that are not in the real of null reference analysis). Then it would make sense to associate casts the same error level as comparisons. > > In fact, instead of an 'Check comparisons to null' being disabled implying that > dereference errors are reported, why not have multiple check boxes named with > the checks they enable: > 'Check dereferences' > 'Check comparisons to null' > and possibly > 'Check null casts' > I would consider that 'Check deferences' is always on by default. This is the main point of our null analysis indeed. And as I write above, I feel that making the separate case of casts stand by itself is a bit overkill. Anyway, the UI will be Martin's team call. Thinking a bit more about this yesterday, I thought I should have the complete options ready at the compiler level (that is, Ignore, Warning, Error for null analysis as a whole, for null comparisons, and for potential null values), probably caping the two later by the former one. Then having check boxes or drop down lists in the UI would be a call about simplicity and overall UI balance vs completeness.
Discussed the topic with Philippe today, and we came up with the following proposals: a) add checkboxes to the existing UI as follows; the main advantage is that we do not break any caller, since the existing option will behave as it did before (provided that checkboxes are checked by default): Null reference: <E/W/I drop down> Potential null values <checkbox> Redundant null checks <checkbox> note that any attempt to replace the checkboxes with drop downs is likely to introduce breaking changes since this would add first class options and we do not correlate such options by ourselves (that is, users who would want to get the same errors on the same code through the programmatic interface would have to set all three options instead of the original one only); b) separate more agressively the null reference vs null check concerns into two specific options: Potential programming problems Null reference: <E/W/I> Include potential nulls: <E/W/I> Unnecessary code Redundant null checks: <E/W/I> this breaks existing users who would rely on a single option to activate all checks, but Philippe argues that since the default was Ignore so far, we should not be causing too much grief; also, Include potential nulls should be capped by Null reference in the user interface (especially, having Null reference set to Ignore should disable the Include potential nulls control). Note that the redundant null checks are normally not needed on potential null values, hence there is no sub-choice for that case. The main advantage of that solution would be to enable the following scenario: Null reference: Error Include potential nulls: Warning which could especially make sense when combined with SuppressWarnings("null"). I will fup with JDT UI to see what they believe is best. On another topic, while we do complain when a variable that is known to be null is used within an instanceof test, we do not complain on casts per se. And complaining on: X x = null; if (x instanceof Y) { // x can only be null // do something } is definitely very close to complaining on: X x = null; if (x != null ) { // x can only be null if (x instanceof Y) { // do something } } which is equivalent at runtime (x being null, x instanceof Y will answer false). So I would stick to the idea that instanceof related checks are in the realm of redundant null checks, and that no option is needed to address them specifically.
Another case that we would place under the redundant checks option, even if it is not a check per se: if (x == null) { x = null; // x can only be null }
I think (b) is superior, since it allows the user to configure different severities for definite and potential problems. And I agree with Philippe that it should not be a problem if we "break" old workspaces (you still get new functionality for it!). I see a problem in the UI: It feels wrong to have the (non-potential) "Null reference" node below "Potential programming problems". I don't have a good solution yet, but maybe we should add a new top-level node like this: "Problems with 'null'" Null reference: <E/W/I> Include potential nulls: <E/W/I> Redundant null checks: <E/W/I>
*** Bug 138009 has been marked as a duplicate of this bug. ***
Re: backward compatibility issues with using new options One concern is for tools generating source code (e.g. EMF), I think they usually rely on defaults (Ignore) so we may be on the safe side there. Now, thoughts for the future... if we have these options, it feels like definite null refs should be flagged as warnings... but this needs to be assessed for source codegen tools (though likely would indicate a bug in the tool).
Regarding comment #9: among (a) and (b) our group is in favor of (b). Regarding comment #11: > I see a problem in the UI: It feels wrong to have the (non-potential) > "Null reference" node below "Potential programming problems". ... I agree. Given (b): > Potential programming problems > Null reference: <E/W/I> > Include potential nulls: <E/W/I> it seems peculiar to have a sub-subcategory named "include potentials..." under the general category of "Potential programming problems". One could consider introducing a top level "REAL programming problems" but genuine errors should be (and are usually) unconditionally reported. One conceptually cleaner solution that fits in with the current top-level categorization is to have: (c) Potential programming problems Null reference: <E/W/I> Unnecessary code Redundant null checks: <E/W/I> where "Null reference" would be understood to mean "Include potential nulls" (because it is under the "Potential programming problems" category). That would leave us with the compiler always reporting "Null dereferences" as a warning (error?) -- if needed, suppressed with @SuppressWarnings. NPEs are such a common programming fault, I cannot see why someone would want a null dereference to be silently ignored by the compiler if the compiler analysis determined that the receiver expression _is_ known to be null (even in the case of autogenerated code -- the only case that I can see this happening would be in dead or unreachable code, but then it should be annotated as such). (d) Yet another alternative which avoids the possible inconsistency in terminology: Potential programming problems Null reference: <E/[W]/I> Include "maybe null": <E/W/[I]> Unnecessary code Redundant null checks: <E/W/[I]>
One last thing about terminology, just to be clear: by "Null reference" do you mean 'checks of null dereferences' through expressions of the form exp.* and exp[*]? To me "reference" means 'to refer to' and hence "a == null" is an expression in which there is reference to null. In contrast, "dereference" means an explicit attempt to 'follow a pointer' in an attempt to access an object or value. Maybe I have my terminology wrong, but at least Wikipedia agrees with me -- http://en.wikipedia.org/wiki/Dereference ;-). Hence, I would recommend renaming "Null reference" to "Null dereference".
You are right that what we would track for 'Null reference' would now be null dereferences. In the existing code, it used to cover the whole null-related series of errors, hence was a rather good name. Now that we want to separate concerns, a more precise name would be welcome. Changing the option name would compell us to deprecate its former value though, which is a call I'll leave to Philippe, especially since we are a bit late in the 3.3 cycle. Even if we decided to keep the option name as it is today, the UI team may still consider adopting the better 'Null dereference' terminology in the UI. Regarding a change of the default from I to W for null dereferences (but not for potential null dereferences), we are debating about it. My own position would be that we know a few cases that look odd, even if the code could make the situation clearer, and that we'd need to give it a try on a few key code sets before making our decision. I'll do some tests in the large once I have the code ready.
A full source workspace build using only null dereferences option raised only three errors, which look correct (that is, the code is wrong). I opened bug 173432 against the affected component. I will build a core jar against HEAD and ask other interested parties to make a few tests before we discuss further the opportunity to activate the null dereference warning by default.
I have a jdt core plugin at hand that will only associate the null ref preference of the UI to the null dereference check (basically because we reuse the existing option for null dereference, and the UI does not position the two newer options yet). Users interested in testing this against their code to tell us whether activating the warning on null dereference by default would hurt them or not, please speak up. I'll attach the matching patch here as well as soon as the tests terminate on my machine.
Created attachment 58555 [details] Suggested fix + test cases This patch only addresses changes in JDT Core. Do not release without a matching JDT UI contribution (else the null checks and potential nulls won't get reported ever, since the corresponding options won't get set by the UI).
Suggested fix looks good to me. Please open a bug against JDT/UI when this is released so that we add the UI.
+1
Released for 3.3 M6.
Verified for 3.3 M6 using build I20070319-1335
*** Bug 155769 has been marked as a duplicate of this bug. ***
