Community
Participate
Working Groups
Build Identifier: 3.7.3.v20120119-1537 public class X { void foo() { Object a = null, b = null; while (a == null || b == null) { // must not complain here b = a; a = new Object(); } } } public class Y { void foo() { Object a = null, b = null; do { b = a; a = new Object(); } while (a == null || b == null); // must not complain here } } public class Z { void foo() { Object a = null, b = null; for (; a == null || b == null; ) { // must not complain here b = a; a = new Object(); } } } Each example results in "Redundant null check: The variable b can only be null at this location". Reproducible: Always
Stephan, please follow up, thanks.
Wow, fixed number of passes during loop analysis is hitting the cealing. We need to do s.t. about this.
(In reply to comment #2) > Wow, fixed number of passes during loop analysis is hitting the cealing. > We need to do s.t. about this. Sorry, this is unclear. Could you elaborate upon this Stephan ?
(In reply to comment #3) > (In reply to comment #2) > > Wow, fixed number of passes during loop analysis is hitting the cealing. > > We need to do s.t. about this. > > Sorry, this is unclear. Could you elaborate upon this Stephan ? We only analyse the loop statements one time, and record every comparison against null for deferred checking. Then we make another pass on only the deferred variables and see how their nullness was affected by all statements in the loop. But that doesn't help in all cases. Eg, here in the first pass, b's nullness changed only once, at b = a. But in first iteration a was only null at that point. So, when we come back to b for the deferred check, we see (1) it was originally null, and (2) it was assigned null in the loop. So we complain. If we'd made a second pass, we'd have seen an assignment to a non-null value as well. :)
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Wow, fixed number of passes during loop analysis is hitting the cealing. > > > We need to do s.t. about this. > > > > Sorry, this is unclear. Could you elaborate upon this Stephan ? > > We only analyse the loop statements one time, and record every comparison > against null for deferred checking. Then we make another pass on only the > deferred variables and see how their nullness was affected by all statements in > the loop. But that doesn't help in all cases. Thanks for the explanation, Ayush. That tells me that this issue has existed from day one and is not a recent regression. Can you confirm ? If so, then I would target this for early 3.9.
(In reply to comment #5) > Thanks for the explanation, Ayush. That tells me that this issue has existed > from day one and is not a recent regression. Can you confirm ? If so, then > I would target this for early 3.9. I agree with Ayush's explanation and I'm pretty sure this never worked. Still this is an ugly bug and I'll start experimenting with possible solutions as soon as I find the time. I'll post an update once I can estimate the complexity of required changes.
Naively I would think it sufficient, to iterate any loop until no variable changes nullness any more. But on second thought I wonder how that performs on a loop like this: Object a = null, b = new Object(); while (...) { Object temp = a; a = b; b = temp; } As the nullness toggles on every iteration, my naive suggestion won't ever finish analyzing that loop?!?
Not marking as blocking the new umbrella bug 453483, because this one smells like correlation analysis, which is significantly harder than bug 453483 (which is already hard enough).
I'm not sure if this is the same issue, but here's another example where null-analysis is wrong for the loop (also there's another null-annotation bug with 'array' field): @org.eclipse.jdt.annotation.NonNullByDefault public class TestNullAnno { // @NonNull // Uncommenting this causes warning: The nullness annotation is redundant with a default that applies to this location // Not uncommenting this causes warning in getArrayValue() below private static String[] array = new @NonNull String[3]; public static void main(String[] args) { Map<String, String> map = new HashMap<>(); map.put("key", "value"); System.out.println(getKeyByValue(map, "value")); } private static <K, V> K getKeyByValue(Map<K, V> map, @Nullable V value) { @Nullable K result = null; // some nullability bug? assigning null results in compiler complaining 'result can only be null' below for (Entry<K, V> entry : map.entrySet()) { boolean equals; if (value == null) equals = (entry.getValue() == null); else equals = value.equals(entry.getValue()); if (equals) { if (result == null) // Incorrect warning: Redundant null check: The variable result can only be null at this location result = entry.getKey(); else throw new IllegalStateException("Multiple matches for looking up key via value [" + value + "]: [" + result + "] and [" + entry.getKey() + "]"); } } if (result == null) // Incorrect warning: Redundant null check: The variable result can only be null at this location throw new IllegalStateException("No matches for looking up key via value [" + value + "]"); return result; // Incorrect warning: Dead code } public static String getArrayValue() { return array[0]; // If array is not annotated in declaration, we get warning here: Null type safety (type annotations): The expression of type 'String' needs unchecked conversion to conform to '@NonNull String' } }
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
FWIW, the examples in comment 0 still raise undesired warnings, but nobody came up with an implementation strategy to this fix. The example from comment 9 causes no problems using latest ecj (today's HEAD).
Reopening because the issue with the examples in comment 0 still exists. Ayushman Jain suggested this implementation strategy for a fix: Make two passes that analyze the loop so that assignments of the first pass can be considered during the second pass. I support this. This will not fix cases that require more than two passes, but those are very, very rare.
(In reply to Julian Ladisch from comment #12) > Reopening because the issue with the examples in comment 0 still exists. > > Ayushman Jain suggested this implementation strategy for a fix: > Make two passes that analyze the loop so that assignments of the first pass > can be considered during the second pass. > > I support this. > > This will not fix cases that require more than two passes, but those are > very, very rare. be our guest :) Unfortunately, even the two pass approach would require huge code changes, because the current one-and-a-half passes implementation is already quite complex. Note that the entire analyseCode phase is implemented in a single traversal of all AST nodes. It will be hard also to allow some statements to be analysed twice. I should have said "nobody came up with a *feasible* implementation strategy". Two phase analysis is theoretically useful, but not feasible when considering the necessary efforts. Sorry.
