False positive unnecessary-dunder-call when overriding dunder method

[jacobtylerwalls]

Bug description

class A(list):
    def __contains__(self, item):
        print("do some special checks")
        return super().__contains__(item)  # raises unnecessary-dunder-call

Configuration

No response

Command used

pylint a.py

Pylint output

************* Module a
a.py:4: [C2801(unnecessary-dunder-call), A.__contains__] Unnecessarily calls dunder method __contains__. Use in keyword.

Expected behavior

No message, since the example cannot be rewritten as return item in super():

TypeError: argument of type 'super' is not iterable

This wasn't happening for __init__ because it was allow-listed, but I think all dunder methods could be overridden in subclasses, so they all need to be made compatible with super().__...

Pylint version

pylint 2.14.0-dev0
astroid 2.12.0-dev0
Python 3.10.2 (v3.10.2:a58ebcc701, Jan 13 2022, 14:50:16) [Clang 13.0.0 (clang-1300.0.29.30)]

OS / Environment

No response

Additional dependencies

No response

[jpy-git]

@jacobtylerwalls apologies, this was an oversight on my behalf. Let's update C2801 so that it only raises iff there is no alternate operator (current behaviour) AND the dunder method is not being called on super() (resolves this issue).

Will pull together a PR for this now 😄

[cdce8p]

Thanks @jpy-git! The last change (to exclude super() calls) did solve a lot of false-positives for me.

I'm just wondering if it would make sense to also exclude calls to names other than super(). Some examples from Home Assistant.

class CustomRegistry(dict):
    def __init__(self) -> None:
        super().__init__()
        self._entry_ids = {}

    def __setitem__(self, key, entry) -> None:
        super().__setitem__(key, entry)
        self._entry_ids.__setitem__(entry.id, entry)  # !
        ...

    def __delitem__(self, key: str) -> None:
        entry = self[key]
        self._entry_ids.__delitem__(entry.id)  # !
        super().__delitem__(key)

# ----
class CustomState(State)
    def __init__(self, state):
        self._state = state

    def __eq__(self, other: Any) -> bool:
        ...
        return self._state.__eq__(other)  # !

# ----
class CustomDict(OrderedDict):
     def __init__(self, *args, **kwds):
         OrderedDict.__init__(self, *args, **kwds)
         ...

    def __setitem__(self, key, value):
        OrderedDict.__setitem__(self, key, value)  # !
        ...

--
There is also the __getattribute__ delegation inside __getattr__.

class A:
    def __getattr__(self, name):
        ...
        return self.__getattribute__(name)

[jpy-git]

So the first example isn't a false positive (at least by the current definition). The super() case is different since you have to call the dunder methods on it, whereas for this you can just use normal operators/keywords.

class CustomRegistry(dict):
    def __init__(self) -> None:
        super().__init__()
        self._entry_ids = {}

    def __setitem__(self, key, entry) -> None:
        super().__setitem__(key, entry)
        self._entry_ids[entry.id] = entry
        ...

    def __delitem__(self, key: str) -> None:
        entry = self[key]
        del self._entry_ids[entry.id]
        super().__delitem__(key)

Likewise in the second case this is fine:

class CustomState(State):
    def __init__(self, state):
        self._state = state

    def __eq__(self, other: Any) -> bool:
        ...
        return self._state == other

[jpy-git]

Think there's a typo in that third case, you'll need a self in OrderedDict.__setitem__(key, value) otherwise you'll get a runtime error

class CustomDict(OrderedDict):
     def __init__(self, *args, **kwds):
         OrderedDict.__init__(self, *args, **kwds)
         ...

    def __setitem__(self, key, value):
        OrderedDict.__setitem__(self, key, value) 
        ...

It's slightly oddly written as given that class structure this should probably be written

class CustomDict(OrderedDict):
     def __init__(self, *args, **kwds):
         super().__init__(*args, **kwds)
         ...

    def __setitem__(self, key, value):
        super().__setitem__(key, value) 
        ...

This one does seem like a false positive so another exception would be calling the dunder method on an uninstantiated class and manually passing self in.

As noted above though super() is more appropriate (at least for this example) so that could be another check idea.

[jpy-git]

There is also the __getattribute__ delegation inside __getattr__.

class A:
    def __getattr__(self, name):
        ...
        return self.__getattribute__(name)

Yeah this one's tricky! When invoking getattr() or accessing an attribute directly it's quite context specific as to whether __getattr__ of __getattribute__ gets called (link), and therefore it'll be tough for us to infer the reverse and figure out whether one of these dunder methods can be replaced with getattr or direct access.

I'd probably suggest the pragmatic approach here is that we exclude these two dunder methods from C2801?

[cdce8p]

So the first example isn't a false positive (at least by the current definition). The super() case is different since you have to call the dunder methods on it, whereas for this you can just use normal operators/keywords.

Technically true, however the way I see it is that reasoning about a dunder method is much easier if the same method names are used in the body. You just don't have to think that much if inside a __setattr__ method you have a call to self.xxx.__setattr__(...). Additionally, in some cases using the operator doesn't work as well for mypy. For self._state == other mypy would raise a no-any-return error, whereas self._state.__eq__(other) is fine. (Might be a mypy issue, but never the less.)

Yeah this one's tricky! When invoking getattr() or accessing an attribute directly it's quite context specific as to whether __getattr__ of __getattribute__ gets called (link), and therefore it'll be tough for us to infer the reverse and figure out whether one of these dunder methods can be replaced with getattr or direct access.

I'd probably suggest the pragmatic approach here is that we exclude these two dunder methods from C2801?

What about a more general approach? I believe we do get the most value of this check for normal functions / methods. What about skipping dunder methods itself all together? That way, we don't need to special case __getattr__ and __getattribute__ and users can still choose to use them where they make sense.

--

Think there's a typo in that third case, you'll need a self in OrderedDict.__setitem__(key, value) otherwise you'll get a runtime error

👍🏻 Got lost in copy - past and trying out super().

This one does seem like a false positive so another exception would be calling the dunder method on an uninstantiated class and manually passing self in.

As noted above though super() is more appropriate (at least for this example) so that could be another check idea.

In this case yes, super() would work just fine, probably. It's a much more common pattern with multi-inheritance though, to use the base class name explicitly.

[jpy-git]

Thanks for elaborating!

Technically true, however the way I see it is that reasoning about a dunder method is much easier if the same method names are used in the body.

What about a more general approach? I believe we do get the most value of this check for normal functions / methods. What about skipping dunder methods itself all together?

Yeah that seems a pretty sensible suggestion 😄
So basically dunder methods are allowed within a dunder method definition?
i.e.

class CustomRegistry(dict):
    def __setitem__(self, key, entry) -> None:
        super().__setitem__(key, entry) # This is fine

    def some_non_dunder_method(self, key, entry) -> None:
        super().__setitem__(key, entry) # This is flagged

It's a much more common pattern with multi-inheritance though, to use the base class name explicitly.

Makes sense, I've fixed this case in #6142 ☝️

[cdce8p]

Yeah that seems a pretty sensible suggestion 😄
So basically dunder methods are allowed within a dunder method definition?

Exactly. At least that's what I was thinking of.

[Pierre-Sassoulas]

Yeah the reason there was a __gt__ in pylint instead of > in the BaseChecker.__gt__ is because it's easier (and less error prone) to just call the dunder you're implementing again. I think it's sensible to authorize it in this context.

[jpy-git]

Will update #6142 to reflect this 👌