From 37c22bf7aba0631c162b5958ca36b526767dce49 Mon Sep 17 00:00:00 2001 From: Tim Martin Date: Sun, 15 May 2022 22:37:29 +0100 Subject: [PATCH] Suppress useless-super-delegation on __hash__ in classes with __eq__ (#6611) * Suppress useless-super-delegation on __hash__ in classes with __eq__ If the class overrides __eq__, then the runtime will look for a definition of __hash__, which will have an effect even if it is trivial. (#3934) * Extract a helper function to avoid "too many returns" pylint warning * Improve comments and add a type declaration * Use safe_infer() for inferring the function expression --- ChangeLog | 4 + doc/whatsnew/2.14.rst | 4 + pylint/checkers/classes/class_checker.py | 109 ++++++++++-------- .../u/useless/useless_super_delegation.py | 24 ++++ .../u/useless/useless_super_delegation.txt | 1 + 5 files changed, 95 insertions(+), 47 deletions(-) diff --git a/ChangeLog b/ChangeLog index aef5727c1..8e1a124eb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -50,6 +50,10 @@ Release date: TBA Closes #5467 +* Don't report ``useless-super-delegation`` for the ``__hash__`` method in classes that also override the ``__eq__`` method. + + Closes #3934 + * Added new checker ``typevar-name-mismatch``: TypeVar must be assigned to a variable with the same name as its name argument. Closes #5224 diff --git a/doc/whatsnew/2.14.rst b/doc/whatsnew/2.14.rst index 6c3f9291d..7ef5be2ef 100644 --- a/doc/whatsnew/2.14.rst +++ b/doc/whatsnew/2.14.rst @@ -215,6 +215,10 @@ Other Changes * The ``set_config_directly`` decorator has been removed. +* Don't report ``useless-super-delegation`` for the ``__hash__`` method in classes that also override the ``__eq__`` method. + + Closes #3934 + * Fix falsely issuing ``useless-suppression`` on the ``wrong-import-position`` checker. Closes #5219 diff --git a/pylint/checkers/classes/class_checker.py b/pylint/checkers/classes/class_checker.py index c130686f4..bc37a4853 100644 --- a/pylint/checkers/classes/class_checker.py +++ b/pylint/checkers/classes/class_checker.py @@ -127,6 +127,59 @@ def _definition_equivalent_to_call(definition, call): return all(kw in call.args or kw in definition.kwonlyargs for kw in call.kws) +def _is_trivial_super_delegation(function: nodes.FunctionDef) -> bool: + """Check whether a function definition is a method consisting only of a + call to the same function on the superclass. + """ + if ( + not function.is_method() + # Adding decorators to a function changes behavior and + # constitutes a non-trivial change. + or function.decorators + ): + return False + + body = function.body + if len(body) != 1: + # Multiple statements, which means this overridden method + # could do multiple things we are not aware of. + return False + + statement = body[0] + if not isinstance(statement, (nodes.Expr, nodes.Return)): + # Doing something else than what we are interested in. + return False + + call = statement.value + if ( + not isinstance(call, nodes.Call) + # Not a super() attribute access. + or not isinstance(call.func, nodes.Attribute) + ): + return False + + # Anything other than a super call is non-trivial. + super_call = safe_infer(call.func.expr) + if not isinstance(super_call, astroid.objects.Super): + return False + + # The name should be the same. + if call.func.attrname != function.name: + return False + + # Should be a super call with the MRO pointer being the + # current class and the type being the current instance. + current_scope = function.parent.scope() + if ( + super_call.mro_pointer != current_scope + or not isinstance(super_call.type, astroid.Instance) + or super_call.type.name != current_scope.name + ): + return False + + return True + + # Deal with parameters overriding in two methods. @@ -1171,7 +1224,7 @@ a metaclass class method.", visit_asyncfunctiondef = visit_functiondef - def _check_useless_super_delegation(self, function): + def _check_useless_super_delegation(self, function: nodes.FunctionDef) -> None: """Check if the given function node is an useless method override. We consider it *useless* if it uses the super() builtin, but having @@ -1182,55 +1235,17 @@ a metaclass class method.", this method, then the method could be removed altogether, by letting other implementation to take precedence. """ - - if ( - not function.is_method() - # With decorators is a change of use - or function.decorators - ): + if not _is_trivial_super_delegation(function): return - body = function.body - if len(body) != 1: - # Multiple statements, which means this overridden method - # could do multiple things we are not aware of. - return + call = function.body[0].value - statement = body[0] - if not isinstance(statement, (nodes.Expr, nodes.Return)): - # Doing something else than what we are interested into. - return - - call = statement.value - if ( - not isinstance(call, nodes.Call) - # Not a super() attribute access. - or not isinstance(call.func, nodes.Attribute) - ): - return - - # Should be a super call. - try: - super_call = next(call.func.expr.infer()) - except astroid.InferenceError: - return - else: - if not isinstance(super_call, astroid.objects.Super): - return - - # The name should be the same. - if call.func.attrname != function.name: - return - - # Should be a super call with the MRO pointer being the - # current class and the type being the current instance. - current_scope = function.parent.scope() - if ( - super_call.mro_pointer != current_scope - or not isinstance(super_call.type, astroid.Instance) - or super_call.type.name != current_scope.name - ): - return + # Classes that override __eq__ should also override + # __hash__, even a trivial override is meaningful + if function.name == "__hash__": + for other_method in function.parent.mymethods(): + if other_method.name == "__eq__": + return # Check values of default args klass = function.parent.frame(future=True) diff --git a/tests/functional/u/useless/useless_super_delegation.py b/tests/functional/u/useless/useless_super_delegation.py index 2f24a2ce4..2a7ecb787 100644 --- a/tests/functional/u/useless/useless_super_delegation.py +++ b/tests/functional/u/useless/useless_super_delegation.py @@ -279,3 +279,27 @@ class NotUselessSuperDecorators(Base): @trigger_something('value1') def method_decorated(self): super(NotUselessSuperDecorators, self).method_decorated() + + +class MyList(list): + def __eq__(self, other): + return len(self) == len(other) + + def __hash__(self): + return hash(len(self)) + + +class ExtendedList(MyList): + def __eq__(self, other): + return super().__eq__(other) and len(self) > 0 + + def __hash__(self): + return super().__hash__() + + +class DecoratedList(MyList): + def __str__(self): + return f'List -> {super().__str__()}' + + def __hash__(self): # [useless-super-delegation] + return super().__hash__() diff --git a/tests/functional/u/useless/useless_super_delegation.txt b/tests/functional/u/useless/useless_super_delegation.txt index b69febe69..8184524af 100644 --- a/tests/functional/u/useless/useless_super_delegation.txt +++ b/tests/functional/u/useless/useless_super_delegation.txt @@ -17,3 +17,4 @@ useless-super-delegation:261:4:261:24:UselessSuper.with_default_arg:Useless supe useless-super-delegation:264:4:264:28:UselessSuper.with_default_arg_bis:Useless super delegation in method 'with_default_arg_bis':UNDEFINED useless-super-delegation:267:4:267:28:UselessSuper.with_default_arg_ter:Useless super delegation in method 'with_default_arg_ter':UNDEFINED useless-super-delegation:270:4:270:29:UselessSuper.with_default_arg_quad:Useless super delegation in method 'with_default_arg_quad':UNDEFINED +useless-super-delegation:304:4:304:16:DecoratedList.__hash__:Useless super delegation in method '__hash__':UNDEFINED