Make the lint string-format check ignore date strings

The string format lint check might be processing strings not intended
for String.format (but intended for example for
android.text.format.Time#format), in which case it will be wrong about
whether two conversions are incompatible. (This does not fix all
scenarios; if a date string looks like a String.format in the sense
that all of its formatting characters are defined by String.format
then the string will assume to be intended for String.format.)

This changeset attempts to recognize this, and also make the "is
incompatible" check a bit more nuanced: it will now consider "d"
compatible with "x" for example.

Change-Id: I63ce082f40169e4033809d25cae3cf116c9e2044

5 files changed, 209 insertions, 47 deletions

diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/StringFormatDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/StringFormatDetector.java
index c115fdf..4d1c8ae 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/StringFormatDetector.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/StringFormatDetector.java
@@ -94,7 +94,14 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
             "(1) Formatting strings that are invalid, meaning that String.format will throw " +
             "exceptions at runtime when attempting to use the format string.\n" +
             "(2) Strings containing '%' that are not formatting strings getting passed to " +
-            "a String.format call. In this case the '%' will need to be escaped as '%%'.",
+            "a String.format call. In this case the '%' will need to be escaped as '%%'.\n" +
+            "\n" +
+            "NOTE: Not all Strings which look like formatting strings are intended for " +
+            "use by String.format; for example, they may contain date formats intended " +
+            "for android.text.format.Time#format(). Lint cannot always figure out that " +
+            "a String is a date format, so you may get false warnings in those scenarios. " +
+            "See the suppress help topic for information on how to suppress errors in " +
+            "that case.",
 
             Category.MESSAGES,
             9,
@@ -224,6 +231,9 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
     }
 
     private void checkTextNode(XmlContext context, Element element, String text) {
+        String name = null;
+        boolean found = false;
+
         // Look at the String and see if it's a format string (contains
         // positional %'s)
         for (int j = 0, m = text.length(); j < m; j++) {
@@ -232,7 +242,9 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
                 j++;
             }
             if (c == '%') {
-                String name = element.getAttribute(ATTR_NAME);
+                if (name == null) {
+                    name = element.getAttribute(ATTR_NAME);
+                }
 
                 // Also make sure this String isn't an unformatted String
                 String formatted = element.getAttribute("formatted"); //$NON-NLS-1$
@@ -241,36 +253,52 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
                         Handle handle = context.parser.createLocationHandle(context, element);
                         mNotFormatStrings.put(name, handle);
                     }
-                    continue;
+                    return;
                 }
 
                 // See if it's not a format string, e.g. "Battery charge is 100%!".
                 // If so we want to record this name in a special list such that we can
                 // make sure you don't attempt to reference this string from a String.format
                 // call.
-                if (!FORMAT.matcher(text).find()) {
+                Matcher matcher = FORMAT.matcher(text);
+                if (!matcher.find(j)) {
                     if (!mNotFormatStrings.containsKey(name)) {
                         Handle handle = context.parser.createLocationHandle(context, element);
                         mNotFormatStrings.put(name, handle);
                     }
-                    continue;
+                    return;
                 }
 
-                // Record it for analysis when seen in Java code
-                if (mFormatStrings == null) {
-                    mFormatStrings = new HashMap<String, List<Pair<Handle,String>>>();
+                String conversion = matcher.group(6);
+                int conversionClass = getConversionClass(conversion.charAt(0));
+                if (conversionClass == CONVERSION_CLASS_UNKNOWN || matcher.group(5) != null) {
+                    if (!mNotFormatStrings.containsKey(name)) {
+                        Handle handle = context.parser.createLocationHandle(context, element);
+                        mNotFormatStrings.put(name, handle);
+                    }
+                    // Don't process any other strings here; some of them could
+                    // accidentally look like a string, e.g. "%H" is a hash code conversion
+                    // in String.format (and hour in Time formatting).
+                    return;
                 }
 
-                List<Pair<Handle, String>> list = mFormatStrings.get(name);
-                if (list == null) {
-                    list = new ArrayList<Pair<Handle, String>>();
-                    mFormatStrings.put(name, list);
-                }
-                Handle handle = context.parser.createLocationHandle(context, element);
-                list.add(Pair.of(handle, text));
+                found = true;
+            }
+        }
 
-                break;
+        if (found && name != null) {
+            // Record it for analysis when seen in Java code
+            if (mFormatStrings == null) {
+                mFormatStrings = new HashMap<String, List<Pair<Handle,String>>>();
             }
+
+            List<Pair<Handle, String>> list = mFormatStrings.get(name);
+            if (list == null) {
+                list = new ArrayList<Pair<Handle, String>>();
+                mFormatStrings.put(name, list);
+            }
+            Handle handle = context.parser.createLocationHandle(context, element);
+            list.add(Pair.of(handle, text));
         }
     }
 
@@ -311,7 +339,7 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
             Handle handle = pair.getFirst();
             String formatString = pair.getSecond();
 
-            boolean warned = false;
+            //boolean warned = false;
             Matcher matcher = FORMAT.matcher(formatString);
             int index = 0;
             int prevIndex = 0;
@@ -354,7 +382,7 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
                                         "character in '%2$s' ?", name, str);
                                 context.report(INVALID, location,
                                         message, null);
-                                warned = true;
+                                //warned = true;
                                 continue;
                             }
                         }
@@ -381,7 +409,8 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
                     if (currentFormat == null) {
                         types.put(number, format);
                         typeDefinition.put(number, handle);
-                    } else if (!currentFormat.equals(format)) {
+                    } else if (!currentFormat.equals(format)
+                            && isIncompatible(currentFormat.charAt(0), format.charAt(0))) {
                         Location location = handle.resolve();
                         // Attempt to limit the location range to just the formatting
                         // string in question
@@ -399,7 +428,7 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
                                 str,
                                 currentFormat, format,
                                 f.getParentFile().getName() + File.separator + f.getName());
-                        warned = true;
+                        //warned = true;
                         // TODO: Compute applicable node scope
                         context.report(ARG_TYPES, location,
                                 message, null);
@@ -438,6 +467,73 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
         }
     }
 
+    /**
+     * Returns true if two String.format conversions are "incompatible" (meaning
+     * that using these two for the same argument across different translations
+     * is more likely an error than intentional. Some conversions are
+     * incompatible, e.g. "d" and "s" where one is a number and string, whereas
+     * others may work (e.g. float versus integer) but are probably not
+     * intentional.
+     */
+    private boolean isIncompatible(char conversion1, char conversion2) {
+        int class1 = getConversionClass(conversion1);
+        int class2 = getConversionClass(conversion2);
+        return class1 != class2
+                && class1 != CONVERSION_CLASS_UNKNOWN
+                && class2 != CONVERSION_CLASS_UNKNOWN;
+    }
+
+    private static final int CONVERSION_CLASS_UNKNOWN = 0;
+    private static final int CONVERSION_CLASS_STRING = 1;
+    private static final int CONVERSION_CLASS_CHARACTER = 2;
+    private static final int CONVERSION_CLASS_INTEGER = 3;
+    private static final int CONVERSION_CLASS_FLOAT = 4;
+    private static final int CONVERSION_CLASS_BOOLEAN = 5;
+    private static final int CONVERSION_CLASS_HASHCODE = 6;
+    private static final int CONVERSION_CLASS_PERCENT = 7;
+    private static final int CONVERSION_CLASS_NEWLINE = 8;
+    private static final int CONVERSION_CLASS_DATETIME = 9;
+
+    private static int getConversionClass(char conversion) {
+        // See http://developer.android.com/reference/java/util/Formatter.html
+        switch (conversion) {
+            case 't':   // Time/date conversion
+            case 'T':
+                return CONVERSION_CLASS_DATETIME;
+            case 's':   // string
+            case 'S':   // Uppercase string
+                return CONVERSION_CLASS_STRING;
+            case 'c':   // character
+            case 'C':   // Uppercase character
+                return CONVERSION_CLASS_CHARACTER;
+            case 'd':   // decimal
+            case 'o':   // octal
+            case 'x':   // hex
+            case 'X':
+                return CONVERSION_CLASS_INTEGER;
+            case 'f':   // decimal float
+            case 'e':   // exponential float
+            case 'E':
+            case 'g':   // decimal or exponential depending on size
+            case 'G':
+            case 'a':   // hex float
+            case 'A':
+                return CONVERSION_CLASS_FLOAT;
+            case 'b':   // boolean
+            case 'B':
+                return CONVERSION_CLASS_BOOLEAN;
+            case 'h':   // boolean
+            case 'H':
+                return CONVERSION_CLASS_HASHCODE;
+            case '%':   // literal
+                return CONVERSION_CLASS_PERCENT;
+            case 'n':   // literal
+                return CONVERSION_CLASS_NEWLINE;
+        }
+
+        return CONVERSION_CLASS_UNKNOWN;
+    }
+
     private Location refineLocation(Context context, Location location, String formatString,
             int substringStart, int substringEnd) {
         Position startLocation = location.getStart();
@@ -489,7 +585,7 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
             for (int i = 1; i <= count; i++) {
                 if (!indices.contains(i)) {
                     Set<Integer> all = new HashSet<Integer>();
-                    for (int j = 0; j < count; j++) {
+                    for (int j = 1; j < count; j++) {
                         all.add(j);
                     }
                     all.removeAll(indices);
@@ -588,35 +684,50 @@ public class StringFormatDetector extends ResourceXmlDetector implements Detecto
      */
     @VisibleForTesting
     static int getFormatArgumentCount(String s, Set<Integer> seenArguments) {
+        Matcher matcher = FORMAT.matcher(s);
+        int index = 0;
+        int prevIndex = 0;
+        int nextNumber = 1;
         int max = 0;
-        for (int i = 0, n = s.length(); i < n; i++) {
-            char c = s.charAt(i);
-            if (c == '\\') {
-                // Skip next char
-                i++;
-            } else if (c == '%') {
-                // Peek ahead: is this a format number like %(\d+)\$ ?
-                String argument = null;
-                for (int j = i + 1; j < n; j++) {
-                    char p = s.charAt(j);
-                    if (p == '$') {
-                        if (j > i + 1) {
-                            argument = s.substring(i + 1, j);
-                        }
-                        break;
-                    } else if (!Character.isDigit(p)) {
-                        break;
+        while (true) {
+            if (matcher.find(index)) {
+                int matchStart = matcher.start();
+                // Make sure this is not an escaped '%'
+                for (; prevIndex < matchStart; prevIndex++) {
+                    char c = s.charAt(prevIndex);
+                    if (c == '\\') {
+                        prevIndex++;
                     }
                 }
-                if (argument != null) {
-                    int number = Integer.parseInt(argument);
-                    if (number > max) {
-                        max = number;
-                    }
-                    if (seenArguments != null) {
-                        seenArguments.add(number);
-                    }
+                if (prevIndex > matchStart) {
+                    // We're in an escape, ignore this result
+                    index = prevIndex;
+                    continue;
+                }
+
+                // Shouldn't throw a number format exception since we've already
+                // matched the pattern in the regexp
+                int number;
+                String numberString = matcher.group(1);
+                if (numberString != null) {
+                    // Strip off trailing $
+                    numberString = numberString.substring(0, numberString.length() - 1);
+                    number = Integer.parseInt(numberString);
+                    nextNumber = number + 1;
+                } else {
+                    number = nextNumber++;
+                }
+
+                if (number > max) {
+                    max = number;
+                }
+                if (seenArguments != null) {
+                    seenArguments.add(number);
                 }
+
+                index = matcher.end();
+            } else {
+                break;
             }
         }
 
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/StringFormatDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/StringFormatDetectorTest.java
index 5bd124a..c6e97a8 100644
--- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/StringFormatDetectorTest.java
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/StringFormatDetectorTest.java
@@ -30,7 +30,7 @@ public class StringFormatDetectorTest  extends AbstractCheckTest {
 
     public void testAll() throws Exception {
         assertEquals(
-            "formatstrings.xml:5: Warning: Formatting string 'missing' is not referencing numbered arguments [0, 1, 2]\n" +
+            "formatstrings.xml:5: Warning: Formatting string 'missing' is not referencing numbered arguments [1, 2]\n" +
             "pkg/StringFormatActivity.java:13: Error: Wrong argument type for formatting argument '#1' in hello: conversion is 'd', received String\n" +
             "=> values-es/formatstrings.xml:3: Conflicting argument declaration here\n" +
             "pkg/StringFormatActivity.java:15: Error: Wrong argument count, format string hello2 requires 3 but format call supplies 2\n" +
@@ -55,6 +55,8 @@ public class StringFormatDetectorTest  extends AbstractCheckTest {
                 "Skipping stuff: %11$s", null));
         assertEquals(1, StringFormatDetector.getFormatArgumentCount(
                 "First: %1$s, Skip \\%2$s", null));
+        assertEquals(1, StringFormatDetector.getFormatArgumentCount(
+                "First: %s, Skip \\%s", null));
 
         Set<Integer> indices = new HashSet<Integer>();
         assertEquals(11, StringFormatDetector.getFormatArgumentCount(
@@ -83,4 +85,24 @@ public class StringFormatDetectorTest  extends AbstractCheckTest {
                     "res/values/formatstrings2.xml"
                 ));
     }
+
+    public void testDateStrings() throws Exception {
+        assertEquals(
+            "No warnings.",
+
+            lintProject(
+                    "res/values/formatstrings-version1.xml=>res/values-tl/donottranslate-cldr.xml",
+                    "res/values/formatstrings-version2.xml=>res/values/donottranslate-cldr.xml"
+                ));
+    }
+
+    public void testUa() throws Exception {
+        assertEquals(
+            "No warnings.",
+
+            lintProject(
+                    "res/values/formatstrings-version1.xml=>res/values-tl/donottranslate-cldr.xml",
+                    "src/test/pkg/StringFormat2.java.txt=>src/test/pkg/StringFormat2.java"
+                ));
+    }
 }
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values/formatstrings-version1.xml b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values/formatstrings-version1.xml
new file mode 100644
index 0000000..4d3e5a7
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values/formatstrings-version1.xml
@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="utf-8"?>
+<resources>
+    <string name="hour_minute_24">%-k:%M</string>
+    <string name="numeric_date">%Y-%m-%d</string>
+    <string name="month_day_year">%Y %B %-e</string>
+    <string translatable="false" name="web_user_agent">
+      Foo (Bar %s) Foo/731.11+ (Foo, like Bar) Version/1.2.3 Foo Bar/123.14.4
+    </string>
+</resources>
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values/formatstrings-version2.xml b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values/formatstrings-version2.xml
new file mode 100644
index 0000000..d1757f7
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/values/formatstrings-version2.xml
@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="utf-8"?>
+<resources>
+    <string name="hour_minute_24">%H:%M</string>
+    <string name="numeric_date">%-m/%-e/%Y</string>
+    <string name="month_day_year">%B %-e, %Y</string>
+</resources>
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/test/pkg/StringFormat2.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/test/pkg/StringFormat2.java.txt
new file mode 100644
index 0000000..a09e44b
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/test/pkg/StringFormat2.java.txt
@@ -0,0 +1,14 @@
+package test.pkg;
+
+import android.app.Activity;
+import android.os.Bundle;
+
+public class StringFormat2 extends Activity {
+    public static final String buildUserAgent(Context context) {
+        StringBuilder arg = new StringBuilder();
+        // Snip
+        final String base = context.getResources().getText(R.string.web_user_agent).toString();
+        String ua = String.format(base, arg);
+        return ua;
+    }
+}