IG: report more problems in "Malformed format string" inspection (fixes IDEA-83792)

plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/InspectionGadgetsBundle.properties

@@ -100,9 +100,10 @@ iterator.hasnext.which.calls.next.problem.descriptor=<code>Iterator.#ref()</code
 iterator.next.does.not.throw.nosuchelementexception.display.name='Iterator.next()' which can't throw 'NoSuchElementException'
 malformed.format.string.display.name=Malformed format string
 malformed.format.string.problem.descriptor.malformed=Format string <code>#ref</code> is malformed #loc
-malformed.format.string.problem.descriptor.too.many.arguments=Too many arguments for format string <code>#ref</code> #loc
-malformed.format.string.problem.descriptor.too.few.arguments=Too few arguments for format string <code>#ref</code> #loc
-malformed.format.string.problem.descriptor.arguments.do.not.match.type=Format string <code>#ref</code> does not match the type of its arguments #loc
+malformed.format.string.problem.descriptor.illegal=Illegal format string specifier ''{0}'' #loc
+malformed.format.string.problem.descriptor.too.many.arguments=Too many arguments for format string (found: {0}, expected: {1}) #loc
+malformed.format.string.problem.descriptor.too.few.arguments=Too few arguments for format string (found: {0}, expected: {1}) #loc
+malformed.format.string.problem.descriptor.arguments.do.not.match.type=Argument type ''{0}'' does not match the type of the format specifier ''{1}'' #loc
 malformed.regular.expression.display.name=Malformed regular expression
 malformed.regular.expression.problem.descriptor1=Regular expression <code>#ref</code> is malformed #loc
 malformed.regular.expression.problem.descriptor2=Regular expression <code>#ref</code> is malformed: {0} #loc

plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/FormatDecode.java

@@ -15,6 +15,7 @@
  */
 package com.siyeh.ig.bugs;
 
+import com.intellij.openapi.util.text.StringUtil;
 import com.intellij.psi.CommonClassNames;
 import com.intellij.psi.PsiType;
 import com.intellij.psi.util.InheritanceUtil;
@@ -37,75 +38,169 @@ class FormatDecode {
 
   private static final Validator ALL_VALIDATOR = new AllValidator();
 
-  private static final Validator DATE_VALIDATOR = new DateValidator();
+  private static final int LEFT_JUSTIFY = 1; // '-'
+  private static final int ALTERNATE = 2; // '#'
+  private static final int PLUS = 4; // '+'
+  private static final int LEADING_SPACE = 8; // ' '
+  private static final int ZERO_PAD = 16; // '0'
+  private static final int GROUP = 32; // ','
+  private static final int PARENTHESES = 64; // '('
+  private static final int PREVIOUS = 128; // '<'
 
-  private static final Validator CHAR_VALIDATOR = new CharValidator();
-
-  private static final Validator INT_VALIDATOR = new IntValidator();
-
-  private static final Validator FLOAT_VALIDATOR = new FloatValidator();
+  private static int flag(char c) {
+    switch (c) {
+      case '-': return LEFT_JUSTIFY;
+      case '#': return ALTERNATE;
+      case '+': return PLUS;
+      case ' ': return LEADING_SPACE;
+      case '0': return ZERO_PAD;
+      case ',': return GROUP;
+      case '(': return PARENTHESES;
+      case '<': return PREVIOUS;
+      default: throw new IllegalFormatException();
+    }
+  }
 
   public static Validator[] decode(String formatString, int argumentCount) {
     final ArrayList<Validator> parameters = new ArrayList<Validator>();
 
     final Matcher matcher = fsPattern.matcher(formatString);
+    boolean previousAllowed = false;
     int implicit = 0;
     int pos = 0;
-    for (int i = 0; matcher.find(i); i = matcher.end()) {
+    int i = 0;
+    while (matcher.find(i)) {
+      final int start = matcher.start();
+      if (start != i) {
+        checkText(formatString.substring(i, start));
+      }
+      i = matcher.end();
+      final String specifier = matcher.group();
       final String posSpec = matcher.group(1);
       final String flags = matcher.group(2);
+      final String width = matcher.group(3);
+      final String precision = matcher.group(4);
       final String dateSpec = matcher.group(5);
       final String spec = matcher.group(6);
 
+      int flagBits = 0;
+      for (int j = 0; j < flags.length(); j++) {
+        final int bit = flag(flags.charAt(j));
+        if ((flagBits | bit) == flagBits) throw new IllegalFormatException(specifier); // duplicate flag
+        flagBits |= bit;
+      }
+      if (isAllBitsSet(flagBits, LEADING_SPACE | PLUS) || isAllBitsSet(flagBits, LEFT_JUSTIFY | ZERO_PAD)) {
+        // illegal flag combination
+        throw new IllegalFormatException(specifier);
+      }
+
       // check this first because it should not affect "implicit"
-      if ("n".equals(spec) || "%".equals(spec)) {
+      if ("n".equals(spec)) {
+        // no flags allowed
+        if (flagBits != 0 || !StringUtil.isEmpty(width) || !StringUtil.isEmpty(precision)) throw new IllegalFormatException(specifier);
+        continue;
+      }
+      else if ("%".equals(spec)) {
+        if (isAnyBitSet(flagBits, ~LEFT_JUSTIFY) || !StringUtil.isEmpty(precision)) throw new IllegalFormatException(specifier);
         continue;
       }
 
       if (posSpec != null) {
+        if (isAnyBitSet(flagBits, PREVIOUS)) throw new IllegalFormatException(specifier);
         final String num = posSpec.substring(0, posSpec.length() - 1);
         pos = Integer.parseInt(num) - 1;
+        previousAllowed = true;
       }
-      else if (flags == null || flags.indexOf('<') < 0) {
+      else if (isAnyBitSet(flagBits, PREVIOUS)) {
+        // reuse last pos
+        if (!previousAllowed) throw new IllegalFormatException(specifier);
+      }
+      else {
+        previousAllowed = true;
         pos = implicit++;
       }
-      // else if the flag has "<" reuse the last pos
 
       final Validator allowed;
       if (dateSpec != null) {  // a t or T
-        allowed = DATE_VALIDATOR;
+        if (isAnyBitSet(flagBits, ~LEFT_JUSTIFY) || !StringUtil.isEmpty(precision)) throw new IllegalFormatException(specifier);
+        allowed = new DateValidator(specifier);
       }
       else {
-        switch (Character.toLowerCase(spec.charAt(0))) {
+        switch (spec.charAt(0)) {
           case 'b':
+          case 'B':
           case 'h':
+          case 'H':
+            if (isAnyBitSet(flagBits, ~LEFT_JUSTIFY)) throw new IllegalFormatException(specifier);
+            allowed = ALL_VALIDATOR;
+            break;
           case 's':
+          case 'S':
+            if (isAnyBitSet(flagBits, ~(LEFT_JUSTIFY | ALTERNATE))) throw new IllegalFormatException(specifier);
             allowed = ALL_VALIDATOR;
             break;
           case 'c':
-            allowed = CHAR_VALIDATOR;
+          case 'C':
+            if (isAnyBitSet(flagBits, ~LEFT_JUSTIFY) || !StringUtil.isEmpty(precision)) throw new IllegalFormatException(specifier);
+            allowed = new CharValidator(specifier);
             break;
           case 'd':
+            if (isAnyBitSet(flagBits, ALTERNATE)) throw new IllegalFormatException(specifier);
+            allowed = new IntValidator(specifier);
+            break;
           case 'o':
           case 'x':
-            allowed = INT_VALIDATOR;
+          case 'X':
+            if (isAnyBitSet(flagBits, PLUS | LEADING_SPACE | GROUP) || !StringUtil.isEmpty(precision)) {
+              throw new IllegalFormatException(specifier);
+            }
+            allowed = new IntValidator(specifier);
+            break;
+          case 'a':
+          case 'A':
+            if (isAnyBitSet(flagBits, PARENTHESES | GROUP)) throw new IllegalFormatException(specifier);
+            allowed = new FloatValidator(specifier);
             break;
           case 'e':
-          case 'f':
+          case 'E':
+            if (isAnyBitSet(flagBits, GROUP)) throw new IllegalFormatException(specifier);
+            allowed = new FloatValidator(specifier);
+            break;
           case 'g':
-          case 'a':
-            allowed = FLOAT_VALIDATOR;
+          case 'G':
+            if (isAnyBitSet(flagBits, ALTERNATE)) throw new IllegalFormatException(specifier);
+            allowed = new FloatValidator(specifier);
+            break;
+          case 'f':
+            allowed = new FloatValidator(specifier);
             break;
           default:
-            throw new UnknownFormatException(matcher.group());
+            throw new IllegalFormatException(specifier);
         }
       }
       storeValidator(allowed, pos, parameters, argumentCount);
     }
+    if (i < formatString.length() - 1) {
+      checkText(formatString.substring(i));
+    }
 
     return parameters.toArray(new Validator[parameters.size()]);
   }
 
+  private static boolean isAnyBitSet(int value, int mask) {
+    return (value & mask) != 0;
+  }
+
+  private static boolean isAllBitsSet(int value, int mask) {
+    return (value & mask) == mask;
+  }
+
+  private static void checkText(String s) {
+    if (s.indexOf('%') != -1) {
+      throw new IllegalFormatException();
+    }
+  }
+
   private static void storeValidator(Validator validator, int pos,
                                      ArrayList<Validator> parameters,
                                      int argumentCount) {
@@ -118,7 +213,7 @@ class FormatDecode {
         ((MultiValidator)existing).addValidator(validator);
       }
       else if (existing != validator) {
-        final MultiValidator multiValidator = new MultiValidator();
+        final MultiValidator multiValidator = new MultiValidator(existing.getSpecifier());
         multiValidator.addValidator(existing);
         multiValidator.addValidator(validator);
         parameters.set(pos, multiValidator);
@@ -132,14 +227,20 @@ class FormatDecode {
     }
   }
 
-  public static class UnknownFormatException extends RuntimeException {
+  public static class IllegalFormatException extends RuntimeException {
 
-    public UnknownFormatException(String message) {
+    public IllegalFormatException(String message) {
       super(message);
     }
+
+    public IllegalFormatException() {}
   }
 
-  private static class AllValidator implements Validator {
+  private static class AllValidator extends Validator {
+
+    public AllValidator() {
+      super("");
+    }
 
     @Override
     public boolean valid(PsiType type) {
@@ -147,7 +248,11 @@ class FormatDecode {
     }
   }
 
-  private static class DateValidator implements Validator {
+  private static class DateValidator extends Validator {
+
+    public DateValidator(String specifier) {
+      super(specifier);
+    }
 
     @Override
     public boolean valid(PsiType type) {
@@ -159,7 +264,11 @@ class FormatDecode {
     }
   }
 
-  private static class CharValidator implements Validator {
+  private static class CharValidator extends Validator {
+
+    public CharValidator(String specifier) {
+      super(specifier);
+    }
 
     @Override
     public boolean valid(PsiType type) {
@@ -174,7 +283,11 @@ class FormatDecode {
     }
   }
 
-  private static class IntValidator implements Validator {
+  private static class IntValidator extends Validator {
+
+    public IntValidator(String specifier) {
+      super(specifier);
+    }
 
     @Override
     public boolean valid(PsiType type) {
@@ -191,7 +304,11 @@ class FormatDecode {
     }
   }
 
-  private static class FloatValidator implements Validator {
+  private static class FloatValidator extends Validator {
+
+    public FloatValidator(String specifier) {
+      super(specifier);
+    }
 
     @Override
     public boolean valid(PsiType type) {
@@ -204,9 +321,13 @@ class FormatDecode {
     }
   }
 
-  private static class MultiValidator implements Validator {
+  private static class MultiValidator extends Validator {
     private final Set<Validator> validators = new HashSet<Validator>(3);
 
+    public MultiValidator(String specifier) {
+      super(specifier);
+    }
+
     @Override
     public boolean valid(PsiType type) {
       for (Validator validator : validators) {
@@ -221,4 +342,19 @@ class FormatDecode {
       validators.add(validator);
     }
   }
+
+  abstract static class Validator {
+
+    private final String mySpecifier;
+
+    public Validator(String specifier) {
+      mySpecifier = specifier;
+    }
+
+    public abstract boolean valid(PsiType type);
+
+    public String getSpecifier() {
+      return mySpecifier;
+    }
+  }
 }

plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/MalformedFormatStringInspectionBase.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2003-2014 Dave Griffith, Bas Leijdekkers
+ * Copyright 2003-2015 Dave Griffith, Bas Leijdekkers
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -78,17 +78,27 @@ public class MalformedFormatStringInspectionBase extends BaseInspection {
   public String buildErrorString(Object... infos) {
     final Object value = infos[0];
     if (value instanceof Exception) {
+      final Exception exception = (Exception)value;
+      final String message = exception.getMessage();
+      if (message != null) {
+        return InspectionGadgetsBundle.message("malformed.format.string.problem.descriptor.illegal", message);
+      }
       return InspectionGadgetsBundle.message("malformed.format.string.problem.descriptor.malformed");
     }
-    final Validator[] validators = (Validator[])value;
+    final FormatDecode.Validator[] validators = (FormatDecode.Validator[])value;
     final int argumentCount = ((Integer)infos[1]).intValue();
     if (validators.length < argumentCount) {
-      return InspectionGadgetsBundle.message("malformed.format.string.problem.descriptor.too.many.arguments");
+      return InspectionGadgetsBundle.message("malformed.format.string.problem.descriptor.too.many.arguments",
+                                             argumentCount, validators.length);
     }
     if (validators.length > argumentCount) {
-      return InspectionGadgetsBundle.message("malformed.format.string.problem.descriptor.too.few.arguments");
+      return InspectionGadgetsBundle.message("malformed.format.string.problem.descriptor.too.few.arguments",
+                                             argumentCount, validators.length);
     }
-    return InspectionGadgetsBundle.message("malformed.format.string.problem.descriptor.arguments.do.not.match.type");
+    final PsiType argumentType = (PsiType)infos[2];
+    final FormatDecode.Validator validator = (FormatDecode.Validator)infos[3];
+    return InspectionGadgetsBundle.message("malformed.format.string.problem.descriptor.arguments.do.not.match.type",
+                                           argumentType.getPresentableText(), validator.getSpecifier());
   }
 
   @Override
@@ -142,7 +152,7 @@ public class MalformedFormatStringInspectionBase extends BaseInspection {
         return;
       }
       final int argumentCount = arguments.length - (formatArgumentIndex + 1);
-      final Validator[] validators;
+      final FormatDecode.Validator[] validators;
       try {
         validators = FormatDecode.decode(value, argumentCount);
       }
@@ -158,17 +168,18 @@ public class MalformedFormatStringInspectionBase extends BaseInspection {
             return;
           }
         }
-        registerError(formatArgument, validators, Integer.valueOf(argumentCount));
+        registerMethodCallError(expression, validators, Integer.valueOf(argumentCount));
         return;
       }
       for (int i = 0; i < validators.length; i++) {
-        final Validator validator = validators[i];
-        final PsiType argumentType = arguments[i + formatArgumentIndex + 1].getType();
+        final FormatDecode.Validator validator = validators[i];
+        final PsiExpression argument = arguments[i + formatArgumentIndex + 1];
+        final PsiType argumentType = argument.getType();
         if (argumentType == null) {
           continue;
         }
         if (validator != null && !validator.valid(argumentType)) {
-          registerError(formatArgument, validators, Integer.valueOf(argumentCount));
+          registerError(argument, validators, Integer.valueOf(argumentCount), argumentType, validator);
           return;
         }
       }

plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/Validator.java

@@ -1,23 +0,0 @@
-/*
- * Copyright 2003-2014 Dave Griffith, Bas Leijdekkers
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package com.siyeh.ig.bugs;
-
-import com.intellij.psi.PsiType;
-
-interface Validator {
-
-  boolean valid(PsiType type);
-}

plugins/InspectionGadgets/test/com/siyeh/igtest/bugs/malformed_format_string/MalformedFormatString.java

@@ -7,30 +7,30 @@ public class MalformedFormatString {
 
     public void foo()
     {
-        String.format(<warning descr="Too many arguments for format string '\"%\"'">"%"</warning>, 3.0);
-        System.out.printf(<warning descr="Too many arguments for format string '\"%\"'">"%"</warning>, 3.0);
-        System.out.printf(<warning descr="Format string '\"%q\"' is malformed">"%q"</warning>, 3.0);
-        System.out.printf(<warning descr="Format string '\"%d\"' does not match the type of its arguments">"%d"</warning>, 3.0);
-        System.out.printf(new Locale(""),<warning descr="Format string '\"%d%s\"' does not match the type of its arguments">"%d%s"</warning>, 3.0, "foo");
+        String.<warning descr="Too many arguments for format string (found: 1, expected: 0)">format</warning>("%", 3.0);
+        System.out.<warning descr="Too many arguments for format string (found: 1, expected: 0)">printf</warning>("%", 3.0);
+        System.out.printf(<warning descr="Illegal format string specifier '%q'">"%q"</warning>, 3.0);
+        System.out.printf("%d", <warning descr="Argument type 'double' does not match the type of the format specifier '%d'">3.0</warning>);
+        System.out.printf(new Locale(""),"%d%s", <warning descr="Argument type 'double' does not match the type of the format specifier '%d'">3.0</warning>, "foo");
     }
 
     public static void main(String[] args)  {
         String local = "hmm";
 
         String good = String.format("%s %s", 1, 2); // this is valid according to the inspector (correct)
-        String warn = String.format(<warning descr="Too few arguments for format string '\"%s %s\"'">"%s %s"</warning>, 1); // this is invalid according to the inspector (correct)
+        String warn = String.<warning descr="Too few arguments for format string (found: 1, expected: 2)">format</warning>("%s %s", 1); // this is invalid according to the inspector (correct)
         String invalid = String.format("%s %s" + local, 1); // this is valid according to the inspector (INCORRECT!)
-        String interesting = String.format(<warning descr="Too few arguments for format string '\"%s %s\" + \"hmm\"'">"%s %s" + "hmm"</warning>, 1); // this is invalid according to the inspector (correct)
+        String interesting = String.<warning descr="Too few arguments for format string (found: 1, expected: 2)">format</warning>("%s %s" + "hmm", 1); // this is invalid according to the inspector (correct)
         String intAsChar = String.format("symbol '%1$c' (numeric value %1$d)", 60); // integer->char conversion is ok (correct)
     }
 
     public void outOfMemory() {
-        String.format(<warning descr="Too few arguments for format string '\"%2147483640$s\"'">"%2147483640$s"</warning>, "s");
+        String.<warning descr="Too few arguments for format string (found: 1, expected: 2)">format</warning>("%2147483640$s", "s");
     }
 
     public void optionalSettings() {
         SomeOtherLogger logger = new SomeOtherLogger();
-        logger.d(<warning descr="Too few arguments for format string '\"%s %s\"'">"%s %s"</warning>, 1); // this is invalid according to the inspector (correct)
+        logger.<warning descr="Too few arguments for format string (found: 1, expected: 2)">d</warning>("%s %s", 1); // this is invalid according to the inspector (correct)
     }
 
     public class SomeOtherLogger {
@@ -40,7 +40,7 @@ public class MalformedFormatString {
     }
 
     void shouldWarn() {
-        String.format(<warning descr="Format string '\"%1$c %1$d\"' does not match the type of its arguments">"%1$c %1$d"</warning>, 10L);
+        String.format("%1$c %1$d", <warning descr="Argument type 'long' does not match the type of the format specifier '%1$c'">10L</warning>);
     }
 
     void shouldNotWarn() {
@@ -51,4 +51,43 @@ public class MalformedFormatString {
     String timestamp(Timestamp ts) {
         return String.format("%tF %tT", ts, ts);
     }
+
+    void badStrings() {
+        // bad format specifier
+        String.format(<warning descr="Format string '\"%) %n\"' is malformed">"%) %n"</warning>);
+
+        // flags on newline not allowed
+        String.format(<warning descr="Illegal format string specifier '%(n'">"%(n"</warning>);
+
+        // unknown conversion
+        String.format(<warning descr="Illegal format string specifier '%D'">"%D"</warning>, 1);
+
+        // duplicate leading space flag
+        String.format(<warning descr="Illegal format string specifier '%  d'">"%  d"</warning>, 1);
+
+        // illegal alternate flag
+        String.format(<warning descr="Illegal format string specifier '%#B'">"%#B"</warning>, true);
+
+        // illegal flag combination
+        String.format(<warning descr="Illegal format string specifier '% +d'">"% +d"</warning>, 1);
+
+        // illegal flag on date/time
+        String.format(<warning descr="Illegal format string specifier '%+T'">"%+T"</warning>, new Timestamp(0));
+
+        // previous flag without previous
+        String.format(<warning descr="Illegal format string specifier '%<s'">"%<s"</warning>, 1);
+
+        // illegal flag
+        String.format(<warning descr="Illegal format string specifier '%(s'">"%(s"</warning>, 1);
+
+        // unknown format conversions
+        String.format(<warning descr="Illegal format string specifier '%F'">"%F"</warning>, 1.0);
+        String.format(<warning descr="Illegal format string specifier '%D'">"%D"</warning>, 1);
+        String.format(<warning descr="Illegal format string specifier '%O'">"%O"</warning>, 1);
+    }
+
+    void goodStrings() {
+        String.format("%-B", true); // left justify flag
+        String.format("%,d", 34567890);
+    }
 }