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
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 @@
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 @@
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 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 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 @@
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 @@
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 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_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_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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
"android.view",
"android.animation",
"android.provider",
- "android.content",
+ ["android.content","android.graphics.drawable"],
"android.database",
"android.graphics",
"android.text",
@@ -508,29 +604,40 @@
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 @@
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 @@
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 @@
# 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