Skip to content
Merged

Bug1085 #1618

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
b100a7e
Add new functionnal tests
Jul 30, 2017
3d7ced8
First try...
Aug 5, 2017
9eaee35
Add new functionnal tests
Jul 30, 2017
7a97110
First try...
Aug 5, 2017
dddde51
Merge branch 'bug1085' of https://github.com/hippo91/pylint into bug1085
Aug 8, 2017
4ca8dd6
Add new functionnal tests
Jul 30, 2017
2345f75
First try...
Aug 5, 2017
ce58c0c
Add of methods and
hippo91 Aug 10, 2017
22f4712
pylint over itself...
hippo91 Aug 10, 2017
8e30a4e
ChangeLog entry
hippo91 Aug 11, 2017
faddbc5
Merge branch 'bug1085' of https://github.com/hippo91/pylint into bug1085
hippo91 Aug 11, 2017
0e5540c
Removing trailing whitespace
hippo91 Aug 11, 2017
404edda
Add new functionnal tests
Jul 30, 2017
24f51c9
First try...
Aug 5, 2017
08178ea
Add of methods and
hippo91 Aug 10, 2017
2cdc01f
pylint over itself...
hippo91 Aug 10, 2017
1b3e73a
Conflict resolved
hippo91 Aug 12, 2017
6959f7f
Merge branch 'bug1085' of https://github.com/hippo91/pylint into bug1085
hippo91 Aug 12, 2017
370d5e1
Adding test for default values that are None. Change in __has_differe…
hippo91 Aug 12, 2017
a59b484
pylint/checkers/classes.py
hippo91 Aug 14, 2017
429aa87
Taking into account @rogalski remarks (use of sentinel object and loc…
hippo91 Aug 14, 2017
eded8cd
Merge branch 'master' into bug1085
hippo91 Aug 15, 2017
be08f68
Merge branch 'master' into bug1085
hippo91 Aug 17, 2017
c3c9ce7
Handling attribute error exception if the attribute value doesn't exist
hippo91 Aug 17, 2017
eec133b
Correction of the previous commit. If value attribute is not availabl…
hippo91 Aug 17, 2017
50208dc
Taking into account @rogalski's remark concerning early return
hippo91 Aug 19, 2017
142ad02
Last commit was a hugly mistake...
hippo91 Aug 19, 2017
8756114
Last commit was a hugly mistake...
hippo91 Aug 19, 2017
34f8264
Merge branch 'bug1085' of https://github.com/hippo91/pylint into bug1085
hippo91 Aug 19, 2017
7c762b0
Taking into account @rogalski remarks
hippo91 Aug 25, 2017
a2dbd84
Merge remote-tracking branch 'origin' into bug1085
Aug 26, 2017
891468f
Merge branch 'bug1085' of https://github.com/hippo91/pylint into bug1085
Aug 26, 2017
4cc1191
typo
Aug 27, 2017
2fb6602
Merge branch 'master' into bug1085
hippo91 Sep 21, 2017
fb5637c
Taking into account @PCManticore remarks: no more use of method. Ins…
hippo91 Sep 23, 2017
4109efe
Rename of a test method.
hippo91 Sep 23, 2017
ad1bcdd
Add of a new contributor
hippo91 Sep 24, 2017
7fc54ca
Add of a What's new entry concerning useless-super-delegation message
hippo91 Sep 24, 2017
2b34d88
Moving call of _has_different_default_values inside _check_useless_su…
hippo91 Sep 24, 2017
f3a4c4b
Instead of emitting a message when the default values are the same, w…
hippo91 Sep 26, 2017
07661b3
Add of a test where the default values in the base method and the chi…
hippo91 Sep 26, 2017
2a2581c
Add of four tests to take into account complex class hierarchy and me…
hippo91 Sep 28, 2017
cd71dc4
Add of a break statement because we only have to compare current func…
hippo91 Sep 28, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ What's New in Pylint 1.8?
After a change message is correctly not emitted for this case.
Close #1559

* Fix a false positive ``useless-super-delegation`` message when
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also need a What's new Entry, in the Other Changes section. Also please add yourself to Contributors file if you are not already there.

parameters default values are different from those used in the base class.
Close #1085

What's New in Pylint 1.7.1?
=========================

Expand Down
56 changes: 53 additions & 3 deletions pylint/checkers/classes.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,24 @@ def _positional_parameters(method):
positional = positional[1:]
return positional

def _has_different_parameters_default_value(original, overridden):
"""
Return True if one of the overridden arguments has a default
value different from the default value of the original argument
"""
orig_name = [param.name for param in original.args]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.as_string() is not kind of API you want to use in this case.

Maybe consider using standard sentinel notion. Something around the lines of:

default_missing = object()
for param_name in orig_name:
  try:
    orig_default = original.default_value(param_name)
  except astroid.exceptions.NoDefault:
    orig_default = default_missing
  try:
    over_default = overridden.default_value(param_name)
  except astroid.exceptions.NoDefault:
    over_default = default_missing
  if orig_default != over_default
    return True
return False

for param_name in orig_name:
try:
orig_default = original.default_value(param_name).value
except astroid.exceptions.NoDefault:
orig_default = None
try:
over_default = overridden.default_value(param_name).value
if orig_default != over_default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when default value is None?
Is overridden.default_value(param_name).value equal to None or equal to nodes.Const(None)? Maybe we can test for that? (default value equal to None is very common).

Copy link
Contributor Author

@hippo91 hippo91 Aug 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice remark. My code fails if the default value is equal to None.
I'm adding a test and working on a fix.

return True
except astroid.exceptions.NoDefault:
pass
return False

def _has_different_parameters(original, overridden, dummy_parameter_regex):
zipped = six.moves.zip_longest(original, overridden)
Expand Down Expand Up @@ -197,7 +215,6 @@ def _different_parameters(original, overridden, dummy_parameter_regex):
different_kwonly
))


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert removing this line

def _is_invalid_base_class(cls):
return cls.name in INVALID_BASE_CLASSES and is_builtin_object(cls)

Expand Down Expand Up @@ -705,7 +722,8 @@ def _check_useless_super_delegation(self, function):
nothing additional whatsoever than not implementing the method at all.
If the method uses super() to delegate an operation to the rest of the MRO,
and if the method called is the same as the current one, the arguments
passed to super() are the same as the parameters that were passed to
passed to super(), including their default value,
are the same as the parameters that were passed to
this method, then the method could be removed altogether, by letting
other implementation to take precedence.
'''
Expand Down Expand Up @@ -757,6 +775,11 @@ def _check_useless_super_delegation(self, function):
return
if super_call.type.name != current_scope.name:
return

# Detect if the method has argument with default value that is
# different from the one used in the base method
if self._check_different_default_values_in_overridden_method(function):
return

# Detect if the parameters are the same as the call's arguments.
params = _signature_from_arguments(function.args)
Expand All @@ -765,6 +788,34 @@ def _check_useless_super_delegation(self, function):
self.add_message('useless-super-delegation', node=function,
args=(function.name, ))

@staticmethod
def _check_different_default_values_in_overridden_method(function):
"""
Check if the parameters default values in overridden methods are
different from those in the base class
"""
klass = function.parent.frame()
for overridden in klass.local_attr_ancestors(function.name):
# get astroid for the searched method
try:
meth_node = overridden[function.name]
except KeyError:
# we have found the method but it's not in the local
# dictionary.
# This may happen with astroid build from living objects
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this branch is not taken in tests - it would be cool to have this covered by some automated test, but I guess we do not have to be that restrictive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, i just copied/pasted code of the visit_functiondef...

if not isinstance(meth_node, astroid.FunctionDef):
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

instance = klass.instantiate_class()
method1 = function_to_method(function, instance)
refmethod = function_to_method(meth_node, instance)
# Don't care about functions with unknown argument (builtins).
if method1.args.args is None or refmethod.args.args is None:
continue
if _has_different_parameters_default_value(refmethod.args, method1.args):
return True
return False

def _check_slots(self, node):
if '__slots__' not in node.locals:
return
Expand Down Expand Up @@ -1229,7 +1280,6 @@ def _check_signature(self, method1, refmethod, class_type, cls):
if (isinstance(decorator, astroid.Attribute) and
decorator.attrname == 'setter'):
return

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert removing this line

if _different_parameters(
refmethod, method1,
dummy_parameter_regex=self._dummy_rgx):
Expand Down
20 changes: 20 additions & 0 deletions pylint/test/functional/useless_super_delegation.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ class Base(object):
def something(self):
pass

def with_default_argument(self, first, default_arg="default"):
pass

def without_default_argument(self, first, second):
pass


class NotUselessSuper(Base):

Expand Down Expand Up @@ -92,6 +98,13 @@ def extraneous_positional_args(self, **args):
super(NotUselessSuper, self).extraneous_positional_args(
1, 2, **args)

def with_default_argument(self, first, default_arg="other"):
# Not useless because the default_arg is different from the one in the base class
super(NotUselessSuper, self).with_default_argument(first, default_arg)

def without_default_argument(self, first, second=True):
# Not useless because in the base class there is not default value for second argument
super(NotUselessSuper, self).without_default_argument(first, second)

class UselessSuper(Base):

Expand All @@ -116,6 +129,13 @@ def equivalent_params_5(self, first, *args): # [useless-super-delegation]
def equivalent_params_6(self, first, *args, **kwargs): # [useless-super-delegation]
return super(UselessSuper, self).equivalent_params_6(first, *args, **kwargs)

def with_default_argument(self, first, default_arg="default"): # [useless-super-delegation]
# useless because the default value here is the same as in the base class
return super(UselessSuper, self).with_default_argument(first, default_arg)

def without_default_argument(self, first, second): # [useless-super-delegation]
return super(UselessSuper, self).without_default_argument(first, second)

def __init__(self): # [useless-super-delegation]
super(UselessSuper, self).__init__()

Expand Down
18 changes: 10 additions & 8 deletions pylint/test/functional/useless_super_delegation.txt
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
useless-super-delegation:98:UselessSuper.equivalent_params:Useless super delegation in method 'equivalent_params'
useless-super-delegation:101:UselessSuper.equivalent_params_1:Useless super delegation in method 'equivalent_params_1'
useless-super-delegation:104:UselessSuper.equivalent_params_2:Useless super delegation in method 'equivalent_params_2'
useless-super-delegation:107:UselessSuper.equivalent_params_3:Useless super delegation in method 'equivalent_params_3'
useless-super-delegation:110:UselessSuper.equivalent_params_4:Useless super delegation in method 'equivalent_params_4'
useless-super-delegation:113:UselessSuper.equivalent_params_5:Useless super delegation in method 'equivalent_params_5'
useless-super-delegation:116:UselessSuper.equivalent_params_6:Useless super delegation in method 'equivalent_params_6'
useless-super-delegation:119:UselessSuper.__init__:Useless super delegation in method '__init__'
useless-super-delegation:111:UselessSuper.equivalent_params:Useless super delegation in method 'equivalent_params'
useless-super-delegation:114:UselessSuper.equivalent_params_1:Useless super delegation in method 'equivalent_params_1'
useless-super-delegation:117:UselessSuper.equivalent_params_2:Useless super delegation in method 'equivalent_params_2'
useless-super-delegation:120:UselessSuper.equivalent_params_3:Useless super delegation in method 'equivalent_params_3'
useless-super-delegation:123:UselessSuper.equivalent_params_4:Useless super delegation in method 'equivalent_params_4'
useless-super-delegation:126:UselessSuper.equivalent_params_5:Useless super delegation in method 'equivalent_params_5'
useless-super-delegation:129:UselessSuper.equivalent_params_6:Useless super delegation in method 'equivalent_params_6'
useless-super-delegation:132:UselessSuper.with_default_argument:Useless super delegation in method 'with_default_argument'
useless-super-delegation:136:UselessSuper.without_default_argument:Useless super delegation in method 'without_default_argument'
useless-super-delegation:139:UselessSuper.__init__:Useless super delegation in method '__init__'