Community
Participate
Working Groups
20070226 if (l == null) { if (l == null) { } } The current error message is: 'The variable l can only be null; it was either set to null or checked for null when last used' Better say: 'Redundant null check: The variable l can only be null at this location'
Need to check if null pointer scenario is using the same message.
I have separated the messages for bug 170704, so we should be safe.
There is at least one case though for which the new message looks odd (at best): Object o = null; if (o instanceof String) { ... Would tell us: 'Redundant null check: The variable o can only be null at this location'. But then if we want better than that, we need to bring in more changes than I'd wish to do (especially because some checks are memorized in deeper contexts; introducing a new error type will call for duplication here). A trick could be to try to detect the instanceof at the time at which we elaborate the error message, but it may miss precision in some cases. Possible courses of action: a) don't change anything for now; our messages are not perfect, but they are reflecting the reality; b) fix our messages along the lines of this bug request, and open a separate bug to address the instanceof case later on (preferably before M6); c) defer this fix until we get it right, still hoping to do it in M6 timeframe. I will pursue c for now.
Another case in which the new message would look odd: Object o = null; o = null; This leaves us with the need for three different messages for the 'redundant null check' cases. Asked Philippe to decide if we want to move forward in 3.3 timeframe (in bug 175570).
The compiler preference for all these warnings is 'Redundant null check'. So maybe you want to find a better description here too. It's just strange that the problem message isn't really naming the problem. It's just making a statement and the user first has to look at the code to see why the compiler made this statement. I guess especially in a code audit report this isn't helpful.
What about 'Unnecessary code as variable 'l' is null at this location' ? The compiler UI could be renamed to: 'Unnecessary code related to null references'
The thing is, redundant null checks represent way over 90% of all cases (could run a full workspace build to quantify that 'intuition', but I do not think I am too widely far from the mark here). The truth is that none of us spontaneously thought of the other cases. Hence 'Redundant null check' is the most representative way of talking about the things we want to track. The instanceof case is a happy side effect (if we happen to get a null, then we can deduce something the user may not have noticed). The null assignment warning is just grabing an odd case because we can (I do not think people so often assign to null twice, and this does not cause too much grief when they do so). So I would be in favour of keeping the broader category unchanged, and adding more natural messages for the instanceof and assignment cases. I'll leave Philippe the final word since he asked me to consider reverting bug 170704 in a chat, possible action plans now being: a) revert 170704; b) keep 170704 as is; c) evolve 170704 as described in bug 175570 comment #4; d) consider fixing 175570 and 175571, d1) without changing constants; d2) aligning constants to better reflect semantics (needs PMC approval?).
To clarify, Maxime I wasn't referring to undo bug 170074; I was only talking about bug 175570 in the case we cannot achieve a good situation.
The 'redundant null check' is OK on the following code: o = null; if (o == null) {} Now, on that one: o = null; if (o != null) { // some code } it no more seems so natural, and may lead the user to believe he should suppress the test. Something along the lines of 'Null comparison always yields false: The variable o can only be null at this location' would probably be better. But this introduces two more problem IDs.
Released for 3.3 M6.
Verified for 3.3 M6 using build I20070320-0010.
