From 2c5cacfd36128f43f5fab4f0665acf69ac049a44 Mon Sep 17 00:00:00 2001 From: Adrian Roos Date: Wed, 19 Dec 2018 17:10:22 +0100 Subject: [PATCH 1/2] apilint: Fix API lint issues Fixes a bug where only the name instead of the fully qualified name was considered when looking for a class, which lead to faulty results for inner classes. Test: python tools/apilint/apilint_test.py Change-Id: Ib015669ed3faef21d2bdd16f1e27bc55c8669d70 --- tools/apilint/apilint.py | 16 ++++++- tools/apilint/apilint_sha_system.sh | 23 +++++++++ tools/apilint/apilint_test.py | 73 +++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 2 deletions(-) create mode 100755 tools/apilint/apilint_sha_system.sh create mode 100644 tools/apilint/apilint_test.py diff --git a/tools/apilint/apilint.py b/tools/apilint/apilint.py index b5a990e6e3cf2..1eec218c7be38 100644 --- a/tools/apilint/apilint.py +++ b/tools/apilint/apilint.py @@ -267,6 +267,18 @@ def _parse_stream_to_generator(f): raise TypeError("send() must be followed by next(), not send()") +def _retry_iterator(it): + """Wraps an iterator, such that calling send(True) on it will redeliver the same element""" + for e in it: + while True: + retry = yield e + if not retry: + break + # send() was called, asking us to redeliver clazz on next(). Still need to yield + # a dummy value to the send() first though. + if (yield "Returning clazz on next()"): + raise TypeError("send() must be followed by next(), not send()") + def _parse_to_matching_class(classes, needle): """Takes a classes generator and parses it until it returns the class we're looking for @@ -276,8 +288,8 @@ def _parse_to_matching_class(classes, needle): if clazz.pkg.name < needle.pkg.name: # We haven't reached the right package yet continue - if clazz.name < needle.name: - # We haven't reached the right class yet + if clazz.pkg.name == needle.pkg.name and clazz.fullname < needle.fullname: + # We're in the right package, but not the right class yet continue if clazz.fullname == needle.fullname: return clazz diff --git a/tools/apilint/apilint_sha_system.sh b/tools/apilint/apilint_sha_system.sh new file mode 100755 index 0000000000000..8538a3d904f5f --- /dev/null +++ b/tools/apilint/apilint_sha_system.sh @@ -0,0 +1,23 @@ +#!/bin/bash + +# Copyright (C) 2018 The Android Open Source Project +# +# 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. + +if git show --name-only --pretty=format: $1 | grep api/ > /dev/null; then + python tools/apilint/apilint.py \ + --base-current <(git show $1:api/current.txt) \ + --base-previous <(git show $1^:api/current.txt) \ + <(git show $1:api/system-current.txt) \ + <(git show $1^:api/system-current.txt) +fi diff --git a/tools/apilint/apilint_test.py b/tools/apilint/apilint_test.py new file mode 100644 index 0000000000000..59fc14d97bee4 --- /dev/null +++ b/tools/apilint/apilint_test.py @@ -0,0 +1,73 @@ +#!/usr/bin/env python + +# Copyright (C) 2018 The Android Open Source Project +# +# 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. + +import unittest + +import apilint + +def cls(pkg, name): + return apilint.Class(apilint.Package(999, "package %s {" % pkg, None), 999, + "public final class %s {" % name, None) + +_ri = apilint._retry_iterator + +c1 = cls("android.app", "ActivityManager") +c2 = cls("android.app", "Notification") +c3 = cls("android.app", "Notification.Action") +c4 = cls("android.graphics", "Bitmap") + +class UtilTests(unittest.TestCase): + def test_retry_iterator(self): + it = apilint._retry_iterator([1, 2, 3, 4]) + self.assertEqual(it.next(), 1) + self.assertEqual(it.next(), 2) + self.assertEqual(it.next(), 3) + it.send("retry") + self.assertEqual(it.next(), 3) + self.assertEqual(it.next(), 4) + with self.assertRaises(StopIteration): + it.next() + + def test_retry_iterator_one(self): + it = apilint._retry_iterator([1]) + self.assertEqual(it.next(), 1) + it.send("retry") + self.assertEqual(it.next(), 1) + with self.assertRaises(StopIteration): + it.next() + + def test_retry_iterator_one(self): + it = apilint._retry_iterator([1]) + self.assertEqual(it.next(), 1) + it.send("retry") + self.assertEqual(it.next(), 1) + with self.assertRaises(StopIteration): + it.next() + + def test_skip_to_matching_class_found(self): + it = _ri([c1, c2, c3, c4]) + self.assertEquals(apilint._parse_to_matching_class(it, c3), + c3) + self.assertEqual(it.next(), c4) + + def test_skip_to_matching_class_not_found(self): + it = _ri([c1, c2, c3, c4]) + self.assertEquals(apilint._parse_to_matching_class(it, cls("android.content", "ContentProvider")), + None) + self.assertEqual(it.next(), c4) + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 61e3730bc07e04181a01760d2eb1db834a854683 Mon Sep 17 00:00:00 2001 From: Adrian Roos Date: Wed, 19 Dec 2018 17:11:21 +0100 Subject: [PATCH 2/2] apilint: Fix API lint issues 2/2 Fixes false positives that occur when a class in current.txt is faulty, and an entry for that class is then added to system-current.txt. This was so because when parsing the previous revison's system-current.txt, we did not know about the class and thus didn't look for it in current.txt, and thus never recorded that the error is preexisting. To avoid that, we track all classes in system-current.txt with a matching entry in current.txt in the current revision, and later use that to look up all classes we may have missed when examining the previous revision. Test: python tools/apilint/apilint_test.py Change-Id: Ibe09f1159e351e56b35b8816ce0db760de4ef791 --- tools/apilint/apilint.py | 87 +++++++++++++++++++++++++---------- tools/apilint/apilint_test.py | 78 ++++++++++++++++++++++++++++++- 2 files changed, 139 insertions(+), 26 deletions(-) diff --git a/tools/apilint/apilint.py b/tools/apilint/apilint.py index 1eec218c7be38..6476abd8268e6 100644 --- a/tools/apilint/apilint.py +++ b/tools/apilint/apilint.py @@ -209,24 +209,42 @@ class Package(): return self.raw -def _parse_stream(f, clazz_cb=None, base_f=None): +def _parse_stream(f, clazz_cb=None, base_f=None, out_classes_with_base=None, + in_classes_with_base=[]): api = {} + in_classes_with_base = _retry_iterator(in_classes_with_base) if base_f: - base_classes = _parse_stream_to_generator(base_f) + base_classes = _retry_iterator(_parse_stream_to_generator(base_f)) else: base_classes = [] - for clazz in _parse_stream_to_generator(f): - base_class = _parse_to_matching_class(base_classes, clazz) - if base_class: - clazz.merge_from(base_class) - + def handle_class(clazz): if clazz_cb: clazz_cb(clazz) else: # In callback mode, don't keep track of the full API api[clazz.fullname] = clazz + def handle_missed_classes_with_base(clazz): + for c in _yield_until_matching_class(in_classes_with_base, clazz): + base_class = _skip_to_matching_class(base_classes, c) + if base_class: + handle_class(base_class) + + for clazz in _parse_stream_to_generator(f): + # Before looking at clazz, let's see if there's some classes that were not present, but + # may have an entry in the base stream. + handle_missed_classes_with_base(clazz) + + base_class = _skip_to_matching_class(base_classes, clazz) + if base_class: + clazz.merge_from(base_class) + if out_classes_with_base is not None: + out_classes_with_base.append(clazz) + handle_class(clazz) + + handle_missed_classes_with_base(None) + return api def _parse_stream_to_generator(f): @@ -257,15 +275,7 @@ def _parse_stream_to_generator(f): elif raw.startswith(" field"): clazz.fields.append(Field(clazz, line, raw, blame)) elif raw.startswith(" }") and clazz: - while True: - retry = yield clazz - if not retry: - break - # send() was called, asking us to redeliver clazz on next(). Still need to yield - # a dummy value to the send() first though. - if (yield "Returning clazz on next()"): - raise TypeError("send() must be followed by next(), not send()") - + yield clazz def _retry_iterator(it): """Wraps an iterator, such that calling send(True) on it will redeliver the same element""" @@ -279,8 +289,8 @@ def _retry_iterator(it): if (yield "Returning clazz on next()"): raise TypeError("send() must be followed by next(), not send()") -def _parse_to_matching_class(classes, needle): - """Takes a classes generator and parses it until it returns the class we're looking for +def _skip_to_matching_class(classes, needle): + """Takes a classes iterator and consumes entries until it returns the class we're looking for This relies on classes being sorted by package and class name.""" @@ -297,6 +307,28 @@ def _parse_to_matching_class(classes, needle): classes.send(clazz) return None +def _yield_until_matching_class(classes, needle): + """Takes a class iterator and yields entries it until it reaches the class we're looking for. + + This relies on classes being sorted by package and class name.""" + + for clazz in classes: + if needle is None: + yield clazz + elif clazz.pkg.name < needle.pkg.name: + # We haven't reached the right package yet + yield clazz + elif clazz.pkg.name == needle.pkg.name and clazz.fullname < needle.fullname: + # We're in the right package, but not the right class yet + yield clazz + elif clazz.fullname == needle.fullname: + # Class found, abort. + return + else: + # We ran past the right class. Send it back into the iterator, then abort. + classes.send(clazz) + return + class Failure(): def __init__(self, sig, clazz, detail, error, rule, msg): self.sig = sig @@ -1555,12 +1587,14 @@ def examine_clazz(clazz): verify_singleton(clazz) -def examine_stream(stream, base_stream=None): +def examine_stream(stream, base_stream=None, in_classes_with_base=[], out_classes_with_base=None): """Find all style issues in the given API stream.""" global failures, noticed failures = {} noticed = {} - _parse_stream(stream, examine_clazz, base_f=base_stream) + _parse_stream(stream, examine_clazz, base_f=base_stream, + in_classes_with_base=in_classes_with_base, + out_classes_with_base=out_classes_with_base) return (failures, noticed) @@ -1746,19 +1780,24 @@ if __name__ == "__main__": show_stats(cur, prev) sys.exit() + classes_with_base = [] + with current_file as f: if base_current_file: with base_current_file as base_f: - cur_fail, cur_noticed = examine_stream(f, base_f) + cur_fail, cur_noticed = examine_stream(f, base_f, + out_classes_with_base=classes_with_base) else: - cur_fail, cur_noticed = examine_stream(f) + cur_fail, cur_noticed = examine_stream(f, out_classes_with_base=classes_with_base) + if not previous_file is None: with previous_file as f: if base_previous_file: with base_previous_file as base_f: - prev_fail, prev_noticed = examine_stream(f, base_f) + prev_fail, prev_noticed = examine_stream(f, base_f, + in_classes_with_base=classes_with_base) else: - prev_fail, prev_noticed = examine_stream(f) + prev_fail, prev_noticed = examine_stream(f, in_classes_with_base=classes_with_base) # ignore errors from previous API level for p in prev_fail: diff --git a/tools/apilint/apilint_test.py b/tools/apilint/apilint_test.py index 59fc14d97bee4..ece69a99f5792 100644 --- a/tools/apilint/apilint_test.py +++ b/tools/apilint/apilint_test.py @@ -59,15 +59,89 @@ class UtilTests(unittest.TestCase): def test_skip_to_matching_class_found(self): it = _ri([c1, c2, c3, c4]) - self.assertEquals(apilint._parse_to_matching_class(it, c3), + self.assertEquals(apilint._skip_to_matching_class(it, c3), c3) self.assertEqual(it.next(), c4) def test_skip_to_matching_class_not_found(self): it = _ri([c1, c2, c3, c4]) - self.assertEquals(apilint._parse_to_matching_class(it, cls("android.content", "ContentProvider")), + self.assertEquals(apilint._skip_to_matching_class(it, cls("android.content", "ContentProvider")), None) self.assertEqual(it.next(), c4) + def test_yield_until_matching_class_found(self): + it = _ri([c1, c2, c3, c4]) + self.assertEquals(list(apilint._yield_until_matching_class(it, c3)), + [c1, c2]) + self.assertEqual(it.next(), c4) + + def test_yield_until_matching_class_not_found(self): + it = _ri([c1, c2, c3, c4]) + self.assertEquals(list(apilint._yield_until_matching_class(it, cls("android.content", "ContentProvider"))), + [c1, c2, c3]) + self.assertEqual(it.next(), c4) + + def test_yield_until_matching_class_None(self): + it = _ri([c1, c2, c3, c4]) + self.assertEquals(list(apilint._yield_until_matching_class(it, None)), + [c1, c2, c3, c4]) + + +faulty_current_txt = """ +package android.app { + public final class Activity { + } + + public final class WallpaperColors implements android.os.Parcelable { + ctor public WallpaperColors(android.os.Parcel); + method public int describeContents(); + method public void writeToParcel(android.os.Parcel, int); + field public static final android.os.Parcelable.Creator CREATOR; + } +} +""".split('\n') + +ok_current_txt = """ +package android.app { + public final class Activity { + } + + public final class WallpaperColors implements android.os.Parcelable { + ctor public WallpaperColors(); + method public int describeContents(); + method public void writeToParcel(android.os.Parcel, int); + field public static final android.os.Parcelable.Creator CREATOR; + } +} +""".split('\n') + +system_current_txt = """ +package android.app { + public final class WallpaperColors implements android.os.Parcelable { + method public int getSomething(); + } +} +""".split('\n') + + + +class BaseFileTests(unittest.TestCase): + def test_base_file_avoids_errors(self): + failures, _ = apilint.examine_stream(system_current_txt, ok_current_txt) + self.assertEquals(failures, {}) + + def test_class_with_base_finds_same_errors(self): + failures_with_classes_with_base, _ = apilint.examine_stream("", faulty_current_txt, + in_classes_with_base=[cls("android.app", "WallpaperColors")]) + failures_with_system_txt, _ = apilint.examine_stream(system_current_txt, faulty_current_txt) + + self.assertEquals(failures_with_classes_with_base.keys(), failures_with_system_txt.keys()) + + def test_classes_with_base_is_emited(self): + classes_with_base = [] + _, _ = apilint.examine_stream(system_current_txt, faulty_current_txt, + out_classes_with_base=classes_with_base) + self.assertEquals(map(lambda x: x.fullname, classes_with_base), ["android.app.WallpaperColors"]) + if __name__ == "__main__": unittest.main() \ No newline at end of file