Fix useless-super-delegation false positive when default keyword argument is a variable. (#5157)

Compare variable default args and simplify the logic of the checkers.

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

6 files changed, 70 insertions, 76 deletions

diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt
index d6e2769ee..fd5d627eb 100644
--- a/CONTRIBUTORS.txt
+++ b/CONTRIBUTORS.txt
@@ -553,6 +553,8 @@ contributors:
 
 * Samuel Forestier: contributor
 
+* Nick Pesce: contributor
+
 * James DesLauriers: contributor
 
 * Youngsoo Sung: contributor
diff --git a/ChangeLog b/ChangeLog
index c97467f74..634e887b6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -49,6 +49,8 @@ Release date: TBA
   * Added ``using-f-string-in-unsupported-version`` checker. Issued when ``py-version``
     is set to a version that does not support f-strings (< 3.6)
 
+* Fix ``useless-super-delegation`` false positive when default keyword argument is a variable.
+
 * Properly emit ``duplicate-key`` when Enum members are duplicate dictionary keys
 
   Closes #5150
diff --git a/doc/whatsnew/2.12.rst b/doc/whatsnew/2.12.rst
index 5f9fcb976..f98406800 100644
--- a/doc/whatsnew/2.12.rst
+++ b/doc/whatsnew/2.12.rst
@@ -33,6 +33,8 @@ New checkers
   * Added ``using-f-string-in-unsupported-version`` checker. Issued when ``py-version``
     is set to a version that does not support f-strings (< 3.6)
 
+* Fix ``useless-super-delegation`` false positive when default keyword argument is a variable.
+
 * Added new checker `use-implicit-booleanness``: Emitted when using collection
   literals for boolean comparisons
 
diff --git a/pylint/checkers/classes.py b/pylint/checkers/classes.py
index 489bb5063..f9a05081b 100644
--- a/pylint/checkers/classes.py
+++ b/pylint/checkers/classes.py
@@ -186,38 +186,6 @@ def _positional_parameters(method):
     return positional
 
 
-def _get_node_type(node, potential_types):
-    """
-    Return the type of the node if it exists in potential_types.
-
-    Args:
-        node (astroid.node): node to get the type of.
-        potential_types (tuple): potential types of the node.
-
-    Returns:
-        type: type of the node or None.
-    """
-    for potential_type in potential_types:
-        if isinstance(node, potential_type):
-            return potential_type
-    return None
-
-
-def _check_arg_equality(node_a, node_b, attr_name):
-    """
-    Check equality of nodes based on the comparison of their attributes named attr_name.
-
-    Args:
-        node_a (astroid.node): first node to compare.
-        node_b (astroid.node): second node to compare.
-        attr_name (str): name of the nodes attribute to use for comparison.
-
-    Returns:
-        bool: True if node_a.attr_name == node_b.attr_name, False otherwise.
-    """
-    return getattr(node_a, attr_name) == getattr(node_b, attr_name)
-
-
 def _has_different_parameters_default_value(original, overridden):
     """
     Check if original and overridden methods arguments have different default values
@@ -244,34 +212,34 @@ def _has_different_parameters_default_value(original, overridden):
             overridden_default = default_missing
 
         default_list = [
-            arg == default_missing for arg in (original_default, overridden_default)
+            arg != default_missing for arg in (original_default, overridden_default)
         ]
-        if any(default_list) and not all(default_list):
-            # Only one arg has no default value
-            return True
-
-        astroid_type_compared_attr = {
-            nodes.Const: "value",
-            nodes.ClassDef: "name",
-            nodes.Tuple: "elts",
-            nodes.List: "elts",
-            nodes.Dict: "items",
-        }
-        handled_types = tuple(
-            astroid_type for astroid_type in astroid_type_compared_attr
-        )
-        original_type = _get_node_type(original_default, handled_types)
-        if original_type:
-            # We handle only astroid types that are inside the dict astroid_type_compared_attr
-            if not isinstance(overridden_default, original_type):
-                # Two args with same name but different types
+        if any(default_list):
+            if not all(default_list):
+                # Only one arg has no default value
                 return True
-            if not _check_arg_equality(
-                original_default,
-                overridden_default,
-                astroid_type_compared_attr[original_type],
-            ):
-                # Two args with same type but different values
+            # Both args have a default value. Compare them
+            astroid_type_comparators = {
+                nodes.Const: lambda a, b: a.value == b.value,
+                nodes.ClassDef: lambda a, b: a.name == b.name,
+                nodes.Tuple: lambda a, b: a.elts == b.elts,
+                nodes.List: lambda a, b: a.elts == b.elts,
+                nodes.Dict: lambda a, b: a.items == b.items,
+                nodes.Name: lambda a, b: set(a.infer()) == set(b.infer()),
+            }
+            original_type = type(original_default)
+            if original_type in astroid_type_comparators:
+                # We handle only astroid types that are inside the dict astroid_type_compared_attr
+                if not isinstance(overridden_default, original_type):
+                    # Two args with same name but different types
+                    return True
+                if not astroid_type_comparators[original_type](
+                    original_default, overridden_default
+                ):
+                    # Two args with same type but different values
+                    return True
+            else:
+                # If the default value comparison is unhandled, assume the value is different
                 return True
     return False
 
diff --git a/tests/functional/u/useless/useless_super_delegation.py b/tests/functional/u/useless/useless_super_delegation.py
index 002c7eb03..2e0384040 100644
--- a/tests/functional/u/useless/useless_super_delegation.py
+++ b/tests/functional/u/useless/useless_super_delegation.py
@@ -3,6 +3,7 @@
 # pylint: disable=line-too-long, useless-object-inheritance, arguments-out-of-order
 # pylint: disable=super-with-arguments, dangerous-default-value
 
+default_var = 1
 def not_a_method(param, param2):
     return super(None, None).not_a_method(param, param2)
 
@@ -52,12 +53,18 @@ class Base(SuperBase):
     def with_default_argument_dict(self, first, default_arg={}):
         pass
 
+    def with_default_argument_var(self, first, default_arg=default_var):
+        pass
+
     def with_default_arg_ter(self, first, default_arg="has_been_changed"):
         super().with_default_arg_ter(first, default_arg)
 
     def with_default_arg_quad(self, first, default_arg="has_been_changed"):
         super().with_default_arg_quad(first, default_arg)
 
+    def with_default_unhandled(self, first, default_arg=lambda: True):
+        super().with_default_arg_quad(first, default_arg)
+
 class NotUselessSuper(Base):
 
     def multiple_statements(self):
@@ -167,6 +174,11 @@ class NotUselessSuper(Base):
         # Not useless because the default_arg is different from the one in the base class
         super(NotUselessSuper, self).with_default_argument_dict(first, default_arg)
 
+    default_var = 2
+    def with_default_argument_var(self, first, default_arg=default_var):
+        # Not useless because the default_arg refers to a different variable from the one in the base class
+        super(NotUselessSuper, self).with_default_argument_var(first, default_arg)
+
     def with_default_argument_bis(self, first, default_arg="default"):
         # Although the default_arg is the same as in the base class, the call signature
         # differs. Thus it is not useless.
@@ -192,6 +204,10 @@ class NotUselessSuper(Base):
         # call is different from the signature
         super(NotUselessSuper, self).with_default_arg_quad(first, default_arg + "_and_modified")
 
+    def with_default_unhandled(self, first, default_arg=lambda: True):
+        # Not useless because the default value type is not explictely handled (Lambda), so assume they are different
+        super(NotUselessSuper, self).with_default_unhandled(first, default_arg)
+
 
 class UselessSuper(Base):
 
@@ -236,6 +252,9 @@ class UselessSuper(Base):
     def with_default_argument_dict(self, first, default_arg={}): # [useless-super-delegation]
         super(UselessSuper, self).with_default_argument_dict(first, default_arg)
 
+    def with_default_argument_var(self, first, default_arg=default_var): # [useless-super-delegation]
+        super(UselessSuper, self).with_default_argument_var(first, default_arg)
+
     def __init__(self): # [useless-super-delegation]
         super(UselessSuper, self).__init__()
 
diff --git a/tests/functional/u/useless/useless_super_delegation.txt b/tests/functional/u/useless/useless_super_delegation.txt
index d2dae5603..0d4b0a1fa 100644
--- a/tests/functional/u/useless/useless_super_delegation.txt
+++ b/tests/functional/u/useless/useless_super_delegation.txt
@@ -1,18 +1,19 @@
-useless-super-delegation:198:4:UselessSuper.equivalent_params:Useless super delegation in method 'equivalent_params'
-useless-super-delegation:201:4:UselessSuper.equivalent_params_1:Useless super delegation in method 'equivalent_params_1'
-useless-super-delegation:204:4:UselessSuper.equivalent_params_2:Useless super delegation in method 'equivalent_params_2'
-useless-super-delegation:207:4:UselessSuper.equivalent_params_3:Useless super delegation in method 'equivalent_params_3'
-useless-super-delegation:210:4:UselessSuper.equivalent_params_4:Useless super delegation in method 'equivalent_params_4'
-useless-super-delegation:213:4:UselessSuper.equivalent_params_5:Useless super delegation in method 'equivalent_params_5'
-useless-super-delegation:216:4:UselessSuper.equivalent_params_6:Useless super delegation in method 'equivalent_params_6'
-useless-super-delegation:219:4:UselessSuper.with_default_argument:Useless super delegation in method 'with_default_argument'
-useless-super-delegation:223:4:UselessSuper.without_default_argument:Useless super delegation in method 'without_default_argument'
-useless-super-delegation:226:4:UselessSuper.with_default_argument_none:Useless super delegation in method 'with_default_argument_none'
-useless-super-delegation:230:4:UselessSuper.with_default_argument_int:Useless super delegation in method 'with_default_argument_int'
-useless-super-delegation:233:4:UselessSuper.with_default_argument_tuple:Useless super delegation in method 'with_default_argument_tuple'
-useless-super-delegation:236:4:UselessSuper.with_default_argument_dict:Useless super delegation in method 'with_default_argument_dict'
-useless-super-delegation:239:4:UselessSuper.__init__:Useless super delegation in method '__init__'
-useless-super-delegation:242:4:UselessSuper.with_default_arg:Useless super delegation in method 'with_default_arg'
-useless-super-delegation:245:4:UselessSuper.with_default_arg_bis:Useless super delegation in method 'with_default_arg_bis'
-useless-super-delegation:248:4:UselessSuper.with_default_arg_ter:Useless super delegation in method 'with_default_arg_ter'
-useless-super-delegation:251:4:UselessSuper.with_default_arg_quad:Useless super delegation in method 'with_default_arg_quad'
+useless-super-delegation:214:4:UselessSuper.equivalent_params:Useless super delegation in method 'equivalent_params'
+useless-super-delegation:217:4:UselessSuper.equivalent_params_1:Useless super delegation in method 'equivalent_params_1'
+useless-super-delegation:220:4:UselessSuper.equivalent_params_2:Useless super delegation in method 'equivalent_params_2'
+useless-super-delegation:223:4:UselessSuper.equivalent_params_3:Useless super delegation in method 'equivalent_params_3'
+useless-super-delegation:226:4:UselessSuper.equivalent_params_4:Useless super delegation in method 'equivalent_params_4'
+useless-super-delegation:229:4:UselessSuper.equivalent_params_5:Useless super delegation in method 'equivalent_params_5'
+useless-super-delegation:232:4:UselessSuper.equivalent_params_6:Useless super delegation in method 'equivalent_params_6'
+useless-super-delegation:235:4:UselessSuper.with_default_argument:Useless super delegation in method 'with_default_argument'
+useless-super-delegation:239:4:UselessSuper.without_default_argument:Useless super delegation in method 'without_default_argument'
+useless-super-delegation:242:4:UselessSuper.with_default_argument_none:Useless super delegation in method 'with_default_argument_none'
+useless-super-delegation:246:4:UselessSuper.with_default_argument_int:Useless super delegation in method 'with_default_argument_int'
+useless-super-delegation:249:4:UselessSuper.with_default_argument_tuple:Useless super delegation in method 'with_default_argument_tuple'
+useless-super-delegation:252:4:UselessSuper.with_default_argument_dict:Useless super delegation in method 'with_default_argument_dict'
+useless-super-delegation:255:4:UselessSuper.with_default_argument_var:Useless super delegation in method 'with_default_argument_var'
+useless-super-delegation:258:4:UselessSuper.__init__:Useless super delegation in method '__init__'
+useless-super-delegation:261:4:UselessSuper.with_default_arg:Useless super delegation in method 'with_default_arg'
+useless-super-delegation:264:4:UselessSuper.with_default_arg_bis:Useless super delegation in method 'with_default_arg_bis'
+useless-super-delegation:267:4:UselessSuper.with_default_arg_ter:Useless super delegation in method 'with_default_arg_ter'
+useless-super-delegation:270:4:UselessSuper.with_default_arg_quad:Useless super delegation in method 'with_default_arg_quad'