Fix useless-super-delegation false positive when default keyword argument is a variable.

[nickpesce]

Type of Changes

	Type
✓	🐛 Bug fix

Description

similar to #3773

Problem

Minimal example of this issue:

X=1
Y=2
class A:
    def __init__(self, u, v=X):
        pass


class B(A):
    def __init__(self, u, v=Y): #  [useless-super-delegation]
        super().__init__(a, b)

variables X an Y are incorrectly assumed to be the same

Fix

Currently, only a select few types of default values are checked and all others are assumed to be the same. nodes.Name was not one of those handled cases, so X and Y above were assumed to be the same. I could think of 2 solutions

1. If a variable type is not explicitly handled for comparison, assume that they are different values instead of the same
2. Explicitly handle nodes.Name

I went with option 2 since it seemed like the least intrusive and less likely to have side effects unknown to me, but I could switch it to option 1 if preferred.

Two variable names are considered the same if the inference system outputs the same possible values. The simpler option would be to always call two variables different but that would give more false negatives. I don't know much about the inference system though, so I'm looking for some input on this approach.

[coveralls]

Pull Request Test Coverage Report for Build 1359894741

• 8 of 9 (88.89%) changed or added relevant lines in 1 file are covered.
• 49 unchanged lines in 2 files lost coverage.
• Overall coverage remained the same at 93.249%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/checkers/classes.py	8	9	88.89%

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/variables.py	24	95.39%
pylint/checkers/typecheck.py	25	95.01%

Totals
Change from base Build 1351143461:	0.0%
Covered Lines:	13632
Relevant Lines:	14619

---

💛 - Coveralls

[Pierre-Sassoulas]

Thank you for the cleanup and adding the missing test case !

[Pierre-Sassoulas]

I like this design, good job 👍

[nickpesce]

Thanks for the review @Pierre-Sassoulas! Did you see my note about the Fix in the PR description?

• Do you think that using infer is the right way to compare two nodes.Name's?
• Would it be better to default to assuming that two default params are different if they are not one of the handled types?

[Pierre-Sassoulas]

In this case, it might be more efficient to check if it's a call of self/cls/ClassName, is that what you meant ?

I did not understand the second part of your question, could you provide an example ?

[nickpesce]

As part of this check, we try to determine of the default arguments of the original and overridden function take the default value. We explicitly check a few "handled" types, like for dicts we compare the items. When an argument takes an unhandled type as the default value, we just assume that the original and overridden values are the same. If you refer to my example in the PR description you'll see that X and Y are assumed to be the same value even though they are quite obviously different.

This PR goes with the approach of adding nodes.Name to the explicit list of handled types and saying that X and Y are the same only if their inferred values are the same.

Another approach would have been to say "If we don't know how to compare the values, assume that they are different". This would change this to produce more false negatives than false positives, which is definitely preferable imo.

[Pierre-Sassoulas]

Thank you for clarifying, I see it now. I think the approach with less false positives is better.

This would change this to produce more false negatives than false positives, which is definitely preferable imo.

Definitely, we very rarely have open issues for false negative 😄

[nickpesce]

Ok! So I'll make the change for unhandled types when I get a chance.

Regarding the explicit nodes.name check, I don't quite understand what you mean by "check if it's a call of self/cls/ClassName". Unless I'm missing something, astroid's AST only knows that it is a named reference and not what it is actually referring to. So two references with the same name may refer to different places depending on code execution and scope. If you're worried that infer might cause a performance degradation, leaving nodes.Name unhandled and always assuming that two variables are different seems reasonable to me. What do you think?

[Pierre-Sassoulas]

infer might cause a performance degradation

Well it's true it might reduce speed, but being able to do that (treading performance for exactness) is also the differentiating factor of pylint, so when there is no clever way of doing something without inference, we should use inference. Let's keep infer, if the result is better with it :)

[Pierre-Sassoulas]

Not sure about this suggestion but this (pre-existing) implementation felt complicated.

[Pierre-Sassoulas]

Suggested change

	original_type = _get_node_type(
	original_default, tuple(handled for handled in astroid_type_comparators)
	)
	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
	return True
	if not astroid_type_comparators[original_type](
	original_default, overridden_default
	):
	# Two args with same type but different values
	return True
	if original_default in astroid_type_comparators:
	# We handle only astroid types that are inside the dict astroid_type_compared_attr
	if not isinstance(overridden_default, original_default):
	# Two args with same name but different types
	return True
	if not astroid_type_comparators[original_default](
	original_default, overridden_default
	):
	# Two args with same type but different values
	return True

Maybe we can even remove _get_node_type ? I can't check usage right now.

[nickpesce]

This isn't equivalent logic - original_default will never be in the astroid_type_comparators since it is an instance of a node, not the type. I agree that _get_node_type seems overly complicated though so I'll see what refactoring I can do

[Pierre-Sassoulas]

Ho right, we need to use type() or __class__

[nickpesce]

I confirmed that _get_node_name was only used here, so I changed this function to just use type() and removed _get_node_name. The change from isinstance() to type() shouldn't matter here since none of the handled nodes types have any subtyes as far as I can tell.

[Pierre-Sassoulas]

👍

[Pierre-Sassoulas]

Great first contribution ! Congratulation on becoming a pylint contributor :)

[cdce8p]

Great first PR @nickpesce!
Could you address these two comments in a followup?

[cdce8p]

This dict doesn't change. It would be better to move that to the module scope instead of recreating it each time the function is called.

[cdce8p]

For ClassDef, it would be better to use qname instead.

[nickpesce]

Created ^ for the followups. Thanks @Pierre-Sassoulas and @cdce8p! Glad I could make this contribution!