From 7167442a294bd9dac498724557f0d737bfad9b58 Mon Sep 17 00:00:00 2001 From: Konstantina Saketou <56515303+ksaketou@users.noreply.github.com> Date: Wed, 5 May 2021 21:57:52 +0300 Subject: [PATCH] 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 --- ChangeLog | 3 + doc/whatsnew/2.9.rst | 2 + pylint/checkers/classes.py | 117 ++++++++++++++++--- tests/functional/a/arguments_differ.py | 33 +++--- tests/functional/a/arguments_differ.txt | 13 ++- tests/functional/a/arguments_differ_py3.py | 18 +-- tests/functional/a/arguments_differ_py3.txt | 10 +- tests/functional/t/too/too_many_ancestors.py | 2 +- 8 files changed, 146 insertions(+), 52 deletions(-) diff --git a/ChangeLog b/ChangeLog index 49de77d161..56a1fd4f10 100644 --- a/ChangeLog +++ b/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? =========================== diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index e8a58bf1af..a0053e98f0 100644 --- a/doc/whatsnew/2.9.rst +++ b/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=`` to return non-zero exit codes regardless of ``fail-under`` value. diff --git a/pylint/checkers/classes.py b/pylint/checkers/classes.py index 5c1ae964f4..4a1a8b77d9 100644 --- a/pylint/checkers/classes.py +++ b/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 diff --git a/tests/functional/a/arguments_differ.py b/tests/functional/a/arguments_differ.py index eda0c326f3..272d997b0b 100644 --- a/tests/functional/a/arguments_differ.py +++ b/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 diff --git a/tests/functional/a/arguments_differ.txt b/tests/functional/a/arguments_differ.txt index 53c141be84..f3bdff2e56 100644 --- a/tests/functional/a/arguments_differ.txt +++ b/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 diff --git a/tests/functional/a/arguments_differ_py3.py b/tests/functional/a/arguments_differ_py3.py index 39df462884..f842b891f9 100644 --- a/tests/functional/a/arguments_differ_py3.py +++ b/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 diff --git a/tests/functional/a/arguments_differ_py3.txt b/tests/functional/a/arguments_differ_py3.txt index c57fcb0b0f..df06d4db5c 100644 --- a/tests/functional/a/arguments_differ_py3.txt +++ b/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 diff --git a/tests/functional/t/too/too_many_ancestors.py b/tests/functional/t/too/too_many_ancestors.py index 58b91ec3d8..2c105ba283 100644 --- a/tests/functional/t/too/too_many_ancestors.py +++ b/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):