Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive unnecessary-dunder-call for __iadd__ #8978

Open
sam-s opened this issue Aug 25, 2023 · 4 comments
Open

False positive unnecessary-dunder-call for __iadd__ #8978

sam-s opened this issue Aug 25, 2023 · 4 comments
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc.

Comments

@sam-s
Copy link

sam-s commented Aug 25, 2023

Bug description

the following code generates an unnecessary-dunder-call message

"https://github.com/pylint-dev/pylint/issues/8978"

class Incrementable:
    "class with an incrementable field"
    def __init__(self, a):
        self.a = a
    def __iadd__(self, other):
        if isinstance(other, Incrementable):
            self.a += other.a
            return self
        return NotImplemented

d = {}
d.setdefault("a", Incrementable(6)).__iadd__(Incrementable(7))

I see 2 work-arounds, both pollute the namespace:

  1. define an intermediate variable:
x = d.setdefault("a", Incrementable(6))
x += Incrementable(7)
  1. Import operator:
import operator
operator.iadd(d.setdefault("a", Incrementable(6)), Incrementable(7))

Using defaultdict is even worse - I will have to convert it back to the ordinary dict for serialization...

Configuration

No response

Command used

pylint z.py

Pylint output

************* Module z
z.py:14:0: C2801: Unnecessarily calls dunder method __iadd__. Use += operator. (unnecessary-dunder-call)

Expected behavior

no diagnostics - or better one.
since += requires an extra variable, it is not an acceptable recommendation.

Pylint version

pylint 2.17.5
astroid 2.15.6
Python 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0]

OS / Environment

windows, linux

Additional dependencies

No response

@sam-s sam-s added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Aug 25, 2023
@Pierre-Sassoulas
Copy link
Member

What about adding the __add__ function directly in Incrementable ?

"""https://github.com/pylint-dev/pylint/issues/8978"""

class Incrementable:
    """class with an incrementable field"""
    def __init__(self, a):
        self.a = a

    def __add__(self, other):
        return self.__iadd__(other)

    def __iadd__(self, other):
        if isinstance(other, Incrementable):
            self.a += other.a
            return self
        return NotImplemented

d = {}
d.setdefault("a", Incrementable(6) + Incrementable(7))

@nickdrozd
Copy link
Collaborator

Following the suggestion given by the warning results in code that fails to parse:

d.setdefault("a", Incrementable(6)) += Incrementable(7)  # Use += operator
E0001: Parsing failed: ''function call' is an illegal expression for augmented assignment (<unknown>, line 14)' (syntax-error)

@nickdrozd nickdrozd added False Positive 🦟 A message is emitted but nothing is wrong with the code and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Aug 28, 2023
@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Aug 28, 2023

Yes, but I think the message should be modified and still raised (in order to make the code more pythonic like in my proposed snippet). Something like 'create an __add__ method in 'Increment' and use it'

@sam-s
Copy link
Author

sam-s commented Sep 3, 2023

What about adding the __add__ function directly in Incrementable ?

"""https://github.com/pylint-dev/pylint/issues/8978"""

class Incrementable:
    """class with an incrementable field"""
    def __init__(self, a):
        self.a = a

    def __add__(self, other):
        return self.__iadd__(other)

    def __iadd__(self, other):
        if isinstance(other, Incrementable):
            self.a += other.a
            return self
        return NotImplemented

d = {}
d.setdefault("a", Incrementable(6) + Incrementable(7))

this does not modify d["a"] if it is already present, i.e.

d = {"a":Incrementable(2)}
d.setdefault("a", Incrementable(6) + Incrementable(7))

is not the same as

d = {"a":Incrementable(2)}
d.setdefault("a", Incrementable(6)).__iadd__(Incrementable(7))

IOW, this code is NOT equivalent to the code that I posted in all cases.

@Pierre-Sassoulas Pierre-Sassoulas added the Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. label Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc.
Projects
None yet
Development

No branches or pull requests

3 participants