Customize arguments-differ error messages (#4422)

Changes the output messages of arguments-differ based on different error cases.
It is part one of the issue #3536

Author: ksaketou

ChangeLog

@@ -51,6 +51,9 @@ Release date: 2021-04-25
 
   Closes #4399
 
+* The warning for ``arguments-differ`` now signals explicitly the difference it detected
+  by naming the argument or arguments that changed and the type of change that occured.
+
 
 What's New in Pylint 2.8.0?
 ===========================

doc/whatsnew/2.9.rst

@@ -20,4 +20,6 @@ Other Changes
 * Fix false-positive ``too-many-ancestors`` when inheriting from builtin classes,
   especially from the ``collections.abc`` module
 
+* The output messages for ``arguments-differ`` error message have been customized based on the different error cases.
+
 * New option ``--fail-on=<msg ids>`` to return non-zero exit codes regardless of ``fail-under`` value.

pylint/checkers/classes.py

@@ -44,6 +44,7 @@
 """
 import collections
 from itertools import chain, zip_longest
+from typing import List, Pattern
 
 import astroid
 
@@ -266,22 +267,48 @@ def _has_different_parameters_default_value(original, overridden):
     return False
 
 
-def _has_different_parameters(original, overridden, dummy_parameter_regex):
+def _has_different_parameters(
+    original: List[astroid.AssignName],
+    overridden: List[astroid.AssignName],
+    dummy_parameter_regex: Pattern,
+    counter: int,
+) -> List[str]:
+    result = []
     zipped = zip_longest(original, overridden)
     for original_param, overridden_param in zipped:
         params = (original_param, overridden_param)
         if not all(params):
-            return True
-
+            return ["Number of parameters "]
+
+        # check for the arguments' type
+        original_type = original_param.parent.annotations[counter]
+        if original_type is not None:
+            overridden_type = overridden_param.parent.annotations[counter]
+            if overridden_type is not None:
+                if original_type.name != overridden_type.name:
+                    result.append(
+                        f"Parameter '{original_param.name}' was of type '{original_type.name}' and is now"
+                        + f" of type '{overridden_type.name}' in"
+                    )
+            counter += 1
+
+        # check for the arguments' name
         names = [param.name for param in params]
         if any(dummy_parameter_regex.match(name) for name in names):
             continue
         if original_param.name != overridden_param.name:
-            return True
-    return False
+            result.append(
+                f"Parameter '{original_param.name}' has been renamed to '{overridden_param.name}' in"
+            )
+
+    return result
 
 
-def _different_parameters(original, overridden, dummy_parameter_regex):
+def _different_parameters(
+    original: astroid.FunctionDef,
+    overridden: astroid.FunctionDef,
+    dummy_parameter_regex: Pattern,
+) -> List[str]:
     """Determine if the two methods have different parameters
 
     They are considered to have different parameters if:
@@ -293,6 +320,7 @@ def _different_parameters(original, overridden, dummy_parameter_regex):
        * they have different keyword only parameters.
 
     """
+    output_messages = []
     original_parameters = _positional_parameters(original)
     overridden_parameters = _positional_parameters(overridden)
 
@@ -315,26 +343,48 @@ def _different_parameters(original, overridden, dummy_parameter_regex):
             v for v in original.args.kwonlyargs if v.name in overidden_names
         ]
 
+    arguments = list(original.args.args)
+    # variable 'count' helps to check if the type of an argument has changed
+    # at the _has_different_parameters method
+    if any(arg.name == "self" for arg in arguments) and len(arguments) > 1:
+        count = 1
+    else:
+        count = 0
+
     different_positional = _has_different_parameters(
-        original_parameters, overridden_parameters, dummy_parameter_regex
+        original_parameters, overridden_parameters, dummy_parameter_regex, count
     )
     different_kwonly = _has_different_parameters(
-        original_kwonlyargs, overridden.args.kwonlyargs, dummy_parameter_regex
+        original_kwonlyargs, overridden.args.kwonlyargs, dummy_parameter_regex, count
     )
+    if different_kwonly and different_positional:
+        if "Number " in different_positional[0] and "Number " in different_kwonly[0]:
+            output_messages.append("Number of parameters ")
+            output_messages += different_positional[1:]
+            output_messages += different_kwonly[1:]
+    else:
+        if different_positional:
+            output_messages += different_positional
+        if different_kwonly:
+            output_messages += different_kwonly
+
     if original.name in PYMETHODS:
         # Ignore the difference for special methods. If the parameter
         # numbers are different, then that is going to be caught by
         # unexpected-special-method-signature.
         # If the names are different, it doesn't matter, since they can't
         # be used as keyword arguments anyway.
-        different_positional = different_kwonly = False
+        output_messages.clear()
 
     # Arguments will only violate LSP if there are variadics in the original
     # that are then removed from the overridden
     kwarg_lost = original.args.kwarg and not overridden.args.kwarg
     vararg_lost = original.args.vararg and not overridden.args.vararg
 
-    return any((different_positional, kwarg_lost, vararg_lost, different_kwonly))
+    if kwarg_lost or vararg_lost:
+        output_messages += ["Variadics removed in"]
+
+    return output_messages
 
 
 def _is_invalid_base_class(cls):
@@ -549,7 +599,7 @@ def _has_same_layout_slots(slots, assigned_value):
         "be written as a function.",
     ),
     "W0221": (
-        "Parameters differ from %s %r method",
+        "%s %s %r method",
         "arguments-differ",
         "Used when a method has a different number of arguments than in "
         "the implemented interface or in an overridden method.",
@@ -1760,12 +1810,47 @@ def _check_signature(self, method1, refmethod, class_type, cls):
         if is_property_setter(method1):
             return
 
-        if _different_parameters(
+        arg_differ_output = _different_parameters(
             refmethod, method1, dummy_parameter_regex=self._dummy_rgx
-        ):
-            self.add_message(
-                "arguments-differ", args=(class_type, method1.name), node=method1
-            )
+        )
+        if len(arg_differ_output) > 0:
+            for msg in arg_differ_output:
+                if "Number" in msg:
+                    total_args_method1 = len(method1.args.args)
+                    if method1.args.vararg:
+                        total_args_method1 += 1
+                    if method1.args.kwarg:
+                        total_args_method1 += 1
+                    if method1.args.kwonlyargs:
+                        total_args_method1 += len(method1.args.kwonlyargs)
+                    total_args_refmethod = len(refmethod.args.args)
+                    if refmethod.args.vararg:
+                        total_args_refmethod += 1
+                    if refmethod.args.kwarg:
+                        total_args_refmethod += 1
+                    if refmethod.args.kwonlyargs:
+                        total_args_refmethod += len(refmethod.args.kwonlyargs)
+                    self.add_message(
+                        "arguments-differ",
+                        args=(
+                            msg
+                            + f"was {total_args_refmethod} in '{refmethod.parent.name}.{refmethod.name}' and "
+                            f"is now {total_args_method1} in",
+                            class_type,
+                            str(method1.parent.name) + "." + str(method1.name),
+                        ),
+                        node=method1,
+                    )
+                else:
+                    self.add_message(
+                        "arguments-differ",
+                        args=(
+                            msg,
+                            class_type,
+                            str(method1.parent.name) + "." + str(method1.name),
+                        ),
+                        node=method1,
+                    )
         elif (
             len(method1.args.defaults) < len(refmethod.args.defaults)
             and not method1.args.vararg

tests/functional/a/arguments_differ.py

@@ -9,7 +9,7 @@ def test(self):
 
 class Child(Parent):
 
-    def test(self, arg): # [arguments-differ]
+    def test(self, arg: int): #[arguments-differ]
         pass
 
 
@@ -27,7 +27,7 @@ def test(self, arg=None): # [arguments-differ]
 class Classmethod(object):
 
     @classmethod
-    def func(cls, data):
+    def func(cls, data: str):
         return data
 
     @classmethod
@@ -56,20 +56,20 @@ def fromkeys(cls, arg, arg1):
 
 class Varargs(object):
 
-    def has_kwargs(self, arg, **kwargs):
+    def has_kwargs(self, arg: bool, **kwargs):
         pass
 
-    def no_kwargs(self, args):
+    def no_kwargs(self, args: bool):
         pass
 
 
 class VarargsChild(Varargs):
 
-    def has_kwargs(self, arg): # [arguments-differ]
-        "Not okay to lose capabilities."
+    def has_kwargs(self, arg: int): #[arguments-differ, arguments-differ]
+        "Not okay to lose capabilities. Also, type has changed."
 
-    def no_kwargs(self, arg, **kwargs): # [arguments-differ]
-        "Not okay to add extra capabilities."
+    def no_kwargs(self, arg: bool, **kwargs): # [arguments-differ]
+        "Addition of kwargs does not violate LSP, but first argument's name has changed."
 
 
 class Super(object):
@@ -111,14 +111,14 @@ def method(self, param='abc'):
 class Staticmethod(object):
 
     @staticmethod
-    def func(data):
+    def func(data: int):
         return data
 
 
 class StaticmethodChild(Staticmethod):
 
     @classmethod
-    def func(cls, data):
+    def func(cls, data: str):
         return data
 
 
@@ -169,7 +169,7 @@ def test(self, *args):
 
 class SecondChangesArgs(FirstHasArgs):
 
-    def test(self, first, second, *args): # [arguments-differ]
+    def test(self, first: int, second: int, *args): # [arguments-differ]
         pass
 
 
@@ -213,23 +213,26 @@ def mixed(self, first, *args, third, **kwargs):
 
 class HasSpecialMethod(object):
 
-    def __getitem__(self, key):
+    def __getitem__(self, key: int):
         return key
 
 
 class OverridesSpecialMethod(HasSpecialMethod):
 
-    def __getitem__(self, cheie):
+    def __getitem__(self, cheie: int):
+        # no error here, method overrides special method
         return cheie + 1
 
 
 class ParentClass(object):
 
-    def meth(self, arg, arg1):
+    def meth(self, arg: str, arg1: str):
         raise NotImplementedError
 
 
 class ChildClass(ParentClass):
 
-    def meth(self, _arg, dummy):
+    def meth(self, _arg: str, dummy: str):
+        # no error here, "dummy" and "_" are being ignored if
+        # spotted in a variable name (declared in dummy_parameter_regex)
         pass

tests/functional/a/arguments_differ.txt

@@ -1,6 +1,7 @@
-arguments-differ:12:4:Child.test:Parameters differ from overridden 'test' method
-arguments-differ:23:4:ChildDefaults.test:Parameters differ from overridden 'test' method
-arguments-differ:41:4:ClassmethodChild.func:Parameters differ from overridden 'func' method
-arguments-differ:68:4:VarargsChild.has_kwargs:Parameters differ from overridden 'has_kwargs' method
-arguments-differ:71:4:VarargsChild.no_kwargs:Parameters differ from overridden 'no_kwargs' method
-arguments-differ:172:4:SecondChangesArgs.test:Parameters differ from overridden 'test' method
+arguments-differ:12:4:Child.test:Number of parameters was 1 in 'Parent.test' and is now 2 in overridden 'Child.test' method
+arguments-differ:23:4:ChildDefaults.test:Number of parameters was 3 in 'ParentDefaults.test' and is now 2 in overridden 'ChildDefaults.test' method
+arguments-differ:41:4:ClassmethodChild.func:Number of parameters was 2 in 'Classmethod.func' and is now 0 in overridden 'ClassmethodChild.func' method
+arguments-differ:68:4:VarargsChild.has_kwargs:Parameter 'arg' was of type 'bool' and is now of type 'int' in overridden 'VarargsChild.has_kwargs' method
+arguments-differ:68:4:VarargsChild.has_kwargs:Variadics removed in overridden 'VarargsChild.has_kwargs' method
+arguments-differ:71:4:VarargsChild.no_kwargs:Parameter 'args' has been renamed to 'arg' in overridden 'VarargsChild.no_kwargs' method
+arguments-differ:172:4:SecondChangesArgs.test:Number of parameters was 2 in 'FirstHasArgs.test' and is now 4 in overridden 'SecondChangesArgs.test' method

tests/functional/a/arguments_differ_py3.py

@@ -1,31 +1,31 @@
 # pylint: disable=missing-docstring,too-few-public-methods
 class AbstractFoo:
 
-    def kwonly_1(self, first, *, second, third):
+    def kwonly_1(self, first: int, *, second: int, third: int):
         "Normal positional with two positional only params."
 
-    def kwonly_2(self, *, first, second):
+    def kwonly_2(self, *, first: str, second: str):
         "Two positional only parameter."
 
-    def kwonly_3(self, *, first, second):
+    def kwonly_3(self, *, first: str, second: str):
         "Two positional only params."
 
-    def kwonly_4(self, *, first, second=None):
+    def kwonly_4(self, *, first: str, second=None):
         "One positional only and another with a default."
 
-    def kwonly_5(self, *, first, **kwargs):
+    def kwonly_5(self, *, first: bool, **kwargs):
         "Keyword only and keyword variadics."
 
-    def kwonly_6(self, first, second, *, third):
+    def kwonly_6(self, first: float, second: float, *, third: int):
         "Two positional and one keyword"
 
 
 class Foo(AbstractFoo):
 
-    def kwonly_1(self, first, *, second): # [arguments-differ]
+    def kwonly_1(self, first: int, *, second: int): # [arguments-differ]
         "One positional and only one positional only param."
 
-    def kwonly_2(self, first): # [arguments-differ]
+    def kwonly_2(self, *, first: str): # [arguments-differ]
         "Only one positional parameter instead of two positional only parameters."
 
     def kwonly_3(self, first, second): # [arguments-differ]
@@ -34,7 +34,7 @@ def kwonly_3(self, first, second): # [arguments-differ]
     def kwonly_4(self, first, second): # [arguments-differ]
         "Two positional params."
 
-    def kwonly_5(self, *, first): # [arguments-differ]
+    def kwonly_5(self, *, first: bool): # [arguments-differ]
         "Keyword only, but no variadics."
 
     def kwonly_6(self, *args, **kwargs): # valid override

tests/functional/a/arguments_differ_py3.txt

@@ -1,5 +1,5 @@
-arguments-differ:25:4:Foo.kwonly_1:Parameters differ from overridden 'kwonly_1' method
-arguments-differ:28:4:Foo.kwonly_2:Parameters differ from overridden 'kwonly_2' method
-arguments-differ:31:4:Foo.kwonly_3:Parameters differ from overridden 'kwonly_3' method
-arguments-differ:34:4:Foo.kwonly_4:Parameters differ from overridden 'kwonly_4' method
-arguments-differ:37:4:Foo.kwonly_5:Parameters differ from overridden 'kwonly_5' method
+arguments-differ:25:4:Foo.kwonly_1:Number of parameters was 4 in 'AbstractFoo.kwonly_1' and is now 3 in overridden 'Foo.kwonly_1' method
+arguments-differ:28:4:Foo.kwonly_2:Number of parameters was 3 in 'AbstractFoo.kwonly_2' and is now 2 in overridden 'Foo.kwonly_2' method
+arguments-differ:31:4:Foo.kwonly_3:Number of parameters was 3 in 'AbstractFoo.kwonly_3' and is now 3 in overridden 'Foo.kwonly_3' method
+arguments-differ:34:4:Foo.kwonly_4:Number of parameters was 3 in 'AbstractFoo.kwonly_4' and is now 3 in overridden 'Foo.kwonly_4' method
+arguments-differ:37:4:Foo.kwonly_5:Variadics removed in overridden 'Foo.kwonly_5' method

tests/functional/t/too/too_many_ancestors.py

@@ -1,4 +1,4 @@
-# pylint: disable=missing-docstring, too-few-public-methods, useless-object-inheritance
+# pylint: disable=missing-docstring, too-few-public-methods, useless-object-inheritance, arguments-differ
 from collections.abc import MutableSequence
 
 class Aaaa(object):