diff --git a/apilint/src/main/resources/apilint.py b/apilint/src/main/resources/apilint.py index e6a2948..b50b4d1 100644 --- a/apilint/src/main/resources/apilint.py +++ b/apilint/src/main/resources/apilint.py @@ -279,7 +279,7 @@ def __repr__(self): def json(self): return { - 'rule': repr(self.rule), + 'rule': self.rule, 'msg': self.msg, 'detail': repr(self.detail), 'line': repr(self.line), @@ -507,44 +507,28 @@ def verify_protected(clazz): if "protected" in f.split: error(clazz, f, "M7", "Protected fields not allowed; must be public") +def verify_final_fields_only_class(clazz): + if clazz.methods or not clazz.fields: + # Not a final field-only class + return + + for f in clazz.fields: + if "final" not in f.split: + # Not a final field-only class + return + + if not clazz.ctors: + error(clazz, None, "GV1", "Field-only classes need at least one constructor for mocking.") + + if "final" in clazz.split: + error(clazz, None, "GV2", "Field-only classes should not be final for mocking.") 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.ComponentInfo", - "android.content.pm.ResolveInfo", - "android.content.pm.FeatureGroupInfo", - "android.content.pm.InstrumentationInfo", - "android.content.pm.PackageInfo", - "android.content.pm.PackageItemInfo", - "android.content.res.Configuration", - "android.graphics.BitmapFactory.Options", - "android.os.Message", - "android.system.StructPollfd", - ] - for f in clazz.fields: - if not "final" in f.split: - 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, "F2", "Bare fields must be marked final, or add accessors if mutable") - - if not "static" in f.split: - if not re.match("[a-z]([a-zA-Z]+)?", f.name): - error(clazz, f, "S1", "Non-static fields must be named using myField style") - if re.match("[ms][A-Z]", f.name): error(clazz, f, "F1", "Internal objects must not be exposed") @@ -1433,6 +1417,7 @@ def examine_clazz(clazz): verify_tense(clazz) verify_icu(clazz) verify_clone(clazz) + verify_final_fields_only_class(clazz) def examine_stream(stream): @@ -1632,6 +1617,13 @@ def dump_result_json(args, compat_fail, api_changes, failures): print("") sys.exit(131) + if len(cur_fail) != 0: + print("%s API style issues %s\n" % ((format(fg=WHITE, bg=BLUE, bold=True), format(reset=True)))) + for f in sorted(cur_fail): + print(cur_fail[f]) + print("") + sys.exit(77) + if args['show_noticed'] and len(cur_noticed) != 0: print("%s API changes noticed %s\n" % ((format(fg=WHITE, bg=BLUE, bold=True), format(reset=True)))) for f in sorted(cur_noticed.keys()): @@ -1639,9 +1631,3 @@ def dump_result_json(args, compat_fail, api_changes, failures): print("") sys.exit(10) - if len(cur_fail) != 0: - print("%s API style issues %s\n" % ((format(fg=WHITE, bg=BLUE, bold=True), format(reset=True)))) - for f in sorted(cur_fail): - print(cur_fail[f]) - print("") - sys.exit(77) diff --git a/apilint/src/test/resources/apilint_test.py b/apilint/src/test/resources/apilint_test.py index b5e926a..5d905a1 100644 --- a/apilint/src/test/resources/apilint_test.py +++ b/apilint/src/test/resources/apilint_test.py @@ -13,6 +13,7 @@ ERROR_CODES = { 'NO_CHANGE': 0, 'API_CHANGE': 10, + 'API_ERROR': 77, 'INCOMPATIBLE': 131, } @@ -43,6 +44,11 @@ error_code = sp.call(test) + with open(json_file) as f: + json_result = json.load(f) + + print(json.dumps(json_result, indent=2)) + expected_error_code = ERROR_CODES[t["expected"]] if error_code != expected_error_code: print("The following test is expected to fail with {} " @@ -51,12 +57,8 @@ print(" ".join(test)) sys.exit(1) - with open(json_file) as f: - json_result = json.load(f) - - print(json.dumps(json_result, indent=2)) - - assert len(json_result['failures']) == 0 + if t['expected'] != 'API_ERROR': + assert len(json_result['failures']) == 0 if t['expected'] == 'INCOMPATIBLE': assert len(json_result['compat_failures']) == 1 @@ -66,6 +68,11 @@ if t['expected'] == 'API_CHANGE': assert len(json_result['api_changes']) > 0 + if t['expected'] == 'API_ERROR': + assert len(json_result['failures']) > 0 + assert json_result['failure'] == True + assert json_result['failures'][0]['rule'] == t['rule'] + if t['expected'] == 'NO_CHANGE': assert len(json_result['api_changes']) == 0 assert json_result['failure'] == False diff --git a/apilint/src/test/resources/apilint_test/test-fields-only-class-final.after.txt b/apilint/src/test/resources/apilint_test/test-fields-only-class-final.after.txt new file mode 100644 index 0000000..62ca122 --- /dev/null +++ b/apilint/src/test/resources/apilint_test/test-fields-only-class-final.after.txt @@ -0,0 +1,8 @@ +package test { + public final class FieldOnlyTest { + ctor protected FieldOnlyTest(); + field public final int testField0; + field public final int testField1; + field public final int testField2; + } +} diff --git a/apilint/src/test/resources/apilint_test/test-fields-only-class-final.before.txt b/apilint/src/test/resources/apilint_test/test-fields-only-class-final.before.txt new file mode 100644 index 0000000..5dc0e0d --- /dev/null +++ b/apilint/src/test/resources/apilint_test/test-fields-only-class-final.before.txt @@ -0,0 +1,2 @@ +package test { +} diff --git a/apilint/src/test/resources/apilint_test/test-fields-only-class.after.txt b/apilint/src/test/resources/apilint_test/test-fields-only-class.after.txt new file mode 100644 index 0000000..ed4caeb --- /dev/null +++ b/apilint/src/test/resources/apilint_test/test-fields-only-class.after.txt @@ -0,0 +1,7 @@ +package test { + public class TestFieldsOnly { + field public final int testField0; + field public final int testField1; + field public final int testField2; + } +} diff --git a/apilint/src/test/resources/apilint_test/test-fields-only-class.before.txt b/apilint/src/test/resources/apilint_test/test-fields-only-class.before.txt new file mode 100644 index 0000000..5dc0e0d --- /dev/null +++ b/apilint/src/test/resources/apilint_test/test-fields-only-class.before.txt @@ -0,0 +1,2 @@ +package test { +} diff --git a/apilint/src/test/resources/apilint_test/tests.py b/apilint/src/test/resources/apilint_test/tests.py index 167955c..8ca99aa 100644 --- a/apilint/src/test/resources/apilint_test/tests.py +++ b/apilint/src/test/resources/apilint_test/tests.py @@ -68,6 +68,14 @@ },{ "test": "test-annotation-add-class", "expected": "API_CHANGE" + },{ + "test": "test-fields-only-class", + "expected": "API_ERROR", + "rule": "GV1" + },{ + "test": "test-fields-only-class-final", + "expected": "API_ERROR", + "rule": "GV2" },{ "test": "test-whitespace-change", "expected": "NO_CHANGE"