From bc9b18cc6998aebe315e98de51c507aa71edb342 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Tue, 10 Apr 2018 17:07:19 +0200 Subject: [PATCH 1/2] [java] NullAssignment false positive Fixes #629 --- docs/pages/release_notes.md | 1 + .../rule/errorprone/NullAssignmentRule.java | 23 ++++-- .../rule/errorprone/xml/NullAssignment.xml | 71 +++++++++++++++++-- 3 files changed, 84 insertions(+), 11 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 7bccb2fbbe..3fcce04aea 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -114,6 +114,7 @@ Other languages are equivalent. * [#1003](https://github.com/pmd/pmd/issues/1003): \[java] UnnecessaryConstructor triggered on required empty constructor (Dagger @Inject) * [#1023](https://github.com/pmd/pmd/issues/1023): \[java] False positive for useless parenthesis * java-errorprone + * [#629](https://github.com/pmd/pmd/issues/629): \[java] NullAssignment false positive * [#816](https://github.com/pmd/pmd/issues/816): \[java] SingleMethodSingleton false positives with inner classes * java-performance * [#586](https://github.com/pmd/pmd/issues/586): \[java] AvoidUsingShortType erroneously triggered on overrides of 3rd party methods diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/NullAssignmentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/NullAssignmentRule.java index 9cbf691a06..195300d7f5 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/NullAssignmentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/NullAssignmentRule.java @@ -5,21 +5,25 @@ package net.sourceforge.pmd.lang.java.rule.errorprone; import net.sourceforge.pmd.lang.java.ast.ASTAssignmentOperator; +import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.java.ast.ASTConditionalExpression; import net.sourceforge.pmd.lang.java.ast.ASTEqualityExpression; import net.sourceforge.pmd.lang.java.ast.ASTExpression; import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTNullLiteral; +import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement; import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression; +import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer; import net.sourceforge.pmd.lang.java.ast.AccessNode; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; -// TODO - should check that this is not the first assignment. e.g., this is OK: -// Object x; -// x = null; public class NullAssignmentRule extends AbstractJavaRule { + public NullAssignmentRule() { + addRuleChainVisit(ASTNullLiteral.class); + } + @Override public Object visit(ASTNullLiteral node, Object data) { @@ -56,6 +60,17 @@ public class NullAssignmentRule extends AbstractJavaRule { } private boolean isBadTernary(ASTConditionalExpression n) { - return n.isTernary() && !(n.jjtGetChild(0) instanceof ASTEqualityExpression); + boolean isInitializer = false; + + ASTVariableInitializer variableInitializer = n.getFirstParentOfType(ASTVariableInitializer.class); + if (variableInitializer != null) { + ASTBlockStatement statement = n.getFirstParentOfType(ASTBlockStatement.class); + isInitializer = statement == variableInitializer.getFirstParentOfType(ASTBlockStatement.class); + } + + return n.isTernary() + && !(n.jjtGetChild(0) instanceof ASTEqualityExpression) + && !isInitializer + && !(n.getNthParent(2) instanceof ASTReturnStatement); } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/NullAssignment.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/NullAssignment.xml index bc183811ac..b3a1273fce 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/NullAssignment.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/NullAssignment.xml @@ -64,10 +64,8 @@ public class Foo { ]]> - - 1 + null assignment in ternary - initialization + 0 - + null assignment in ternary 1 + + + null assignment in ternary, part deux - initialization + 0 + + + + null assignment in ternary, part deux + 1 + @@ -129,4 +149,41 @@ public class Foo { } ]]> + + + [java] NullAssignment false positive - initialization #629 + 0 + test ? truthy : null); + } +} + ]]> + + + + [java] NullAssignment false positive - actual assignment #629 + 1 + test ? truthy : null); + } +} + ]]> + + + + [java] NullAssignment false positive - return with ternary #629 + 0 + + From fd71799c8310f28c69e4eda30241322a298c1b4e Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Mon, 23 Apr 2018 19:30:58 +0200 Subject: [PATCH 2/2] Don't consider null literal inside lambda --- .../java/rule/errorprone/NullAssignmentRule.java | 14 ++++++++------ .../java/rule/errorprone/xml/NullAssignment.xml | 4 ++-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/NullAssignmentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/NullAssignmentRule.java index 195300d7f5..668e98eca4 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/NullAssignmentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/NullAssignmentRule.java @@ -9,6 +9,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.java.ast.ASTConditionalExpression; import net.sourceforge.pmd.lang.java.ast.ASTEqualityExpression; import net.sourceforge.pmd.lang.java.ast.ASTExpression; +import net.sourceforge.pmd.lang.java.ast.ASTLambdaExpression; import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTNullLiteral; import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement; @@ -59,18 +60,19 @@ public class NullAssignmentRule extends AbstractJavaRule { && ((AccessNode) ((VariableNameDeclaration) name.getNameDeclaration()).getAccessNodeParent()).isFinal(); } - private boolean isBadTernary(ASTConditionalExpression n) { + private boolean isBadTernary(ASTConditionalExpression ternary) { boolean isInitializer = false; - ASTVariableInitializer variableInitializer = n.getFirstParentOfType(ASTVariableInitializer.class); + ASTVariableInitializer variableInitializer = ternary.getFirstParentOfType(ASTVariableInitializer.class); if (variableInitializer != null) { - ASTBlockStatement statement = n.getFirstParentOfType(ASTBlockStatement.class); + ASTBlockStatement statement = ternary.getFirstParentOfType(ASTBlockStatement.class); isInitializer = statement == variableInitializer.getFirstParentOfType(ASTBlockStatement.class); } - return n.isTernary() - && !(n.jjtGetChild(0) instanceof ASTEqualityExpression) + return ternary.isTernary() + && !(ternary.jjtGetChild(0) instanceof ASTEqualityExpression) && !isInitializer - && !(n.getNthParent(2) instanceof ASTReturnStatement); + && !(ternary.getNthParent(2) instanceof ASTReturnStatement) + && !(ternary.getNthParent(2) instanceof ASTLambdaExpression); } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/NullAssignment.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/NullAssignment.xml index b3a1273fce..ef656c0d50 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/NullAssignment.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/NullAssignment.xml @@ -163,8 +163,8 @@ public class NullAssignmentFP { - [java] NullAssignment false positive - actual assignment #629 - 1 + [java] NullAssignment false positive - no direct assignment, but lambda #629 + 0