Skip to content

Commit

Permalink
Merge branch 'pr-2284'
Browse files Browse the repository at this point in the history 
[java] MisplacedNullCheck - fix false positive
  • Loading branch information
@adangel
adangel committed Feb 29, 2020
2 parents d21e256 + 3dbbb8a commit 30b6f14
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 44 deletions.
1 change: 1 addition & 0 deletions docs/pages/release_notes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ the suppressions with a `NOPMD` comment. See [Suppressing warnings](pmd_userdocs
* java-bestpractices
* [#2277](https://github.com/pmd/pmd/issues/2277): \[java] FP in UnusedImports for ambiguous static on-demand imports
* java-errorprone
* [#2242](https://github.com/pmd/pmd/issues/2242): \[java] False-positive MisplacedNullCheck reported
* [#2250](https://github.com/pmd/pmd/issues/2250): \[java] InvalidLogMessageFormat flags logging calls using a slf4j-Marker
* [#2255](https://github.com/pmd/pmd/issues/2255): \[java] InvalidLogMessageFormat false-positive for a lambda argument
* java-performance
Expand Down
53 changes: 32 additions & 21 deletions pmd-java/src/main/resources/category/java/errorprone.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2288,7 +2288,7 @@ public class MyClass {
<rule name="MisplacedNullCheck"
language="java"
since="3.5"
message="The null check here is misplaced; if the variable is null there will be a NullPointerException"
message="The null check here is misplaced; if the variable ''{0}'' is null there will be a NullPointerException"
class="net.sourceforge.pmd.lang.rule.XPathRule"
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_errorprone.html#misplacednullcheck">
<description>
Expand All @@ -2297,26 +2297,33 @@ Either the check is useless (the variable will never be "null") or it is incorre
</description>
<priority>3</priority>
<properties>
<property name="version" value="2.0"/>
<property name="xpath">
<value>
<![CDATA[
//Expression
/*[self::ConditionalOrExpression or self::ConditionalAndExpression]
/descendant::PrimaryExpression/PrimaryPrefix
/Name[starts-with(@Image,
concat(ancestor::PrimaryExpression/following-sibling::EqualityExpression
[./PrimaryExpression/PrimaryPrefix/Literal/NullLiteral]
/PrimaryExpression/PrimaryPrefix
/Name[count(../../PrimarySuffix)=0]/@Image,".")
)
]
[count(ancestor::ConditionalAndExpression/EqualityExpression
[@Image='!=']
[./PrimaryExpression/PrimaryPrefix/Literal/NullLiteral]
[starts-with(following-sibling::*/PrimaryExpression/PrimaryPrefix/Name/@Image,
concat(./PrimaryExpression/PrimaryPrefix/Name/@Image, '.'))]
) = 0
]
//ConditionalAndExpression
/EqualityExpression
[@Image = '!=']
(: one side is null :)
[PrimaryExpression/PrimaryPrefix/Literal/NullLiteral]
(: other side checks for the variable used somewhere in the first child of conditional and expression :)
[some $var in preceding-sibling::PrimaryExpression//Name
[not(ancestor::ConditionalOrExpression/EqualityExpression[@Image = '=='])]
/@Image
satisfies starts-with($var, concat(PrimaryExpression/PrimaryPrefix/Name/@Image, '.'))]
/PrimaryExpression/PrimaryPrefix/Name
|
//ConditionalOrExpression
/EqualityExpression
[@Image = '==']
(: one side is null :)
[PrimaryExpression/PrimaryPrefix/Literal/NullLiteral]
(: other side checks for the variable used somewhere in the first child of conditional or expression :)
[some $var in preceding-sibling::PrimaryExpression//Name
[not(ancestor::ConditionalAndExpression/EqualityExpression[@Image = '!='])]
/@Image
satisfies starts-with($var, concat(PrimaryExpression/PrimaryPrefix/Name/@Image, '.'))]
/PrimaryExpression/PrimaryPrefix/Name
]]>
</value>
</property>
Expand All @@ -2325,16 +2332,20 @@ Either the check is useless (the variable will never be "null") or it is incorre
<![CDATA[
public class Foo {
void bar() {
if (a.equals(baz) && a != null) {}
}
if (a.equals(baz) && a != null) {} // a could be null, misplaced null check
if (a != null && a.equals(baz)) {} // correct null check
}
}
]]>
</example>
<example>
<![CDATA[
public class Foo {
void bar() {
if (a.equals(baz) || a == null) {}
if (a.equals(baz) || a == null) {} // a could be null, misplaced null check
if (a == null || a.equals(baz)) {} // correct null check
}
}
]]>
Expand Down
134 changes: 111 additions & 23 deletions ...c/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/MisplacedNullCheck.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,61 @@
xmlns="http://pmd.sourceforge.net/rule-tests"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">

<test-code>
<description><![CDATA[
null check after method invocation with conditional AND and !=
]]></description>
<expected-problems>1</expected-problems>
<description>null check after method invocation with conditional AND and !=</description>
<expected-problems>4</expected-problems>
<expected-linenumbers>3,4,6,7</expected-linenumbers>
<expected-messages>
<message>The null check here is misplaced; if the variable 'a' is null there will be a NullPointerException</message>
<message>The null check here is misplaced; if the variable 'a' is null there will be a NullPointerException</message>
<message>The null check here is misplaced; if the variable 'a' is null there will be a NullPointerException</message>
<message>The null check here is misplaced; if the variable 'a' is null there will be a NullPointerException</message>
</expected-messages>
<code><![CDATA[
public class Foo {
void bar() {
if (a.equals(baz) && a!=null) {}
}
void bar() {
if (a.equals(baz) && a != null) {} // a could be null, misplaced null check
if (a.equals(baz) || a == null) {} // a could be null, misplaced null check
if (a.equals(baz) && null != a) {} // a could be null, misplaced null check
if (a.equals(baz) || null == a) {} // a could be null, misplaced null check
if (a != null && a.equals(baz)) {} // correct null check
if (a == null || a.equals(baz)) {} // correct null check
}
}
]]></code>
</test-code>

<test-code>
<description><![CDATA[
null check after nested method invocation
]]></description>
<expected-problems>1</expected-problems>
<description>null check after nested method invocation</description>
<expected-problems>4</expected-problems>
<expected-linenumbers>3,4,6,7</expected-linenumbers>
<expected-messages>
<message>The null check here is misplaced; if the variable 'baz' is null there will be a NullPointerException</message>
<message>The null check here is misplaced; if the variable 'baz' is null there will be a NullPointerException</message>
<message>The null check here is misplaced; if the variable 'baz' is null there will be a NullPointerException</message>
<message>The null check here is misplaced; if the variable 'baz' is null there will be a NullPointerException</message>
</expected-messages>
<code><![CDATA[
public class Foo {
void bar() {
if (a.equals(baz.foo()) && baz != null) {}
}
void bar() {
if (a.equals(baz.foo()) && baz != null) {} // baz could be null, misplaced null check
if (a.equals(baz.foo()) || baz == null) {} // baz could be null, misplaced null check
if (a.equals(baz.foo()) && null != baz) {} // baz could be null, misplaced null check
if (a.equals(baz.foo()) || null == baz) {} // baz could be null, misplaced null check
if (baz != null && a.equals(baz.foo())) {} // correct null check
if (baz == null || a.equals(baz.foo())) {} // correct null check
}
}
]]></code>
</test-code>

<test-code>
<description><![CDATA[
null check before nested method invocation
]]></description>
<description>null check before nested method invocation</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
Expand All @@ -42,11 +67,14 @@ public class Foo {
}
]]></code>
</test-code>

<test-code>
<description><![CDATA[
1610730: null check after method invocation with conditional OR and ==
]]></description>
<description>1610730: null check after method invocation with conditional OR and ==</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>3</expected-linenumbers>
<expected-messages>
<message>The null check here is misplaced; if the variable 'a' is null there will be a NullPointerException</message>
</expected-messages>
<code><![CDATA[
public class Foo {
void bar() {
Expand All @@ -55,10 +83,9 @@ public class Foo {
}
]]></code>
</test-code>

<test-code>
<description><![CDATA[
3372128: False positive: ArrayIsStoredDirectly
]]></description>
<description>3372128: False positive: ArrayIsStoredDirectly</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
Expand All @@ -72,6 +99,7 @@ public class Foo {
}
]]></code>
</test-code>

<test-code>
<description>#977 MisplacedNullCheck makes false positives</description>
<expected-problems>0</expected-problems>
Expand All @@ -81,8 +109,68 @@ public class Test {
if ((value != null && !value.equals(oldValue)) || value == null) {
// Do something
}
if ((value == null || !value.equals(oldValue)) && value != null) {}
}
}
]]></code>
</test-code>

<test-code>
<description>#2242 False-positive MisplacedNullCheck reported (1)</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Test {
String field;
Test(String f) { field = f; }
public boolean method(final String value) {
boolean c = value != null
&& ((field != null && !value.equals(field.toString())) || field == null);
return c;
}
}
]]></code>
</test-code>

<test-code>
<description>#2242 False-positive MisplacedNullCheck reported (2)</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Test {
String field;
Test(String f) { field = f; }
public boolean method(final String value) {
boolean c = value != null
&& (field == null || (field != null && !value.equals(field.toString())));
return c;
}
}
]]></code>
</test-code>

<test-code>
<description>False-positive/negative with multiple conditions</description>
<expected-problems>2</expected-problems>
<expected-linenumbers>7,8</expected-linenumbers>
<expected-messages>
<message>The null check here is misplaced; if the variable 'attributes' is null there will be a NullPointerException</message>
<message>The null check here is misplaced; if the variable 'attributes' is null there will be a NullPointerException</message>
</expected-messages>
<code><![CDATA[
public class Test {
public void method(String annotationType, Map<String, Object> attributes) {
boolean isStereotype = annotationType.equals("javax.inject.Named");
if (isStereotype && attributes != null && attributes.containsKey("value")) {}
if (isStereotype || attributes == null || attributes.containsKey("value")) {}
if (isStereotype && attributes.containsKey("value") && attributes != null) {}
if (isStereotype || attributes.containsKey("value") || attributes == null) {}
}
}
]]></code>
</test-code>
</test-data>

0 comments on commit 30b6f14

Please sign in to comment.