Fix #5371: Correctly count arguments to static methods missing @staticmethod decorator

[jacobtylerwalls]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Before
Static methods lacking a @staticmethod decorator, such as Enum._generate_next_value() in the stdlib would have their first argument stripped when counting their arguments, even though it wasn't self or cls. This caused comparison issues in arguments-differ, including both false negatives and positives.

Now
Check if the method is bound before stripping the first argument.

Closes #5371

[jacobtylerwalls]

This is what was reported as generating a false positive.

[jacobtylerwalls]

Here was a false negative!

[Pierre-Sassoulas]

Nice fix !

[coveralls]

Pull Request Test Coverage Report for Build 1536924678

• 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
• 214 unchanged lines in 13 files lost coverage.
• Overall coverage increased (+0.2%) to 93.694%

---

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/typecheck.py	1	94.9%
pylint/reporters/multi_reporter.py	1	98.28%
pylint/checkers/refactoring/refactoring_checker.py	2	98.16%
pylint/testutils/decorator.py	2	93.94%
pylint/pyreverse/main.py	3	87.5%
pylint/checkers/refactoring/recommendation_checker.py	5	96.3%
pylint/lint/run.py	7	76.1%
pylint/config/option.py	17	83.33%
pylint/extensions/_check_docs_utils.py	24	92.63%
pylint/checkers/utils.py	30	95.45%

Totals
Change from base Build 1511393507:	0.2%
Covered Lines:	14160
Relevant Lines:	15113

---

💛 - Coveralls

[Pierre-Sassoulas]

Could we add a functional test using the problematic Enum directly ? I think it might be a case where a specific handling of it to not have false positive elsewhere would be warranted.

[Pierre-Sassoulas]

Nice fix !

[Pierre-Sassoulas]

Imo it should emit argument-differ, there are two errors here, the first one lack a self argument but this one's argument differ as there are 3 instead of 2.

[jacobtylerwalls]

Interesting, let me see if I can follow. How are you counting three arguments? I think that's a code-style suggestion to say that there should be no static methods inside classes without a decorator, so if someone has done that, we get here and they just have 2 == 2.

[Pierre-Sassoulas]

Sorry I meant 2 instead of 3. The first function would need self, arg1, arg2 and this one too but as it's static having 2 is equivalent to 3 (like the false positive you found higher up)

[Pierre-Sassoulas]

In the first function arg1 should be called self but as long as it's not fixed we don't know if it has 2 or 3 args (will the user rename arg1 to self or add self on top of arg1 arg2 ?). Assuming arg1 will be renamed to self and as the second function is static it means it should only have one arg equivalent to arg2 as the self is implicit. If we assume self will be added before arg1 after the user fix it then no error makes more sense. But we have to detect two problems correctly instead of one for that to work, not making assumption is safer imo.

[jacobtylerwalls]

In the first function arg1 should be called self but as long as it's not fixed we don't know if it has 2 or 3 args (will the user rename arg1 to self or add self on top of arg1 arg2 ?).

I guess this is the part I'm having trouble with -- let's say a user has disabled no-self-use because they don't care whether their static methods are decorated as such. Now following your idea pylint will be coupling the behavior of arguments-differ to the choice of no-self-use which will be frustrating for that user. There's nothing strictly wrong with the Enum class in the report, it just makes typing harder. Some users don't type.

will the user rename arg1 to self or add self on top of arg1 arg2 ?

This is the assumption I would be reluctant to make -- why are we assuming self needs to be added at all?

[Pierre-Sassoulas]

This is the assumption I would be reluctant to make -- why are we assuming self needs to be added at all?

I agree we should not couple no-self-argument to argument-differ.

But argument differ here:

class Mother:
   def should_have_been_decorated_as_static(arg1, arg2):  # pylint: disable=no-self-argument
        # Enum._generate_next_value in stdlib should be static
        return arg1 + arg2

class Daugther(Mother):
    @staticmethod
    def should_have_been_decorated_as_static(arg1, arg2): #[argument-differ]
        return arg1 + arg2

Mother need 1 explicit args (self is implicit + arg2):

>>> m = Mother()
>>> m.should_have_been_decorated_as_static(1, 2 )
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: should_have_been_decorated_as_static() takes 2 positional arguments but 3 were given
>>> m.should_have_been_decorated_as_static(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 4, in should_have_been_decorated_as_static
TypeError: unsupported operand type(s) for +: 'Mother' and 'int'

While Daugter needs 2 explicit args:

>>> d = Daugther()
>>> d.should_have_been_decorated_as_static(1, 2)
3
>>> d.should_have_been_decorated_as_static(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: should_have_been_decorated_as_static() missing 1 required positional argument: 'arg2'

Thus argument-differ should be triggered.

[jacobtylerwalls]

Ah, but only if you call it on the instance!

>>> Mother.should_have_been_decorated_as_static(1, 2)
3
>>> Daugther.should_have_been_decorated_as_static(1, 2)
3

I think this was the scenario in the stdlib, which is why I didn't raise a bug against CPython. What do you think?

[Pierre-Sassoulas]

OK it makes sense now. I think @staticmethod is a stronger signal than the arg not being called self but there is already a message for the argument not being called self.

[Pierre-Sassoulas]

Thanks for clarifying the misunderstanding from my previous comment :) Would it be possible to add an example using the enum from the original issue ?

[Pierre-Sassoulas]

Very nice, thank you !