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

C2801 False positive when __setitem__ appears in a lambda #8769

Closed
asarkar opened this issue Jun 13, 2023 · 6 comments · Fixed by #9034 or #9093
Closed

C2801 False positive when __setitem__ appears in a lambda #8769

asarkar opened this issue Jun 13, 2023 · 6 comments · Fixed by #9034 or #9093
Assignees
Labels
Bug 🪲 False Positive 🦟 A message is emitted but nothing is wrong with the code Good first issue Friendly and approachable by new contributors
Milestone

Comments

@asarkar
Copy link

asarkar commented Jun 13, 2023

Bug description

I have a function that accepts a lambda as follows:

zigzag(
        encoded_message,
        num_rails,
        lambda i, _: rail_lengths.__setitem__(i, rail_lengths[i] + 1)
)

pylint generates an error:

C2801: Unnecessarily calls dunder method __setitem__. Set item via subscript. (unnecessary-dunder-call)

But assignment isn’t valid in a lambda, so, the suggestion is incorrect.

Configuration

No response

Command used

pylint \
  --score n \
  --ignore-paths '^.*_test.py$' \
  --disable C0103,C0104,C0114,C0115,C0116 \
  "$basedir"/**/*.py

Pylint output

C2801: Unnecessarily calls dunder method __setitem__. Set item via subscript. (unnecessary-dunder-call)

Expected behavior

No error.

Pylint version

pylint 2.17.4
Python 3.11.3

OS / Environment

No response

Additional dependencies

No response

@asarkar asarkar added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Jun 13, 2023
@jacobtylerwalls

This comment was marked as resolved.

@jacobtylerwalls jacobtylerwalls added Waiting on author Indicate that maintainers are waiting for a message of the author Bug 🪲 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 Waiting on author Indicate that maintainers are waiting for a message of the author labels Jun 13, 2023
@jacobtylerwalls
Copy link
Member

Ouch, sorry, that was your original report.

I see now:

>>> lamb = lambda x: x[1] = 2
  File "<stdin>", line 1
    lamb = lambda x: x[1] = 2
           ^^^^^^^^^^^^^^
SyntaxError: cannot assign to lambda

Thanks for the report!

@jacobtylerwalls jacobtylerwalls changed the title C2801 False positive C2801 False positive when __setitem__ appears in a lambda Jun 13, 2023
@jacobtylerwalls jacobtylerwalls added the Good first issue Friendly and approachable by new contributors label Jun 13, 2023
@badsketch
Copy link
Contributor

badsketch commented Sep 10, 2023

Hey, I'd like to take a stab at this 🙂

First time contributing to pylint, so looking for some advice. I think I have an idea on how to modify checkers/dunder_methods.py to ignore cases when inside a lambda, but I'm stuck on how to test and verify to get it PR ready.

  1. In some cases like lamb = lambda x: x.__gt__(3), we do want it to raise a flag. Should I create a functional test that goes through all possible dunder methods to verify there aren't false negatives?
  2. Generally, I was going to add logic to traverse a dunder's parent node until it encounters a Lambda node (so it can be ignored). Say, a dunder method node is a very far ancestor of a lambda (Don't even think this is possible, but hang on).
<Lambda>
   <something>
     <somethinga>
     <somethingb>
          .
          .
          <dunder_method>

Do I need to prove using the official Python grammar, that intermediate nodes (something1,2,3 etc.) would still make this C2801 exception valid? Having a hard time considering all the sheer possibilities of exceptions of this exceptions.

Hope I'm making sense, thanks!

@Pierre-Sassoulas
Copy link
Member

Should I create a functional test that goes through all possible dunder methods to verify there aren't false negatives?

Sounds great (but "all" might be hard to reach :) ). There's probably already functional tests with a lot of cases, search for # [unnecessary-dunder-call] in the tests directory.

Do I need to prove using the official Python grammar, that intermediate nodes (something1,2,3 etc.) would still make this C2801 exception valid?

Theoretically yes as long as astroid is not properly typed, but let's start by opening a merge request and seeing how it goes in the primer (new code run on major open source projects). We generally only deal with what is usually used in python not code that is correct python but actually never used by anyone.

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Sep 11, 2023

Generally, I was going to add logic to traverse a dunder's parent node until it encounters a Lambda node

Be sure to evaluate if you can use existing utils like get_node_first_ancestor_of_type() instead of adding new logic.

Thanks for the help!

@asarkar
Copy link
Author

asarkar commented Sep 11, 2023

OP here. I think the fix shouldn’t exclude just any dunder method, but only those with side effects. In the Python world, those are probably those that return None. For example, a call to __hash__ should still be a violation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 False Positive 🦟 A message is emitted but nothing is wrong with the code Good first issue Friendly and approachable by new contributors
Projects
None yet
4 participants