From 74ed544500e1a79a8b80ad28024402c044a2d975 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Mon, 13 Aug 2018 09:14:36 +0200 Subject: [PATCH] [java] InvalidSlf4jMessageFormat false positive: too many arguments with string concatenation operator Fixes #1291 If there are multiple Literals for the first parameter, we assume, they are concatenated together and sum up all placeholders. Such String literals are concatenated already by the java compiler, so splitting a String in that way has no negative impact. This change also makes use of the RuleChain. --- docs/pages/release_notes.md | 1 + .../InvalidSlf4jMessageFormatRule.java | 38 ++++++++++++------- .../xml/InvalidSlf4jMessageFormat.xml | 25 +++++++++++- 3 files changed, 49 insertions(+), 15 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 65deb54c44..7e421bc5d0 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -45,6 +45,7 @@ This is a minor release. * java-errorprone * [#1078](https://github.com/pmd/pmd/issues/1078): \[java] MissingSerialVersionUID rule does not seem to catch inherited classes * java-performance + * [#1291](https://github.com/pmd/pmd/issues/1291): \[java] InvalidSlf4jMessageFormat false positive: too many arguments with string concatenation operator * [#1298](https://github.com/pmd/pmd/issues/1298): \[java] RedundantFieldInitializer - NumberFormatException with Long * jsp * [#1274](https://github.com/pmd/pmd/issues/1274): \[jsp] Support EL in tag attributes diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/InvalidSlf4jMessageFormatRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/InvalidSlf4jMessageFormatRule.java index 86ab80b76c..a3a56cf22e 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/InvalidSlf4jMessageFormatRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/InvalidSlf4jMessageFormatRule.java @@ -42,18 +42,22 @@ public class InvalidSlf4jMessageFormatRule extends AbstractJavaRule { .unmodifiableSet(new HashSet(Arrays.asList("trace", "debug", "info", "warn", "error"))); } + public InvalidSlf4jMessageFormatRule() { + addRuleChainVisit(ASTName.class); + } + @Override public Object visit(final ASTName node, final Object data) { final NameDeclaration nameDeclaration = node.getNameDeclaration(); // ignore imports or methods if (!(nameDeclaration instanceof VariableNameDeclaration)) { - return super.visit(node, data); + return data; } // ignore non slf4j logger Class type = ((VariableNameDeclaration) nameDeclaration).getType(); if (type == null || !type.getName().equals(LOGGER_CLASS)) { - return super.visit(node, data); + return data; } // get the node that contains the logger @@ -65,7 +69,7 @@ public class InvalidSlf4jMessageFormatRule extends AbstractJavaRule { // ignore if not a log level if (!LOGGER_LEVELS.contains(method)) { - return super.visit(node, data); + return data; } // find the arguments @@ -73,13 +77,13 @@ public class InvalidSlf4jMessageFormatRule extends AbstractJavaRule { .getFirstDescendantOfType(ASTArgumentList.class).findChildrenOfType(ASTExpression.class); // remove the message parameter - final ASTPrimaryExpression messageParam = argumentList.remove(0).getFirstDescendantOfType(ASTPrimaryExpression.class); + final ASTExpression messageParam = argumentList.remove(0); final int expectedArguments = expectedArguments(messageParam); if (expectedArguments == 0) { // ignore if we are not expecting arguments to format the message // or if we couldn't analyze the message parameter - return super.visit(node, data); + return data; } // Remove throwable param, since it is shown separately. @@ -89,12 +93,14 @@ public class InvalidSlf4jMessageFormatRule extends AbstractJavaRule { } if (argumentList.size() < expectedArguments) { - addViolationWithMessage(data, node, "Missing arguments," + getExpectedMessage(argumentList, expectedArguments)); + addViolationWithMessage(data, node, + "Missing arguments," + getExpectedMessage(argumentList, expectedArguments)); } else if (argumentList.size() > expectedArguments) { - addViolationWithMessage(data, node, "Too many arguments," + getExpectedMessage(argumentList, expectedArguments)); + addViolationWithMessage(data, node, + "Too many arguments," + getExpectedMessage(argumentList, expectedArguments)); } - return super.visit(node, data); + return data; } private boolean isNewThrowable(ASTPrimaryExpression last) { @@ -145,7 +151,7 @@ public class InvalidSlf4jMessageFormatRule extends AbstractJavaRule { + params.size(); } - private int expectedArguments(final ASTPrimaryExpression node) { + private int expectedArguments(final ASTExpression node) { int count = 0; // look if the logger have a literal message if (node.getFirstDescendantOfType(ASTLiteral.class) != null) { @@ -153,7 +159,8 @@ public class InvalidSlf4jMessageFormatRule extends AbstractJavaRule { } else if (node.getFirstDescendantOfType(ASTName.class) != null) { final String variableName = node.getFirstDescendantOfType(ASTName.class).getImage(); // look if the message is defined locally - final List localVariables = node.getFirstParentOfType(ASTMethodOrConstructorDeclaration.class) + final List localVariables = node + .getFirstParentOfType(ASTMethodOrConstructorDeclaration.class) .findDescendantsOfType(ASTVariableDeclarator.class); count = getAmountOfExpectedArguments(variableName, localVariables); @@ -183,10 +190,13 @@ public class InvalidSlf4jMessageFormatRule extends AbstractJavaRule { } private int countPlaceholders(final AbstractJavaTypeNode node) { - int result = 0; // zero means, no placeholders, or we could not analyze the message parameter - ASTLiteral stringLiteral = node.getFirstDescendantOfType(ASTLiteral.class); - if (stringLiteral != null) { - result = StringUtils.countMatches(stringLiteral.getImage(), "{}"); + // zero means, no placeholders, or we could not analyze the message parameter + int result = 0; + List literals = node.findDescendantsOfType(ASTLiteral.class); + // if there are multiple literals, we just assume, they are concatenated + // together... + for (ASTLiteral stringLiteral : literals) { + result += StringUtils.countMatches(stringLiteral.getImage(), "{}"); } return result; } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/InvalidSlf4jMessageFormat.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/InvalidSlf4jMessageFormat.xml index 4163fdb66c..7dd73838b5 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/InvalidSlf4jMessageFormat.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/InvalidSlf4jMessageFormat.xml @@ -240,5 +240,28 @@ public class Foo ]]> - + + #1291 [java] InvalidSlf4jMessageFormat false positive: too many arguments with string concatenation operator + 0 + + \ No newline at end of file