semver checker: do not return error msg on new args with default values

compatibility_lib/compatibility_lib/package_crawler_static.py

@@ -63,9 +63,14 @@ def get_package_info(root_dir):
             module_name: {
                 'classes': {
                     class_name: {
-                        'args': [arg1, arg2, ...],
+                        'args': {
+                            'single_args': [arg1, arg2, ...],
+                            'defaults': {arg1: value1, ...},
+                            'vararg': argv,
+                            'kwarg': kwargs,
+                        },
                         'functions': {
-                            function_name: {'args': [...]},
+                            function_name: {'args': {...}},
                         }
                     },
                     class_name: {...},
@@ -118,9 +123,14 @@ def get_module_info(node):
         {
             'classes': {
                 class_name: {
-                    'args': [arg1, arg2, ...],
+                    'args': {
+                        'single_args': [arg1, arg2, ...],
+                        'defaults': {arg1: value1, ...},
+                        'vararg': argv,
+                        'kwarg': kwargs,
+                    },
                     'functions': {
-                        function_name: {'args': [...]},
+                        function_name: {'args': {...}},
                     }
                 },
                 class_name: {...},
@@ -151,7 +161,8 @@ def _get_class_info(classes):
 
         # assumption is that bases are user-defined within the same module
         init_func, subclasses, functions = _get_class_attrs(node, classes)
-        args = []
+        args = {'single_args': [], 'defaults': {},
+                'vararg': None, 'kwarg': None}
         if init_func is not None:
             args = _get_args(init_func.args)
 
@@ -219,11 +230,47 @@ def _get_function_info(functions):
 
 
 def _get_args(node):
-    """returns a list of args"""
-    args = []
-    for arg in node.args:
-        if isinstance(arg, ast.arg):
-            args.append(arg.arg)
-        elif isinstance(arg, ast.Name):
-            args.append(arg.id)
-    return args
+    """returns a dict mapping arg type to arg names"""
+    args, default_args, vararg, kwarg = [], {}, None, None
+    num_required_args = len(node.args) - len(node.defaults)
+
+    for i, argnode in enumerate(node.args):
+        arg = None
+        if isinstance(argnode, ast.arg):
+            arg = argnode.arg
+        elif isinstance(argnode, ast.Name):
+            arg = argnode.id
+        args.append(arg)
+
+        if i >= num_required_args:
+            valnode = node.defaults[i-len(node.args)]
+            if isinstance(valnode, ast.NameConstant):
+                value = valnode.value
+            elif isinstance(valnode, ast.Num):
+                value = valnode.n
+            elif isinstance(valnode, ast.Str):
+                value = valnode.s
+            elif isinstance(valnode, (ast.List, ast.Tuple)):
+                value = valnode.elts
+            elif isinstance(valnode, ast.Dict):
+                value = {}
+                for i, key in enumerate(valnode.keys):
+                    value[key] = valnode.values[i]
+            else:
+                # TODO: provide better error messaging
+                raise Exception('%s:%s: unsupported default arg type' %
+                                (valnode.lineno, valnode.col_offset))
+            default_args[arg] = value
+
+    if node.vararg:
+        vararg = node.vararg.arg
+    if node.kwarg:
+        kwarg = node.kwarg.arg
+
+    res = {}
+    res['single_args'] = args
+    res['defaults'] = default_args
+    res['vararg'] = vararg
+    res['kwarg'] = kwarg
+
+    return res

compatibility_lib/compatibility_lib/semver_checker.py

@@ -14,11 +14,20 @@
 
 """checks two packages for semver breakage"""
 
+import enum
 from compatibility_lib import package_crawler_static as crawler
 
 
+class _Error(enum.Enum):
+    MISSING = '%s: missing arg "%s"'
+    NUM_TOTAL = '%s: expected %s args, got %s'
+    NUM_REQUIRED = '%s: expected %s required args, got %s'
+    BAD_ARG = '%s: bad arg name; expected "%s", got "%s"'
+    BAD_VALUE = ('%s: default value was not preserved; '
+                 'expecting "%s=%s", got "%s=%s"')
+
+
 # TODO: This needs more sophisticated logic
-# - needs to look at args
 def check(old_dir, new_dir):
     """checks for semver breakage for two local directories
     it looks at all the attributes found by get_package_info
@@ -30,29 +39,91 @@ def check(old_dir, new_dir):
         new_dir: directory containing new files
 
     Returns:
-        False if changes breaks semver, True if semver is preserved
+        A list of error strings describing semver breakages
     """
+    qn = ()
     old_pkg_info = crawler.get_package_info(old_dir)
     new_pkg_info = crawler.get_package_info(new_dir)
 
-    unseen = [(old_pkg_info, new_pkg_info)]
+    unseen = [(qn, old_pkg_info, new_pkg_info)]
     errors = []
-    missing = 'missing attribute "%s" from new version'
-    bad_args = 'args do not match; expecting: "%s", got: "%s"'
 
     i = 0
     while i < len(unseen):
-        old, new = unseen[i]
+        qn, old, new = unseen[i]
         for key in old.keys():
             if new.get(key) is None:
-                errors.append(missing % key)
+                msg = _Error.MISSING.value
+                errors.append(msg % (_get_qn_str(qn), key))
                 continue
             if key != 'args':
-                unseen.append((old[key], new[key]))
-            elif old[key] != new[key]:
-                old_args = ', '.join(old[key])
-                new_args = ', '.join(new[key])
-                errors.append(bad_args % (old_args, new_args))
+                new_qn = qn + (key,)
+                unseen.append((new_qn, old[key], new[key]))
+            else:
+                # TODO: better error messages
+                new_errors = _check_args(qn, old[key], new[key])
+                errors.extend(new_errors)
         i += 1
 
     return errors
+
+
+def _get_qn_str(qn_list):
+    """returns the qualified name string"""
+    clean_qn = [name for i, name in enumerate(qn_list) if i % 2 == 1]
+    res = '.'.join(clean_qn)
+    return res
+
+
+def _check_args(qn_list, old_args, new_args):
+    errors = []
+    qn = _get_qn_str(qn_list)
+    missing = _Error.MISSING.value.replace('%s', qn, 1)
+    num_total = _Error.NUM_TOTAL.value.replace('%s', qn, 1)
+    num_required = _Error.NUM_REQUIRED.value.replace('%s', qn, 1)
+    bad_arg = _Error.BAD_ARG.value.replace('%s', qn, 1)
+    bad_value = _Error.BAD_VALUE.value.replace('%s', qn, 1)
+
+    # check old args against new args
+    old_single_args = old_args['single_args']
+    new_single_args = new_args['single_args']
+    if len(old_single_args) > len(new_single_args):
+        res = [num_total % (len(old_single_args), len(new_single_args))]
+        return res
+    for i, _ in enumerate(old_single_args):
+        if old_single_args[i] != new_single_args[i]:
+            res = [bad_arg % (old_single_args[i], new_single_args[i])]
+            return res
+
+    old_defaults = old_args['defaults']
+    new_defaults = new_args['defaults']
+    num_old_req_args = len(old_single_args) - len(old_defaults)
+    num_new_req_args = len(new_single_args) - len(new_defaults)
+
+    # check required args match up
+    for i in range(max(num_old_req_args, num_new_req_args)):
+        if i == len(old_single_args) or i == len(new_single_args):
+            msg = num_required % (num_old_req_args, num_new_req_args)
+            errors.append(msg)
+            break
+
+        # if old_single_args[i] != new_single_args[i]:
+        #     msg = bad_arg % (old_single_args[i], new_single_args[i])
+        #     errors.append(msg)
+        #     break
+
+    # check default arg values
+    for key in old_defaults:
+        if key not in new_defaults:
+            errors.append(missing % key)
+        if old_defaults[key] != new_defaults[key]:
+            errors.append(bad_value % (key, old_defaults[key],
+                                       key, new_defaults[key]))
+
+    # check vararg and kwarg
+    if old_args['vararg'] and old_args['vararg'] != new_args['vararg']:
+        errors.append(missing % old_args['vararg'])
+    if old_args['kwarg'] and old_args['kwarg'] != new_args['kwarg']:
+        errors.append(missing % old_args['kwarg'])
+
+    return errors

compatibility_lib/compatibility_lib/test_semver_checker.py

@@ -33,7 +33,12 @@ def test_get_package_info(self):
                     'classes': {},
                     'functions': {
                         'hello': {
-                            'args': []
+                            'args': {
+                                'single_args': [],
+                                'defaults': {},
+                                'vararg': None,
+                                'kwarg': None
+                            }
                         }
                     }
                 },
@@ -56,21 +61,79 @@ def test_semver_check_on_removed_func(self):
         new_dir = os.path.join(TEST_DIR, 'removed_func/0.2.0')
 
         res = check(old_dir, new_dir)
-        expected = ['missing attribute "bar" from new version']
+        expected = ['main: missing arg "bar"']
         self.assertEqual(expected, res)
 
     def test_semver_check_on_added_args(self):
         old_dir = os.path.join(TEST_DIR, 'added_args/0.1.0')
         new_dir = os.path.join(TEST_DIR, 'added_args/0.2.0')
 
         res = check(old_dir, new_dir)
-        expected = ['args do not match; expecting: "self, x", got: "self, x, y"']
-        self.assertEqual(expected, res)
+        expected = ['main.Foo: expected 2 required args, got 3',
+                    'main.bar: expected 0 required args, got 1']
+        for errmsg in res:
+            self.assertTrue(errmsg in expected)
 
     def test_semver_check_on_removed_args(self):
         old_dir = os.path.join(TEST_DIR, 'removed_args/0.1.0')
         new_dir = os.path.join(TEST_DIR, 'removed_args/0.2.0')
 
         res = check(old_dir, new_dir)
-        expected = ['args do not match; expecting: "self, x", got: "self"']
+        expected = ['main.Foo: expected 2 args, got 1',
+                    'main.bar: expected 1 args, got 0']
+        for errmsg in res:
+            self.assertTrue(errmsg in expected)
+
+    def test_semver_check_on_added_optional_args(self):
+        ver1 = os.path.join(TEST_DIR, 'optional_args/appended/0.1.0')
+        ver2 = os.path.join(TEST_DIR, 'optional_args/appended/0.2.0')
+        ver3 = os.path.join(TEST_DIR, 'optional_args/appended/0.3.0')
+
+        res12 = check(ver1, ver2)
+        res23 = check(ver2, ver3)
+        res13 = check(ver1, ver3)
+
+        self.assertEqual([], res12)
+        self.assertEqual([], res23)
+        self.assertEqual([], res13)
+
+    def test_semver_check_on_removed_optional_args(self):
+        old_dir = os.path.join(TEST_DIR, 'optional_args/removed/0.1.0')
+        new_dir = os.path.join(TEST_DIR, 'optional_args/removed/0.2.0')
+
+        res = check(old_dir, new_dir)
+        expected = ['main.Foo: expected 3 args, got 2',
+                    'main.bar: missing arg "d"',
+                    'main.baz: expected 1 args, got 0']
+        for errmsg in res:
+            self.assertTrue(errmsg in expected)
+
+    def test_semver_check_on_inserted_optional_args(self):
+        old_dir = os.path.join(TEST_DIR, 'optional_args/inserted/0.1.0')
+        new_dir = os.path.join(TEST_DIR, 'optional_args/inserted/0.2.0')
+
+        res = check(old_dir, new_dir)
+        expected = ['main.Foo: bad arg name; expected "y", got "z"']
         self.assertEqual(expected, res)
+
+    def test_semver_check_on_modified_optional_args(self):
+        old_dir = os.path.join(TEST_DIR, 'optional_args/modified/0.1.0')
+        new_dir = os.path.join(TEST_DIR, 'optional_args/modified/0.2.0')
+
+        res = check(old_dir, new_dir)
+        expected = [('main.Foo: default value was not preserved; '
+                     'expecting "y=None", got "y=True"'),
+                    ('main.bar: default value was not preserved; '
+                     'expecting "c=1", got "c=2"'),
+                    ('main.bar: default value was not preserved; '
+                     'expecting "d=2", got "d=1"'),
+                    'main.baz: bad arg name; expected "name", got "first_name"']
+        for errmsg in res:
+            self.assertTrue(errmsg in expected)
+
+    def test_semver_check_on_converted_optional_args(self):
+        old_dir = os.path.join(TEST_DIR, 'optional_args/converted/0.1.0')
+        new_dir = os.path.join(TEST_DIR, 'optional_args/converted/0.2.0')
+
+        res = check(old_dir, new_dir)
+        self.assertEqual([], res)

compatibility_lib/compatibility_lib/testpkgs/added_args/0.1.0/main.py

@@ -17,3 +17,7 @@ class Foo(object):
 
     def __init__(self, x):
         pass
+
+
+def bar():
+    pass

compatibility_lib/compatibility_lib/testpkgs/added_args/0.2.0/main.py

@@ -17,3 +17,7 @@ class Foo(object):
 
     def __init__(self, x, y):
         pass
+
+
+def bar(a):
+    pass

compatibility_lib/compatibility_lib/testpkgs/optional_args/appended/0.1.0/main.py

@@ -0,0 +1,31 @@
+# Copyright 2018 Google LLC
+#
+# 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.
+
+
+class Foo(object):
+
+    def __init__(self, x):
+        pass
+
+
+def bar(a, b, c=1):
+    return (a + b) * c
+
+
+def baz():
+    pass
+
+
+if __name__ == '__main__':
+    print(bar(1, 2))