Bug 423873 - [compiler][null] Should warn on unnecessary null checks.
Summary: [compiler][null] Should warn on unnecessary null checks.
Status: NEW
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.4   Edit
Hardware: PC Windows 7
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: JDT-Core-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-11 22:12 EST by Srikanth Sankaran CLA
Modified: 2013-12-12 05:28 EST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Srikanth Sankaran CLA 2013-12-11 22:12:45 EST
I see a fair amount of code of the form:

if (this.currentElement != null && this.currentElement instanceof RecoveredLocalVariable) {
    // ...
}

The null check is wasteful, since instanceof will never answer true for any
type for null references.

Compiler should catch and warn on these null checks.

I think Stephan has some code for "syntactic" null checks that should immediately
be useful here.
Comment 1 Stephan Herrmann CLA 2013-12-12 05:28:26 EST
Consider these

class X {
    Object field;

    void test(Object local) {
	if (this.field != null && this.field instanceof String) { // 1
	    // ...
	}
	if (this.field instanceof String && this.field != null) { // 2
	    // ...
	}
	if (local != null && local instanceof String) { // 3
	    // ...
	}
	if (local instanceof String && local != null) { // 4
	    // ...
	}
    }
}

Of these four locations, #2 and #4 can be flagged redundant (#2 indeed requires the option "enable syntactic null analysis for fields").

Cases #1 and #3 are not normally in the scope of flow analysis, because they would require to *look back*: at the location of the null-check it is not redundant, only in retrospective we can see that it was subsumed in the subsequent predicate.

So, if flow analysis is not a good tool for this, indeed speaking of syntactic checks would be the way to go: detect exactly this pattern:
- an '&&' expression
- left is a '!= null' expression
- right is an 'instanceof' expression
- left and right refer to the same variable

It would also need a new message explaining why the check is redundant.