pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.24k stars 1.12k forks source link

False positive `C2801` `unnecessary-dunder-call` for descriptor binding `__get__` #8265

Open eqvinox opened 1 year ago

eqvinox commented 1 year ago

Bug description

When converting an unbound method (on a class) to a bound method (on an object) by calling __get__, pylint emits an unnecessary-dunder-call warning:

class Cls:
    def __init__(self, foobar):
        self.foobar = foobar

    def get_foobar(self):
        return self.foobar

def test(class_ref, method_ref):
    print(method_ref)
    # <function Cls.get_foobar at 0x7f0e0f151f80>

    instance = class_ref(1234)
    bound_method = method_ref.__get__(instance, class_ref)
    # ^- C2801: Unnecessarily calls dunder method __get__. Use get method. (unnecessary-dunder-call)

    print(bound_method)
    # <bound method Cls.get_foobar of <__main__.Cls object at 0x7fc81afa6d90>>
    print(bound_method())
    # 1234

test(Cls, Cls.get_foobar)

The use case of this is in pytest, class attributes are scanned for test case methods first (without an instance) and then later bound to an instance of their class.

To my knowledge (happy to be proven wrong?), the descriptor dunder methods (__get__, __set__, __delete__) don't have any other way of being invoked, there is no get/set/delete. So they should be ignored for the dunder warning.

Command used

python3 method_bind.py

Pylint output

[…]
method_bind.py:13:19: C2801: Unnecessarily calls dunder method __get__. Use get method. (unnecessary-dunder-call)

Expected behavior

No warning.

Pylint version

pylint 2.15.10
astroid 2.13.3
Python 3.11.2 (main, Feb  8 2023, 21:22:32) [GCC 12.2.0]

OS / Environment

Debian unstable

eqvinox commented 1 year ago

Ping?

DanielNoord commented 1 year ago

This feels like it is pytest specific right? In that case I think it makes more sense to put this in a pytest plugin.

eqvinox commented 1 year ago

This feels like it is pytest specific right? In that case I think it makes more sense to put this in a pytest plugin.

It's not pytest specific, I just wanted to give the real-world use case that caused me to bump into this. The reproduction script above doesn't use pytest. There is no get that you can call instead of __get__ on a descriptor. Anything working with descriptors on an advanced level can/will run into this.

DanielNoord commented 1 year ago

Sorry I misread. I think the main issue here is that we can't really infer the type of method_ref. This feels like such an edge case that I would argue that disabling the message here is better than not raising it ever for __get__.

eqvinox commented 1 year ago

I don't think there's another __get__ that the warning is useful for? AFAIK __get__ __set__ and __delete__ are specific to descriptors; if you're calling them you likely have no other choice — at least that's my understanding…

eqvinox commented 1 year ago

Double checking with https://docs.python.org/3/genindex-_.html - there is no other use of those 3 dunder methods, they're only used on descriptors.

Pierre-Sassoulas commented 1 year ago

@jpy-git as the implementer of the check what do you think ?

eqvinox commented 1 year ago

Random ping to keep this alive - @jpy-git ?

To rephrase/repeat my understanding: __get__ (and __set__ and __delete__) dunder methods only exist on descriptors, and have no non-dunder replacement, thus there is no warning that makes sense for these 3 dunder methods. (The Use get method. part of the warning could in fact be harmful to up-and-learning developers on first encountering python descriptors — there is no "get" method on descriptors.)

couteau commented 3 weeks ago

I agree with @eqvinox. At the very least, this is a bug in the warning message because it is recommending that the developer use an alternative (use the get method) that doesn't exist. And if you are going to fix the warning message for these three dunder methods, it might be best to just eliminate the warning altogether, since the lack of an alternative means that the use of the dunder method is not, in fact, unnecessary.