From 75dff6418f4446df6c667893f8d9d2c40a654343 Mon Sep 17 00:00:00 2001 From: Bas Leijdekkers Date: Tue, 17 Nov 2015 10:43:45 +0100 Subject: [PATCH] IDEA-147395 (Slf4j placeholder inspection should be able to handle simple concatenation in logStringArgument) --- .../siyeh/InspectionGadgetsBundle.properties | 4 +- ...erCountMatchesArgumentCountInspection.java | 60 +++++++++++++++---- ...untMatchesArgumentCountInspectionTest.java | 21 +++++-- 3 files changed, 68 insertions(+), 17 deletions(-) diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/InspectionGadgetsBundle.properties b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/InspectionGadgetsBundle.properties index 861e97abaa70..c317c9724f73 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/InspectionGadgetsBundle.properties +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/InspectionGadgetsBundle.properties @@ -1996,8 +1996,8 @@ string.concatenation.argument.to.log.call.display.name=Non-constant string conca string.concatenation.argument.to.log.call.problem.descriptor=Non-constant string concatenation as argument to #ref() logging call #loc string.concatenation.argument.to.log.call.quickfix=Replace concatenation with parameterized log message placeholder.count.matches.argument.count.display.name=Number of placeholders does not match number of arguments in logging call -placeholder.count.matches.argument.count.more.problem.descriptor=More arguments provided ({0}) than placeholders specified ({1}) in ''{2}'' #loc -placeholder.count.matches.argument.count.fewer.problem.descriptor=Fewer arguments provided ({0}) than placeholders specified ({1}) in ''{2}'' #loc +placeholder.count.matches.argument.count.more.problem.descriptor=More arguments provided ({0}) than placeholders specified ({1}) #loc +placeholder.count.matches.argument.count.fewer.problem.descriptor=Fewer arguments provided ({0}) than placeholders specified ({1}) #loc assignment.to.superclass.field.display.name=Constructor assigns value to field defined in superclass assignment.to.superclass.field.problem.descriptor=Assignment to field ''{0}'' defined in superclass ''{1}'' #loc junit.rule.display.name=Malformed @Rule/@ClassRule field diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/logging/PlaceholderCountMatchesArgumentCountInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/logging/PlaceholderCountMatchesArgumentCountInspection.java index 51b050bb4198..3ad4f7b80e4a 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/logging/PlaceholderCountMatchesArgumentCountInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/logging/PlaceholderCountMatchesArgumentCountInspection.java @@ -45,14 +45,13 @@ public class PlaceholderCountMatchesArgumentCountInspection extends BaseInspecti protected String buildErrorString(Object... infos) { final Integer argumentCount = (Integer)infos[0]; final Integer placeholderCount = (Integer)infos[1]; - final Object value = infos[2]; if (argumentCount.intValue() > placeholderCount.intValue()) { return InspectionGadgetsBundle.message("placeholder.count.matches.argument.count.more.problem.descriptor", - argumentCount, placeholderCount, value); + argumentCount, placeholderCount); } else { return InspectionGadgetsBundle.message("placeholder.count.matches.argument.count.fewer.problem.descriptor", - argumentCount, placeholderCount, value); + argumentCount, placeholderCount); } } @@ -96,8 +95,7 @@ public class PlaceholderCountMatchesArgumentCountInspection extends BaseInspecti else { argumentCount = countArguments(arguments, 1); } - final Object value = ExpressionUtils.computeConstantExpression(logStringArgument); - final int placeholderCount = countPlaceholders(value); + final int placeholderCount = countPlaceholders(logStringArgument); if (placeholderCount < 0 || argumentCount < 0 || placeholderCount == argumentCount) { return; } @@ -106,7 +104,7 @@ public class PlaceholderCountMatchesArgumentCountInspection extends BaseInspecti // the exception, then the stack trace won't be logged. return; } - registerError(logStringArgument, Integer.valueOf(argumentCount), Integer.valueOf(placeholderCount), value); + registerError(logStringArgument, Integer.valueOf(argumentCount), Integer.valueOf(placeholderCount)); } private static boolean hasThrowableType(PsiExpression lastArgument) { @@ -123,11 +121,53 @@ public class PlaceholderCountMatchesArgumentCountInspection extends BaseInspecti return InheritanceUtil.isInheritor(type, CommonClassNames.JAVA_LANG_THROWABLE); } - public static int countPlaceholders(Object value) { - if (!(value instanceof String)) { - return -1; + public static int countPlaceholders(PsiExpression expression) { + final Object value = ExpressionUtils.computeConstantExpression(expression); + if (value == null) { + final StringBuilder builder = new StringBuilder(); + return buildString(expression, builder) ? countPlaceholders(builder.toString()) : -1; } - final String string = (String)value; + return value instanceof String ? countPlaceholders((String)value) : 0; + } + + private static boolean buildString(PsiExpression expression, StringBuilder builder) { + if (expression instanceof PsiParenthesizedExpression) { + final PsiParenthesizedExpression parenthesizedExpression = (PsiParenthesizedExpression)expression; + return buildString(parenthesizedExpression.getExpression(), builder); + } + else if (expression instanceof PsiPolyadicExpression) { + if (!ExpressionUtils.hasStringType(expression)) { + return false; + } + final PsiPolyadicExpression polyadicExpression = (PsiPolyadicExpression)expression; + for (PsiExpression operand : polyadicExpression.getOperands()) { + if (!buildString(operand, builder)) { + return false; + } + } + return true; + } + else if (expression instanceof PsiLiteralExpression) { + if (ExpressionUtils.hasStringType(expression)) { + final PsiLiteralExpression literalExpression = (PsiLiteralExpression)expression; + builder.append(literalExpression.getValue()); + } + return true; + } + else { + if (!ExpressionUtils.hasStringType(expression)) { + return true; + } + final Object value = ExpressionUtils.computeConstantExpression(expression); + if (value == null) { + return false; + } + builder.append(value); + return true; + } + } + + private static int countPlaceholders(String string) { int count = 0; int index = string.indexOf("{}"); while (index >= 0) { diff --git a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/logging/PlaceholderCountMatchesArgumentCountInspectionTest.java b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/logging/PlaceholderCountMatchesArgumentCountInspectionTest.java index 0ea2750366cf..bde8289d249b 100644 --- a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/logging/PlaceholderCountMatchesArgumentCountInspectionTest.java +++ b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/logging/PlaceholderCountMatchesArgumentCountInspectionTest.java @@ -21,7 +21,7 @@ public class PlaceholderCountMatchesArgumentCountInspectionTest extends LightIns "class X {" + " void foo() {" + " RuntimeException e = new RuntimeException();" + - " LoggerFactory.getLogger(X.class).info(/*Fewer arguments provided (0) than placeholders specified (1) in 'this: {}'*/\"this: {}\"/**/, e);" + + " LoggerFactory.getLogger(X.class).info(/*Fewer arguments provided (0) than placeholders specified (1)*/\"this: {}\"/**/, e);" + " }" + "}"); } @@ -41,7 +41,7 @@ public class PlaceholderCountMatchesArgumentCountInspectionTest extends LightIns "class X {" + " void foo() {" + " RuntimeException e = new RuntimeException();" + - " LoggerFactory.getLogger(X.class).info(/*Fewer arguments provided (1) than placeholders specified (3) in '1: {} {} {}'*/\"1: {} {} {}\"/**/, 1, e);" + + " LoggerFactory.getLogger(X.class).info(/*Fewer arguments provided (1) than placeholders specified (3)*/\"1: {} {} {}\"/**/, 1, e);" + " }" + "}"); } @@ -62,7 +62,7 @@ public class PlaceholderCountMatchesArgumentCountInspectionTest extends LightIns "class X {\n" + " void foo() {\n" + " Logger logger = LoggerFactory.getLogger(X.class);\n" + - " logger.info(/*Fewer arguments provided (1) than placeholders specified (2) in 'string {}{}'*/\"string {}{}\"/**/, 1);\n" + + " logger.info(/*Fewer arguments provided (1) than placeholders specified (2)*/\"string {}{}\"/**/, 1);\n" + " }\n" + "}" ); @@ -73,7 +73,7 @@ public class PlaceholderCountMatchesArgumentCountInspectionTest extends LightIns "class X {\n" + " void foo() {\n" + " Logger logger = LoggerFactory.getLogger(X.class);\n" + - " logger.info(/*More arguments provided (1) than placeholders specified (0) in 'string'*/\"string\"/**/, 1);\n" + + " logger.info(/*More arguments provided (1) than placeholders specified (0)*/\"string\"/**/, 1);\n" + " }\n" + "}" ); @@ -144,7 +144,18 @@ public class PlaceholderCountMatchesArgumentCountInspectionTest extends LightIns " Logger LOG = LoggerFactory.getLogger(X.class);" + " private static final String message = \"HELLO {}\";" + " void m() {" + - " LOG.info(/*Fewer arguments provided (0) than placeholders specified (1) in 'HELLO {}'*/message/**/);" + + " LOG.info(/*Fewer arguments provided (0) than placeholders specified (1)*/message/**/);" + + " }" + + "}"); + } + + public void testNonConstantString() { + doTest("import org.slf4j.*;" + + "class X {" + + " Logger LOG = LoggerFactory.getLogger(X.class);" + + " private static final String S = \"{}\";" + + " void m() {" + + " LOG.info(/*Fewer arguments provided (0) than placeholders specified (2)*/S +\"{}\" + Integer.class/**/);" + " }" + "}"); }