Add blame to API lint, some exemptions.

Now offers to parse the output of git blame, and includes the last
person to modify that API for each reported failure.

Add more exemptions, and check for boolean setFoo() method inside a
separate Builder inner class.

Change-Id: Id32dcbd5edf17d2360e4f782110bc1c445f7936e

1 files changed, 186 insertions, 77 deletions

diff --git a/tools/apilint/apilint.py b/tools/apilint/apilint.py
index 6bb28e1..d9d571a 100644
--- a/tools/apilint/apilint.py
+++ b/tools/apilint/apilint.py
@@ -20,15 +20,20 @@ a previous API level, if provided.
 
 Usage: apilint.py current.txt
 Usage: apilint.py current.txt previous.txt
+
+You can also splice in blame details like this:
+$ git blame api/current.txt -t -e > /tmp/currentblame.txt
+$ apilint.py /tmp/currentblame.txt previous.txt --no-color
 """
 
-import re, sys, collections
+import re, sys, collections, traceback
 
 
 BLACK, RED, GREEN, YELLOW, BLUE, MAGENTA, CYAN, WHITE = range(8)
 
 def format(fg=None, bg=None, bright=False, bold=False, dim=False, reset=False):
     # manually derived from http://en.wikipedia.org/wiki/ANSI_escape_code#Codes
+    if "--no-color" in sys.argv: return ""
     codes = []
     if reset: codes.append("0")
     else:
@@ -43,9 +48,10 @@ def format(fg=None, bg=None, bright=False, bold=False, dim=False, reset=False):
 
 
 class Field():
-    def __init__(self, clazz, raw):
+    def __init__(self, clazz, raw, blame):
         self.clazz = clazz
         self.raw = raw.strip(" {;")
+        self.blame = blame
 
         raw = raw.split()
         self.split = list(raw)
@@ -65,9 +71,13 @@ class Field():
 
 
 class Method():
-    def __init__(self, clazz, raw):
+    def __init__(self, clazz, raw, blame):
         self.clazz = clazz
         self.raw = raw.strip(" {;")
+        self.blame = blame
+
+        # drop generics for now
+        raw = re.sub("<.+?>", "", raw)
 
         raw = re.split("[\s(),;]+", raw)
         for r in ["", ";"]:
@@ -89,9 +99,10 @@ class Method():
 
 
 class Class():
-    def __init__(self, pkg, raw):
+    def __init__(self, pkg, raw, blame):
         self.pkg = pkg
         self.raw = raw.strip(" {;")
+        self.blame = blame
         self.ctors = []
         self.fields = []
         self.methods = []
@@ -103,18 +114,18 @@ class Class():
         elif "interface" in raw:
             self.fullname = raw[raw.index("interface")+1]
 
-        if "." in self.fullname:
-            self.name = self.fullname[self.fullname.rindex(".")+1:]
-        else:
-            self.name = self.fullname
+        self.fullname = self.pkg.name + "." + self.fullname
+        self.name = self.fullname[self.fullname.rindex(".")+1:]
+
 
     def __repr__(self):
         return self.raw
 
 
 class Package():
-    def __init__(self, raw):
+    def __init__(self, raw, blame):
         self.raw = raw.strip(" {;")
+        self.blame = blame
 
         raw = raw.split()
         self.name = raw[raw.index("package")+1]
@@ -124,44 +135,57 @@ class Package():
 
 
 def parse_api(fn):
-    api = []
+    api = {}
     pkg = None
     clazz = None
+    blame = None
+
+    re_blame = re.compile("^([a-z0-9]{7,}) \(<([^>]+)>.+?\) (.+?)$")
 
     with open(fn) as f:
         for raw in f.readlines():
             raw = raw.rstrip()
+            match = re_blame.match(raw)
+            if match is not None:
+                blame = match.groups()[0:2]
+                raw = match.groups()[2]
+            else:
+                blame = None
 
             if raw.startswith("package"):
-                pkg = Package(raw)
+                pkg = Package(raw, blame)
             elif raw.startswith("  ") and raw.endswith("{"):
-                clazz = Class(pkg, raw)
-                api.append(clazz)
+                clazz = Class(pkg, raw, blame)
+                api[clazz.fullname] = clazz
             elif raw.startswith("    ctor"):
-                clazz.ctors.append(Method(clazz, raw))
+                clazz.ctors.append(Method(clazz, raw, blame))
             elif raw.startswith("    method"):
-                clazz.methods.append(Method(clazz, raw))
+                clazz.methods.append(Method(clazz, raw, blame))
             elif raw.startswith("    field"):
-                clazz.fields.append(Field(clazz, raw))
+                clazz.fields.append(Field(clazz, raw, blame))
 
     return api
 
 
-failures = []
-
-def filter_dupe(s):
-    return s.replace(" deprecated ", " ")
+failures = {}
 
 def _fail(clazz, detail, msg):
     """Records an API failure to be processed later."""
     global failures
 
+    sig = "%s-%s-%s" % (clazz.fullname, repr(detail), msg)
+    sig = sig.replace(" deprecated ", " ")
+
     res = msg
+    blame = clazz.blame
     if detail is not None:
         res += "\n    in " + repr(detail)
+        blame = detail.blame
     res += "\n    in " + repr(clazz)
     res += "\n    in " + repr(clazz.pkg)
-    failures.append(filter_dupe(res))
+    if blame is not None:
+        res += "\n    last modified by %s in %s" % (blame[1], blame[0])
+    failures[sig] = res
 
 def warn(clazz, detail, msg):
     _fail(clazz, detail, "%sWarning:%s %s" % (format(fg=YELLOW, bg=BLACK), format(reset=True), msg))
@@ -172,7 +196,7 @@ def error(clazz, detail, msg):
 
 def verify_constants(clazz):
     """All static final constants must be FOO_NAME style."""
-    if re.match("R\.[a-z]+", clazz.fullname): return
+    if re.match("android\.R\.[a-z]+", clazz.fullname): return
 
     for f in clazz.fields:
         if "static" in f.split and "final" in f.split:
@@ -188,6 +212,10 @@ def verify_enums(clazz):
 
 def verify_class_names(clazz):
     """Try catching malformed class names like myMtp or MTPUser."""
+    if clazz.fullname.startswith("android.opengl"): return
+    if clazz.fullname.startswith("android.renderscript"): return
+    if re.match("android\.R\.[a-z]+", clazz.fullname): return
+
     if re.search("[A-Z]{2,}", clazz.name) is not None:
         warn(clazz, None, "Class name style should be Mtp not MTP")
     if re.match("[^A-Z]", clazz.name):
@@ -196,32 +224,35 @@ def verify_class_names(clazz):
 
 def verify_method_names(clazz):
     """Try catching malformed method names, like Foo() or getMTU()."""
-    if clazz.pkg.name == "android.opengl": return
+    if clazz.fullname.startswith("android.opengl"): return
+    if clazz.fullname.startswith("android.renderscript"): return
+    if clazz.fullname == "android.system.OsConstants": return
 
     for m in clazz.methods:
         if re.search("[A-Z]{2,}", m.name) is not None:
             warn(clazz, m, "Method name style should be getMtu() instead of getMTU()")
         if re.match("[^a-z]", m.name):
-            error(clazz, None, "Method name must start with lowercase char")
+            error(clazz, m, "Method name must start with lowercase char")
 
 
 def verify_callbacks(clazz):
     """Verify Callback classes.
     All callback classes must be abstract.
     All methods must follow onFoo() naming style."""
+    if clazz.fullname == "android.speech.tts.SynthesisCallback": return
 
     if clazz.name.endswith("Callbacks"):
-        error(clazz, None, "Class must be named exactly Callback")
+        error(clazz, None, "Class name must not be plural")
     if clazz.name.endswith("Observer"):
-        warn(clazz, None, "Class should be named Callback")
+        warn(clazz, None, "Class should be named FooCallback")
 
     if clazz.name.endswith("Callback"):
         if "interface" in clazz.split:
-            error(clazz, None, "Callback must be abstract class")
+            error(clazz, None, "Callback must be abstract class to enable extension in future API levels")
 
         for m in clazz.methods:
             if not re.match("on[A-Z][a-z]*", m.name):
-                error(clazz, m, "Callback method names must be onFoo style")
+                error(clazz, m, "Callback method names must be onFoo() style")
 
 
 def verify_listeners(clazz):
@@ -233,16 +264,16 @@ def verify_listeners(clazz):
 
     if clazz.name.endswith("Listener"):
         if " abstract class " in clazz.raw:
-            error(clazz, None, "Listener should be interface")
+            error(clazz, None, "Listener should be an interface, otherwise renamed Callback")
 
         for m in clazz.methods:
             if not re.match("on[A-Z][a-z]*", m.name):
-                error(clazz, m, "Listener method names must be onFoo style")
+                error(clazz, m, "Listener method names must be onFoo() style")
 
         if len(clazz.methods) == 1 and clazz.name.startswith("On"):
             m = clazz.methods[0]
             if (m.name + "Listener").lower() != clazz.name.lower():
-                error(clazz, m, "Single method name should match class name")
+                error(clazz, m, "Single listener method name should match class name")
 
 
 def verify_actions(clazz):
@@ -255,21 +286,24 @@ def verify_actions(clazz):
     for f in clazz.fields:
         if f.value is None: continue
         if f.name.startswith("EXTRA_"): continue
+        if f.name == "SERVICE_INTERFACE" or f.name == "PROVIDER_INTERFACE": continue
 
         if "static" in f.split and "final" in f.split and f.typ == "java.lang.String":
             if "_ACTION" in f.name or "ACTION_" in f.name or ".action." in f.value.lower():
                 if not f.name.startswith("ACTION_"):
-                    error(clazz, f, "Intent action must be ACTION_FOO")
+                    error(clazz, f, "Intent action constant name must be ACTION_FOO")
                 else:
-                    if clazz.name == "Intent":
+                    if clazz.fullname == "android.content.Intent":
                         prefix = "android.intent.action"
-                    elif clazz.name == "Settings":
+                    elif clazz.fullname == "android.provider.Settings":
                         prefix = "android.settings"
+                    elif clazz.fullname == "android.app.admin.DevicePolicyManager" or clazz.fullname == "android.app.admin.DeviceAdminReceiver":
+                        prefix = "android.app.action"
                     else:
                         prefix = clazz.pkg.name + ".action"
                     expected = prefix + "." + f.name[7:]
                     if f.value != expected:
-                        error(clazz, f, "Inconsistent action value")
+                        error(clazz, f, "Inconsistent action value; expected %s" % (expected))
 
 
 def verify_extras(clazz):
@@ -279,6 +313,9 @@ def verify_extras(clazz):
         package android.foo {
             String EXTRA_BAR = "android.foo.extra.BAR";
         }"""
+    if clazz.fullname == "android.app.Notification": return
+    if clazz.fullname == "android.appwidget.AppWidgetManager": return
+
     for f in clazz.fields:
         if f.value is None: continue
         if f.name.startswith("ACTION_"): continue
@@ -288,13 +325,15 @@ def verify_extras(clazz):
                 if not f.name.startswith("EXTRA_"):
                     error(clazz, f, "Intent extra must be EXTRA_FOO")
                 else:
-                    if clazz.name == "Intent":
+                    if clazz.pkg.name == "android.content" and clazz.name == "Intent":
                         prefix = "android.intent.extra"
+                    elif clazz.pkg.name == "android.app.admin":
+                        prefix = "android.app.extra"
                     else:
                         prefix = clazz.pkg.name + ".extra"
                     expected = prefix + "." + f.name[6:]
                     if f.value != expected:
-                        error(clazz, f, "Inconsistent extra value")
+                        error(clazz, f, "Inconsistent extra value; expected %s" % (expected))
 
 
 def verify_equals(clazz):
@@ -303,7 +342,7 @@ def verify_equals(clazz):
     eq = "equals" in methods
     hc = "hashCode" in methods
     if eq != hc:
-        error(clazz, None, "Must override both equals and hashCode")
+        error(clazz, None, "Must override both equals and hashCode; missing one")
 
 
 def verify_parcelable(clazz):
@@ -314,34 +353,59 @@ def verify_parcelable(clazz):
         describe = [ i for i in clazz.methods if i.name == "describeContents" ]
 
         if len(creator) == 0 or len(write) == 0 or len(describe) == 0:
-            error(clazz, None, "Parcelable requires CREATOR, writeToParcel, and describeContents")
+            error(clazz, None, "Parcelable requires CREATOR, writeToParcel, and describeContents; missing one")
 
 
 def verify_protected(clazz):
     """Verify that no protected methods are allowed."""
     for m in clazz.methods:
         if "protected" in m.split:
-            error(clazz, m, "Protected method")
+            error(clazz, m, "No protected methods; must be public")
     for f in clazz.fields:
         if "protected" in f.split:
-            error(clazz, f, "Protected field")
+            error(clazz, f, "No protected fields; must be public")
 
 
 def verify_fields(clazz):
     """Verify that all exposed fields are final.
     Exposed fields must follow myName style.
     Catch internal mFoo objects being exposed."""
+
+    IGNORE_BARE_FIELDS = [
+        "android.app.ActivityManager.RecentTaskInfo",
+        "android.app.Notification",
+        "android.content.pm.ActivityInfo",
+        "android.content.pm.ApplicationInfo",
+        "android.content.pm.FeatureGroupInfo",
+        "android.content.pm.InstrumentationInfo",
+        "android.content.pm.PackageInfo",
+        "android.content.pm.PackageItemInfo",
+        "android.os.Message",
+        "android.system.StructPollfd",
+    ]
+
     for f in clazz.fields:
         if not "final" in f.split:
-            error(clazz, f, "Bare fields must be final; consider adding accessors")
+            if clazz.fullname in IGNORE_BARE_FIELDS:
+                pass
+            elif clazz.fullname.endswith("LayoutParams"):
+                pass
+            elif clazz.fullname.startswith("android.util.Mutable"):
+                pass
+            else:
+                error(clazz, f, "Bare fields must be marked final; consider adding accessors")
 
         if not "static" in f.split:
             if not re.match("[a-z]([a-zA-Z]+)?", f.name):
-                error(clazz, f, "Non-static fields must be myName")
+                error(clazz, f, "Non-static fields must be named with myField style")
 
-        if re.match("[m][A-Z]", f.name):
+        if re.match("[ms][A-Z]", f.name):
             error(clazz, f, "Don't expose your internal objects")
 
+        if re.match("[A-Z_]+", f.name):
+            if "static" not in f.split or "final" not in f.split:
+                error(clazz, f, "Constants must be marked static final")
+
 
 def verify_register(clazz):
     """Verify parity of registration methods.
@@ -353,34 +417,34 @@ def verify_register(clazz):
             if m.name.startswith("register"):
                 other = "unregister" + m.name[8:]
                 if other not in methods:
-                    error(clazz, m, "Missing unregister")
+                    error(clazz, m, "Missing unregister method")
             if m.name.startswith("unregister"):
                 other = "register" + m.name[10:]
                 if other not in methods:
-                    error(clazz, m, "Missing register")
+                    error(clazz, m, "Missing register method")
 
             if m.name.startswith("add") or m.name.startswith("remove"):
-                error(clazz, m, "Callback should be register/unregister")
+                error(clazz, m, "Callback methods should be named register/unregister")
 
         if "Listener" in m.raw:
             if m.name.startswith("add"):
                 other = "remove" + m.name[3:]
                 if other not in methods:
-                    error(clazz, m, "Missing remove")
+                    error(clazz, m, "Missing remove method")
             if m.name.startswith("remove") and not m.name.startswith("removeAll"):
                 other = "add" + m.name[6:]
                 if other not in methods:
-                    error(clazz, m, "Missing add")
+                    error(clazz, m, "Missing add method")
 
             if m.name.startswith("register") or m.name.startswith("unregister"):
-                error(clazz, m, "Listener should be add/remove")
+                error(clazz, m, "Listener methods should be named add/remove")
 
 
 def verify_sync(clazz):
     """Verify synchronized methods aren't exposed."""
     for m in clazz.methods:
         if "synchronized" in m.split:
-            error(clazz, m, "Lock exposed")
+            error(clazz, m, "Internal lock exposed")
 
 
 def verify_intent_builder(clazz):
@@ -392,7 +456,7 @@ def verify_intent_builder(clazz):
             if m.name.startswith("create") and m.name.endswith("Intent"):
                 pass
             else:
-                warn(clazz, m, "Should be createFooIntent()")
+                error(clazz, m, "Methods creating an Intent should be named createFooIntent()")
 
 
 def verify_helper_classes(clazz):
@@ -402,25 +466,57 @@ def verify_helper_classes(clazz):
     if "extends android.app.Service" in clazz.raw:
         test_methods = True
         if not clazz.name.endswith("Service"):
-            error(clazz, None, "Inconsistent class name")
+            error(clazz, None, "Inconsistent class name; should be FooService")
+
+        found = False
+        for f in clazz.fields:
+            if f.name == "SERVICE_INTERFACE":
+                found = True
+                if f.value != clazz.fullname:
+                    error(clazz, f, "Inconsistent interface constant; expected %s" % (clazz.fullname))
+
+        if not found:
+            warn(clazz, None, "Missing SERVICE_INTERFACE constant")
+
+        if "abstract" in clazz.split and not clazz.fullname.startswith("android.service."):
+            warn(clazz, None, "Services extended by developers should be under android.service")
+
     if "extends android.content.ContentProvider" in clazz.raw:
         test_methods = True
         if not clazz.name.endswith("Provider"):
-            error(clazz, None, "Inconsistent class name")
+            error(clazz, None, "Inconsistent class name; should be FooProvider")
+
+        found = False
+        for f in clazz.fields:
+            if f.name == "PROVIDER_INTERFACE":
+                found = True
+                if f.value != clazz.fullname:
+                    error(clazz, f, "Inconsistent interface name; expected %s" % (clazz.fullname))
+
+        if not found:
+            warn(clazz, None, "Missing PROVIDER_INTERFACE constant")
+
+        if "abstract" in clazz.split and not clazz.fullname.startswith("android.provider."):
+            warn(clazz, None, "Providers extended by developers should be under android.provider")
+
     if "extends android.content.BroadcastReceiver" in clazz.raw:
         test_methods = True
         if not clazz.name.endswith("Receiver"):
-            error(clazz, None, "Inconsistent class name")
+            error(clazz, None, "Inconsistent class name; should be FooReceiver")
+
     if "extends android.app.Activity" in clazz.raw:
         test_methods = True
         if not clazz.name.endswith("Activity"):
-            error(clazz, None, "Inconsistent class name")
+            error(clazz, None, "Inconsistent class name; should be FooActivity")
 
     if test_methods:
         for m in clazz.methods:
             if "final" in m.split: continue
             if not re.match("on[A-Z]", m.name):
-                error(clazz, m, "Extendable methods should be onFoo() style, otherwise final")
+                if "abstract" in m.split:
+                    error(clazz, m, "Methods implemented by developers must be named onFoo()")
+                else:
+                    warn(clazz, m, "If implemented by developer, should be named onFoo(); otherwise consider marking final")
 
 
 def verify_builder(clazz):
@@ -430,7 +526,7 @@ def verify_builder(clazz):
     if not clazz.name.endswith("Builder"): return
 
     if clazz.name != "Builder":
-        warn(clazz, None, "Should be standalone Builder class")
+        warn(clazz, None, "Builder should be defined as inner class")
 
     has_build = False
     for m in clazz.methods:
@@ -442,11 +538,11 @@ def verify_builder(clazz):
         if m.name.startswith("clear"): continue
 
         if m.name.startswith("with"):
-            error(clazz, m, "Builder methods must be setFoo()")
+            error(clazz, m, "Builder methods names must follow setFoo() style")
 
         if m.name.startswith("set"):
             if not m.typ.endswith(clazz.fullname):
-                warn(clazz, m, "Should return the builder")
+                warn(clazz, m, "Methods should return the builder")
 
     if not has_build:
         warn(clazz, None, "Missing build() method")
@@ -474,7 +570,7 @@ def verify_layering(clazz):
         "android.view",
         "android.animation",
         "android.provider",
-        "android.content",
+        ["android.content","android.graphics.drawable"],
         "android.database",
         "android.graphics",
         "android.text",
@@ -508,29 +604,40 @@ def verify_layering(clazz):
                 warn(clazz, m, "Method argument type violates package layering")
 
 
-def verify_boolean(clazz):
+def verify_boolean(clazz, api):
     """Catches people returning boolean from getFoo() style methods.
     Ignores when matching setFoo() is present."""
+
     methods = [ m.name for m in clazz.methods ]
+
+    builder = clazz.fullname + ".Builder"
+    builder_methods = []
+    if builder in api:
+        builder_methods = [ m.name for m in api[builder].methods ]
+
     for m in clazz.methods:
         if m.typ == "boolean" and m.name.startswith("get") and m.name != "get" and len(m.args) == 0:
             setter = "set" + m.name[3:]
-            if setter not in methods:
-                error(clazz, m, "Methods returning boolean should be isFoo or hasFoo")
+            if setter in methods:
+                pass
+            elif builder is not None and setter in builder_methods:
+                pass
+            else:
+                warn(clazz, m, "Methods returning boolean should be named isFoo, hasFoo, areFoo")
 
 
 def verify_collections(clazz):
     """Verifies that collection types are interfaces."""
+    if clazz.fullname == "android.os.Bundle": return
+
     bad = ["java.util.Vector", "java.util.LinkedList", "java.util.ArrayList", "java.util.Stack",
            "java.util.HashMap", "java.util.HashSet", "android.util.ArraySet", "android.util.ArrayMap"]
     for m in clazz.methods:
-        filt = re.sub("<.+>", "", m.typ)
-        if filt in bad:
-            error(clazz, m, "Return type is concrete collection")
+        if m.typ in bad:
+            error(clazz, m, "Return type is concrete collection; should be interface")
         for arg in m.args:
-            filt = re.sub("<.+>", "", arg)
-            if filt in bad:
-                error(clazz, m, "Argument is concrete collection")
+            if arg in bad:
+                error(clazz, m, "Argument is concrete collection; should be interface")
 
 
 def verify_flags(clazz):
@@ -545,15 +652,17 @@ def verify_flags(clazz):
 
             scope = f.name[0:f.name.index("FLAG_")]
             if val & known[scope]:
-                warn(clazz, f, "Found overlapping flag")
+                warn(clazz, f, "Found overlapping flag constant value")
             known[scope] |= val
 
 
 def verify_all(api):
     global failures
 
-    failures = []
-    for clazz in api:
+    failures = {}
+    for key in sorted(api.keys()):
+        clazz = api[key]
+
         if clazz.pkg.name.startswith("java"): continue
         if clazz.pkg.name.startswith("junit"): continue
         if clazz.pkg.name.startswith("org.apache"): continue
@@ -581,7 +690,7 @@ def verify_all(api):
         verify_aidl(clazz)
         verify_internal(clazz)
         verify_layering(clazz)
-        verify_boolean(clazz)
+        verify_boolean(clazz, api)
         verify_collections(clazz)
         verify_flags(clazz)
 
@@ -598,9 +707,9 @@ if len(sys.argv) > 2:
     # ignore errors from previous API level
     for p in prev_fail:
         if p in cur_fail:
-            cur_fail.remove(p)
+            del cur_fail[p]
 
 
-for f in cur_fail:
-    print f
+for f in sorted(cur_fail):
+    print cur_fail[f]
     print